Bug 147601 - [INTL] Implement Intl.Collator.prototype.resolvedOptions ()
Summary: [INTL] Implement Intl.Collator.prototype.resolvedOptions ()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 150631 90906
  Show dependency treegraph
 
Reported: 2015-08-03 16:51 PDT by Andy VanWagoner
Modified: 2015-11-05 13:57 PST (History)
8 users (show)

See Also:


Attachments
Patch (58.90 KB, patch)
2015-10-07 06:32 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (634.47 KB, application/zip)
2015-10-07 07:16 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (725.37 KB, application/zip)
2015-10-07 07:23 PDT, Build Bot
no flags Details
Patch (51.17 KB, patch)
2015-10-08 00:11 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (52.65 KB, patch)
2015-10-20 04:00 PDT, Sukolsak Sakshuwong
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-08-03 16:51:24 PDT
Implement ECMA-402 2.0 10.3.5 Intl.Collator.prototype.resolvedOptions ()
http://ecma-international.org/publications/standards/Ecma-402.htm

This function provides access to the locale and collation options computed during initialization of the object.

The function returns a new object whose properties and attributes are set as if constructed by an object literal assigning to each of the following properties the value of the corresponding internal slot of this Collator object (see 10.4): locale, usage, sensitivity, ignorePunctuation, collation, as well as those properties shown in Table 1 whose keys are included in the %Collator%[[relevantExtensionKeys]] internal slot of the standard built-in object that is the initial value of Intl.Collator.
Comment 1 Sukolsak Sakshuwong 2015-10-07 06:32:29 PDT
Created attachment 262598 [details]
Patch
Comment 2 Build Bot 2015-10-07 07:16:19 PDT
Comment on attachment 262598 [details]
Patch

Attachment 262598 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/254085

New failing tests:
js/intl-collator.html
Comment 3 Build Bot 2015-10-07 07:16:23 PDT
Created attachment 262602 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 4 Build Bot 2015-10-07 07:22:58 PDT
Comment on attachment 262598 [details]
Patch

Attachment 262598 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/254090

New failing tests:
js/intl-collator.html
Comment 5 Build Bot 2015-10-07 07:23:01 PDT
Created attachment 262603 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 6 Sukolsak Sakshuwong 2015-10-08 00:11:17 PDT
Created attachment 262678 [details]
Patch

Make the tests more tolerant of older versions of ICU.
Comment 7 Benjamin Poulain 2015-10-19 16:12:03 PDT
Comment on attachment 262678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262678&action=review

I did not find any error. Some minor comments:

> Source/JavaScriptCore/ChangeLog:3
> +        [INTL] Implement Intl.Collator.prototype.resolvedOptions ()

Don't forget to add your copyright on the files with significant changes.

> Source/JavaScriptCore/runtime/IntlCollator.h:74
> +    bool m_numeric;
> +    String m_sensitivity;
> +    bool m_ignorePunctuation;

Let's move the booleans together to improve packing a bit.

> Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:123
>      // The function returns a new object whose properties and attributes are set as if constructed by an object literal assigning to each of the following properties the value of the corresponding internal slot of this Collator object (see 10.4): locale, usage, sensitivity, ignorePunctuation, collation, as well as those properties shown in Table 1 whose keys are included in the %Collator%[[relevantExtensionKeys]] internal slot of the standard built-in object that is the initial value of Intl.Collator.

Can you please break this comment over multiple lines?

> Source/JavaScriptCore/runtime/IntlObject.cpp:631
> +    String locale, noExtensionsLocale;

Please move noExtensionsLocale to its own line. One statement per declaration.

> Source/JavaScriptCore/runtime/IntlObject.cpp:646
> +            ASSERT(extensionIndex != notFound);

Let's make that a RELEASE_ASSERT().

I don't like the idea of a notFound accidentally being used for length computation.
Comment 8 Sukolsak Sakshuwong 2015-10-20 04:00:16 PDT
Created attachment 263567 [details]
Patch
Comment 9 Sukolsak Sakshuwong 2015-10-20 04:06:57 PDT
Thanks for the review!

(In reply to comment #7)
> Don't forget to add your copyright on the files with significant changes.

Added my copyright to IntlCollatorConstructor.cpp and IntlObject.cpp. These are the source files that I add more than 100 lines.

> > Source/JavaScriptCore/runtime/IntlCollator.h:74
> > +    bool m_numeric;
> > +    String m_sensitivity;
> > +    bool m_ignorePunctuation;
> 
> Let's move the booleans together to improve packing a bit.

Done.

> > Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp:123
> >      // The function returns a new object whose properties and attributes are set as if constructed by an object literal assigning to each of the following properties the value of the corresponding internal slot of this Collator object (see 10.4): locale, usage, sensitivity, ignorePunctuation, collation, as well as those properties shown in Table 1 whose keys are included in the %Collator%[[relevantExtensionKeys]] internal slot of the standard built-in object that is the initial value of Intl.Collator.
> 
> Can you please break this comment over multiple lines?

Done.

> > Source/JavaScriptCore/runtime/IntlObject.cpp:631
> > +    String locale, noExtensionsLocale;
> 
> Please move noExtensionsLocale to its own line. One statement per
> declaration.

Done.

> > Source/JavaScriptCore/runtime/IntlObject.cpp:646
> > +            ASSERT(extensionIndex != notFound);
> 
> Let's make that a RELEASE_ASSERT().
> 
> I don't like the idea of a notFound accidentally being used for length
> computation.

Done.
Comment 10 Benjamin Poulain 2015-10-21 13:58:11 PDT
Comment on attachment 263567 [details]
Patch

You just updated from the comments right? No need for a second review in this case.
Comment 11 WebKit Commit Bot 2015-10-21 15:15:20 PDT
Comment on attachment 263567 [details]
Patch

Clearing flags on attachment: 263567

Committed r191406: <http://trac.webkit.org/changeset/191406>
Comment 12 WebKit Commit Bot 2015-10-21 15:15:25 PDT
All reviewed patches have been landed.  Closing bug.