Bug 49414 – Implement ECMAScript I18N APIs (proposed)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jungshik Shin]

We proposed a set of APIs for I18N to the ECMA / TC39 (ECMAscript standard wg). The details of our proposal is at http://wiki.ecmascript.org/doku.php?id=strawman:i18n_api

Our proposal was well-received in that we got a green light to start experimental/pilot implementation. The APIs are not yet finalized and subject to changes per on-going discussion at TC39 (next meeting will be held at Apple on November 16 which will be attended by Google, Microsoft, Adobe, Mozilla and W3C I18N WG representatives). 

In the first round, we'll start to implement the APIs as a V8 extension because it's easy for us  to prototype and test. 

BTW, this started as a chromium bug (http://crbug.com/28604 ) .

[Nebojša Ćirić]

Created attachment 73681 [details]
First patch to implement Locale class of the JS i18n API

[Darin Adler]

Comment on attachment 73681 [details]
First patch to implement Locale class of the JS i18n API

ECMAScript features should be in the JavaScript engine, not in WebCore.

[Nebojša Ćirić]

The i18n part is treated as an extension to ECMAScript, and it will have separate standard (so EcmaScript 262 stays unchanged wrt to i18n). This is a library so it makes sense to implement it as a extension not as a core part of the JS engine.

(In reply to comment #2)
> (From update of attachment 73681 [details])
> ECMAScript features should be in the JavaScript engine, not in WebCore.

[Sam Weinig]

(In reply to comment #3)
> The i18n part is treated as an extension to ECMAScript, and it will have separate standard (so EcmaScript 262 stays unchanged wrt to i18n). This is a library so it makes sense to implement it as a extension not as a core part of the JS engine.

In that case, why isn't this implemented like all the bound APIs, with a C++ implementation and auto generated bindings based on IDL?

[WebKit Review Bot]

Attachment 73681 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5666007

[Nebojša Ćirić]

(In reply to comment #4)
> (In reply to comment #3)
> > The i18n part is treated as an extension to ECMAScript, and it will have separate standard (so EcmaScript 262 stays unchanged wrt to i18n). This is a library so it makes sense to implement it as a extension not as a core part of the JS engine.
> 
> In that case, why isn't this implemented like all the bound APIs, with a C++ implementation and auto generated bindings based on IDL?

Not all of the APIs are implemented through IDL based bindings (see WebCore/bindings/v8/DateExtension.{cpp,h} for example). Darin (fishd) thought that would be a good path for us too.

We actually started with IDL implementation, hit couple problems with it, and asked for help on webkit-dev. Jeremy Orlow (jorlow) pointed out that V8 Extensions my be better/easier way to implement the library.

I'll let them add comments.

[Alexey Proskuryakov]

> The i18n part is treated as an extension to ECMAScript, and it will have separate standard
> (so EcmaScript 262 stays unchanged wrt to i18n). 

I don't see how this makes it appropriate to implement i18n in WebCore. Applications can link to JavaScriptCore alone, so anything that's not specific to DOM bindings should be there.

> http://wiki.ecmascript.org/doku.php?id=strawman:i18n_api

Implementations of "strawman proposals" are generally not permitted in webkit.org trunk per <http://webkit.org/projects/goals.html>. This seems to be at the stage where a private branch would probably be appropriate - please correct me if I'm wrong about the level of existing agreement.

[Nebojša Ćirić]

> > http://wiki.ecmascript.org/doku.php?id=strawman:i18n_api
> 
> Implementations of "strawman proposals" are generally not permitted in webkit.org trunk per <http://webkit.org/projects/goals.html>. This seems to be at the stage where a private branch would probably be appropriate - please correct me if I'm wrong about the level of existing agreement.

Level of agreement:

Strawman proposal was accepted, and new group was formed - we have support from ECMAScript wg. We are going to have our first meeting next week, where we are going to discuss the API among other things.

API is not agreed upon. Our goal was to have a working prototype that others could use to discuss and comment on (thus the flag and V8 only implementation).

[Darin Fisher (:fishd, Google)]

I proposed adding this as a V8 extension because:

Once standardized, this would be implemented natively by V8 and I presume JSC.  If we implement it using IDL, then it would not be available when V8/JSC are used standalone.

I think it is reasonable to implement proposed APIs behind a run-time flag, and only bring them out from behind the flag once they are stable / accepted.

[Oliver Hunt]

(In reply to comment #9)
> I proposed adding this as a V8 extension because:
> 
> Once standardized, this would be implemented natively by V8 and I presume JSC.  If we implement it using IDL, then it would not be available when V8/JSC are used standalone.
> 
> I think it is reasonable to implement proposed APIs behind a run-time flag, and only bring them out from behind the flag once they are stable / accepted.

If it's being implemented in v8, then it shouldn't be in the webkit tree, if it's being implemented inside webcore (as it is currently) it should be implemented using the idl bindings.

[Darin Fisher (:fishd, Google)]

(In reply to comment #10)
> (In reply to comment #9)
> > I proposed adding this as a V8 extension because:
> > 
> > Once standardized, this would be implemented natively by V8 and I presume JSC.  If we implement it using IDL, then it would not be available when V8/JSC are used standalone.
> > 
> > I think it is reasonable to implement proposed APIs behind a run-time flag, and only bring them out from behind the flag once they are stable / accepted.
> 
> If it's being implemented in v8, then it shouldn't be in the webkit tree, if it's being implemented inside webcore (as it is currently) it should be implemented using the idl bindings.

Are you saying that standalone JSC would never have an implementation of these APIs?

[Oliver Hunt]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > I proposed adding this as a V8 extension because:
> > > 
> > > Once standardized, this would be implemented natively by V8 and I presume JSC.  If we implement it using IDL, then it would not be available when V8/JSC are used standalone.
> > > 
> > > I think it is reasonable to implement proposed APIs behind a run-time flag, and only bring them out from behind the flag once they are stable / accepted.
> > 
> > If it's being implemented in v8, then it shouldn't be in the webkit tree, if it's being implemented inside webcore (as it is currently) it should be implemented using the idl bindings.
> 
> Are you saying that standalone JSC would never have an implementation of these APIs?

No I'm saying if the experimental implementation isn't going to be implemented with the IDL bindings logic (so unnecessarily tying it to v8 alone) there's no reason for the experimental impl to be in the webkit tree.  The final implementations are going to be in their respective engines (assuming it gets standardised, etc, etc) which means a webcore level impl that only targets a single engine (either engine) seems to be a poor design decision.  A much better approach would be to simply implement it in the engine, then make spec changes later as needed.  This current approach has no benefit that i can see.

If this approach were made using the idl binding mechanism then two engines would be able to expose the api to users which would arguably be valuable in the short term, even though it would need to be reimplemented at the ES engine level later.

Making a single engine only implementation at the webcore level does not give you that advantage, but still carries with it the cost of needing to repeat the implementation.

I think the two approaches that make sense are:
* Implement it in WebCore using the IDL binding mechanism so that users of two different browsers can experiment with the API, and take the cost of reimplementing it in the ES engines later if so desired.
* Implement it in the engine (in this case V8) in the first place.  This loses nothing over what is currently being proposed, but means we're not littering webcore with functionality for a single engine, and not going to have to remove and reimplement it at a later date.

I also have a concern that this initial implementation could just be left in webkit as it would "just work" for the purpose of the browser, so not benefiting anyone simply using the ES engine alone, but also not being available to all webkit implementations.

[Jeremy Orlow]

Ughhh.  I tried to post this earlier right after Sam's comment, but apparently the internet failed and it never posted.  I still think it's 100% relevant.

(In reply to comment #4)
> (In reply to comment #3)
> > The i18n part is treated as an extension to ECMAScript, and it will have separate standard (so EcmaScript 262 stays unchanged wrt to i18n). This is a library so it makes sense to implement it as a extension not as a core part of the JS engine.
> 
> In that case, why isn't this implemented like all the bound APIs, with a C++ implementation and auto generated bindings based on IDL?

I think the 3 possible options for implementing this are:
1) in WebCore via IDL files and normal binding methods
2) in V8/JSC directly
3) in Chromium with v8 extensions

This patch seems to be v8 extensions + WebCore which probably doesn't make sense.

As for what the proper approach is, I think that Nebojša Ćirić has now been told that each one of these is definitely the way it should be done.  Does everyone on this bug agree with Sam (and thus option #1 is the correct way to do it)?

[Darin Fisher (:fishd, Google)]

(In reply to comment #13)
> Ughhh.  I tried to post this earlier right after Sam's comment, but apparently the internet failed and it never posted.  I still think it's 100% relevant.
> 
> (In reply to comment #4)
> > (In reply to comment #3)
> > > The i18n part is treated as an extension to ECMAScript, and it will have separate standard (so EcmaScript 262 stays unchanged wrt to i18n). This is a library so it makes sense to implement it as a extension not as a core part of the JS engine.
> > 
> > In that case, why isn't this implemented like all the bound APIs, with a C++ implementation and auto generated bindings based on IDL?
> 
> I think the 3 possible options for implementing this are:
> 1) in WebCore via IDL files and normal binding methods
> 2) in V8/JSC directly
> 3) in Chromium with v8 extensions
> 
> This patch seems to be v8 extensions + WebCore which probably doesn't make sense.
> 
> As for what the proper approach is, I think that Nebojša Ćirić has now been told that each one of these is definitely the way it should be done.  Does everyone on this bug agree with Sam (and thus option #1 is the correct way to do it)?

I objected to option #3 when I saw it appear in the Chromium code review tool because that would not allow us to test the feature using layout tests.  Also, web platform features really belong in either WebCore or the ES engine.

I proposed a V8 extension because I presumed that JSC folks would want to do a native JSC implementation instead of bothering with an intermediate IDL implementation.

The reason for a V8 extension over directly modifying V8 is that the V8 team does not like to add features to the V8 engine until the specs are stable.  As such, for V8 we only have two choices: implement using a V8 extension or implement using IDL.  This means having to re-implement later once the V8 team is ready to support the APIs officially, but that is a cost we are willing to accept.

So, it seems like there are only two choices:

1) in WebCore via IDL files and normal binding methods
2) in WebCore as a V8 extension and JSC directly

I assumed that #2 would be preferred by JSC folks since it would allow the APIs to be available to users of standalone JSC and it would also avoid the need to re-implement in the future.

[Nebojša Ćirić]

(In reply to comment #14)
> (In reply to comment #13)
> > Ughhh.  I tried to post this earlier right after Sam's comment, but apparently the internet failed and it never posted.  I still think it's 100% relevant.
> > 
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > The i18n part is treated as an extension to ECMAScript, and it will have separate standard (so EcmaScript 262 stays unchanged wrt to i18n). This is a library so it makes sense to implement it as a extension not as a core part of the JS engine.
> > > 
> > > In that case, why isn't this implemented like all the bound APIs, with a C++ implementation and auto generated bindings based on IDL?
> > 
> > I think the 3 possible options for implementing this are:
> > 1) in WebCore via IDL files and normal binding methods
> > 2) in V8/JSC directly
> > 3) in Chromium with v8 extensions
> > 
> > This patch seems to be v8 extensions + WebCore which probably doesn't make sense.
> > 
> > As for what the proper approach is, I think that Nebojša Ćirić has now been told that each one of these is definitely the way it should be done.  Does everyone on this bug agree with Sam (and thus option #1 is the correct way to do it)?
> 
> I objected to option #3 when I saw it appear in the Chromium code review tool because that would not allow us to test the feature using layout tests.  Also, web platform features really belong in either WebCore or the ES engine.
> 
> I proposed a V8 extension because I presumed that JSC folks would want to do a native JSC implementation instead of bothering with an intermediate IDL implementation.
> 
> The reason for a V8 extension over directly modifying V8 is that the V8 team does not like to add features to the V8 engine until the specs are stable.  As such, for V8 we only have two choices: implement using a V8 extension or implement using IDL.  This means having to re-implement later once the V8 team is ready to support the APIs officially, but that is a cost we are willing to accept.
> 
> So, it seems like there are only two choices:
> 
> 1) in WebCore via IDL files and normal binding methods
> 2) in WebCore as a V8 extension and JSC directly
> 
> I assumed that #2 would be preferred by JSC folks since it would allow the APIs to be available to users of standalone JSC and it would also avoid the need to re-implement in the future.

Oliver, Darin, Sam,
 given Darin's (Fisher) and Jeremy's comments, do you still feel we should go with IDL implementation or does what we have now (V8 Extensions) makes sense for short term implementation?

Thanks for your comments.

[Oliver Hunt]

> Oliver, Darin, Sam,
>  given Darin's (Fisher) and Jeremy's comments, do you still feel we should go with IDL implementation or does what we have now (V8 Extensions) makes sense for short term implementation?
> 
> Thanks for your comments.

If the code is in webcore it should be in idl, using the standard bindings mechanism.

[Nebojša Ćirić]

Created attachment 78264 [details]
New patch for i18n API.

Code for the extension is in v8 now - src/extensions/experimental/i18n-extension.{cc,h}. WebKit patch contains only plumbing for a flag to enable/disable the feature (disabled by default), tests for the Locale object and build rules.

[Jungshik Shin]

Created attachment 78450 [details]
patch updated to the new source tree structure 

Uploading on behalf of cira so that all the try bots run.

[Nebojša Ćirić]

To clarify the new set of patches - Maciej, Darin Fisher and Oliver had short discussion outside of this bug thread, and I think we came up with acceptable solution (at least nobody complained when it was proposed):

1- We will contribute layout tests for the new feature in WebKit. - this patch
2- We will enable support for V8 outside of the WebKit repository. - this patch
3- We will contribute JSC support.

#2 and #3 will be optional (disabled by default) since the spec is not finalized.

Flag control and extension loading could be done in Chromium, but we feel it would be beneficial to other ports to have it in WebKit (to avoid duplicate work in the future).

[Darin Fisher (:fishd, Google)]

Comment on attachment 78450 [details]
patch updated to the new source tree structure 

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

> Source/WebCore/WebCore.gyp/WebCore.gyp:1404
> +            'USING_EXPERIMENTAL_I18N_API=1',

I think this needs to be a WTF_ENABLE_ style macro, controlled via features.gypi (and features_override.gypi).

> Source/WebCore/WebCore.gyp/WebCore.gyp:1498
> +      'target_name': 'experimental_i18n_api',

It seems like this target should live in v8.gyp.

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:65
> +#ifdef USING_EXPERIMENTAL_I18N_API

#if ENABLE(EXPERIMENTAL_I18N_API)

> Source/WebCore/page/Settings.h:356
> +        void setJSI18NAPIEnabled(bool flag) { m_jsI18NAPIEnabled = flag; }

I think this may be better added to WebRuntimeFeatures since that is how we
enable other, similar features at runtime.  We don't need to vary this flag
on a per-Page basis, so storing this on Settings is not necessary.

[Nebojša Ćirić]

> > Source/WebCore/WebCore.gyp/WebCore.gyp:1498
> > +      'target_name': 'experimental_i18n_api',
> 
> It seems like this target should live in v8.gyp.

V8 team let us put the experimental code in their tree (thanks for helping out with that), but they were against changing the build rules - they already refused to do so for some Harmony features, and they felt it wouldn't be fair if they made an exception in our case.

[Nebojša Ćirić]

Created attachment 80086 [details]
Using WebRuntimeFeatures instead of WebSettings for i18n API

We are using features.gypi to define conditional ENABLE_EXPERIMENTAL_I18N_API guard, and WebRuntimeFeatures to pass the command line settings.

CL (http://codereview.chromium.org/6250025/) was landed in v8 tree and should propagate to WebKit deps before this patch gets submitted.

[WebKit Review Bot]

Attachment 80086 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7552298

[Darin Fisher (:fishd, Google)]

Comment on attachment 80086 [details]
Using WebRuntimeFeatures instead of WebSettings for i18n API

oops, this is not ready to land yet.  we need to update the version of v8 used, first.

[Oliver Hunt]

I'm concerned about the absence of any prefix on these, given the early stages of the spec i'm worried we may have problems with changes in the final version being incompatible with these.

[Darin Fisher (:fishd, Google)]

Comment on attachment 80086 [details]
Using WebRuntimeFeatures instead of WebSettings for i18n API

Good point!  Cira, sorry to add another round trip, but we really should add
a vendor prefix to these methods.  I suggest using "v8" as the prefix since
these are part of V8 and not part of WebKit, yet.

[Nebojša Ćirić]

I'll add v8 prefix to Locale object (all methods and properties are under Locale global). Prefixing everything with v8 may be an overkill.
I'll ping you again once v8 change lands.

(In reply to comment #26)
> (From update of attachment 80086 [details])
> Good point!  Cira, sorry to add another round trip, but we really should add
> a vendor prefix to these methods.  I suggest using "v8" as the prefix since
> these are part of V8 and not part of WebKit, yet.

[Darin Fisher (:fishd, Google)]

(In reply to comment #27)
> I'll add v8 prefix to Locale object (all methods and properties are under Locale global). Prefixing everything with v8 may be an overkill.
> I'll ping you again once v8 change lands.

Yes, you only need to prefix the top-level Locale object.

[Xiaomei Ji]

Created attachment 81250 [details]
patch w/ layout test

On behalf of Cira.

Difference from 80086:
Locale class renamed to v8Locale.
Command line options and define guard renamed from ExperimentalI18NAPI to JavaScriptI18NAPI.

[Nebojša Ćirić]

I made the v8 changes, they landed in WebKit tree/deps... Any other concerns?

(In reply to comment #29)
> Created an attachment (id=81250) [details]
> patch w/ layout test
> 
> On behalf of Cira.
> 
> Difference from 80086:
> Locale class renamed to v8Locale.
> Command line options and define guard renamed from ExperimentalI18NAPI to JavaScriptI18NAPI.

(In reply to comment #29)
> Created an attachment (id=81250) [details]
> patch w/ layout test
> 
> On behalf of Cira.
> 
> Difference from 80086:
> Locale class renamed to v8Locale.
> Command line options and define guard renamed from ExperimentalI18NAPI to JavaScriptI18NAPI.

[WebKit Commit Bot]

Comment on attachment 81250 [details]
patch w/ layout test

Clearing flags on attachment: 81250

Committed r78095: <http://trac.webkit.org/changeset/78095>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Zhenyao Mo]

fast/js/i18n-bindings-locale.html is failing on all chromium canary bots.  Please have a look.

[Zhenyao Mo]

Just added the test to platform/chromium/test_expectations.txt.  Please remove this entry once the issue is resolved.

[Nebojša Ćirić]

Created attachment 104536 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 104536 [details]
Patch

I think you should file a new bug for this patch.