Summary: | Implement ECMAScript Internationalization API | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jungshik Shin <jshin> | ||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Benjamin Poulain <benjamin> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | allan.jardine, andy, ap, barraclough, benjamin, cira, commit-queue, dak, darin, ddkilzer, dtrebbien, dustan.kasten, feliziani.emanuele, fishd, ggaren, mail.dennisbecker, marcalj, mickmon, mjs, patrick, phil, ray, rniwa, srl295, tkent, turadg, webkit-bug-importer, ysuzuki | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Bug Depends on: | 146926, 147599, 147601, 147602, 147603, 147604, 147605, 147606, 147607, 147608, 147609, 147610, 147611, 147612, 147613, 147614, 152448, 153679, 156993, 159693 | ||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||
Attachments: |
|
Description
Jungshik Shin
2012-07-10 13:47:57 PDT
(In reply to comment #0) > Moreover, some of HTML5 elements (e.g. <input type="date">) can be reimplemented with this API. Currently, some locale-specific data are embedded in the implementation (e.g. http://trac.webkit.org/browser/trunk/Source/WebCore/Resources/calendarPicker.js?rev=122212 ), which may not be scalable for tens of languages/locales. > > [2] http://code.google.com/p/v8-i18n I have never used v8-i18n, but does it apply OS's setting correctly? A locale data in ICU doesn't match to the system's locale data (Bug 84388), and ICU doesn't reflect user's customization; e.g. One uses "fr" locale, but he can change the system locale setting so that '.' is used for decimal separators. v8-i18n takes all of the settings from ICU, it doesn't use OS settings. It would be possible to implement that functionality using native system calls, but as the spec goes there is nothing in there that requires this feature (it is left to implementation to decide). re: comment ! The date picker etc has to follow the locale specified by 'lang/xml:lang'. If lang/xml:lang is not specified, which one to follow, the current UI language of a browser or the OS settings, is debatable. Currently, v8-i18n uses the current UI language. Ecma TC39 (in charge of EcmaScript standard) approved EcmaScript I18N API and referred it to Ecma GA (General Assembly) for the final approval. Chrome has this implemented under v8Intl namespace and just switched to Intl.* in ToT (will show up next dev channel as Intl.*). Mozilla will have it soon (their implementation will also use ICU). Microsoft has an ActiveX-based add-on. FYI: https://bugs.webkit.org/show_bug.cgi?id=137616 is a dup. *** Bug 137616 has been marked as a duplicate of this bug. *** When will this be implemented in WebKit / Safari? It is now 2015, it is a standard since December 2012. On caniuse.com you can see every other major browser vendor supports this API. There is a huge (~130kb) polyfill Intl.js to fill this gap which is really ugly for an up-to-date browser! Multi language websites really need such an implementation for localised dates, currencies or numbers. ECMAScript Internationalization API Specification http://www.ecma-international.org/ecma-402/1.0/ Can I Use? comparison internationalization (In reply to comment #8) > When will this be implemented in WebKit / Safari? It is now 2015, it is a > standard since December 2012. On by default in node.js as of 0.12 http://blog.nodejs.org/2015/02/06/node-v0-12-0-stable/ Dennis, (comment #8) You said "There is a huge (~130kb) polyfill Intl.js". Whose Intl.js is that? Would you have a pointer? Do you know if it implements collation? Anyone else, What about Webkit support? What alternates do you suggest for collating international data on Safari if no support is planed? Patrick, The polyfill is available at https://github.com/andyearnshaw/Intl.js/ It's about to get bigger https://github.com/andyearnshaw/Intl.js/pull/85 This polyfill does not implement Intl.Collator, and I haven't found any that do. Turadg, you probably won't find a native JS collation implementation. Collation data size (and complexity of the implementation) was the primary reason we started the ECMA 402 standardization process. Number and date formatting was just nice to have (other libraries already do that). Created attachment 253565 [details]
Patch
Does this seem like the right place for the Intl namespace to live? Collator, DateTimeFormat, and NumberFormat will be sibling files as they are created. Comment on attachment 253565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253565&action=review r- for the CMakeLists.txt. No real complain otherwise. > Source/JavaScriptCore/ChangeLog:9 > + Started work on implementing the Internationaization APIs by adding the feature flag > + (disabled until ready), and adding a global Intl object to serve as the namespace. It would be good to provide a link to the spec here for reference. > Source/JavaScriptCore/ChangeLog:16 > + * runtime/JSGlobalObject.cpp: There is one more build file that needs to know about JSGlobalObject.cpp: Source/JavaScriptCore/CMakeLists.txt CMake is used for GTK, EFL, and some Windows variant. > Source/WTF/ChangeLog:9 > + Started work on implementing the Internationaization APIs by adding the feature flag > + (disabled until ready), and adding a global Intl object to serve as the namespace. You don't need to repeat yourself in each changelog. Each file has the description for the changes in their particular subpart of the project. Created attachment 253799 [details]
Patch
I've tried adding the missing headers so I can use NumberFormat from ICU, but I get a build error in trying to find UnicodeString. You can see the error in the new patchset. Comment on attachment 253799 [details]
Patch
Generally we need to use the C API to ICU, not the C++ API. I think you’ll have better luck if you take that approach. The code here seems to be trying to use the C++.
ok. I will try that. (In reply to comment #18) > Comment on attachment 253799 [details] > Patch > > Generally we need to use the C API to ICU, not the C++ API. I think you’ll > have better luck if you take that approach. The code here seems to be trying > to use the C++. Comment on attachment 253799 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253799&action=review review- because EWS shows this is not complying on iOS or Mac yet; didn’t review in detail > Source/JavaScriptCore/icu/unicode/bytestream.h:48 > +class U_COMMON_API ByteSink : public UMemory { Please don’t add ICU headers for ICU’s C++ classes to the project. We cannot create a dependency on ICU’s C++ API; unlike the ICU C API it’s not guaranteed compatible across versions of the ICU framework. > Source/JavaScriptCore/icu/unicode/uobject.h:101 > +class U_COMMON_API UMemory { Pl Created attachment 253950 [details]
Patch
I'm afraid that I'm too frustrated to keep working on this in my limited free time. I hope my patch is at least a helpful start for someone who knows what they are doing in the webkit source. Sure, I agree that this is not a good “first WebKit patch” project. Usually better to start with something smaller. Created attachment 253997 [details]
Patch
If I can get help on the basic structure, building out the `intl` namespace and the objects in it, then I can work on getting it to have the right behavior. Right now, the patch compiles, but crashes immediately. It looks like I'm not allocating the memory for the NumberFormat properly. Can someone please take a look and offer any pointers? Created attachment 254051 [details]
Add feature flag and base Intl namespace object
Comment on attachment 254051 [details]
Add feature flag and base Intl namespace object
This latest patch compiles, and when the flag is enabled, adds the `intl` object. There are tests (currently skipped, but pass when flag is enabled) to ensure the object matches the spec.
Comment on attachment 254051 [details] Add feature flag and base Intl namespace object View in context: https://bugs.webkit.org/attachment.cgi?id=254051&action=review This is a good patch. Let's land it. I'll help Andy move forward with Intl, and I'll take on myself to remove the code if needed. > LayoutTests/js/script-tests/intl.js:17 > +// is deletable, inferred from use of "Initial" in spec, consistent with other implementations Something useful for testing is testing all the properties. You can use Object.getOwnPropertyDescritpor() to get them for any property. Comment on attachment 254051 [details] Add feature flag and base Intl namespace object Rejecting attachment 254051 [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-02', 'apply-attachment', '--no-update', '--non-interactive', 254051, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: /intl.html patching file LayoutTests/js/script-tests/intl.js patching file ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file WebKitLibraries/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file WebKitLibraries/win/tools/vsprops/FeatureDefines.props patching file WebKitLibraries/win/tools/vsprops/FeatureDefinesCairo.props Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Benjamin Poulain']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/4582240751190016 Created attachment 255874 [details]
Patch for landing
Created attachment 255886 [details]
Add namespace object and feature flag
Comment on attachment 255886 [details] Add namespace object and feature flag Clearing flags on attachment: 255886 Committed r186161: <http://trac.webkit.org/changeset/186161> All reviewed patches have been landed. Closing bug. The landed patch did not implement the whole API, just got the process started. This broke tests on the 32-bit JSC bot: https://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/11124/steps/webkit-32bit-jsc-test/logs/stdio Please fix or it will be rolled out. (In reply to comment #35) > This broke tests on the 32-bit JSC bot: > https://build.webkit.org/builders/Apple%20Mavericks%2032- > bit%20JSC%20%28BuildAndTest%29/builds/11124/steps/webkit-32bit-jsc-test/logs/ > stdio > > Please fix or it will be rolled out. The failed tests look like the new tests I added. I put in the TestExpectations to Skip these tests until the flag is enabled by default. Is there another way I can indicate these tests shouldn't be run? The failing tests are under LayoutTests/js/script-tests/, but I don't know how to disable these. Filip fixed it in http://trac.webkit.org/changeset/186166 Created attachment 257013 [details]
Patch
Attachment 257013 [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.
All of the dependent bugs are closed, and we report on the status page[1] that Intl is supported. Should we close this bug? [1] https://webkit.org/status/#specification-internationalization-api (In reply to Andy VanWagoner from comment #41) > All of the dependent bugs are closed, and we report on the status page[1] > that Intl is supported. Should we close this bug? > > [1] https://webkit.org/status/#specification-internationalization-api Intl.NumberFormat().formatToParts() is still not released in Safari (12.1 (14607.1.40.1.4)), though it is in the Tech Preview. This is the related bug: https://bugs.webkit.org/show_bug.cgi?id=185375 which is marked as RESOLVED FIXED, but it hasn't been released. Hope this message helps to bump up formatToParts. It looks like it has been forgotten. |