Bug 147601

Summary:

[INTL] Implement Intl.Collator.prototype.resolvedOptions ()

Product:

WebKit

Reporter:

Andy VanWagoner <andy>

Component:

JavaScriptCore

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, buildbot, commit-queue, ggaren, rniwa, saam, sukolsak, ysuzuki

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=150943

Bug Depends on:

Bug Blocks:

150631, 90906

Attachments:

Description	Flags
Patch	none
Archive of layout-test-results from ews100 for mac-mavericks	none
Archive of layout-test-results from ews104 for mac-mavericks-wk2	none
Patch	none
Patch	none

Andy VanWagoner

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

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Sukolsak Sakshuwong

Comment 1 2015-10-07 06:32:29 PDT

Created attachment 262598 [details] Patch

Build Bot

Comment 2 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

Build Bot

Comment 3 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

Build Bot

Comment 4 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

Build Bot

Comment 5 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

Sukolsak Sakshuwong

Comment 6 2015-10-08 00:11:17 PDT

Created attachment 262678 [details] Patch Make the tests more tolerant of older versions of ICU.

Benjamin Poulain

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

Sukolsak Sakshuwong

Comment 8 2015-10-20 04:00:16 PDT

Created attachment 263567 [details] Patch

Sukolsak Sakshuwong

Comment 9 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.

Benjamin Poulain

Comment 10 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.

WebKit Commit Bot

Comment 11 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>

WebKit Commit Bot

Comment 12 2015-10-21 15:15:25 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.