MD Settings: Languages model for language pages

MD Settings: Languages model for language pages

This fleshes out the languages model and partially implements
language handling.

BUG=512479
Committed: https://crrev.com/0f479fec97b28c2d195777919f4abb4a8bced98e
Cr-Commit-Position: refs/heads/master@{#350762}

[michaelpg, 2015-09-21 02:19:59]

Patchset #2 (id:20001) has been deleted

[michaelpg, 2015-09-21 02:20:03]

Patchset #1 (id:1) has been deleted

[michaelpg, 2015-09-21 02:22:53]

michaelpg@chromium.org changed reviewers:
+ dbeam@chromium.org, stevenjb@chromium.org

[michaelpg, 2015-09-21 02:22:54]

Partial implementation of Languages, PTAL.

dbeam: web stuff and closure
stevenjb: other stuff (C++, gyp, bower)

https://codereview.chromium.org/1351623008/diff/20002/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/compiled_resources.gyp
(right):

https://codereview.chromium.org/1351623008/diff/20002/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/compiled_resources.gyp:31:
'../prefs/compiled_resources.gyp:prefs',
This is a pain: languages_model depends on prefs and prefs_types, and prefs
depends on prefs_types, so why don't these dependencies propagate without me
having to list them here?

[michaelpg, 2015-09-21 07:53:00]

https://codereview.chromium.org/1351623008/diff/20002/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/compiled_resources.gyp
(right):

https://codereview.chromium.org/1351623008/diff/20002/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/compiled_resources.gyp:32:
'languages_model.js',
Same thing here: adding languages_model.js seems to make languages_model.js be
compiled twice. But if I remove it, languages_page can't compile:

## src/chrome/browser/resources/settings/languages_page/languages_page.js:37:
ERROR - Bad type annotation. Unknown type LanguagesModel
##      * @type {(LanguagesModel|undefined)}
##                ^
## 
## src/chrome/browser/resources/settings/languages_page/languages_page.js:52:
ERROR - Property setUILanguage never defined on ?
##     this.$.languages.setUILanguage(e.model.item.language.code);
##                      ^

[stevenjb, 2015-09-21 20:43:55]

Looks like PS2 includes a Polymer update. If that was intentional, it should be
moved to a separate CL...

[michaelpg, 2015-09-21 21:19:07]

On 2015/09/21 20:43:55, stevenjb wrote:
> Looks like PS2 includes a Polymer update. If that was intentional, it should
be
> moved to a separate CL...

Quite right, done.

[stevenjb, 2015-09-22 01:45:16]

Only had a chance to get through languages_model.js so far. Will continue first
thing tomorrow.

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages_model.js
(right):

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_model.js:55: * Shared
private state.
nit: This only kind of makes sense to me because we've talked about it some. I
think that a comment or reference to a document describing how this works would
be helpful.

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_model.js:74: *
language. This dosen't affect the actual UI language until a restart.
Rather than duplicating these comments, I would either comment each with
something like '* Forwarding function' or document the whole section since it is
pretty clear what these are doing.

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_model.js:133:
this.squelch_ = true;
I don't see anywhere that we check |squelch_| ?

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_model.js:172: * Hash
map of model.supportedLanguages for fast lookup. Not data bound.
nit: describe what the key is for this map and the one below (language code I
assume?)

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_model.js:234: var
languageCodes = this.get(preferredLanguagesPath).split(',');
Could this.preferredLanguagesPath be undefined if the pref isn't set for some
reason? It seems like something we probably shouldn't assume here and should
handle (maybe log an error) instead of calling 'split' on undefined.

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_model.js:254:
this.get('prefs.spellcheck.dictionaries'));
Here too, maybe handle spellCheckPref == undefined

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_model.js:303:
chrome.send('setUILanguage', [languageCode]);
FWIW: Since we're using a chrome.send call here anyway, I would personally
rather do the win/cros logic there.

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_model.js:311: var
languageCodes = this.get(preferredLanguagesPath);
Handle undefined?

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_model.js:325:
navigator.language;
Handle undefined?

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_model.js:329: var
languageCodes = this.get(preferredLanguagesPath).split(',');
Handle undefined?

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_model.js:338:
this.splice('prefs.spellcheck.dictionaries.value', spellCheckIndex, 1);
Handle undefined?

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_model.js:362:
this.get('prefs.spellcheck.dictionaries'));
Handle undefined?

[michaelpg, 2015-09-22 03:58:44]

Patchset #4 (id:90001) has been deleted

[michaelpg, 2015-09-22 04:03:15]

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages_model.js
(right):

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_model.js:55: * Shared
private state.
On 2015/09/22 01:45:15, stevenjb wrote:
> nit: This only kind of makes sense to me because we've talked about it some. I
> think that a comment or reference to a document describing how this works
would
> be helpful.

I've added documentation in the file overview and expanded some of the comments.

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_model.js:74: *
language. This dosen't affect the actual UI language until a restart.
On 2015/09/22 01:45:15, stevenjb wrote:
> Rather than duplicating these comments, I would either comment each with
> something like '* Forwarding function' or document the whole section since it
is
> pretty clear what these are doing.

Done.

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_model.js:133:
this.squelch_ = true;
On 2015/09/22 01:45:15, stevenjb wrote:
> I don't see anywhere that we check |squelch_| ?

You're right, we don't need to because model is readOnly: we don't have to
propagate changes *to* the singleton, so we're not in danger of repeating
events. Done.

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_model.js:172: * Hash
map of model.supportedLanguages for fast lookup. Not data bound.
On 2015/09/22 01:45:16, stevenjb wrote:
> nit: describe what the key is for this map and the one below (language code I
> assume?)

Done.

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_model.js:234: var
languageCodes = this.get(preferredLanguagesPath).split(',');
On 2015/09/22 01:45:15, stevenjb wrote:
> Could this.preferredLanguagesPath be undefined if the pref isn't set for some
> reason? It seems like something we probably shouldn't assume here and should
> handle (maybe log an error) instead of calling 'split' on undefined.

Once prefs are initialized, I believe every pref should always be set, and all
of our code would be a lot simpler with that assumption. Whether they have a
user value or just a default value, the value won't be undefined.

If that assumption is wrong or we break it (e.g. by changing pref names),
functionality would be broken anyway, and the errors here will alert us to that.

TL;DR: I don't want to add a lot of guards for conditions that shouldn't happen.
I'll add some asserts to ensure these functions are only called after prefs are
initialized.

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_model.js:303:
chrome.send('setUILanguage', [languageCode]);
On 2015/09/22 01:45:15, stevenjb wrote:
> FWIW: Since we're using a chrome.send call here anyway, I would personally
> rather do the win/cros logic there.
> 

Good call -- we should be setting the LocalState pref on Windows anyway.

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages_model.js
(right):

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_model.js:114: //
Forward public methods to the singleton.
Obviously forwarding all these methods is ugly. But doing them in the instance
of this element, instead of in the singleton, is tricky at best.

We could actually implement these methods here, and say:

<cr-settings-languages-model> has functions which change prefs
<cr-settings-languages-model-private> provides the read-only data model that
reflects the prefs

But I don't know how to do that without other ugly changes -- can chat more in
depth if someone wants.

[stevenjb, 2015-09-22 16:47:34]

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages_model.js
(right):

https://codereview.chromium.org/1351623008/diff/70001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages_model.js:234: var
languageCodes = this.get(preferredLanguagesPath).split(',');
On 2015/09/22 04:03:15, michaelpg wrote:
> On 2015/09/22 01:45:15, stevenjb wrote:
> > Could this.preferredLanguagesPath be undefined if the pref isn't set for
some
> > reason? It seems like something we probably shouldn't assume here and should
> > handle (maybe log an error) instead of calling 'split' on undefined.
> 
> Once prefs are initialized, I believe every pref should always be set, and all
> of our code would be a lot simpler with that assumption. Whether they have a
> user value or just a default value, the value won't be undefined.
> 
> If that assumption is wrong or we break it (e.g. by changing pref names),
> functionality would be broken anyway, and the errors here will alert us to
that.
> 
> TL;DR: I don't want to add a lot of guards for conditions that shouldn't
happen.
> I'll add some asserts to ensure these functions are only called after prefs
are
> initialized.

Hmm. I'm not super excited about a typo in C++ code (or removal of a pref)
causing, e.g. 'languageCodes.length' to fail as an undefined reference. I don't
feel that the reason for failure will be abundantly clear.

I'd love to figure out a way to add an assert to these getters. Maybe attach a
method to |prefs|? Or a CrSettingsPrefs method? Maybe we can discuss this in
today's meeting.

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages_model.js
(right):

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_model.js:33: * This
data binding is one-way, child-to-host: the model property is read-only.
This seems to contradict 'model="{{model}}"' above? I *think* that is because
the [[]] one-way data binding is the wrong direction? We should call that out.

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_model.js:85: * The
singleton's private model, exposed as a property so hosts can bind
This is super confusing, why is the above member named 'privateModel_' but this
one described as 'private model'?

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages_page.html
(right):

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.html:77:
prefs="{{prefs}}"></cr-settings-manage-languages-page>
So... while I think I mostly get the singleton model concept... it seems like it
might be more clear just to pass model="{{model}}" here instead, the same as we
do for prefs?

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages_page.js (right):

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.js:36: *
Languages model.
I think a bit more of a comment would be useful here, e.g. Read-only reference
to this.$.languages.model.

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.js:39: model: {
I think it would clarify things a little for me if we named this property
something other than just 'model', e.g. languagesModel.

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.js:41: notify:
true,
can we make this readOnly?

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.js:52: if
(cr.isChromeOS || cr.isWindows)
I think this if() would be better in setUILanguage (or in the C++ code).
Platforms that don't support this should be hiding the UI entirely. If they
don't for some reason it should just be a no-op (or log an error).

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.js:76: //
TODO(michaelpg): This is necessary to show the list when navigating to
nit: TODO(): Remove... (?)

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/manage_languages_page.css
(right):

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/manage_languages_page.css:1:
iron-list {
+ Copyright

https://codereview.chromium.org/1351623008/diff/110001/ui/webui/resources/pol...
File ui/webui/resources/polymer_resources.grdp (right):

https://codereview.chromium.org/1351623008/diff/110001/ui/webui/resources/pol...
ui/webui/resources/polymer_resources.grdp:136: type="chrome_html" />
nit: This probably should have been in the other CL... maybe separate it? It's
probably fine in this CL, but if we do a revert and something else depends on it
it could get ugly...

[michaelpg, 2015-09-23 21:20:29]

Patchset #6 (id:150001) has been deleted

[michaelpg, 2015-09-23 21:27:28]

PTAL.

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages_model.js
(right):

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_model.js:33: * This
data binding is one-way, child-to-host: the model property is read-only.
On 2015/09/22 16:47:33, stevenjb wrote:
> This seems to contradict 'model="{{model}}"' above? I *think* that is because
> the [[]] one-way data binding is the wrong direction? We should call that out.
> 

Yes, as described by
https://www.polymer-project.org/1.0/docs/devguide/data-binding.html:

If the child property is notify: true and readOnly:true, and the host binding
uses curly-brace syntax, the binding is one-way, upward (child-to-host).

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages_page.html
(right):

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.html:77:
prefs="{{prefs}}"></cr-settings-manage-languages-page>
On 2015/09/22 16:47:33, stevenjb wrote:
> So... while I think I mostly get the singleton model concept... it seems like
it
> might be more clear just to pass model="{{model}}" here instead, the same as
we
> do for prefs?

You're not wrong, that would work, but:

* functions like enableLanguage, setUILanguage, etc. would have to be attached
to that object; we don't mix data and functions in bindings elsewhere
* i'm not a fan of sharing models via the views
* if we need to reference language settings in other pages, suddenly we have to
move the model up to the nearest common ancestor

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages_page.js (right):

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.js:36: *
Languages model.
On 2015/09/22 16:47:33, stevenjb wrote:
> I think a bit more of a comment would be useful here, e.g. Read-only reference
> to this.$.languages.model.

Done.

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.js:39: model: {
On 2015/09/22 16:47:34, stevenjb wrote:
> I think it would clarify things a little for me if we named this property
> something other than just 'model', e.g. languagesModel.
> 

Done (model -> languages).

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.js:41: notify:
true,
On 2015/09/22 16:47:34, stevenjb wrote:
> can we make this readOnly?

no, readOnly would prevent it from being updatable by data binding.
Unfortunately, Polymer doesn't have a concept of "read-only for manual updates,
but data binding works".

simplified example: http://jsbin.com/rirososude/3/edit?html,output

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.js:52: if
(cr.isChromeOS || cr.isWindows)
On 2015/09/22 16:47:33, stevenjb wrote:
> I think this if() would be better in setUILanguage (or in the C++ code).
> Platforms that don't support this should be hiding the UI entirely. If they
> don't for some reason it should just be a no-op (or log an error).
> 

The UI (a list of languages on the advanced page) is always there -- but tapping
can only "select" a language on platforms where that's possible.

Moved to setUILanguage.

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.js:76: //
TODO(michaelpg): This is necessary to show the list when navigating to
On 2015/09/22 16:47:33, stevenjb wrote:
> nit: TODO(): Remove... (?)

Done.

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/manage_languages_page.css
(right):

https://codereview.chromium.org/1351623008/diff/110001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/manage_languages_page.css:1:
iron-list {
On 2015/09/22 16:47:34, stevenjb wrote:
> + Copyright

Done.

https://codereview.chromium.org/1351623008/diff/110001/ui/webui/resources/pol...
File ui/webui/resources/polymer_resources.grdp (right):

https://codereview.chromium.org/1351623008/diff/110001/ui/webui/resources/pol...
ui/webui/resources/polymer_resources.grdp:136: type="chrome_html" />
On 2015/09/22 16:47:34, stevenjb wrote:
> nit: This probably should have been in the other CL... maybe separate it? It's
> probably fine in this CL, but if we do a revert and something else depends on
it
> it could get ugly...
> 

Done.

[stevenjb, 2015-09-23 22:27:54]

lgtm

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages.js (right):

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:62: singleton_: {
I like 'singleton' here.

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:72: languages: {
Heh, so, FWIW, I was / would be fine with this being 'model' and in the HTML
hosting an instance use model="{{languages}}", but this is fine too.

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:139:
preferredLanguagesPath = 'prefs.intl.accept_languages.value';
nit: I know that it would add a tiny bit of complexity below, but I think these
would be more clear without the 'prefs.' or '.value'.

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:155: languages: {
Same here, but it definitely makes sense to me to match the property name in
cr-settings-languages above.

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:236: var
languageCodes = this.get(preferredLanguagesPath).split(',');
What do you think of:

getPref_: function(path) {
  var result = this.get(path, this.prefs);
  cr.assert(result != undefined, 'Pref is missing: ' + path);
  return result;
}
...
var languagesCode = this.getPref_(preferredLanguagesPath).value.split(',');
?

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages_page.html
(right):

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.html:77:
prefs="{{prefs}}"></cr-settings-manage-languages-page>
With your prefs refactor, we won't need the prefs="{{prefs}}" either, correct?

[Dan Beam, 2015-09-23 23:28:17]

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages.js (right):

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:139:
preferredLanguagesPath = 'prefs.intl.accept_languages.value';
can haz ternary?

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:244: var state =
{spellCheckEnabled: !!spellCheckMap[code]};
why does this need to be a full dictionary?

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:336: if
(languageCode == appLocale)
wait, how does this happen?  can this be assert(languageCode != appLocale);?

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:339: // Don't
disable the only enabled language.
same question: don't we prevent this in the UI (so this should never be hit)?

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:377: // TODO: need
update externs before committing
TODO(michaelpg): or actually do

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages_page.html
(right):

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.html:25:
<neon-animatable id="">
id=""?

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/manage_languages_page.js
(right):

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/manage_languages_page.js:33: * 
        undefined}
nit: indent off

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/manage_languages_page.js:45: *
@param {!{model: !{item: !Language}}} e
!!!

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/manage_languages_page.js:45: *
@param {!{model: !{item: !Language}}} e
@private for all of these

[michaelpg, 2015-09-24 02:15:20]

Thanks guys, hope this gets everything that was left.

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages.js (right):

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:62: singleton_: {
On 2015/09/23 22:27:54, stevenjb wrote:
> I like 'singleton' here.

Acknowledged.

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:72: languages: {
On 2015/09/23 22:27:54, stevenjb wrote:
> Heh, so, FWIW, I was / would be fine with this being 'model' and in the HTML
> hosting an instance use model="{{languages}}", but this is fine too.

Acknowledged.

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:139:
preferredLanguagesPath = 'prefs.intl.accept_languages.value';
On 2015/09/23 22:27:54, stevenjb wrote:
> nit: I know that it would add a tiny bit of complexity below, but I think
these
> would be more clear without the 'prefs.' or '.value'.

Done.

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:139:
preferredLanguagesPath = 'prefs.intl.accept_languages.value';
On 2015/09/23 23:28:17, Dan Beam wrote:
> can haz ternary?

Done.

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:155: languages: {
On 2015/09/23 22:27:54, stevenjb wrote:
> Same here, but it definitely makes sense to me to match the property name in
> cr-settings-languages above.

yep. If we don't match property names, we'd have to rewrite the event names and
paths -- otherwise we'll forward events for properties that don't exist

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:236: var
languageCodes = this.get(preferredLanguagesPath).split(',');
On 2015/09/23 22:27:54, stevenjb wrote:
> What do you think of:
> 
> getPref_: function(path) {
>   var result = this.get(path, this.prefs);
>   cr.assert(result != undefined, 'Pref is missing: ' + path);
>   return result;
> }
> ...
> var languagesCode = this.getPref_(preferredLanguagesPath).value.split(',');
> ?

Done. It's not bad, but I'm not sure about the precedence it sets for other
files. (We could move getPref to CrSettingsPrefs but then it's a mouthful every
time we want to get a pref.)

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:244: var state =
{spellCheckEnabled: !!spellCheckMap[code]};
On 2015/09/23 23:28:17, Dan Beam wrote:
> why does this need to be a full dictionary?

other properties are a'comin, like translateEnabled

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:336: if
(languageCode == appLocale)
On 2015/09/23 23:28:17, Dan Beam wrote:
> wait, how does this happen?  can this be assert(languageCode != appLocale);?

Done.

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:339: // Don't
disable the only enabled language.
On 2015/09/23 23:28:17, Dan Beam wrote:
> same question: don't we prevent this in the UI (so this should never be hit)?

Done.

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:377: // TODO: need
update externs before committing
On 2015/09/23 23:28:17, Dan Beam wrote:
> TODO(michaelpg): or actually do

actually done (after four tries): https://codereview.chromium.org/1363853002/

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages_page.html
(right):

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.html:25:
<neon-animatable id="">
On 2015/09/23 23:28:17, Dan Beam wrote:
> id=""?

Copied from other examples. I suspect it's because of
<neon-animated-pages attr-for-selected="id">
requiring the id attribute to actually exist.

Removing the attribute results in the Languages section being blank.

https://code.google.com/p/chromium/codesearch#search/&q=file%3Aresources/sett...

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.html:77:
prefs="{{prefs}}"></cr-settings-manage-languages-page>
On 2015/09/23 22:27:54, stevenjb wrote:
> With your prefs refactor, we won't need the prefs="{{prefs}}" either, correct?

Correct. For now I'm going to keep following the convention anyway -- if this
page is buggy I'd rather it be easier to track down which CL caused it. In later
changes (once the prefs change has been in more than an hour) I'll see whether
it makes sense to keep prefs here or migrate it into one or more sub-pages.

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/manage_languages_page.js
(right):

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/manage_languages_page.js:33: * 
        undefined}
On 2015/09/23 23:28:17, Dan Beam wrote:
> nit: indent off

Done.

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/manage_languages_page.js:45: *
@param {!{model: !{item: !Language}}} e
On 2015/09/23 23:28:17, Dan Beam wrote:
> !!!

???

would you prefer it not be typed?

"model" is the binding scope of the template, so it would not be easy for the
Closure pass to provide/auto-generate a simpler type.

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/manage_languages_page.js:45: *
@param {!{model: !{item: !Language}}} e
On 2015/09/23 23:28:17, Dan Beam wrote:
> @private for all of these

Done.

[stevenjb, 2015-09-24 15:46:06]

LGTM!

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages.js (right):

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:236: var
languageCodes = this.get(preferredLanguagesPath).split(',');
On 2015/09/24 02:15:20, michaelpg wrote:
> On 2015/09/23 22:27:54, stevenjb wrote:
> > What do you think of:
> > 
> > getPref_: function(path) {
> >   var result = this.get(path, this.prefs);
> >   cr.assert(result != undefined, 'Pref is missing: ' + path);
> >   return result;
> > }
> > ...
> > var languagesCode = this.getPref_(preferredLanguagesPath).value.split(',');
> > ?
> 
> Done. It's not bad, but I'm not sure about the precedence it sets for other
> files. (We could move getPref to CrSettingsPrefs but then it's a mouthful
every
> time we want to get a pref.)

Yeah, I've been trying to think of a nice way we could integrate this into
CrSettingsPrefs, but as you say it would be an awkward mouthful. Maybe we could
use a behavior?

That said, most pages lack the complexity of this one and the data binding will
just fail (although I've been thinking about adding checks in the settings
elements, e.g. cr-settings-checkbox). Anyway, thanks for adding the check here.

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:236: var
languageCodes = this.get(preferredLanguagesPath).split(',');
On 2015/09/24 02:15:20, michaelpg wrote:
> On 2015/09/23 22:27:54, stevenjb wrote:
> > What do you think of:
> > 
> > getPref_: function(path) {
> >   var result = this.get(path, this.prefs);
> >   cr.assert(result != undefined, 'Pref is missing: ' + path);
> >   return result;
> > }
> > ...
> > var languagesCode = this.getPref_(preferredLanguagesPath).value.split(',');
> > ?
> 
> Done. It's not bad, but I'm not sure about the precedence it sets for other
> files. (We could move getPref to CrSettingsPrefs but then it's a mouthful
every
> time we want to get a pref.)

Yeah, I've been trying to think of a nice way we could integrate this into
CrSettingsPrefs, but as you say it would be an awkward mouthful. Maybe we could
use a behavior?

That said, most pages lack the complexity of this one and the data binding will
just fail (although I've been thinking about adding checks in the settings
elements, e.g. cr-settings-checkbox). Anyway, thanks for adding the check here.

https://codereview.chromium.org/1351623008/diff/210001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages.js (right):

https://codereview.chromium.org/1351623008/diff/210001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:332: languageCodes
+ ',' + languageCode);
nit: maybe add setPref_() too?

https://codereview.chromium.org/1351623008/diff/210001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:332: languageCodes
+ ',' + languageCode);
nit: maybe add setPref_() too?

https://codereview.chromium.org/1351623008/diff/210001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:361:
languageCodes.join(','));
nit: setPref_()?

https://codereview.chromium.org/1351623008/diff/210001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:361:
languageCodes.join(','));
nit: setPref_()?

[Dan Beam, 2015-09-24 16:35:26]

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/manage_languages_page.js
(right):

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/manage_languages_page.js:45: *
@param {!{model: !{item: !Language}}} e
On 2015/09/24 02:15:20, michaelpg wrote:
> On 2015/09/23 23:28:17, Dan Beam wrote:
> > !!!
> 
> ???
> 
> would you prefer it not be typed?

nah, it was just funny
> 
> "model" is the binding scope of the template, so it would not be easy for the
> Closure pass to provide/auto-generate a simpler type.

[michaelpg, 2015-09-24 17:21:11]

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages.js (right):

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:236: var
languageCodes = this.get(preferredLanguagesPath).split(',');
On 2015/09/24 15:46:06, stevenjb wrote:
> On 2015/09/24 02:15:20, michaelpg wrote:
> > On 2015/09/23 22:27:54, stevenjb wrote:
> > > What do you think of:
> > > 
> > > getPref_: function(path) {
> > >   var result = this.get(path, this.prefs);
> > >   cr.assert(result != undefined, 'Pref is missing: ' + path);
> > >   return result;
> > > }
> > > ...
> > > var languagesCode =
this.getPref_(preferredLanguagesPath).value.split(',');
> > > ?
> > 
> > Done. It's not bad, but I'm not sure about the precedence it sets for other
> > files. (We could move getPref to CrSettingsPrefs but then it's a mouthful
> every
> > time we want to get a pref.)
> 
> Yeah, I've been trying to think of a nice way we could integrate this into
> CrSettingsPrefs, but as you say it would be an awkward mouthful. Maybe we
could
> use a behavior?

Yeah, a behavior would be a good way to make this available elsewhere.

> 
> That said, most pages lack the complexity of this one and the data binding
will
> just fail (although I've been thinking about adding checks in the settings
> elements, e.g. cr-settings-checkbox).

We do have cr-settings-pref-tracker at least.

> Anyway, thanks for adding the check here.
> 
>

https://codereview.chromium.org/1351623008/diff/210001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages.js (right):

https://codereview.chromium.org/1351623008/diff/210001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:332: languageCodes
+ ',' + languageCode);
On 2015/09/24 15:46:06, stevenjb wrote:
> nit: maybe add setPref_() too?

Done.

https://codereview.chromium.org/1351623008/diff/210001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:361:
languageCodes.join(','));
On 2015/09/24 15:46:06, stevenjb wrote:
> nit: setPref_()?

Done.

[Dan Beam, 2015-09-25 01:50:50]

lgtm

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages_page.html
(right):

https://codereview.chromium.org/1351623008/diff/170001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages_page.html:25:
<neon-animatable id="">
On 2015/09/24 02:15:20, michaelpg wrote:
> On 2015/09/23 23:28:17, Dan Beam wrote:
> > id=""?
> 
> Copied from other examples. I suspect it's because of
> <neon-animated-pages attr-for-selected="id">
> requiring the id attribute to actually exist.
> 
> Removing the attribute results in the Languages section being blank.
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=file%3Aresources/sett...

sux

https://codereview.chromium.org/1351623008/diff/230001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages.js (right):

https://codereview.chromium.org/1351623008/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:306: assert(pref
!= undefined, 'Pref is missing: ' + key);
nit: typeof pref != 'undefined' or pref !== undefined

https://codereview.chromium.org/1351623008/diff/230001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/manage_languages_page.js
(right):

https://codereview.chromium.org/1351623008/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/manage_languages_page.js:32:
/** @type {Array<{code: string, displayName: string, enabled: boolean}>|
/**
 * @type

[michaelpg, 2015-09-25 02:08:44]

https://codereview.chromium.org/1351623008/diff/230001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/languages.js (right):

https://codereview.chromium.org/1351623008/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/languages.js:306: assert(pref
!= undefined, 'Pref is missing: ' + key);
On 2015/09/25 01:50:50, Dan Beam wrote:
> nit: typeof pref != 'undefined' or pref !== undefined

Done.

https://codereview.chromium.org/1351623008/diff/230001/chrome/browser/resourc...
File chrome/browser/resources/settings/languages_page/manage_languages_page.js
(right):

https://codereview.chromium.org/1351623008/diff/230001/chrome/browser/resourc...
chrome/browser/resources/settings/languages_page/manage_languages_page.js:32:
/** @type {Array<{code: string, displayName: string, enabled: boolean}>|
On 2015/09/25 01:50:50, Dan Beam wrote:
> /**
>  * @type

Done.

[michaelpg, 2015-09-25 02:08:50]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2015-09-25 02:08:51]

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

[commit-bot: I haz the power, 2015-09-25 02:09:16]

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

[commit-bot: I haz the power, 2015-09-25 02:11:00]

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

[commit-bot: I haz the power, 2015-09-25 02:11:01]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[michaelpg, 2015-09-25 03:14:49]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2015-09-25 03:14:50]

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

[commit-bot: I haz the power, 2015-09-25 03:15:01]

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

[commit-bot: I haz the power, 2015-09-25 04:04:01]

Committed patchset #11 (id:270001)

[commit-bot: I haz the power, 2015-09-25 04:05:08]

Patchset 11 (id:??) landed as
https://crrev.com/0f479fec97b28c2d195777919f4abb4a8bced98e
Cr-Commit-Position: refs/heads/master@{#350762}