Bug 153679

See https://github.com/tc39/ecma402/issues/57

Unlike the github bug thread or what we heard at the tc39 mtg, the first post in this thread uses .apply rather than .call. The code at https://github.com/tc39/ecma402/issues/57#issuecomment-186856109 will need some adjustment to give these objects their own overriding .call and .apply methods without stomping on each other. But https://github.com/tc39/ecma402/issues/57#issuecomment-186856109 , if it otherwise works, gives the idea.

Ok, the gist is essentially that rather than try to swap the object in place, we let it be a proxy of sorts to a real Collator/NumberFormat/DateTimeFormat. I'll try this week to write a patch that does that. Thanks!

Created attachment 272426 [details] Patch

Comment on attachment 272426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272426&action=review > Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:121 > + IntlDateTimeFormatConstructor* callee = jsCast<IntlDateTimeFormatConstructor*>(state->callee()); Since callee is used below and thisValue is not, I suggest moving this up one line and maybe initializing vm and callee in a separate paragraph. > Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:124 > + JSValue prototype = callee->getDirect(vm, vm.propertyNames->prototype); > + IntlDateTimeFormat* dateTimeFormat = jsDynamicCast<IntlDateTimeFormat*>(thisValue); > + if (!dateTimeFormat && JSObject::defaultHasInstance(state, thisValue, prototype)) { We it would be nice to not do the work to get the prototype unless thisValue is not a dateTime, so we should be using two nested if statements here. First checking dateTimeFormat for null before getting the prototype. > Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:129 > + ASSERT(dateTimeFormat); This assertion seems a little excessive to me; not all that helpful. > Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:132 > + JSValue locales = state->argument(0); > + JSValue options = state->argument(1); > + dateTimeFormat->initializeDateTimeFormat(*state, locales, options); Not all that useful to put these into local variables; I’d suggest just writing this out in a single line. Same in the main code below. > Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:136 > + if (state->hadException()) > + return JSValue::encode(jsUndefined()); Where is this exception coming from? If it’s coming from the call to initializeDateTimeFormat, then I suggest doing it before calling putDirect. I am assuming that putDirect cannot return an exception. > Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:144 > // 3. ReturnIfAbrupt(dateTimeFormat). > ASSERT(dateTimeFormat); I’m a little mystified by this. The standard seems to contemplate an error case here that we assert does not exist! > Source/JavaScriptCore/runtime/IntlNumberFormatConstructor.cpp:118 > + // FIXME: Workaround to provide compatibility with ECMA-402 1.0 call/apply patterns. All the same comments apply here as in the IntlDateTimeFormatConstructor.cpp file. It’s also bothering me that we are writing out separate copies of almost identical code for these two classes. Not great factoring.

Comment on attachment 272426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272426&action=review >> Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:121 >> + IntlDateTimeFormatConstructor* callee = jsCast<IntlDateTimeFormatConstructor*>(state->callee()); > > Since callee is used below and thisValue is not, I suggest moving this up one line and maybe initializing vm and callee in a separate paragraph. will do. >> Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:124 >> + if (!dateTimeFormat && JSObject::defaultHasInstance(state, thisValue, prototype)) { > > We it would be nice to not do the work to get the prototype unless thisValue is not a dateTime, so we should be using two nested if statements here. First checking dateTimeFormat for null before getting the prototype. will do. >> Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:129 >> + ASSERT(dateTimeFormat); > > This assertion seems a little excessive to me; not all that helpful. yeah, I'll pull it out, and the one below that also just asserts that create worked. >> Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:132 >> + dateTimeFormat->initializeDateTimeFormat(*state, locales, options); > > Not all that useful to put these into local variables; I’d suggest just writing this out in a single line. Same in the main code below. will do. >> Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:136 >> + return JSValue::encode(jsUndefined()); > > Where is this exception coming from? If it’s coming from the call to initializeDateTimeFormat, then I suggest doing it before calling putDirect. I am assuming that putDirect cannot return an exception. I assumed that putDirect can cause an exception on a sealed object. Is that not the case? >> Source/JavaScriptCore/runtime/IntlNumberFormatConstructor.cpp:118 >> + // FIXME: Workaround to provide compatibility with ECMA-402 1.0 call/apply patterns. > > All the same comments apply here as in the IntlDateTimeFormatConstructor.cpp file. It’s also bothering me that we are writing out separate copies of almost identical code for these two classes. Not great factoring. Personally, I think writing the same thing twice is better than trying to make an abstraction that makes sense and works in both places, especially when I expect this workaround to just get ripped out after a while. If it were 3 or 4 times, I would feel more pressure to create an abstraction.

Created attachment 273426 [details] Patch

Comment on attachment 272426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272426&action=review >>> Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:136 >>> + return JSValue::encode(jsUndefined()); >> >> Where is this exception coming from? If it’s coming from the call to initializeDateTimeFormat, then I suggest doing it before calling putDirect. I am assuming that putDirect cannot return an exception. > > I assumed that putDirect can cause an exception on a sealed object. Is that not the case? I do not think it can. I believe that putDirect is only safe to use when you know a sealed object is not involved.

Comment on attachment 273426 [details] Patch Clearing flags on attachment: 273426 Committed r197925: <http://trac.webkit.org/changeset/197925>

All reviewed patches have been landed. Closing bug.

Looks like this caused 14 JSC tests to fail: https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/4090/steps/jscore-test/logs/stdio

(In reply to comment #12) > Looks like this caused 14 JSC tests to fail: > https://build.webkit.org/builders/ > Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/4090/steps/jscore- > test/logs/stdio Please run run-javascriptcore-tests next time before landing a JSC change to avoid similar breakages.

(In reply to comment #13) > (In reply to comment #12) > > Looks like this caused 14 JSC tests to fail: > > https://build.webkit.org/builders/ > > Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/4090/steps/jscore- > > test/logs/stdio > > Please run run-javascriptcore-tests next time before > landing a JSC change to avoid similar breakages. It's sad that this is not part of EWS.

shouldNotBe isn't defined in the test harness used by run-javascriptcore-tests, but is in the harness use by run-webkit-tests.

(In reply to comment #15) > shouldNotBe isn't defined in the test harness used by > run-javascriptcore-tests, but is in the harness use by run-webkit-tests. In this case you should add it to standalone-pre.js too.

(In reply to comment #16) > (In reply to comment #15) > > shouldNotBe isn't defined in the test harness used by > > run-javascriptcore-tests, but is in the harness use by run-webkit-tests. > > In this case you should add it to standalone-pre.js too. I'm preparing a patch now.

Re-opened since this is blocked by bug 155308