[MD settings] i18n template replacements in shared HTML resources

[MD settings] i18n template replacements in shared HTML resources

This CL unifies the DictionaryValue to TemplateReplacements and provides
for $i18n{} replacements in shared resources. This CL doesn't include
the html changes to convert i18n-* to $i18n{} replacements. This is so
that those changes (which may be large) can be mechanical replacements
only (without these code changs).

BUG=677338
Committed: https://crrev.com/fe6f5905169ab6f89e7a84a69358269686a8274b
Cr-Commit-Position: refs/heads/master@{#441540}

[dschuyler, 2016-12-28 23:13:00]

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

[commit-bot: I haz the power, 2016-12-28 23:13:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-28 23:22:19]

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

[commit-bot: I haz the power, 2016-12-28 23:22:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2016-12-28 23:23:26]

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

[commit-bot: I haz the power, 2016-12-29 00:09:58]

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

[commit-bot: I haz the power, 2016-12-29 00:09:59]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-01-03 20:03:15]

do we really want to support booleans in $i18n{}?  is that helpful?

https://codereview.chromium.org/2607843002/diff/20001/ui/base/template_expres...
File ui/base/template_expressions.cc (right):

https://codereview.chromium.org/2607843002/diff/20001/ui/base/template_expres...
ui/base/template_expressions.cc:43: // if truthy and by complete absence (empty
string) if falsy.
this comment is confusing.

hidden="" hides stuff, and disabled="" disables.

https://jsfiddle.net/1ezdb6cs/

I think what you mean is that if setAttribute('blah', '') is called, a boolean
attribute is added (boolean attribute means no value).  if a value is processed
with a false value, we shouldn't be calling setAttribute().  as in, we should
drop a 2-liner into i18n_template_no_process.js after we've gotten the propName
and value:

if (value === false)
  continue;

and update any code that relies on this (i.e. CSS that looks for
=="true"/"false" or something).

https://cs.chromium.org/chromium/src/ui/webui/resources/js/i18n_template_no_p...

[dschuyler, 2017-01-03 20:36:25]

On 2017/01/03 20:03:15, Dan Beam wrote:
> do we really want to support booleans in $i18n{}?  is that helpful?

It is helpful in a few cases, such as guestMode. But it's not a fix-all
replacement. In some cases, JavaScript handling of the boolean value is still a
better choice.

> 
>
https://codereview.chromium.org/2607843002/diff/20001/ui/base/template_expres...
> File ui/base/template_expressions.cc (right):
> 
>
https://codereview.chromium.org/2607843002/diff/20001/ui/base/template_expres...
> ui/base/template_expressions.cc:43: // if truthy and by complete absence
(empty
> string) if falsy.
> this comment is confusing.
> 
> hidden="" hides stuff, and disabled="" disables.
> 
> https://jsfiddle.net/1ezdb6cs/
> 
> I think what you mean is that if setAttribute('blah', '') is called, a boolean
> attribute is added (boolean attribute means no value).  if a value is
processed
> with a false value, we shouldn't be calling setAttribute().  as in, we should
> drop a 2-liner into i18n_template_no_process.js after we've gotten the
propName
> and value:
> 
> if (value === false)
>   continue;
> 
> and update any code that relies on this (i.e. CSS that looks for
> =="true"/"false" or something).
> 
>
https://cs.chromium.org/chromium/src/ui/webui/resources/js/i18n_template_no_p...

[dschuyler, 2017-01-03 20:36:57]

https://codereview.chromium.org/2607843002/diff/20001/ui/base/template_expres...
File ui/base/template_expressions.cc (right):

https://codereview.chromium.org/2607843002/diff/20001/ui/base/template_expres...
ui/base/template_expressions.cc:43: // if truthy and by complete absence (empty
string) if falsy.
On 2017/01/03 20:03:15, Dan Beam wrote:
> this comment is confusing.
> 
> hidden="" hides stuff, and disabled="" disables.
> 
> https://jsfiddle.net/1ezdb6cs/
> 
> I think what you mean is that if setAttribute('blah', '') is called, a boolean
> attribute is added (boolean attribute means no value).  if a value is
processed
> with a false value, we shouldn't be calling setAttribute().  as in, we should
> drop a 2-liner into i18n_template_no_process.js after we've gotten the
propName
> and value:
> 
> if (value === false)
>   continue;
> 
> and update any code that relies on this (i.e. CSS that looks for
> =="true"/"false" or something).
> 
>
https://cs.chromium.org/chromium/src/ui/webui/resources/js/i18n_template_no_p...

Correct, so in this case:

<div $i18n{guestMode}>...</div>

TemplateReplacementsSetBool("guestMode", true, ...) will add a guestMode
attribute and 

TemplateReplacementsSetBool("guestMode", false, ...) will not add a guestMode
attribute.

You make a good point about hidden and disabled though. Having a i18n tag of 
mplateReplacementsSetBool("hidden", true, ...) is not very specific (i.e. what's
being hidden).

For cases that need to be handled in JavaScript, the current loadTimeData will
continue to operate with actual true and false Boolean values.

[dschuyler, 2017-01-03 21:33:03]

On 2017/01/03 20:36:57, dschuyler wrote:
>
https://codereview.chromium.org/2607843002/diff/20001/ui/base/template_expres...
> File ui/base/template_expressions.cc (right):
> 
>
https://codereview.chromium.org/2607843002/diff/20001/ui/base/template_expres...
> ui/base/template_expressions.cc:43: // if truthy and by complete absence
(empty
> string) if falsy.
> On 2017/01/03 20:03:15, Dan Beam wrote:
> > this comment is confusing.
> > 
> > hidden="" hides stuff, and disabled="" disables.
> > 
> > https://jsfiddle.net/1ezdb6cs/
> > 
> > I think what you mean is that if setAttribute('blah', '') is called, a
boolean
> > attribute is added (boolean attribute means no value).  if a value is
> processed
> > with a false value, we shouldn't be calling setAttribute().  as in, we
should
> > drop a 2-liner into i18n_template_no_process.js after we've gotten the
> propName
> > and value:
> > 
> > if (value === false)
> >   continue;
> > 
> > and update any code that relies on this (i.e. CSS that looks for
> > =="true"/"false" or something).
> > 
> >
>
https://cs.chromium.org/chromium/src/ui/webui/resources/js/i18n_template_no_p...
> 
> Correct, so in this case:
> 
> <div $i18n{guestMode}>...</div>
> 
> TemplateReplacementsSetBool("guestMode", true, ...) will add a guestMode
> attribute and 
> 
> TemplateReplacementsSetBool("guestMode", false, ...) will not add a guestMode
> attribute.
> 
> You make a good point about hidden and disabled though. Having a i18n tag of 
> mplateReplacementsSetBool("hidden", true, ...) is not very specific (i.e.
what's
> being hidden).
> 
> For cases that need to be handled in JavaScript, the current loadTimeData will
> continue to operate with actual true and false Boolean values.

As just discussed off-line, we may not want to do this for guestMode anyway.
I'll look into where and what uses this style of boolean attribute (i.e. is it
just guestMode).

[Dan Beam, 2017-01-03 21:33:42]

On 2017/01/03 20:36:57, dschuyler wrote:
>
https://codereview.chromium.org/2607843002/diff/20001/ui/base/template_expres...
> File ui/base/template_expressions.cc (right):
> 
>
https://codereview.chromium.org/2607843002/diff/20001/ui/base/template_expres...
> ui/base/template_expressions.cc:43: // if truthy and by complete absence
(empty
> string) if falsy.
> On 2017/01/03 20:03:15, Dan Beam wrote:
> > this comment is confusing.
> > 
> > hidden="" hides stuff, and disabled="" disables.
> > 
> > https://jsfiddle.net/1ezdb6cs/
> > 
> > I think what you mean is that if setAttribute('blah', '') is called, a
boolean
> > attribute is added (boolean attribute means no value).  if a value is
> processed
> > with a false value, we shouldn't be calling setAttribute().  as in, we
should
> > drop a 2-liner into i18n_template_no_process.js after we've gotten the
> propName
> > and value:
> > 
> > if (value === false)
> >   continue;
> > 
> > and update any code that relies on this (i.e. CSS that looks for
> > =="true"/"false" or something).
> > 
> >
>
https://cs.chromium.org/chromium/src/ui/webui/resources/js/i18n_template_no_p...
> 
> Correct, so in this case:
> 
> <div $i18n{guestMode}>...</div>
> 
> TemplateReplacementsSetBool("guestMode", true, ...) will add a guestMode
> attribute and 
> 
> TemplateReplacementsSetBool("guestMode", false, ...) will not add a guestMode
> attribute.
> 
> You make a good point about hidden and disabled though. Having a i18n tag of 
> mplateReplacementsSetBool("hidden", true, ...) is not very specific (i.e.
what's
> being hidden).
> 
> For cases that need to be handled in JavaScript, the current loadTimeData will
> continue to operate with actual true and false Boolean values.

right, I say we make i18n-values ignore boolean attributes (or maybe even only
allow strings) and fix the specific cases that use

  <div id="blah" i18n-values="attrName:boolValue">

to just do

  if (loadTimeData.getBoolean('boolValue'))
    $('blah').setAttribute('attrName', 'true');  // or ''

[dschuyler, 2017-01-03 22:01:17]

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

[commit-bot: I haz the power, 2017-01-03 22:02:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-01-03 22:04:07]

There are only a few places that would use the boolean values.
Removed the boolean handling.

[dschuyler, 2017-01-03 22:38:17]

On 2017/01/03 22:04:07, dschuyler wrote:
> There are only a few places that would use the boolean values.
> Removed the boolean handling.

Though doing it via JavaScript may introduce a flash or unwanted state on page
load. E.g. bookmarkbarattached is set by i18n and by JavaScript. My machine is
likely to be too fast to see the flash clearly, but the CL that adds it says
that the CL is fixing an issue it how it's being centered (i.e. So that the
correct CSS is applied).

[commit-bot: I haz the power, 2017-01-03 23:23:33]

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

[commit-bot: I haz the power, 2017-01-03 23:23:35]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-01-04 03:03:07]

On 2017/01/03 22:38:17, dschuyler wrote:
> On 2017/01/03 22:04:07, dschuyler wrote:
> > There are only a few places that would use the boolean values.
> > Removed the boolean handling.
> 
> Though doing it via JavaScript may introduce a flash or unwanted state on page
> load. E.g. bookmarkbarattached is set by i18n and by JavaScript. My machine is
> likely to be too fast to see the flash clearly, but the CL that adds it says
> that the CL is fixing an issue it how it's being centered (i.e. So that the
> correct CSS is applied).

for the profile, I think we should make an @enum {string} for profile mode,
essentially, as in: 'supervised', 'guest', or 'user'.

for the other stuff: I'd just stuff a string into an attribute instead of a
boolean, i.e.:

bookmarksbarattached="$i18n{bookmarksBarAttached}"

where the contents of map["bookmarksBarAttached"] in the preprocessor in C++ are
like "yes" or "no" depending on the pref.

[dschuyler, 2017-01-04 03:11:23]

On 2017/01/04 03:03:07, Dan Beam wrote:
> On 2017/01/03 22:38:17, dschuyler wrote:
> > On 2017/01/03 22:04:07, dschuyler wrote:
> > > There are only a few places that would use the boolean values.
> > > Removed the boolean handling.
> > 
> > Though doing it via JavaScript may introduce a flash or unwanted state on
page
> > load. E.g. bookmarkbarattached is set by i18n and by JavaScript. My machine
is
> > likely to be too fast to see the flash clearly, but the CL that adds it says
> > that the CL is fixing an issue it how it's being centered (i.e. So that the
> > correct CSS is applied).
> 
> for the profile, I think we should make an @enum {string} for profile mode,
> essentially, as in: 'supervised', 'guest', or 'user'.
> 
> for the other stuff: I'd just stuff a string into an attribute instead of a
> boolean, i.e.:
> 
> bookmarksbarattached="$i18n{bookmarksBarAttached}"
> 
> where the contents of map["bookmarksBarAttached"] in the preprocessor in C++
are
> like "yes" or "no" depending on the pref.

[dschuyler, 2017-01-04 03:12:01]

On 2017/01/04 03:11:23, dschuyler wrote:
> On 2017/01/04 03:03:07, Dan Beam wrote:
> > On 2017/01/03 22:38:17, dschuyler wrote:
> > > On 2017/01/03 22:04:07, dschuyler wrote:
> > > > There are only a few places that would use the boolean values.
> > > > Removed the boolean handling.
> > > 
> > > Though doing it via JavaScript may introduce a flash or unwanted state on
> page
> > > load. E.g. bookmarkbarattached is set by i18n and by JavaScript. My
machine
> is
> > > likely to be too fast to see the flash clearly, but the CL that adds it
says
> > > that the CL is fixing an issue it how it's being centered (i.e. So that
the
> > > correct CSS is applied).
> > 
> > for the profile, I think we should make an @enum {string} for profile mode,
> > essentially, as in: 'supervised', 'guest', or 'user'.
> > 
> > for the other stuff: I'd just stuff a string into an attribute instead of a
> > boolean, i.e.:
> > 
> > bookmarksbarattached="$i18n{bookmarksBarAttached}"
> > 
> > where the contents of map["bookmarksBarAttached"] in the preprocessor in C++
> are
> > like "yes" or "no" depending on the pref.

That sounds reasonable. How does the CL look without the special handling for
Booleans?

[Dan Beam, 2017-01-04 22:41:20]

lgtm

https://codereview.chromium.org/2607843002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/options_ui.cc (right):

https://codereview.chromium.org/2607843002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/options_ui.cc:205:
ui::TemplateReplacementsFromDictionaryValue(*(localized_strings_.get()),
nit: just *localized_strings_

[Dan Beam, 2017-01-04 22:41:51]

https://codereview.chromium.org/2607843002/diff/40001/ui/base/template_expres...
File ui/base/template_expressions.cc (right):

https://codereview.chromium.org/2607843002/diff/40001/ui/base/template_expres...
ui/base/template_expressions.cc:27: if
(it.value().IsType(base::Value::Type::STRING)) {
might be worth writing a small test for this (but it's basically dead simple, so
whatevs)

[dschuyler, 2017-01-04 23:16:22]

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

[commit-bot: I haz the power, 2017-01-04 23:17:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-01-04 23:18:29]

I'll think a bit on the unit test.

https://codereview.chromium.org/2607843002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/options_ui.cc (right):

https://codereview.chromium.org/2607843002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/options_ui.cc:205:
ui::TemplateReplacementsFromDictionaryValue(*(localized_strings_.get()),
On 2017/01/04 22:41:20, Dan Beam wrote:
> nit: just *localized_strings_

Done.

https://codereview.chromium.org/2607843002/diff/40001/ui/base/template_expres...
File ui/base/template_expressions.cc (right):

https://codereview.chromium.org/2607843002/diff/40001/ui/base/template_expres...
ui/base/template_expressions.cc:27: if
(it.value().IsType(base::Value::Type::STRING)) {
On 2017/01/04 22:41:51, Dan Beam wrote:
> might be worth writing a small test for this (but it's basically dead simple,
so
> whatevs)

Acknowledged.

[commit-bot: I haz the power, 2017-01-05 01:01:18]

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

[commit-bot: I haz the power, 2017-01-05 01:01:18]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-01-05 01:03:56]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2017-01-05 01:03:56]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2607843002/#ps60001
(title: "nits")

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

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-01-05 01:10:12]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483578236169070,
"parent_rev": "7abb597b7772f7656ad29f03e8d0e51499c7b0b4", "commit_rev":
"75ed9a34944a2e18ffd5c2d588c240b6767c304d"}

[commit-bot: I haz the power, 2017-01-05 01:11:44]

Description was changed from

==========
[MD settings] i18n template replacements in shared HTML resources

This CL unifies the DictionaryValue to TemplateReplacements and provides
for $i18n{} replacements in shared resources. This CL doesn't include
the html changes to convert i18n-* to $i18n{} replacements. This is so
that those changes (which may be large) can be mechanical replacements
only (without these code changs).

BUG=677338
==========

to

==========
[MD settings] i18n template replacements in shared HTML resources

This CL unifies the DictionaryValue to TemplateReplacements and provides
for $i18n{} replacements in shared resources. This CL doesn't include
the html changes to convert i18n-* to $i18n{} replacements. This is so
that those changes (which may be large) can be mechanical replacements
only (without these code changs).

BUG=677338

Review-Url: https://codereview.chromium.org/2607843002
==========

[commit-bot: I haz the power, 2017-01-05 01:11:45]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2017-01-05 01:15:30]

Description was changed from

==========
[MD settings] i18n template replacements in shared HTML resources

This CL unifies the DictionaryValue to TemplateReplacements and provides
for $i18n{} replacements in shared resources. This CL doesn't include
the html changes to convert i18n-* to $i18n{} replacements. This is so
that those changes (which may be large) can be mechanical replacements
only (without these code changs).

BUG=677338

Review-Url: https://codereview.chromium.org/2607843002
==========

to

==========
[MD settings] i18n template replacements in shared HTML resources

This CL unifies the DictionaryValue to TemplateReplacements and provides
for $i18n{} replacements in shared resources. This CL doesn't include
the html changes to convert i18n-* to $i18n{} replacements. This is so
that those changes (which may be large) can be mechanical replacements
only (without these code changs).

BUG=677338

Committed: https://crrev.com/fe6f5905169ab6f89e7a84a69358269686a8274b
Cr-Commit-Position: refs/heads/master@{#441540}
==========

[commit-bot: I haz the power, 2017-01-05 01:15:32]

Patchset 4 (id:??) landed as
https://crrev.com/fe6f5905169ab6f89e7a84a69358269686a8274b
Cr-Commit-Position: refs/heads/master@{#441540}