Implement basic types for ECMAScript Internationalization API

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy VanWagoner]

Implement basic types for ECMAScript Internationalization APII

[Andy VanWagoner]

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

[WebKit Commit Bot]

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.

[Andy VanWagoner]

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

[Andy VanWagoner]

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.

[Andy VanWagoner]

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?

[Benjamin Poulain]

(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 :(

[Andy VanWagoner]

(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?

[Benjamin Poulain]

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.

[Benjamin Poulain]

(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.

[Benjamin Poulain]

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

[Andy VanWagoner]

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.

[Andy VanWagoner]

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

[WebKit Commit Bot]

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.

[Andy VanWagoner]

Created attachment 257030 [details]
Patch

[WebKit Commit Bot]

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.

[Andy VanWagoner]

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

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

http://trac.webkit.org/changeset/187142

NewTarget from C++ world is supported!

[Andy VanWagoner]

(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.

[Benjamin Poulain]

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.

[Andy VanWagoner]

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.

[Andy VanWagoner]

Created attachment 257436 [details]
Patch

[Andy VanWagoner]

Comment on attachment 257436 [details]
Patch

Looks like the new test files didn't make it into the patch. I will update it.

[Geoffrey Garen]

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.

[Andy VanWagoner]

Created attachment 257475 [details]
Patch

[Benjamin Poulain]

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".

[Andy VanWagoner]

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

[Andy VanWagoner]

Created attachment 257722 [details]
rebase

[Andy VanWagoner]

Comment on attachment 257722 [details]
rebase

Missed a conflict.

[Andy VanWagoner]

Created attachment 257726 [details]
Patch

[Benjamin Poulain]

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.

[Benjamin Poulain]

Created attachment 257794 [details]
Patch for landing

[WebKit Commit Bot]

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

[Benjamin Poulain]

Created attachment 257796 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 257796 [details]
Patch

Clearing flags on attachment: 257796

Committed r187575: <http://trac.webkit.org/changeset/187575>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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.

[Benjamin Poulain]

(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.

[Csaba Osztrogonác]

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