Bug 146926

Created attachment 256755 [details] Add Collator, NumberFormat, and DateTimeFormat skeletons.

Attachment 256755 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/IntlCollatorConstructor.cpp:49: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlDateTimeFormatPrototype.cpp:48: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlNumberFormatConstructor.cpp:36: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlNumberFormatConstructor.cpp:49: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:48: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:36: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:49: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:48: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 8 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.

http://test262.ecmascript.org/testcases_intl402.html Results in Safari 8.0.7 Pass: 12 | Fail: 141 Results with this patch in MiniBrowser Pass: 91 | Fail: 62

The style fails because the .lut.h headers are included after the rest of the headers, per the pattern of every other file I found with a .lut.h If need be, I can move it up into alphabetical order to pass the style check.

The windows build contains multiple errors like this: IntlObject.obj : error LNK2019: unresolved external symbol "public: static class JSC::Structure * __cdecl JSC::IntlCollator::createStructure(class JSC::VM &,class JSC::JSGlobalObject *,class JSC::JSValue)" (?createStructure@IntlCollator@JSC@@SAPAVStructure@2@AAVVM@2@PAVJSGlobalObject@2@VJSValue@2@@Z) referenced in function "protected: void __thiscall JSC::IntlObject::finishCreation(class JSC::VM &,class JSC::JSGlobalObject *)" (?finishCreation@IntlObject@JSC@@IAEXAAVVM@2@PAVJSGlobalObject@2@@Z) [C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\JavaScriptCore.vcxproj\JavaScriptCore.vcxproj] IntlObject.cpp includes all the headers for Collator, DateTimeFormat, and NumberFormat. Any ideas what might cause the linker to not be able to resolve the create methods?

(In reply to comment #5) > The windows build contains multiple errors like this: > > IntlObject.obj : error LNK2019: unresolved external symbol "public: static > class JSC::Structure * __cdecl JSC::IntlCollator::createStructure(class > JSC::VM &,class JSC::JSGlobalObject *,class JSC::JSValue)" > (?createStructure@IntlCollator@JSC@@SAPAVStructure@2@AAVVM@2@PAVJSGlobalObjec > t@2@VJSValue@2@@Z) referenced in function "protected: void __thiscall > JSC::IntlObject::finishCreation(class JSC::VM &,class JSC::JSGlobalObject > *)" (?finishCreation@IntlObject@JSC@@IAEXAAVVM@2@PAVJSGlobalObject@2@@Z) > [C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\JavaScriptCore. > vcxproj\JavaScriptCore.vcxproj] > > IntlObject.cpp includes all the headers for Collator, DateTimeFormat, and > NumberFormat. Any ideas what might cause the linker to not be able to > resolve the create methods? On Windows, symbols need to be exported explicitly :(

(In reply to comment #6) > (In reply to comment #5) > > The windows build contains multiple errors like this: > > > > IntlObject.obj : error LNK2019: unresolved external symbol "public: static > > class JSC::Structure * __cdecl JSC::IntlCollator::createStructure(class > > JSC::VM &,class JSC::JSGlobalObject *,class JSC::JSValue)" > > (?createStructure@IntlCollator@JSC@@SAPAVStructure@2@AAVVM@2@PAVJSGlobalObjec > > t@2@VJSValue@2@@Z) referenced in function "protected: void __thiscall > > JSC::IntlObject::finishCreation(class JSC::VM &,class JSC::JSGlobalObject > > *)" (?finishCreation@IntlObject@JSC@@IAEXAAVVM@2@PAVJSGlobalObject@2@@Z) > > [C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\JavaScriptCore. > > vcxproj\JavaScriptCore.vcxproj] > > > > IntlObject.cpp includes all the headers for Collator, DateTimeFormat, and > > NumberFormat. Any ideas what might cause the linker to not be able to > > resolve the create methods? > > On Windows, symbols need to be exported explicitly :( How is this done?

Comment on attachment 256755 [details] Add Collator, NumberFormat, and DateTimeFormat skeletons. View in context: https://bugs.webkit.org/attachment.cgi?id=256755&action=review Starting to review files at random. It is really good overall. I am impressed how much you progressed without asking any question. > Source/JavaScriptCore/ChangeLog:8 > + Can you provide a link to the draft you used here? Otherwise the comments referencing spec sections lack context. > Source/JavaScriptCore/runtime/IntlCollator.h:58 > + WriteBarrier<IntlCollatorConstructor> m_constructor; Why do you keep this one around and expose it through constructor()? > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:105 > + if (exec->hadException()) > + return JSValue::encode(jsUndefined()); You could already start a new test file for NumberFormat and test the behaviors you implemented. For example, with the code you already have, you could test: -not passing arguments -passing and integer -passing an object with valueOf() that throws an exception -passing an object with valueOf() returning an integer. > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:110 > + // TODO: Implement FormatNumber. We always use "FIXME" in Webkit instead of TODO. > Source/JavaScriptCore/runtime/IntlNumberFormat.h:59 > +/* Let's not commit commented code. You'll add the attributes as needed. > Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:104 > + JSFunction* targetObject = JSFunction::create(vm, globalObject, 1, String("format"), IntlNumberFormatFuncFormatNumber, NoIntrinsic); String("format") -> ASCIILiteral("string") ASCIILiteral is a faster way to initialize Strings from literal constants. > Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:110 > + boundFormat = JSBoundFunction::create(vm, globalObject, targetObject, nf, boundArgs, 1, String("format")); String("format") -> ASCIILiteral("string") > Source/JavaScriptCore/runtime/IntlObject.cpp:73 > + // set up Collator Uppercase first leter, missing period. > Source/JavaScriptCore/runtime/IntlObject.cpp:80 > + // set up NumberFormat Uppercase first leter, missing period. > Source/JavaScriptCore/runtime/IntlObject.cpp:87 > + // set up DateTimeFormat Uppercase first leter, missing period. > Source/JavaScriptCore/runtime/IntlObject.h:54 > + IntlCollatorConstructor* collatorConstructor() const { return m_collatorConstructor.get(); } > + IntlCollatorPrototype* collatorPrototype() const { return m_collatorPrototype.get(); } > + Structure* collatorStructure() const { return m_collatorStructure.get(); } It looks like none of those are used (nor the ones following). You should not expose internal attributes unless needed. > Source/JavaScriptCore/runtime/IntlObject.h:67 > + WriteBarrier<IntlCollatorConstructor> m_collatorConstructor; Why are the members protected and not private? Do you actually need all those structures? Once they are set as properties, the GC will know about them. If you do not need direct access after setting the properties, you do not need to keep a reference here.

(In reply to comment #7) > > > IntlObject.cpp includes all the headers for Collator, DateTimeFormat, and > > > NumberFormat. Any ideas what might cause the linker to not be able to > > > resolve the create methods? > > > > On Windows, symbols need to be exported explicitly :( > > How is this done? My bad, I did not read the full error and misunderstood the problem. When linking two libraries, the symbols need to be exported explicitly. In WebKit, this is done through export macros. But that's not the problem we have here. We are linking JavaScriptCore itself, not JavaScriptCore to an other library. Here, the linker does not find the symbols for the new objects you added. The reason is their CPP files were not compiled, so there is not object file with the symbols. You probably just need to add the .cpp files to the Windows build system.

Chris, this patch fails to build with CMake. The generated file sections looks good to me. Any idea what is missing for CMake?

Comment on attachment 256755 [details] Add Collator, NumberFormat, and DateTimeFormat skeletons. View in context: https://bugs.webkit.org/attachment.cgi?id=256755&action=review >> Source/JavaScriptCore/ChangeLog:8 >> + > > Can you provide a link to the draft you used here? Otherwise the comments referencing spec sections lack context. Will do. >> Source/JavaScriptCore/runtime/IntlCollator.h:58 >> + WriteBarrier<IntlCollatorConstructor> m_constructor; > > Why do you keep this one around and expose it through constructor()? I think it was a pattern I saw elsewhere. I'll remove it for now, and if I find I need it I can add it back. >> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:105 >> + return JSValue::encode(jsUndefined()); > > You could already start a new test file for NumberFormat and test the behaviors you implemented. > > For example, with the code you already have, you could test: > -not passing arguments > -passing and integer > -passing an object with valueOf() that throws an exception > -passing an object with valueOf() returning an integer. I have been adding to the Intl test file, but I can break it out and do more in depth testing. >> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:110 >> + // TODO: Implement FormatNumber. > > We always use "FIXME" in Webkit instead of TODO. I will change it. >> Source/JavaScriptCore/runtime/IntlNumberFormat.h:59 >> +/* > > Let's not commit commented code. You'll add the attributes as needed. Will remove. >> Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:104 >> + JSFunction* targetObject = JSFunction::create(vm, globalObject, 1, String("format"), IntlNumberFormatFuncFormatNumber, NoIntrinsic); > > String("format") -> ASCIILiteral("string") > > ASCIILiteral is a faster way to initialize Strings from literal constants. Good to know. I'll make the switch. >> Source/JavaScriptCore/runtime/IntlObject.h:67 >> + WriteBarrier<IntlCollatorConstructor> m_collatorConstructor; > > Why are the members protected and not private? > > Do you actually need all those structures? > Once they are set as properties, the GC will know about them. If you do not need direct access after setting the properties, you do not need to keep a reference here. I was following the pattern from JSGlobalObject. If it's not needed I can remove them.

Created attachment 257016 [details] updated comments, enabled more flags

Attachment 257016 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/IntlCollatorConstructor.cpp:49: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlDateTimeFormatPrototype.cpp:48: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlNumberFormatConstructor.cpp:36: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlNumberFormatConstructor.cpp:49: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:48: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:36: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:49: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:48: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 8 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 257030 [details] Patch

Attachment 257030 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/IntlCollatorConstructor.cpp:49: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlDateTimeFormatPrototype.cpp:48: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlNumberFormatConstructor.cpp:36: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlNumberFormatConstructor.cpp:49: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:48: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:36: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlDateTimeFormatConstructor.cpp:49: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/IntlNumberFormatPrototype.cpp:48: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 8 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.

Any help on how to fix the Windows linking errors would be much appreciated.

(In reply to comment #16) > Any help on how to fix the Windows linking errors would be much appreciated. The current windows EWS seems flaky. So if you said about the Windows EWS, I think it's not related to the patch. If the working copy (with the patch) in the Windows cannot be built, it's related to your patch.

http://trac.webkit.org/changeset/187142 NewTarget from C++ world is supported!

(In reply to comment #18) > http://trac.webkit.org/changeset/187142 > > NewTarget from C++ world is supported! Awesome! I will update the patch to take advantage of NewTarget.

Comment on attachment 257030 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257030&action=review Quick review round. > Source/JavaScriptCore/ChangeLog:5 > + Adds basic types for ECMA-402 2nd edition, but does not implement the full locale-aware features yet. > + http://www.ecma-international.org/ecma-402/2.0/ECMA-402.pdf Can you please move this below the bug URL? The format is: -Title -Bug number -Blank line -Description of the changes, as many paragraphs as you need. > Source/JavaScriptCore/runtime/IntlCollator.cpp:103 > + if (exec->hadException()) > + return JSValue::encode(jsUndefined()); I don't see any tests for this. Could you start a new file for the Collator. Move all the tests you have for Intl.Collator to it, and start testing behaviors? In this case, you can already check that: -arg0.toString() is called before arg1.toString(). -If arg0.toString() raises an exception, arg1.toString() and arg1.valueOf() are never invoked. -If either arg0 or arg1 raise an exception, the return value is undefined. > Source/JavaScriptCore/runtime/IntlCollator.cpp:117 > + // Return simple check until properly implemented. > + return JSValue::encode(jsNumber(codePointCompare(a->value(exec).impl(), b->value(exec).impl()))); I guess you could have some tests for this behavior when it is already correct. That's not really important at this point though. > Source/JavaScriptCore/runtime/IntlCollatorConstructor.cpp:99 > + if (!collator) > + return JSValue::encode(jsUndefined()); It looks like this condition can never happen. IntlCollator::create() creates uses "new (NotNull, allocateCell<IntlCollator>(vm.heap))". That call crashes the process if we run out of memory. You could turn this into an ASSERT() for documentation purpose. > Source/JavaScriptCore/runtime/IntlCollatorConstructor.cpp:119 > + if (!collator) > + return JSValue::encode(jsUndefined()); Ditto. > Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:101 > + JSGlobalObject* globalObject = exec->callee()->globalObject(); It may look a bit cleaner to get the global object out of "this": collator->globalObject() > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:111 > + if (exec->hadException()) > + return JSValue::encode(jsUndefined()); This condition could already be tested. > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:120 > + if (!std::isfinite(value)) > + return JSValue::encode(throwRangeError(exec, String("date value is not finite in DateTimeFormat.format()"))); Could be tested too.

Comment on attachment 257030 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257030&action=review >> Source/JavaScriptCore/runtime/IntlCollator.cpp:103 >> + return JSValue::encode(jsUndefined()); > > I don't see any tests for this. > > Could you start a new file for the Collator. Move all the tests you have for Intl.Collator to it, and start testing behaviors? > > In this case, you can already check that: > -arg0.toString() is called before arg1.toString(). > -If arg0.toString() raises an exception, arg1.toString() and arg1.valueOf() are never invoked. > -If either arg0 or arg1 raise an exception, the return value is undefined. If an exception is thrown, how could I possibly check the return value? >> Source/JavaScriptCore/runtime/IntlCollatorConstructor.cpp:99 >> + return JSValue::encode(jsUndefined()); > > It looks like this condition can never happen. > > IntlCollator::create() creates uses "new (NotNull, allocateCell<IntlCollator>(vm.heap))". That call crashes the process if we run out of memory. > You could turn this into an ASSERT() for documentation purpose. I will switch all of these in constructors to ASSERT() statements. >> Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:101 >> + JSGlobalObject* globalObject = exec->callee()->globalObject(); > > It may look a bit cleaner to get the global object out of "this": > collator->globalObject() will do.

Created attachment 257436 [details] Patch

Comment on attachment 257436 [details] Patch Looks like the new test files didn't make it into the patch. I will update it.

Side note: I think this feature should be disabled by default until we have an implementation of the meat of the feature, which performs the internationalization. In general, we disable features by default until they are in a mostly working state, with just a few bugs left to fix.

Created attachment 257475 [details] Patch

Comment on attachment 257475 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257475&action=review > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:44 > +const ClassInfo IntlDateTimeFormat::s_info = { "Object", &Base::s_info, 0, CREATE_METHOD_TABLE(IntlDateTimeFormat) }; Ok, I am halfway through the review. So far so good. I'll start here next. Some test case ideas: > LayoutTests/js/script-tests/intl-collator.js:64 > +// 10.3.4 Collator Compare Functions Can you add a test verifying that if you instantiate 2 Collator, and verify the identity of their "compare()" function with "!=="? You can also compare them to Intl.Collator.prototype.compare. > LayoutTests/js/script-tests/intl-collator.js:80 > +shouldThrow("Intl.Collator.prototype.compare('a', { toString() { throw Error('8') } })", "'Error: 8'"); Can you please add coverage for call? Intl.Collator.prototype.compare.call(5, "foo", "bar") And same deal with an instance? > LayoutTests/js/script-tests/intl-collator.js:91 > + Maybe add Intl.Collator.prototype.resolvedOptions.call(5) or something like that? to test the "this".

Created attachment 257721 [details] Added more tests and more descriptive errors

Created attachment 257722 [details] rebase

Comment on attachment 257722 [details] rebase Missed a conflict.

Created attachment 257726 [details] Patch

Comment on attachment 257726 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257726&action=review I can't find any problem. Let's land !t! You are doing seriously high quality work. The code is very clear and well written. The comments are positioned to be easy to follow. Every time I wanted something tested, you already had a test for it. > Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:105 > + // 4. Else I would move this comment in the "} else {" branch.

Created attachment 257794 [details] Patch for landing

Comment on attachment 257794 [details] Patch for landing Rejecting attachment 257794 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 257794, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: numberformat-expected.txt patching file LayoutTests/js/intl-numberformat.html patching file LayoutTests/js/script-tests/intl-collator.js patching file LayoutTests/js/script-tests/intl-datetimeformat.js patching file LayoutTests/js/script-tests/intl-numberformat.js patching file LayoutTests/js/script-tests/intl.js patching file ChangeLog Hunk #1 succeeded at 1 with fuzz 3. Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/4658751164383232

Created attachment 257796 [details] Patch

Comment on attachment 257796 [details] Patch Clearing flags on attachment: 257796 Committed r187575: <http://trac.webkit.org/changeset/187575>

All reviewed patches have been landed. Closing bug.

(In reply to comment #35) > Comment on attachment 257796 [details] > Patch > > Clearing flags on attachment: 257796 > > Committed r187575: <http://trac.webkit.org/changeset/187575> It caused many jsc test failures, check the bots for details.

(In reply to comment #37) > (In reply to comment #35) > > Comment on attachment 257796 [details] > > Patch > > > > Clearing flags on attachment: 257796 > > > > Committed r187575: <http://trac.webkit.org/changeset/187575> > > It caused many jsc test failures, check the bots for details. Uh. Thanks for the notification. It looks like a difference in the harnesses.

Already fixed in https://trac.webkit.org/changeset/187581. Thanks.