Bug 146926 - Implement basic types for ECMAScript Internationalization API
Summary: Implement basic types for ECMAScript Internationalization API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy VanWagoner
URL:
Keywords:
Depends on:
Blocks: 90906
  Show dependency treegraph
 
Reported: 2015-07-13 19:11 PDT by Andy VanWagoner
Modified: 2015-07-30 04:56 PDT (History)
7 users (show)

See Also:


Attachments
Add Collator, NumberFormat, and DateTimeFormat skeletons. (151.71 KB, patch)
2015-07-13 19:51 PDT, Andy VanWagoner
no flags Details | Formatted Diff | Diff
updated comments, enabled more flags (150.19 KB, patch)
2015-07-17 19:25 PDT, Andy VanWagoner
no flags Details | Formatted Diff | Diff
Patch (150.29 KB, patch)
2015-07-18 07:42 PDT, Andy VanWagoner
no flags Details | Formatted Diff | Diff
Patch (146.08 KB, patch)
2015-07-23 22:35 PDT, Andy VanWagoner
no flags Details | Formatted Diff | Diff
Patch (168.77 KB, patch)
2015-07-24 13:58 PDT, Andy VanWagoner
no flags Details | Formatted Diff | Diff
Added more tests and more descriptive errors (176.80 KB, patch)
2015-07-28 19:06 PDT, Andy VanWagoner
no flags Details | Formatted Diff | Diff
rebase (177.00 KB, patch)
2015-07-28 19:10 PDT, Andy VanWagoner
no flags Details | Formatted Diff | Diff
Patch (176.86 KB, patch)
2015-07-28 20:05 PDT, Andy VanWagoner
no flags Details | Formatted Diff | Diff
Patch for landing (176.93 KB, patch)
2015-07-29 18:00 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (176.89 KB, patch)
2015-07-29 18:43 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy VanWagoner 2015-07-13 19:11:37 PDT
Implement basic types for ECMAScript Internationalization APII
Comment 1 Andy VanWagoner 2015-07-13 19:51:18 PDT
Created attachment 256755 [details]
Add Collator, NumberFormat, and DateTimeFormat skeletons.
Comment 2 WebKit Commit Bot 2015-07-13 19:53:17 PDT
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.
Comment 3 Andy VanWagoner 2015-07-13 19:55:56 PDT
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
Comment 4 Andy VanWagoner 2015-07-13 19:57:26 PDT
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.
Comment 5 Andy VanWagoner 2015-07-14 20:08:05 PDT
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?
Comment 6 Benjamin Poulain 2015-07-15 14:00:23 PDT
(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 :(
Comment 7 Andy VanWagoner 2015-07-15 14:55:21 PDT
(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 8 Benjamin Poulain 2015-07-15 18:16:04 PDT
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.
Comment 9 Benjamin Poulain 2015-07-15 18:22:42 PDT
(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.
Comment 10 Benjamin Poulain 2015-07-15 18:26:03 PDT
Chris, this patch fails to build with CMake. The generated file sections looks good to me. Any idea what is missing for CMake?
Comment 11 Andy VanWagoner 2015-07-15 19:45:41 PDT
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.
Comment 12 Andy VanWagoner 2015-07-17 19:25:06 PDT
Created attachment 257016 [details]
updated comments, enabled more flags
Comment 13 WebKit Commit Bot 2015-07-17 19:27:46 PDT
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.
Comment 14 Andy VanWagoner 2015-07-18 07:42:25 PDT
Created attachment 257030 [details]
Patch
Comment 15 WebKit Commit Bot 2015-07-18 07:45:44 PDT
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.
Comment 16 Andy VanWagoner 2015-07-18 14:06:37 PDT
Any help on how to fix the Windows linking errors would be much appreciated.
Comment 17 Yusuke Suzuki 2015-07-21 13:24:33 PDT
(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.
Comment 18 Yusuke Suzuki 2015-07-21 17:35:50 PDT
http://trac.webkit.org/changeset/187142

NewTarget from C++ world is supported!
Comment 19 Andy VanWagoner 2015-07-23 08:58:58 PDT
(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 20 Benjamin Poulain 2015-07-23 20:46:52 PDT
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 21 Andy VanWagoner 2015-07-23 22:10:09 PDT
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.
Comment 22 Andy VanWagoner 2015-07-23 22:35:19 PDT
Created attachment 257436 [details]
Patch
Comment 23 Andy VanWagoner 2015-07-24 10:35:12 PDT
Comment on attachment 257436 [details]
Patch

Looks like the new test files didn't make it into the patch. I will update it.
Comment 24 Geoffrey Garen 2015-07-24 10:47:10 PDT
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.
Comment 25 Andy VanWagoner 2015-07-24 13:58:22 PDT
Created attachment 257475 [details]
Patch
Comment 26 Benjamin Poulain 2015-07-27 21:08:41 PDT
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".
Comment 27 Andy VanWagoner 2015-07-28 19:06:55 PDT
Created attachment 257721 [details]
Added more tests and more descriptive errors
Comment 28 Andy VanWagoner 2015-07-28 19:10:12 PDT
Created attachment 257722 [details]
rebase
Comment 29 Andy VanWagoner 2015-07-28 19:14:20 PDT
Comment on attachment 257722 [details]
rebase

Missed a conflict.
Comment 30 Andy VanWagoner 2015-07-28 20:05:10 PDT
Created attachment 257726 [details]
Patch
Comment 31 Benjamin Poulain 2015-07-29 17:31:50 PDT
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.
Comment 32 Benjamin Poulain 2015-07-29 18:00:21 PDT
Created attachment 257794 [details]
Patch for landing
Comment 33 WebKit Commit Bot 2015-07-29 18:02:56 PDT
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
Comment 34 Benjamin Poulain 2015-07-29 18:43:50 PDT
Created attachment 257796 [details]
Patch
Comment 35 WebKit Commit Bot 2015-07-29 20:35:01 PDT
Comment on attachment 257796 [details]
Patch

Clearing flags on attachment: 257796

Committed r187575: <http://trac.webkit.org/changeset/187575>
Comment 36 WebKit Commit Bot 2015-07-29 20:35:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 37 Csaba Osztrogonác 2015-07-29 22:09:50 PDT
(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.
Comment 38 Benjamin Poulain 2015-07-30 01:28:03 PDT
(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.
Comment 39 Csaba Osztrogonác 2015-07-30 04:56:37 PDT
Already fixed in https://trac.webkit.org/changeset/187581. Thanks.