Refactors parse_html_subset() into i18n_behavior.

Refactors parse_html_subset() into i18n_behavior.

R=dbeam@chromium.org
BUG=605226
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/20bb518435ea84cea2584891f4bf2a4180814bf9
Cr-Commit-Position: refs/heads/master@{#392057}

[Moe, 2016-04-26 15:22:51]

Hi Dan, please review this CL.

[Dan Beam, 2016-04-27 02:38:27]

https://codereview.chromium.org/1920253002/diff/1/ui/webui/resources/js/i18n_...
File ui/webui/resources/js/i18n_behavior.js (right):

https://codereview.chromium.org/1920253002/diff/1/ui/webui/resources/js/i18n_...
ui/webui/resources/js/i18n_behavior.js:28: opt_values.unshift(id);
don't do this, opt_values is passed by reference and now you've effectively
modified it from the call site

https://codereview.chromium.org/1920253002/diff/1/ui/webui/resources/js/i18n_...
ui/webui/resources/js/i18n_behavior.js:52: },
i don't really understand what you're doing here, but your code should probably
look something like this:

rename i18n -> i18nRaw

make a new i18n that calls i18nRaw() and runs the result through 

  return parseHtmlSubset('<div>' + raw + '</div>').firstChild.innerHTML;

why would the public API of this code need to change?  where are opt_extraTags
or opt_extraAttrs actually used?  they could probably be one-offs instead and
only allow the defaults of <a href={chrome,https}:// target=_blank> or whatever

[Moe, 2016-04-27 15:08:46]

https://codereview.chromium.org/1920253002/diff/1/ui/webui/resources/js/i18n_...
File ui/webui/resources/js/i18n_behavior.js (right):

https://codereview.chromium.org/1920253002/diff/1/ui/webui/resources/js/i18n_...
ui/webui/resources/js/i18n_behavior.js:28: opt_values.unshift(id);
On 2016/04/27 02:38:27, Dan Beam wrote:
> don't do this, opt_values is passed by reference and now you've effectively
> modified it from the call site

yep. this is pretty dumb.

https://codereview.chromium.org/1920253002/diff/1/ui/webui/resources/js/i18n_...
ui/webui/resources/js/i18n_behavior.js:52: },
On 2016/04/27 02:38:27, Dan Beam wrote:
> i don't really understand what you're doing here, but your code should
probably
> look something like this:
> 
> rename i18n -> i18nRaw
> 
> make a new i18n that calls i18nRaw() and runs the result through 
> 
>   return parseHtmlSubset('<div>' + raw + '</div>').firstChild.innerHTML;
> 
> why would the public API of this code need to change?  where are opt_extraTags
> or opt_extraAttrs actually used?  they could probably be one-offs instead and
> only allow the defaults of <a href={chrome,https}:// target=_blank> or
whatever

This code is doing exactly that (the new function being i18nNoSanitizeHtml
instead of i18nRaw).

As far as the public API, if we were to expose opt_extraTags and opt_extraAttrs
it would have to change. 
IMO, there's no harm in exposing these optional arguments. There could well be
strings with <br>, <em>, <span> etc that will be using this. Of course, the
other option is to include any new case we come across in the parseHtmlSubset
defaults!
As per your comment though, I'll go ahead and revert the changes to public API.

[Dan Beam, 2016-05-03 04:25:18]

i'm fine with this, but is there any noticeable performance difference?  it'd be
nice to make sure this doesn't slow anybody down

lgtm

[Moe, 2016-05-03 18:15:12]

On 2016/05/03 04:25:18, Dan Beam wrote:
> i'm fine with this, but is there any noticeable performance difference?  it'd
be
> nice to make sure this doesn't slow anybody down
> 
> lgtm


Here are the averages of 'page load time' and 'test run time' of browser tests
for the settings page, User Manager page, the Extensions page over 5 runs:

--gtest_filter=CrSettings*:

before: (avg page load time: 1059 ms, avg test run time: 1582 ms)
after: (avg page load time: 1078 ms, avg test run time: 1635 ms)

--gtest_filter=UserManagerBrowserTest*:

before: (avg page load time: 1579 ms, avg test run time: 5072 ms)
after: (avg page load time: 1521 ms, avg test run time: 5018 ms)

--gtest_filter=CrExtensionsBrowserTest*:

before: (avg page load time: 1467 ms, avg test run time: 646 ms)
after: (avg page load time: 1480 ms, avg test run time: 616 ms)

Unless there numbers are not the best metrics to track, they don't show any
significant performance difference.

[Moe, 2016-05-03 23:11:08]

Description was changed from

==========
Refactors parse_html_subset() into i18n_behavior.

R=dbeam@chromium.org
BUG=605226
==========

to

==========
Refactors parse_html_subset() into i18n_behavior.

R=dbeam@chromium.org
BUG=605226
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-05-03 23:23:18]

i might be cool with is="action-link", but why can't we do this for the specific
translations that require them?

[Moe, 2016-05-04 13:47:38]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2016-05-04 13:47:39]

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/1920253002/#ps60001
(title: "Whitelists <span> and <a is='action-link>")

[Moe, 2016-05-04 13:47:41]

The CQ bit was unchecked by mahmadi@chromium.org

[Moe, 2016-05-04 13:53:39]

Patchset #4 (id:60001) has been deleted

[Moe, 2016-05-04 14:32:10]

Dan, I replaced the previous patch with one that exposes another function in the
i18n_behavior API in order to pass in all the action-link related attributes for
strings that need them.

[Dan Beam, 2016-05-04 16:02:35]

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

https://codereview.chromium.org/1920253002/diff/80001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:59: return parseHtmlSubset('<b>' +
rawString + '</b>',
what if opt_extraTags or opt_extraAttrs doesn't allow 'B'?

https://codereview.chromium.org/1920253002/diff/80001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:62: },
i don't like having to pass undefined in order to use a later-ordered parameter.
 can we make these named, optional keys in a dictionary instead?

/**
 * @param {string} id
 * @param {{substitutions: (Array<string>|undefined),
 *          attrs: (Object<function(Node, string):boolean>|undefined),
 *          tags: (Array<string>|undefined)}} opts
 */
i18nAdvanced: function(id, opts) {
  var rawString = this.i18nRaw.apply(this, [id].concat(opts.substitutions));
  var tags = (opts.tags || []).concat('B');
  var df = parseHtmlSubset('<b>' + rawString + '</b>', tags, opts.attrs);
  return df.firstChild.innerHTML;
}

[Moe, 2016-05-04 21:41:18]

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

https://codereview.chromium.org/1920253002/diff/80001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:59: return parseHtmlSubset('<b>' +
rawString + '</b>',
On 2016/05/04 16:02:35, Dan Beam wrote:
> what if opt_extraTags or opt_extraAttrs doesn't allow 'B'?

parseHtmlSubset allows B by default. But, it safer to pass in 'B' here as well.

https://codereview.chromium.org/1920253002/diff/80001/ui/webui/resources/js/i...
ui/webui/resources/js/i18n_behavior.js:62: },
On 2016/05/04 16:02:35, Dan Beam wrote:
> i don't like having to pass undefined in order to use a later-ordered
parameter.
>  can we make these named, optional keys in a dictionary instead?
> 
> /**
>  * @param {string} id
>  * @param {{substitutions: (Array<string>|undefined),
>  *          attrs: (Object<function(Node, string):boolean>|undefined),
>  *          tags: (Array<string>|undefined)}} opts
>  */
> i18nAdvanced: function(id, opts) {
>   var rawString = this.i18nRaw.apply(this, [id].concat(opts.substitutions));
>   var tags = (opts.tags || []).concat('B');
>   var df = parseHtmlSubset('<b>' + rawString + '</b>', tags, opts.attrs);
>   return df.firstChild.innerHTML;
> }

Makes sense. Done!

[Dan Beam, 2016-05-05 16:43:36]

lgtm with very optional nits

https://codereview.chromium.org/1920253002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/create_profile.js (right):

https://codereview.chromium.org/1920253002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/create_profile.js:413: return
node.tagName == 'A' && value != '';
i suppose checking for tagName == 'A' isn't bad, but why != ''?

https://codereview.chromium.org/1920253002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/create_profile.js:418: return
this.i18nAdvanced(id, opts);
this would be so much prettier

  return this.i18nAdvanced(id, {attrs: {id: () => true}})

oh well, we can't use => everywhere yet :(

https://codereview.chromium.org/1920253002/diff/100001/ui/webui/resources/js/...
File ui/webui/resources/js/i18n_behavior.js (right):

https://codereview.chromium.org/1920253002/diff/100001/ui/webui/resources/js/...
ui/webui/resources/js/i18n_behavior.js:55: [id].concat(opts.substitutions ||
[]));
i personally find this prettier

var args = [id].concat(opts.substitutions || []);
var rawString = this.i18nRaw.apply(this, args);

https://codereview.chromium.org/1920253002/diff/100001/ui/webui/resources/js/...
ui/webui/resources/js/i18n_behavior.js:56: var tags = (opts.tags ||
[]).concat('B');
ah, I thought passing in a list of tags /replaced/ the default list, but it
actually merges with.

in that case, you could probably just do:

return parseHtmlSubset('<b>' + rawString + '</b>', opt.tags, opts.attrs)
    .firstChild.innerHTML;

similar to what you were doing previously.  up to you.  the default list is not
likely to remove 'B' soon, if I had to guess.

[Moe, 2016-05-06 14:24:59]

Thank you!

https://codereview.chromium.org/1920253002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/create_profile.js (right):

https://codereview.chromium.org/1920253002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/create_profile.js:413: return
node.tagName == 'A' && value != '';
On 2016/05/05 16:43:36, Dan Beam wrote:
> i suppose checking for tagName == 'A' isn't bad, but why != ''?

It's not needed. removed.

https://codereview.chromium.org/1920253002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/create_profile.js:418: return
this.i18nAdvanced(id, opts);
On 2016/05/05 16:43:36, Dan Beam wrote:
> this would be so much prettier
> 
>   return this.i18nAdvanced(id, {attrs: {id: () => true}})
> 
> oh well, we can't use => everywhere yet :(

I agree. When can we start using ES6 in chromium? Also on a different note, I
saw a reviewer asking in a CL not to use 'let' instead of 'var'. Is the use of
'let' discouraged?

https://codereview.chromium.org/1920253002/diff/100001/ui/webui/resources/js/...
File ui/webui/resources/js/i18n_behavior.js (right):

https://codereview.chromium.org/1920253002/diff/100001/ui/webui/resources/js/...
ui/webui/resources/js/i18n_behavior.js:55: [id].concat(opts.substitutions ||
[]));
On 2016/05/05 16:43:36, Dan Beam wrote:
> i personally find this prettier
> 
> var args = [id].concat(opts.substitutions || []);
> var rawString = this.i18nRaw.apply(this, args);

Done.

https://codereview.chromium.org/1920253002/diff/100001/ui/webui/resources/js/...
ui/webui/resources/js/i18n_behavior.js:56: var tags = (opts.tags ||
[]).concat('B');
On 2016/05/05 16:43:36, Dan Beam wrote:
> ah, I thought passing in a list of tags /replaced/ the default list, but it
> actually merges with.
> 
> in that case, you could probably just do:
> 
> return parseHtmlSubset('<b>' + rawString + '</b>', opt.tags, opts.attrs)
>     .firstChild.innerHTML;
> 
> similar to what you were doing previously.  up to you.  the default list is
not
> likely to remove 'B' soon, if I had to guess.

Done.

[Moe, 2016-05-06 14:25:04]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2016-05-06 14:25:04]

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/1920253002/#ps120001
(title: "Addressed comments")

[commit-bot: I haz the power, 2016-05-06 14:25:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1920253002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1920253002/120001

[commit-bot: I haz the power, 2016-05-06 14:44:13]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-05-06 14:45:28]

Description was changed from

==========
Refactors parse_html_subset() into i18n_behavior.

R=dbeam@chromium.org
BUG=605226
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Refactors parse_html_subset() into i18n_behavior.

R=dbeam@chromium.org
BUG=605226
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/20bb518435ea84cea2584891f4bf2a4180814bf9
Cr-Commit-Position: refs/heads/master@{#392057}
==========

[commit-bot: I haz the power, 2016-05-06 14:45:29]

Patchset 6 (id:??) landed as
https://crrev.com/20bb518435ea84cea2584891f4bf2a4180814bf9
Cr-Commit-Position: refs/heads/master@{#392057}