[i18n] use Polymer data binding to change locale strings

[i18n] use Polymer data binding to change locale strings

This CL uses Polymer variable binding to swap out i18n strings rather
than using i18n-content.

BUG=692763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2886843005
Cr-Commit-Position: refs/heads/master@{#476358}
Committed: https://chromium.googlesource.com/chromium/src/+/cd00499d569a693685c81d4042b3cfe3219edd61

[dschuyler, 2017-05-17 23:50:50]

Description was changed from

==========
[i18n] use Polymer data binding to change locale strings

This CL uses Polymer variable binding to swap out i18n strings rather
than using i18n-content.

BUG=692763
==========

to

==========
[i18n] use Polymer data binding to change locale strings

This CL uses Polymer variable binding to swap out i18n strings rather
than using i18n-content.

BUG=692763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2017-05-17 23:50:52]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 23:51:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-05-17 23:58:23]

dschuyler@chromium.org changed reviewers:
+ dbeam@chromium.org

[dschuyler, 2017-05-17 23:58:23]

wdyt?

[Dan Beam, 2017-05-18 00:04:57]

i think this would generally be fine but you should get an OOBE owner like
achuith@ or alemate@ to look

https://codereview.chromium.org/2886843005/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_welcome.js (left):

https://codereview.chromium.org/2886843005/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_welcome.js:142: var screens =
Polymer.dom(this.root).querySelectorAll('oobe-dialog')
this line should've ended with a ;

https://codereview.chromium.org/2886843005/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_welcome.js (right):

https://codereview.chromium.org/2886843005/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_welcome.js:87: return {};
set this only when ready, IMO

https://codereview.chromium.org/2886843005/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_welcome.js:161: ];
var i18n = {};
i18nTags.forEach(function(tag) {
  i18n[tag] = loadTimeData.getString(tag);
});
this.i18n_ = i18n;

https://codereview.chromium.org/2886843005/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_welcome.js:198: var screens =
Polymer.dom(this.root).querySelectorAll(
same

[dschuyler, 2017-05-18 00:46:57]

>i think this would generally be fine but you should 
>get an OOBE owner like achuith@ or alemate@ to look

Yep, will do.

https://codereview.chromium.org/2886843005/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_welcome.js (left):

https://codereview.chromium.org/2886843005/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_welcome.js:142: var screens =
Polymer.dom(this.root).querySelectorAll('oobe-dialog')
On 2017/05/18 00:04:57, Dan Beam wrote:
> this line should've ended with a ;

Done.

https://codereview.chromium.org/2886843005/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_welcome.js (right):

https://codereview.chromium.org/2886843005/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_welcome.js:87: return {};
On 2017/05/18 00:04:57, Dan Beam wrote:
> set this only when ready, IMO

Done.

https://codereview.chromium.org/2886843005/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_welcome.js:161: ];
On 2017/05/18 00:04:57, Dan Beam wrote:
> var i18n = {};
> i18nTags.forEach(function(tag) {
>   i18n[tag] = loadTimeData.getString(tag);
> });
> this.i18n_ = i18n;

Done.

https://codereview.chromium.org/2886843005/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_welcome.js:198: var screens =
Polymer.dom(this.root).querySelectorAll(
On 2017/05/18 00:04:57, Dan Beam wrote:
> same

Done.

[dschuyler, 2017-05-18 00:47:49]

dschuyler@chromium.org changed reviewers:
+ alemate@chromium.org

[dschuyler, 2017-05-18 00:47:49]

ptal

This doesn't yet convert the i18n-values, that can be done later.

[dschuyler, 2017-05-18 00:50:15]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 00:51:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 06:21:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 06:21:54]

Dry run: This issue passed the CQ dry run.

[Alexander Alekseev, 2017-05-19 23:57:29]

https://codereview.chromium.org/2886843005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_welcome.js (right):

https://codereview.chromium.org/2886843005/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_welcome.js:161: this.i18n_ = i18n;
Is this an example of what could be done, or a final code?

I really don't like the idea of having a copy of message-ids in the JS source in
addition to HTML. This is error-prone.

Using polymer-based approach, we could have something like
http://stackoverflow.com/a/31129122 .

[Alexander Alekseev, 2017-05-19 23:59:35]

alemate@chromium.org changed reviewers:
+ achuith@chromium.org, xiyuan@chromium.org

[Alexander Alekseev, 2017-05-19 23:59:35]

+achuith@
+xiyuan@

[dschuyler, 2017-05-20 00:04:56]

https://codereview.chromium.org/2886843005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_welcome.js (right):

https://codereview.chromium.org/2886843005/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_welcome.js:161: this.i18n_ = i18n;
On 2017/05/19 23:57:29, Alexander Alekseev wrote:
> Is this an example of what could be done, or a final code?
> 
> I really don't like the idea of having a copy of message-ids in the JS source
in
> addition to HTML. This is error-prone.
> 
> Using polymer-based approach, we could have something like
> http://stackoverflow.com/a/31129122 .
> 
> 

I see it as working, potentially final code. I'm good with it being improved
upon following this CL.

[dschuyler, 2017-05-20 00:26:43]

https://codereview.chromium.org/2886843005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_welcome.js (right):

https://codereview.chromium.org/2886843005/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_welcome.js:161: this.i18n_ = i18n;
On 2017/05/20 00:04:56, dschuyler wrote:
> On 2017/05/19 23:57:29, Alexander Alekseev wrote:
> > Is this an example of what could be done, or a final code?
> > 
> > I really don't like the idea of having a copy of message-ids in the JS
source
> in
> > addition to HTML. This is error-prone.
> > 
> > Using polymer-based approach, we could have something like
> > http://stackoverflow.com/a/31129122 .
> > 
> > 
> 
> I see it as working, potentially final code. I'm good with it being improved
> upon following this CL.

dbeam@ suggested an improvement that may eliminate the need for this list in the
JS. Let me give that a go and put up another patch to see if that looks better.

[dschuyler, 2017-05-20 00:54:13]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-20 00:54:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-05-20 00:56:10]

https://codereview.chromium.org/2886843005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_welcome.js (right):

https://codereview.chromium.org/2886843005/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_welcome.js:161: this.i18n_ = i18n;
On 2017/05/20 00:26:43, dschuyler wrote:
> On 2017/05/20 00:04:56, dschuyler wrote:
> > On 2017/05/19 23:57:29, Alexander Alekseev wrote:
> > > Is this an example of what could be done, or a final code?
> > > 
> > > I really don't like the idea of having a copy of message-ids in the JS
> source
> > in
> > > addition to HTML. This is error-prone.
> > > 
> > > Using polymer-based approach, we could have something like
> > > http://stackoverflow.com/a/31129122 .
> > > 
> > > 
> > 
> > I see it as working, potentially final code. I'm good with it being improved
> > upon following this CL.
> 
> dbeam@ suggested an improvement that may eliminate the need for this list in
the
> JS. Let me give that a go and put up another patch to see if that looks
better.

WDYT of patch #3

[Alexander Alekseev, 2017-05-20 01:21:13]

https://codereview.chromium.org/2886843005/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_welcome.js (right):

https://codereview.chromium.org/2886843005/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_welcome.js:145: this.set('i18n_',
this.i18n_ + 1);
I don't like the idea of per-screen JS depencence.

This particular screen object has updateLocalizedContent() call for a very
special reason: it is using "special" cr-network-list element that requires this
ugly code.

UI objects should not have explicit updateLocalizedContent() methods. We can
hide them in behavior, but I don't like the idea of explicit
updateLocalizedContent() in every object.

[dschuyler, 2017-05-20 01:44:48]

https://codereview.chromium.org/2886843005/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_welcome.js (right):

https://codereview.chromium.org/2886843005/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_welcome.js:145: this.set('i18n_',
this.i18n_ + 1);
On 2017/05/20 01:21:12, Alexander Alekseev wrote:
> I don't like the idea of per-screen JS depencence.
> 
> This particular screen object has updateLocalizedContent() call for a very
> special reason: it is using "special" cr-network-list element that requires
this
> ugly code.
> 
> UI objects should not have explicit updateLocalizedContent() methods. We can
> hide them in behavior, but I don't like the idea of explicit
> updateLocalizedContent() in every object.

I agree that it is not pretty, but it's a special case (changing the language a
page is displayed in without reloading the page). I think oobe and login are the
corner case rather than the common case (and for that reason I believe that
special casing here is appropriate).

[Alexander Alekseev, 2017-05-20 01:50:57]

https://codereview.chromium.org/2886843005/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_welcome.js (right):

https://codereview.chromium.org/2886843005/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_welcome.js:145: this.set('i18n_',
this.i18n_ + 1);
On 2017/05/20 01:44:48, dschuyler wrote:
> On 2017/05/20 01:21:12, Alexander Alekseev wrote:
> > I don't like the idea of per-screen JS depencence.
> > 
> > This particular screen object has updateLocalizedContent() call for a very
> > special reason: it is using "special" cr-network-list element that requires
> this
> > ugly code.
> > 
> > UI objects should not have explicit updateLocalizedContent() methods. We can
> > hide them in behavior, but I don't like the idea of explicit
> > updateLocalizedContent() in every object.
> 
> I agree that it is not pretty, but it's a special case (changing the language
a
> page is displayed in without reloading the page). I think oobe and login are
the
> corner case rather than the common case (and for that reason I believe that
> special casing here is appropriate).

I believe that OOBE/Login is likely a half of Chrome OS UI.
(It also includes a lot of chrome://settings code).

Could you wrap it all into a polymer behavior?

[Dan Beam, 2017-05-20 02:03:30]

https://codereview.chromium.org/2886843005/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_welcome.js (right):

https://codereview.chromium.org/2886843005/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_welcome.js:145: this.set('i18n_',
this.i18n_ + 1);
fwiw: this could also just be

this.i18n_++;

https://codereview.chromium.org/2886843005/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_welcome.js:145: this.set('i18n_',
this.i18n_ + 1);
On 2017/05/20 01:50:57, Alexander Alekseev wrote:
> On 2017/05/20 01:44:48, dschuyler wrote:
> > On 2017/05/20 01:21:12, Alexander Alekseev wrote:
> > > I don't like the idea of per-screen JS depencence.
> > > 
> > > This particular screen object has updateLocalizedContent() call for a very
> > > special reason: it is using "special" cr-network-list element that
requires
> > this
> > > ugly code.
> > > 
> > > UI objects should not have explicit updateLocalizedContent() methods. We
can
> > > hide them in behavior, but I don't like the idea of explicit
> > > updateLocalizedContent() in every object.
> > 
> > I agree that it is not pretty, but it's a special case (changing the
language
> a
> > page is displayed in without reloading the page). I think oobe and login are
> the
> > corner case rather than the common case (and for that reason I believe that
> > special casing here is appropriate).
> 
> I believe that OOBE/Login is likely a half of Chrome OS UI.
> (It also includes a lot of chrome://settings code).
> 
> Could you wrap it all into a polymer behavior?

i don't know how a behavior would help that the binding syntax needs something
that changes to re-run

[Alexander Alekseev, 2017-05-20 02:25:21]

https://codereview.chromium.org/2886843005/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_welcome.js (right):

https://codereview.chromium.org/2886843005/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_welcome.js:145: this.set('i18n_',
this.i18n_ + 1);
On 2017/05/20 02:03:29, Dan Beam wrote:
> On 2017/05/20 01:50:57, Alexander Alekseev wrote:
> > On 2017/05/20 01:44:48, dschuyler wrote:
> > > On 2017/05/20 01:21:12, Alexander Alekseev wrote:
> > > > I don't like the idea of per-screen JS depencence.
> > > > 
> > > > This particular screen object has updateLocalizedContent() call for a
very
> > > > special reason: it is using "special" cr-network-list element that
> requires
> > > this
> > > > ugly code.
> > > > 
> > > > UI objects should not have explicit updateLocalizedContent() methods. We
> can
> > > > hide them in behavior, but I don't like the idea of explicit
> > > > updateLocalizedContent() in every object.
> > > 
> > > I agree that it is not pretty, but it's a special case (changing the
> language
> > a
> > > page is displayed in without reloading the page). I think oobe and login
are
> > the
> > > corner case rather than the common case (and for that reason I believe
that
> > > special casing here is appropriate).
> > 
> > I believe that OOBE/Login is likely a half of Chrome OS UI.
> > (It also includes a lot of chrome://settings code).
> > 
> > Could you wrap it all into a polymer behavior?
> 
> i don't know how a behavior would help that the binding syntax needs something
> that changes to re-run

updateLocalizedContent should go into the behavior.
Behaviors can have properties.

Also I posted this above: http://stackoverflow.com/a/31129122 .

[commit-bot: I haz the power, 2017-05-20 04:23:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-20 04:23:14]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-05-22 22:56:48]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 22:57:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[stevenjb, 2017-05-22 23:07:36]

stevenjb@chromium.org changed reviewers:
+ stevenjb@chromium.org

[stevenjb, 2017-05-22 23:07:37]

I like this approach. LGTM w/ nits/suggestions.

https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
File ui/webui/resources/js/i18n_behavior.js (right):

https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:70: * Similar to 'i18n', with updates
whenever |language| changes.
nit: Similar to 'i18n', with an unused |locale| parameter used to trigger
updates when |this.locale| changes.

https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:71: * @param {string} language The UI
language used.
locale ?

https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:91: * Called when UI strings change.
nit: Call this when UI strings may have changed (e.g. because the locale
changed). This will an update to any data bindings to i18nDynamic.

[commit-bot: I haz the power, 2017-05-22 23:10:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 23:10:50]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[Dan Beam, 2017-05-22 23:15:56]

https://codereview.chromium.org/2886843005/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_welcome.js (right):

https://codereview.chromium.org/2886843005/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_welcome.js:136:
I18nBehavior.updateLocalizedContent.call(this);
why are you overriding the same name in the behavior and this class?

https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
File ui/webui/resources/js/i18n_behavior.js (right):

https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:93: updateLocalizedContent: function() {
why shouldn't callers just set this.locale directly?

[Dan Beam, 2017-05-22 23:26:12]

https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
File ui/webui/resources/js/i18n_behavior.js (right):

https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:14: /*
should be /**

[dschuyler, 2017-05-22 23:30:18]

https://codereview.chromium.org/2886843005/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_welcome.js (right):

https://codereview.chromium.org/2886843005/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_welcome.js:136:
I18nBehavior.updateLocalizedContent.call(this);
On 2017/05/22 23:15:56, Dan Beam wrote:
> why are you overriding the same name in the behavior and this class?

It's a function rather than setting this.locale for encapsulation, it's the same
name for polymorphism.

I'm cool with something like this too, if preferred:

this.locale = ...;

or 

this.i18nUpdateLocale();

https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
File ui/webui/resources/js/i18n_behavior.js (right):

https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:70: * Similar to 'i18n', with updates
whenever |language| changes.
On 2017/05/22 23:07:37, stevenjb wrote:
> nit: Similar to 'i18n', with an unused |locale| parameter used to trigger
> updates when |this.locale| changes.

Done.

https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:71: * @param {string} language The UI
language used.
On 2017/05/22 23:07:37, stevenjb wrote:
> locale ?
> 

Done.

https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:78: return this.i18n.apply(this,
Array.prototype.slice.call(arguments, 1));
Hmm, I've recently read that splicing |arguments| may be a bad idea on V8. I'm
tempted to replace the above with:

    var args = new Array(arguments.length);
    for (var i = 0; i < arguments.length; ++i)
      args[i] = arguments[i];
    return this.i18n.apply(this, args.slice(1));

Thought I'm not sure that it's necessary.

https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:91: * Called when UI strings change.
On 2017/05/22 23:07:37, stevenjb wrote:
> nit: Call this when UI strings may have changed (e.g. because the locale
> changed). This will an update to any data bindings to i18nDynamic.

Changed to something similar, though not exact.

done.

https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:93: updateLocalizedContent: function() {
On 2017/05/22 23:15:56, Dan Beam wrote:
> why shouldn't callers just set this.locale directly?

They sure could. I was trying to separate I18nBehavior internals from the class
mixing in the behavior.

[dschuyler, 2017-05-22 23:31:43]

https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
File ui/webui/resources/js/i18n_behavior.js (right):

https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:14: /*
On 2017/05/22 23:26:12, Dan Beam wrote:
> should be /**

Done in prior patch (this was the error reported by closure).

done.

[dschuyler, 2017-05-22 23:33:26]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 23:34:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-05-22 23:34:12]

On 2017/05/22 23:31:43, dschuyler wrote:
>
https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
> File ui/webui/resources/js/i18n_behavior.js (right):
> 
>
https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
> ui/webui/resources/js/i18n_behavior.js:14: /*
> On 2017/05/22 23:26:12, Dan Beam wrote:
> > should be /**
> 
> Done in prior patch (this was the error reported by closure).
> 
> done.

Unfortunately, patch 5 had not gone up when I made the replies above. Path 5 is
now up.

[stevenjb, 2017-05-22 23:37:00]

https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
File ui/webui/resources/js/i18n_behavior.js (right):

https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:78: return this.i18n.apply(this,
Array.prototype.slice.call(arguments, 1));
On 2017/05/22 23:30:17, dschuyler wrote:
> Hmm, I've recently read that splicing |arguments| may be a bad idea on V8. I'm
> tempted to replace the above with:
> 
>     var args = new Array(arguments.length);
>     for (var i = 0; i < arguments.length; ++i)
>       args[i] = arguments[i];
>     return this.i18n.apply(this, args.slice(1));
> 
> Thought I'm not sure that it's necessary.

splicing or slicing? I wouldn't think that slice here would be that bad, I'd
expect it to pretty much do the same as above but hopefully more efficiently...

[Dan Beam, 2017-05-22 23:38:05]

https://codereview.chromium.org/2886843005/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_welcome.js (right):

https://codereview.chromium.org/2886843005/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_welcome.js:136:
I18nBehavior.updateLocalizedContent.call(this);
On 2017/05/22 23:30:17, dschuyler wrote:
> On 2017/05/22 23:15:56, Dan Beam wrote:
> > why are you overriding the same name in the behavior and this class?
> 
> It's a function rather than setting this.locale for encapsulation,

but we still have to expose locale publicly anyways for the bindings to work,
right?

> it's the same
> name for polymorphism.
> 
> I'm cool with something like this too, if preferred:
> 
> this.locale = ...;
> 
> or 
> 
> this.i18nUpdateLocale();
> 

both are better, imo

[Dan Beam, 2017-05-22 23:39:02]

https://codereview.chromium.org/2886843005/diff/80001/ui/webui/resources/js/i...
File ui/webui/resources/js/i18n_behavior.js (right):

https://codereview.chromium.org/2886843005/diff/80001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:78: return this.i18n.apply(this,
Array.prototype.slice.call(arguments, 1));
i think this slice here is fine

[commit-bot: I haz the power, 2017-05-22 23:46:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 23:46:37]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[dschuyler, 2017-05-22 23:54:54]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 23:55:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-05-22 23:57:08]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 23:58:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-05-23 00:03:19]

https://codereview.chromium.org/2886843005/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_welcome.js (right):

https://codereview.chromium.org/2886843005/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_welcome.js:145: this.set('i18n_',
this.i18n_ + 1);
On 2017/05/20 02:03:29, Dan Beam wrote:
> fwiw: this could also just be
> 
> this.i18n_++;

Acknowledged.

https://codereview.chromium.org/2886843005/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_welcome.js:145: this.set('i18n_',
this.i18n_ + 1);
On 2017/05/20 02:03:29, Dan Beam wrote:
> On 2017/05/20 01:50:57, Alexander Alekseev wrote:
> > On 2017/05/20 01:44:48, dschuyler wrote:
> > > On 2017/05/20 01:21:12, Alexander Alekseev wrote:
> > > > I don't like the idea of per-screen JS depencence.
> > > > 
> > > > This particular screen object has updateLocalizedContent() call for a
very
> > > > special reason: it is using "special" cr-network-list element that
> requires
> > > this
> > > > ugly code.
> > > > 
> > > > UI objects should not have explicit updateLocalizedContent() methods. We
> can
> > > > hide them in behavior, but I don't like the idea of explicit
> > > > updateLocalizedContent() in every object.
> > > 
> > > I agree that it is not pretty, but it's a special case (changing the
> language
> > a
> > > page is displayed in without reloading the page). I think oobe and login
are
> > the
> > > corner case rather than the common case (and for that reason I believe
that
> > > special casing here is appropriate).
> > 
> > I believe that OOBE/Login is likely a half of Chrome OS UI.
> > (It also includes a lot of chrome://settings code).
> > 
> > Could you wrap it all into a polymer behavior?
> 
> i don't know how a behavior would help that the binding syntax needs something
> that changes to re-run

Acknowledged.

https://codereview.chromium.org/2886843005/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_welcome.js:145: this.set('i18n_',
this.i18n_ + 1);
On 2017/05/20 01:50:57, Alexander Alekseev wrote:
> On 2017/05/20 01:44:48, dschuyler wrote:
> > On 2017/05/20 01:21:12, Alexander Alekseev wrote:
> > > I don't like the idea of per-screen JS depencence.
> > > 
> > > This particular screen object has updateLocalizedContent() call for a very
> > > special reason: it is using "special" cr-network-list element that
requires
> > this
> > > ugly code.
> > > 
> > > UI objects should not have explicit updateLocalizedContent() methods. We
can
> > > hide them in behavior, but I don't like the idea of explicit
> > > updateLocalizedContent() in every object.
> > 
> > I agree that it is not pretty, but it's a special case (changing the
language
> a
> > page is displayed in without reloading the page). I think oobe and login are
> the
> > corner case rather than the common case (and for that reason I believe that
> > special casing here is appropriate).
> 
> I believe that OOBE/Login is likely a half of Chrome OS UI.
> (It also includes a lot of chrome://settings code).
> 
> Could you wrap it all into a polymer behavior?

Done.

https://codereview.chromium.org/2886843005/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_welcome.js:145: this.set('i18n_',
this.i18n_ + 1);
On 2017/05/20 02:25:21, Alexander Alekseev wrote:
> On 2017/05/20 02:03:29, Dan Beam wrote:
> > On 2017/05/20 01:50:57, Alexander Alekseev wrote:
> > > On 2017/05/20 01:44:48, dschuyler wrote:
> > > > On 2017/05/20 01:21:12, Alexander Alekseev wrote:
> > > > > I don't like the idea of per-screen JS depencence.
> > > > > 
> > > > > This particular screen object has updateLocalizedContent() call for a
> very
> > > > > special reason: it is using "special" cr-network-list element that
> > requires
> > > > this
> > > > > ugly code.
> > > > > 
> > > > > UI objects should not have explicit updateLocalizedContent() methods.
We
> > can
> > > > > hide them in behavior, but I don't like the idea of explicit
> > > > > updateLocalizedContent() in every object.
> > > > 
> > > > I agree that it is not pretty, but it's a special case (changing the
> > language
> > > a
> > > > page is displayed in without reloading the page). I think oobe and login
> are
> > > the
> > > > corner case rather than the common case (and for that reason I believe
> that
> > > > special casing here is appropriate).
> > > 
> > > I believe that OOBE/Login is likely a half of Chrome OS UI.
> > > (It also includes a lot of chrome://settings code).
> > > 
> > > Could you wrap it all into a polymer behavior?
> > 
> > i don't know how a behavior would help that the binding syntax needs
something
> > that changes to re-run
> 
> updateLocalizedContent should go into the behavior.
> Behaviors can have properties.
> 
> Also I posted this above: http://stackoverflow.com/a/31129122 .

I'd like that (moving the existing code) to be a separate CL.

https://codereview.chromium.org/2886843005/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_welcome.js (right):

https://codereview.chromium.org/2886843005/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_welcome.js:136:
I18nBehavior.updateLocalizedContent.call(this);
On 2017/05/22 23:38:05, Dan Beam wrote:
> On 2017/05/22 23:30:17, dschuyler wrote:
> > On 2017/05/22 23:15:56, Dan Beam wrote:
> > > why are you overriding the same name in the behavior and this class?
> > 
> > It's a function rather than setting this.locale for encapsulation,
> 
> but we still have to expose locale publicly anyways for the bindings to work,
> right?
> 
> > it's the same
> > name for polymorphism.
> > 
> > I'm cool with something like this too, if preferred:
> > 
> > this.locale = ...;
> > 
> > or 
> > 
> > this.i18nUpdateLocale();
> > 
> 
> both are better, imo

Went with this.i18nUpdateLocale();

done.

https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
File ui/webui/resources/js/i18n_behavior.js (right):

https://codereview.chromium.org/2886843005/diff/60001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:78: return this.i18n.apply(this,
Array.prototype.slice.call(arguments, 1));
On 2017/05/22 23:37:00, stevenjb wrote:
> On 2017/05/22 23:30:17, dschuyler wrote:
> > Hmm, I've recently read that splicing |arguments| may be a bad idea on V8.
I'm
> > tempted to replace the above with:
> > 
> >     var args = new Array(arguments.length);
> >     for (var i = 0; i < arguments.length; ++i)
> >       args[i] = arguments[i];
> >     return this.i18n.apply(this, args.slice(1));
> > 
> > Thought I'm not sure that it's necessary.
> 
> splicing or slicing? I wouldn't think that slice here would be that bad, I'd
> expect it to pretty much do the same as above but hopefully more
efficiently...

Yep, slicing. Thanks for catching the typo.

https://codereview.chromium.org/2886843005/diff/80001/ui/webui/resources/js/i...
File ui/webui/resources/js/i18n_behavior.js (right):

https://codereview.chromium.org/2886843005/diff/80001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:78: return this.i18n.apply(this,
Array.prototype.slice.call(arguments, 1));
On 2017/05/22 23:39:02, Dan Beam wrote:
> i think this slice here is fine

Acknowledged.

[stevenjb, 2017-05-23 00:14:09]

LGTM++

[commit-bot: I haz the power, 2017-05-23 01:16:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 01:16:29]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-05-23 16:28:13]

lgtm but can you write a test?

[dschuyler, 2017-05-24 00:55:50]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 00:56:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-05-24 01:03:51]

On 2017/05/23 16:28:13, Dan Beam wrote:
> lgtm but can you write a test?

I added a (minor) test to i18n_behavior_test.html. 

I considered adding more tests to the oobe_welcome unit tests, but haven't found
those tests yet.

[Dan Beam, 2017-05-24 02:08:45]

https://codereview.chromium.org/2886843005/diff/140001/chrome/test/data/webui...
File chrome/test/data/webui/i18n_behavior_test.html (right):

https://codereview.chromium.org/2886843005/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/i18n_behavior_test.html:42: }
hmmmm, seems like we could make a more comprehensive test.  it doesn't
necessarily have to create an OOBE UI, but it does seem like we could continue
to use I18nBehavior statically or [preferably] make a PolymerElement that has
behaviors: [I18nBehavior] and use this in conjunction with
loadTimeData.overrideValues({language: '...'}) and call i18nUpdateLocale() (or
just set locale = directly)

[commit-bot: I haz the power, 2017-05-24 04:07:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 04:07:43]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-05-24 21:09:48]

ping for other reviewers to lgtm or request a change.

[xiyuan, 2017-05-24 21:10:55]

lgtm

[Alexander Alekseev, 2017-05-25 00:20:08]

I still don't like the approach that requires (non-trivial!) changes to JS to
support the feature that never needed this.

But as xiyuan@ and stevenjb@ agree with this approach, let's leave the decision
to achuith@.

[stevenjb, 2017-05-25 00:28:55]

On 2017/05/25 00:20:08, Alexander Alekseev wrote:
> I still don't like the approach that requires (non-trivial!) changes to JS to
> support the feature that never needed this.
> 
> But as xiyuan@ and stevenjb@ agree with this approach, let's leave the
decision
> to achuith@.

FWIW, adding a single call to this.i18nUpdateLocale() in the JS is about as
trivial as you could hope for to enable dynamic string updates :)

[Dan Beam, 2017-05-25 00:37:22]

On 2017/05/25 00:20:08, Alexander Alekseev wrote:
> I still don't like the approach that requires (non-trivial!) changes to JS to
> support the feature that never needed this.
> 
> But as xiyuan@ and stevenjb@ agree with this approach, let's leave the
decision
> to achuith@.

hmmm, I'm a little confused.

loadTimeData and things like i18n_process.js are much more complicated than the
code dschuyler@ has written here.  so while this change might be a little more
than you expected, I think it's still running less code and in a very simple
way.

here's what the i18n-content stuff is running:
https://cs.chromium.org/chromium/src/ui/webui/resources/js/i18n_template_no_p...
https://cs.chromium.org/chromium/src/ui/webui/resources/js/load_time_data.js

and there's actually a bit of overhead of querying the DOM just to find those
attributes then replacing them.  this is not free and will slow down the UI a
touch (and is also generally flickery).  it's unfortunate that we have yet to
figure out how to integrate $i18n{} dynamically, but that's kind of the point.

with i18n-content, there's a bunch of frames rendered:

1. blank while html loads
2. html without JS
3. there's some waiting on load_time_data.js, i18n_template.js, strings.js
4. there's a bit of JS and DOM to querySelectorAll('[i18n-content]') and replace
all the node's textContent

and none of this particularly smart about how it happens.  it's all synchronous,
blocking, and as soon as possible (but JS is no longer ensured to run before the
first paint).

in the $i18n{} case, it's just:

1. blank while html loads
2. html with translations embedded (because the substitutions happen in the
browser process before returning the resource)

in the case of this Polymer behavior-y version, Polymer is at least smart enough
to yield control back to the user and render the template asynchronously on
micro (or macro?) task timing (think setTimeout()).

So overall, I think either $i18n{} or using Polymer binding are better for this
use case.

Does that make sense?  Am I missing something?

[Alexander Alekseev, 2017-05-25 01:19:54]

On 2017/05/25 00:37:22, Dan Beam wrote:
> On 2017/05/25 00:20:08, Alexander Alekseev wrote:
> > I still don't like the approach that requires (non-trivial!) changes to JS
to
> > support the feature that never needed this.
> > 
> > But as xiyuan@ and stevenjb@ agree with this approach, let's leave the
> decision
> > to achuith@.
> 
> hmmm, I'm a little confused.
> 
> loadTimeData and things like i18n_process.js are much more complicated than
the
> code dschuyler@ has written here.  so while this change might be a little more
> than you expected, I think it's still running less code and in a very simple
> way.

Sorry, actually the last time I looked into this CL, there were no i18nbehavior,
so one needed to update polymer properties bound to the strings in each Polymer
object JS.

Now what I still don't like, the call this.i18nUpdateLocale(); inside
updateLocalizedContent(). I commented above that you cannot rely on this method.

This should happen automatically. behavior can have registed() method that
allows to perform initialization tasks. 

So we probably should add a list of i18n observers in
ui/login/display_manager.js . registed() method defined in behavior should
register self as an observer, so that
ui/login/display_manager.js::updateLocalizedContent_() can call these observers
explicitly and do not rely on screens.

This behavior should work automatically regardless of the element type that it
is attached to. I.e. if this is a menu, or a label, or whatever, it should work
regardless of whether it has updateLocalizedContent() method or not.




> here's what the i18n-content stuff is running:
>
https://cs.chromium.org/chromium/src/ui/webui/resources/js/i18n_template_no_p...
> https://cs.chromium.org/chromium/src/ui/webui/resources/js/load_time_data.js
> 
> and there's actually a bit of overhead of querying the DOM just to find those
> attributes then replacing them.  this is not free and will slow down the UI a
> touch (and is also generally flickery).  it's unfortunate that we have yet to
> figure out how to integrate $i18n{} dynamically, but that's kind of the point.

These files are a part of "UI infrastructure", and out of scope of UI
developers. (They may break, but I don't remember that happen.) I don't even
know how and when they are loaded. They just work. This is what we need from
the UI point of view.


> with i18n-content, there's a bunch of frames rendered:
> 
> 1. blank while html loads
> 2. html without JS
> 3. there's some waiting on load_time_data.js, i18n_template.js, strings.js
> 4. there's a bit of JS and DOM to querySelectorAll('[i18n-content]') and
replace
> all the node's textContent
> 
> and none of this particularly smart about how it happens.  it's all
synchronous,
> blocking, and as soon as possible (but JS is no longer ensured to run before
the
> first paint).
> 
> in the $i18n{} case, it's just:
> 
> 1. blank while html loads
> 2. html with translations embedded (because the substitutions happen in the
> browser process before returning the resource)
> 
> in the case of this Polymer behavior-y version, Polymer is at least smart
enough
> to yield control back to the user and render the template asynchronously on
> micro (or macro?) task timing (think setTimeout()).
> 
> So overall, I think either $i18n{} or using Polymer binding are better for
this
> use case.
> 
> Does that make sense?  Am I missing something?

It is better, because it is potentially faster. It is worse, because in its
current
implementation, it depends on subtle elements interaction. Which is bad.

The initial idea, if I remember it correctly, was to move i18n completely to
C++.
I think this is reasonable. We just need to move the C++ code to renderer, and
possibly to Blink.

[Alexander Alekseev, 2017-05-25 01:22:30]

On 2017/05/25 00:28:55, stevenjb wrote:
> On 2017/05/25 00:20:08, Alexander Alekseev wrote:
> > I still don't like the approach that requires (non-trivial!) changes to JS
to
> > support the feature that never needed this.
> > 
> > But as xiyuan@ and stevenjb@ agree with this approach, let's leave the
> decision
> > to achuith@.
> 
> FWIW, adding a single call to this.i18nUpdateLocale() in the JS is about as
> trivial as you could hope for to enable dynamic string updates :)

This will be immediately broken as we figure out that updateLocalizedContent
is not propagated through the tree.

We need to have special observers for i18n which can be registed in registed()
method of the behavior.

[stevenjb, 2017-05-25 14:56:59]

On 2017/05/25 01:22:30, Alexander Alekseev wrote:
> On 2017/05/25 00:28:55, stevenjb wrote:
> > On 2017/05/25 00:20:08, Alexander Alekseev wrote:
> > > I still don't like the approach that requires (non-trivial!) changes to JS
> to
> > > support the feature that never needed this.
> > > 
> > > But as xiyuan@ and stevenjb@ agree with this approach, let's leave the
> > decision
> > > to achuith@.
> > 
> > FWIW, adding a single call to this.i18nUpdateLocale() in the JS is about as
> > trivial as you could hope for to enable dynamic string updates :)
> 
> This will be immediately broken as we figure out that updateLocalizedContent
> is not propagated through the tree.
> 
> We need to have special observers for i18n which can be registed in registed()
> method of the behavior.

I didn't entirely everything in your previous comments. I think there is a lot
of confusion because some of the comments addressed earlier patches.

Could you maybe address your current concerns more concisely or in the context
of the CL? It sounds like you are suggesting that updateLocalizedContent() will
not always be called after the locale changes, despite the comment "This is
called when UI strings are changed"? That concerns me because if it is not
called when the locale changes, then networking strings will also not be
updated. It seems like we need to fix that if it is indeed broken (maybe in a
separate CL?).

[Alexander Alekseev, 2017-05-25 19:24:32]

On 2017/05/25 14:56:59, stevenjb wrote:
> On 2017/05/25 01:22:30, Alexander Alekseev wrote:
> > On 2017/05/25 00:28:55, stevenjb wrote:
> > > On 2017/05/25 00:20:08, Alexander Alekseev wrote:
> > > > I still don't like the approach that requires (non-trivial!) changes to
JS
> > to
> > > > support the feature that never needed this.
> > > > 
> > > > But as xiyuan@ and stevenjb@ agree with this approach, let's leave the
> > > decision
> > > > to achuith@.
> > > 
> > > FWIW, adding a single call to this.i18nUpdateLocale() in the JS is about
as
> > > trivial as you could hope for to enable dynamic string updates :)
> > 
> > This will be immediately broken as we figure out that updateLocalizedContent
> > is not propagated through the tree.
> > 
> > We need to have special observers for i18n which can be registed in
registed()
> > method of the behavior.
> 
> I didn't entirely everything in your previous comments. I think there is a lot
> of confusion because some of the comments addressed earlier patches.
> 
> Could you maybe address your current concerns more concisely or in the context
> of the CL? It sounds like you are suggesting that updateLocalizedContent()
will
> not always be called after the locale changes, despite the comment "This is
> called when UI strings are changed"? That concerns me because if it is not
> called when the locale changes, then networking strings will also not be
> updated. It seems like we need to fix that if it is indeed broken (maybe in a
> separate CL?).


Let's look at another object:

https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/login/...

this is just another Polymer UI object. Even if we add updateLocalizedContent
method to it, it won't work, because noone is going to ever call it.


My point is: the fact that oobe_welcome object has updateLocalizedContent()
method is very unusual.
Most Polymer objects do not have it.

Only OOBE screens have it, and it is a hack.


I mean that we need universal i18n support that will not depend on whether the
Polymer object itself needs updateLocalizedContent() method for its internal
use, and if anyone is going to call it.

We probably need something that will make strings to magically correct without
adding manual method calls.
That is why we need to make I18nBehavior to subscribe to updates on its own.
And we probably need a separate event flow for it.

We can still start that event flow from
ui/login/display_manager.js::updateLocalizedContent_() , but it should be
independent of oobe screens starting from that point.

[stevenjb, 2017-05-25 20:26:17]

On 2017/05/25 19:24:32, Alexander Alekseev wrote:
> On 2017/05/25 14:56:59, stevenjb wrote:
> > On 2017/05/25 01:22:30, Alexander Alekseev wrote:
> > > On 2017/05/25 00:28:55, stevenjb wrote:
> > > > On 2017/05/25 00:20:08, Alexander Alekseev wrote:
> > > > > I still don't like the approach that requires (non-trivial!) changes
to
> JS
> > > to
> > > > > support the feature that never needed this.
> > > > > 
> > > > > But as xiyuan@ and stevenjb@ agree with this approach, let's leave the
> > > > decision
> > > > > to achuith@.
> > > > 
> > > > FWIW, adding a single call to this.i18nUpdateLocale() in the JS is about
> as
> > > > trivial as you could hope for to enable dynamic string updates :)
> > > 
> > > This will be immediately broken as we figure out that
updateLocalizedContent
> > > is not propagated through the tree.
> > > 
> > > We need to have special observers for i18n which can be registed in
> registed()
> > > method of the behavior.
> > 
> > I didn't entirely everything in your previous comments. I think there is a
lot
> > of confusion because some of the comments addressed earlier patches.
> > 
> > Could you maybe address your current concerns more concisely or in the
context
> > of the CL? It sounds like you are suggesting that updateLocalizedContent()
> will
> > not always be called after the locale changes, despite the comment "This is
> > called when UI strings are changed"? That concerns me because if it is not
> > called when the locale changes, then networking strings will also not be
> > updated. It seems like we need to fix that if it is indeed broken (maybe in
a
> > separate CL?).
> 
> 
> Let's look at another object:
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/login/...
> 
> this is just another Polymer UI object. Even if we add updateLocalizedContent
> method to it, it won't work, because noone is going to ever call it.
> 
> 
> My point is: the fact that oobe_welcome object has updateLocalizedContent()
> method is very unusual.
> Most Polymer objects do not have it.
> 
> Only OOBE screens have it, and it is a hack.
> 
> 
> I mean that we need universal i18n support that will not depend on whether the
> Polymer object itself needs updateLocalizedContent() method for its internal
> use, and if anyone is going to call it.
> 
> We probably need something that will make strings to magically correct without
> adding manual method calls.
> That is why we need to make I18nBehavior to subscribe to updates on its own.
> And we probably need a separate event flow for it.
> 
> We can still start that event flow from
> ui/login/display_manager.js::updateLocalizedContent_() , but it should be
> independent of oobe screens starting from that point.

All that we need to do to make the content of oobe-welcome-dialog, etc dynamic
is:
* <oobe-welcome-dialog locale="[[locale]]">
* Replace i18n-content with i18nDynamic() like we did here

If you are looking for a more "magical" way of updating all of the OOBE strings
when the locale changes, someone on the OOBE team is going to need to invest the
time and energy to develop something. As Dan described, a lot of effort was made
to make the loading process as fast and seamless as it is for the common case
where we do not support locale changes.

In the meanwhile this seems like a pretty reasonable approach to me.

[Alexander Alekseev, 2017-05-25 20:33:09]

On 2017/05/25 20:26:17, stevenjb wrote:
> On 2017/05/25 19:24:32, Alexander Alekseev wrote:
> > On 2017/05/25 14:56:59, stevenjb wrote:
> > > On 2017/05/25 01:22:30, Alexander Alekseev wrote:
> > > > On 2017/05/25 00:28:55, stevenjb wrote:
> > > > > On 2017/05/25 00:20:08, Alexander Alekseev wrote:
> > > > > > I still don't like the approach that requires (non-trivial!) changes
> to
> > JS
> > > > to
> > > > > > support the feature that never needed this.
> > > > > > 
> > > > > > But as xiyuan@ and stevenjb@ agree with this approach, let's leave
the
> > > > > decision
> > > > > > to achuith@.
> > > > > 
> > > > > FWIW, adding a single call to this.i18nUpdateLocale() in the JS is
about
> > as
> > > > > trivial as you could hope for to enable dynamic string updates :)
> > > > 
> > > > This will be immediately broken as we figure out that
> updateLocalizedContent
> > > > is not propagated through the tree.
> > > > 
> > > > We need to have special observers for i18n which can be registed in
> > registed()
> > > > method of the behavior.
> > > 
> > > I didn't entirely everything in your previous comments. I think there is a
> lot
> > > of confusion because some of the comments addressed earlier patches.
> > > 
> > > Could you maybe address your current concerns more concisely or in the
> context
> > > of the CL? It sounds like you are suggesting that updateLocalizedContent()
> > will
> > > not always be called after the locale changes, despite the comment "This
is
> > > called when UI strings are changed"? That concerns me because if it is not
> > > called when the locale changes, then networking strings will also not be
> > > updated. It seems like we need to fix that if it is indeed broken (maybe
in
> a
> > > separate CL?).
> > 
> > 
> > Let's look at another object:
> > 
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/login/...
> > 
> > this is just another Polymer UI object. Even if we add
updateLocalizedContent
> > method to it, it won't work, because noone is going to ever call it.
> > 
> > 
> > My point is: the fact that oobe_welcome object has updateLocalizedContent()
> > method is very unusual.
> > Most Polymer objects do not have it.
> > 
> > Only OOBE screens have it, and it is a hack.
> > 
> > 
> > I mean that we need universal i18n support that will not depend on whether
the
> > Polymer object itself needs updateLocalizedContent() method for its internal
> > use, and if anyone is going to call it.
> > 
> > We probably need something that will make strings to magically correct
without
> > adding manual method calls.
> > That is why we need to make I18nBehavior to subscribe to updates on its own.
> > And we probably need a separate event flow for it.
> > 
> > We can still start that event flow from
> > ui/login/display_manager.js::updateLocalizedContent_() , but it should be
> > independent of oobe screens starting from that point.
> 
> All that we need to do to make the content of oobe-welcome-dialog, etc dynamic
> is:
> * <oobe-welcome-dialog locale="[[locale]]">
> * Replace i18n-content with i18nDynamic() like we did here
> 
> If you are looking for a more "magical" way of updating all of the OOBE
strings
> when the locale changes, someone on the OOBE team is going to need to invest
the
> time and energy to develop something. As Dan described, a lot of effort was
made
> to make the loading process as fast and seamless as it is for the common case
> where we do not support locale changes.
> 
> In the meanwhile this seems like a pretty reasonable approach to me.

Ok, let's wait for Achuith's opinion.

[achuithb, 2017-05-29 01:15:28]

On 2017/05/25 20:33:09, Alexander Alekseev wrote:
> On 2017/05/25 20:26:17, stevenjb wrote:
> > On 2017/05/25 19:24:32, Alexander Alekseev wrote:
> > > On 2017/05/25 14:56:59, stevenjb wrote:
> > > > On 2017/05/25 01:22:30, Alexander Alekseev wrote:
> > > > > On 2017/05/25 00:28:55, stevenjb wrote:
> > > > > > On 2017/05/25 00:20:08, Alexander Alekseev wrote:
> > > > > > > I still don't like the approach that requires (non-trivial!)
changes
> > to
> > > JS
> > > > > to
> > > > > > > support the feature that never needed this.
> > > > > > > 
> > > > > > > But as xiyuan@ and stevenjb@ agree with this approach, let's leave
> the
> > > > > > decision
> > > > > > > to achuith@.
> > > > > > 
> > > > > > FWIW, adding a single call to this.i18nUpdateLocale() in the JS is
> about
> > > as
> > > > > > trivial as you could hope for to enable dynamic string updates :)
> > > > > 
> > > > > This will be immediately broken as we figure out that
> > updateLocalizedContent
> > > > > is not propagated through the tree.
> > > > > 
> > > > > We need to have special observers for i18n which can be registed in
> > > registed()
> > > > > method of the behavior.
> > > > 
> > > > I didn't entirely everything in your previous comments. I think there is
a
> > lot
> > > > of confusion because some of the comments addressed earlier patches.
> > > > 
> > > > Could you maybe address your current concerns more concisely or in the
> > context
> > > > of the CL? It sounds like you are suggesting that
updateLocalizedContent()
> > > will
> > > > not always be called after the locale changes, despite the comment "This
> is
> > > > called when UI strings are changed"? That concerns me because if it is
not
> > > > called when the locale changes, then networking strings will also not be
> > > > updated. It seems like we need to fix that if it is indeed broken (maybe
> in
> > a
> > > > separate CL?).
> > > 
> > > 
> > > Let's look at another object:
> > > 
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/login/...
> > > 
> > > this is just another Polymer UI object. Even if we add
> updateLocalizedContent
> > > method to it, it won't work, because noone is going to ever call it.
> > > 
> > > 
> > > My point is: the fact that oobe_welcome object has
updateLocalizedContent()
> > > method is very unusual.
> > > Most Polymer objects do not have it.
> > > 
> > > Only OOBE screens have it, and it is a hack.
> > > 
> > > 
> > > I mean that we need universal i18n support that will not depend on whether
> the
> > > Polymer object itself needs updateLocalizedContent() method for its
internal
> > > use, and if anyone is going to call it.
> > > 
> > > We probably need something that will make strings to magically correct
> without
> > > adding manual method calls.
> > > That is why we need to make I18nBehavior to subscribe to updates on its
own.
> > > And we probably need a separate event flow for it.
> > > 
> > > We can still start that event flow from
> > > ui/login/display_manager.js::updateLocalizedContent_() , but it should be
> > > independent of oobe screens starting from that point.
> > 
> > All that we need to do to make the content of oobe-welcome-dialog, etc
dynamic
> > is:
> > * <oobe-welcome-dialog locale="[[locale]]">
> > * Replace i18n-content with i18nDynamic() like we did here
> > 
> > If you are looking for a more "magical" way of updating all of the OOBE
> strings
> > when the locale changes, someone on the OOBE team is going to need to invest
> the
> > time and energy to develop something. As Dan described, a lot of effort was
> made
> > to make the loading process as fast and seamless as it is for the common
case
> > where we do not support locale changes.
> > 
> > In the meanwhile this seems like a pretty reasonable approach to me.
> 
> Ok, let's wait for Achuith's opinion.

I'm really sorry, but my experience with this code, and polymer in general is
very limited.

[dschuyler, 2017-05-30 18:49:57]

dschuyler@chromium.org changed reviewers:
- achuith@chromium.org

[dschuyler, 2017-05-30 18:49:58]

achuithb@, thanks for the info. IMO, it's perfectly reasonable to be unfamiliar
with Polymer details. For now, I'll go ahead and reduce the list of reviewers on
this CL. Feel fee to add yourself back, if you desire.

If you'd like to chat about Polymer (to confidently weigh in on this CL), please
let me know. I think we could go over enough of Polymer in ~30 minutes to
effectively review this CL. OTOH, dbeam@ and stevenjb@ know Polymer very well,
so going with their LGTM's seems reasonable too (maybe consider an "RS LGTM"
(rubber stamp) to say that nothing looks alarming to you, but you're relying on
others for the detailed review).

[dschuyler, 2017-05-31 00:47:53]

On 2017/05/30 18:49:58, dschuyler wrote:
> achuithb@, thanks for the info. IMO, it's perfectly reasonable to be
unfamiliar
> with Polymer details. For now, I'll go ahead and reduce the list of reviewers
on
> this CL. Feel fee to add yourself back, if you desire.
> 
> If you'd like to chat about Polymer (to confidently weigh in on this CL),
please
> let me know. I think we could go over enough of Polymer in ~30 minutes to
> effectively review this CL. OTOH, dbeam@ and stevenjb@ know Polymer very well,
> so going with their LGTM's seems reasonable too (maybe consider an "RS LGTM"
> (rubber stamp) to say that nothing looks alarming to you, but you're relying
on
> others for the detailed review).

alemate@ Thanks for getting thorough consideration for this CL from several
stakeholders. It's a better CL now than when it started. It it ready for an LGTM
(or otherwise allow the other LGTM's to prevail) or is there more to consider?

[dschuyler, 2017-05-31 00:48:28]

On 2017/05/31 00:47:53, dschuyler wrote:
> On 2017/05/30 18:49:58, dschuyler wrote:
> > achuithb@, thanks for the info. IMO, it's perfectly reasonable to be
> unfamiliar
> > with Polymer details. For now, I'll go ahead and reduce the list of
reviewers
> on
> > this CL. Feel fee to add yourself back, if you desire.
> > 
> > If you'd like to chat about Polymer (to confidently weigh in on this CL),
> please
> > let me know. I think we could go over enough of Polymer in ~30 minutes to
> > effectively review this CL. OTOH, dbeam@ and stevenjb@ know Polymer very
well,
> > so going with their LGTM's seems reasonable too (maybe consider an "RS LGTM"
> > (rubber stamp) to say that nothing looks alarming to you, but you're relying
> on
> > others for the detailed review).
> 
> alemate@ Thanks for getting thorough consideration for this CL from several
> stakeholders. It's a better CL now than when it started. It it ready for an
LGTM
> (or otherwise allow the other LGTM's to prevail) or is there more to consider?

*Is it ready...

[Dan Beam, 2017-05-31 00:50:18]

still lgtm

[xiyuan, 2017-05-31 15:38:41]

https://codereview.chromium.org/2886843005/diff/140001/chrome/test/data/webui...
File chrome/test/data/webui/i18n_behavior_test.html (right):

https://codereview.chromium.org/2886843005/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/i18n_behavior_test.html:40: assertEquals("I'm just text,
nobody should have a problem with me!",
nit: " -> ' ?

[stevenjb, 2017-05-31 16:21:18]

still lgtm

[dschuyler, 2017-05-31 18:23:17]

https://codereview.chromium.org/2886843005/diff/140001/chrome/test/data/webui...
File chrome/test/data/webui/i18n_behavior_test.html (right):

https://codereview.chromium.org/2886843005/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/i18n_behavior_test.html:40: assertEquals("I'm just text,
nobody should have a problem with me!",
On 2017/05/31 15:38:41, xiyuan wrote:
> nit: " -> ' ?

I could go either way on this, but there does seem to be a consistent pattern of
using "" if there is a ' in the string. e.g. lines 10 and 13 above. If ya have a
strong preference for only using  '' strings, I'd go ahead and change lines 10
and 13 too. If it's not too important, I'd leave it as is. wdyt?

https://codereview.chromium.org/2886843005/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/i18n_behavior_test.html:42: }
On 2017/05/24 02:08:45, Dan Beam wrote:
> hmmmm, seems like we could make a more comprehensive test.  it doesn't
> necessarily have to create an OOBE UI, but it does seem like we could continue
> to use I18nBehavior statically or [preferably] make a PolymerElement that has
> behaviors: [I18nBehavior] and use this in conjunction with
> loadTimeData.overrideValues({language: '...'}) and call i18nUpdateLocale() (or
> just set locale = directly)

I plan to follow up on this later (tests and the future of i18n+loadTimeData).

[xiyuan, 2017-05-31 18:31:50]

https://codereview.chromium.org/2886843005/diff/140001/chrome/test/data/webui...
File chrome/test/data/webui/i18n_behavior_test.html (right):

https://codereview.chromium.org/2886843005/diff/140001/chrome/test/data/webui...
chrome/test/data/webui/i18n_behavior_test.html:40: assertEquals("I'm just text,
nobody should have a problem with me!",
On 2017/05/31 18:23:16, dschuyler wrote:
> On 2017/05/31 15:38:41, xiyuan wrote:
> > nit: " -> ' ?
> 
> I could go either way on this, but there does seem to be a consistent pattern
of
> using "" if there is a ' in the string. e.g. lines 10 and 13 above. If ya have
a
> strong preference for only using  '' strings, I'd go ahead and change lines 10
> and 13 too. If it's not too important, I'd leave it as is. wdyt?

Both JS style guides (google or chromium) prefer ' over " and do not mention the
exception of having a ' in the string literal.

https://google.github.io/styleguide/javascriptguide.xml?showone=Strings#Strings
https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md...

Anyway, I don't feel strongly about this. It is a nit so you can leave it as-is.

[dschuyler, 2017-05-31 21:20:06]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2017-05-31 21:21:30]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 22:21:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 22:21:16]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dschuyler, 2017-06-01 18:16:23]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2017-06-01 18:17:16]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 18:22:21]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1496340983576630,
"parent_rev": "4dc3eafa33faa138cf508bdcea933813c4a042dd", "commit_rev":
"cd00499d569a693685c81d4042b3cfe3219edd61"}

[commit-bot: I haz the power, 2017-06-01 18:22:32]

Description was changed from

==========
[i18n] use Polymer data binding to change locale strings

This CL uses Polymer variable binding to swap out i18n strings rather
than using i18n-content.

BUG=692763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[i18n] use Polymer data binding to change locale strings

This CL uses Polymer variable binding to swap out i18n strings rather
than using i18n-content.

BUG=692763
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2886843005
Cr-Commit-Position: refs/heads/master@{#476358}
Committed:
https://chromium.googlesource.com/chromium/src/+/cd00499d569a693685c81d4042b3...
==========

[commit-bot: I haz the power, 2017-06-01 18:22:35]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/cd00499d569a693685c81d4042b3...