MD Settings: reduce complexity and overhead of Languages singleton

MD Settings: reduce complexity and overhead of Languages singleton

This weird private-singleton + multiple-instance-observer element
pattern didn't really pan out.

TL;DR: Remove <settings-languages-singleton> and put all the logic in
<settings-languages>, create one of those in the languages page,
and pass the model around with simple data binding.

Currently, we do this:

  <settings-languages languages="{{languages}}"></settings-languages>

in any subpage that needs access to the languages model. While it's
cool that they don't have to rely on ancestors for data binding,
the logic to make this work is confusing.

It's easier to just have one element at the top (i.e. the singleton) and
use normal data binding to provide the languages model to subpages.
This reduces complexity (eliminates custom-rolled notify/listener logic)
and matches how the rest of MD Settings works.

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

Committed: https://crrev.com/75252ef73a6e34b53a5ee8ffdf30e1ffe0b9a300
Cr-Commit-Position: refs/heads/master@{#413019}

[michaelpg, 2016-08-18 02:48:34]

Description was changed from

==========
MD Settings: reduce complexity and overhead of Languages singleton

This weird private-singleton + multiple-instance-observer element
pattern didn't really pan out.

Currently, we do this:

  <settings-languages languages="{{languages}}"></settings-languages>

in any subpage that needs access to the languages model. While it's
cool that they don't have to rely on ancestors for data binding,
the logic to make this work is confusing.

It's easier to just have one element at the top (i.e. the singleton) and
use normal data binding to provide the languages model to subpages.
This reduces complexity (eliminates custom-rolled notify/listener logic)
and matches how the rest of MD Settings works.

BUG=638802
==========

to

==========
MD Settings: reduce complexity and overhead of Languages singleton

This weird private-singleton + multiple-instance-observer element
pattern didn't really pan out.

Currently, we do this:

  <settings-languages languages="{{languages}}"></settings-languages>

in any subpage that needs access to the languages model. While it's
cool that they don't have to rely on ancestors for data binding,
the logic to make this work is confusing.

It's easier to just have one element at the top (i.e. the singleton) and
use normal data binding to provide the languages model to subpages.
This reduces complexity (eliminates custom-rolled notify/listener logic)
and matches how the rest of MD Settings works.

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

[michaelpg, 2016-08-18 02:53:48]

Description was changed from

==========
MD Settings: reduce complexity and overhead of Languages singleton

This weird private-singleton + multiple-instance-observer element
pattern didn't really pan out.

Currently, we do this:

  <settings-languages languages="{{languages}}"></settings-languages>

in any subpage that needs access to the languages model. While it's
cool that they don't have to rely on ancestors for data binding,
the logic to make this work is confusing.

It's easier to just have one element at the top (i.e. the singleton) and
use normal data binding to provide the languages model to subpages.
This reduces complexity (eliminates custom-rolled notify/listener logic)
and matches how the rest of MD Settings works.

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

to

==========
MD Settings: reduce complexity and overhead of Languages singleton

This weird private-singleton + multiple-instance-observer element
pattern didn't really pan out.

TL;DR: Remove <settings-languages-singleton> and put all the logic in
<settings-languages>, create one of those in the languages page,
and pass the model around with simple data binding.

Currently, we do this:

  <settings-languages languages="{{languages}}"></settings-languages>

in any subpage that needs access to the languages model. While it's
cool that they don't have to rely on ancestors for data binding,
the logic to make this work is confusing.

It's easier to just have one element at the top (i.e. the singleton) and
use normal data binding to provide the languages model to subpages.
This reduces complexity (eliminates custom-rolled notify/listener logic)
and matches how the rest of MD Settings works.

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

[michaelpg, 2016-08-18 02:53:48]

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

[michaelpg, 2016-08-18 02:54:55]

Patchset #2 (id:20001) has been deleted

[michaelpg, 2016-08-18 03:01:33]

Patchset #2 (id:40001) has been deleted

[michaelpg, 2016-08-18 03:05:29]

PTAL. The diff summary is a lie, it's just barely a net negative (-30 lines).

https://codereview.chromium.org/2252323002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages.js (right):

https://codereview.chromium.org/2252323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages.js:13: 'use strict';
Everything inside this function will be indented (see previous patchset). But it
completely destroys the diff, so please review with this diff.

[michaelpg, 2016-08-18 03:06:37]

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

[commit-bot: I haz the power, 2016-08-18 03:06:55]

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, 2016-08-18 03:59:25]

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

[commit-bot: I haz the power, 2016-08-18 03:59:26]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2016-08-18 16:22:03]

This lgtm as-is, but FWIW there would be fewer changes and, I think, just as
good or better readability if we set:

this.languageHelper_ = LanguageHelper.getInstance();

in the elements that already have this.languageHelper_.

https://codereview.chromium.org/2252323002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/language_detail_page.js
(left):

https://codereview.chromium.org/2252323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/language_detail_page.js:40:
this.languageHelper_ = LanguageHelperImpl.getInstance();
QQ: These changes seem fine, but any particular reason not to just set this to
settings.LanguageHelperImpl.getInstance()? Does the getInstance() result change
after created() is called in tests? Could we move this to ready() or attached()
in that case, or use data binding? I used data binding for networkingPrivate in
the internet code to support a fake implementation in tests, and I am curious
what if any the tradeoffs are.

https://codereview.chromium.org/2252323002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/language_detail_page.js
(right):

https://codereview.chromium.org/2252323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/language_detail_page.js:65:
settings.LanguageHelperImpl.getInstance().getProspectiveUILanguage();
nit: In C++ we would make getInstance() part of the LanguageHelper interface and
it would return a LanguageHelperImpl class. Can we do the same thing here?
Accessing an Impl class externally is a bit wonky.

https://codereview.chromium.org/2252323002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages.js (right):

https://codereview.chromium.org/2252323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages.js:13: 'use strict';
On 2016/08/18 03:05:28, michaelpg wrote:
> Everything inside this function will be indented (see previous patchset). But
it
> completely destroys the diff, so please review with this diff.

Thank you!

[michaelpg, 2016-08-18 22:32:27]

This is way better, I wish I had thought of it before sending out the first CL.
The diff is much smaller now, too, and now we're -40 LOC!

https://codereview.chromium.org/2252323002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/language_detail_page.js
(left):

https://codereview.chromium.org/2252323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/language_detail_page.js:40:
this.languageHelper_ = LanguageHelperImpl.getInstance();
On 2016/08/18 16:22:02, stevenjb wrote:
> QQ: These changes seem fine, but any particular reason not to just set this to
> settings.LanguageHelperImpl.getInstance()? Does the getInstance() result
change
> after created() is called in tests? Could we move this to ready() or
attached()
> in that case, or use data binding? I used data binding for networkingPrivate
in
> the internet code to support a fake implementation in tests, and I am curious
> what if any the tradeoffs are.

Changed to a data-bound  member. The instance wasn't set until the element was
created, and we don't have guaranteed timings for that. Data binding is an
obviously superior solution to ensure the property is set when it needs to be,
and it seems to work perfectly.

https://codereview.chromium.org/2252323002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/language_detail_page.js
(right):

https://codereview.chromium.org/2252323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/language_detail_page.js:65:
settings.LanguageHelperImpl.getInstance().getProspectiveUILanguage();
On 2016/08/18 16:22:02, stevenjb wrote:
> nit: In C++ we would make getInstance() part of the LanguageHelper interface
and
> it would return a LanguageHelperImpl class. Can we do the same thing here?
> Accessing an Impl class externally is a bit wonky.
> 

Changed to a data-bound  member.

https://codereview.chromium.org/2252323002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages.js (right):

https://codereview.chromium.org/2252323002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages.js:13: 'use strict';
On 2016/08/18 16:22:03, stevenjb wrote:
> On 2016/08/18 03:05:28, michaelpg wrote:
> > Everything inside this function will be indented (see previous patchset).
But
> it
> > completely destroys the diff, so please review with this diff.
> 
> Thank you!

I've further decided to leave it this way -- the horrible diff and
every-line-blame isn't worth the consistency, since it was incorrect before
anyway.

[michaelpg, 2016-08-18 22:32:28]

This is way better, I wish I had thought of it before sending out the first CL.
The diff is much smaller now, too, and now we're -40 LOC!

[stevenjb, 2016-08-19 00:16:39]

nice. lgtm!

[michaelpg, 2016-08-19 00:21:42]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2016-08-19 00:22:29]

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, 2016-08-19 01:47:58]

Description was changed from

==========
MD Settings: reduce complexity and overhead of Languages singleton

This weird private-singleton + multiple-instance-observer element
pattern didn't really pan out.

TL;DR: Remove <settings-languages-singleton> and put all the logic in
<settings-languages>, create one of those in the languages page,
and pass the model around with simple data binding.

Currently, we do this:

  <settings-languages languages="{{languages}}"></settings-languages>

in any subpage that needs access to the languages model. While it's
cool that they don't have to rely on ancestors for data binding,
the logic to make this work is confusing.

It's easier to just have one element at the top (i.e. the singleton) and
use normal data binding to provide the languages model to subpages.
This reduces complexity (eliminates custom-rolled notify/listener logic)
and matches how the rest of MD Settings works.

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

to

==========
MD Settings: reduce complexity and overhead of Languages singleton

This weird private-singleton + multiple-instance-observer element
pattern didn't really pan out.

TL;DR: Remove <settings-languages-singleton> and put all the logic in
<settings-languages>, create one of those in the languages page,
and pass the model around with simple data binding.

Currently, we do this:

  <settings-languages languages="{{languages}}"></settings-languages>

in any subpage that needs access to the languages model. While it's
cool that they don't have to rely on ancestors for data binding,
the logic to make this work is confusing.

It's easier to just have one element at the top (i.e. the singleton) and
use normal data binding to provide the languages model to subpages.
This reduces complexity (eliminates custom-rolled notify/listener logic)
and matches how the rest of MD Settings works.

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

[commit-bot: I haz the power, 2016-08-19 01:47:59]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-08-19 01:52:08]

Description was changed from

==========
MD Settings: reduce complexity and overhead of Languages singleton

This weird private-singleton + multiple-instance-observer element
pattern didn't really pan out.

TL;DR: Remove <settings-languages-singleton> and put all the logic in
<settings-languages>, create one of those in the languages page,
and pass the model around with simple data binding.

Currently, we do this:

  <settings-languages languages="{{languages}}"></settings-languages>

in any subpage that needs access to the languages model. While it's
cool that they don't have to rely on ancestors for data binding,
the logic to make this work is confusing.

It's easier to just have one element at the top (i.e. the singleton) and
use normal data binding to provide the languages model to subpages.
This reduces complexity (eliminates custom-rolled notify/listener logic)
and matches how the rest of MD Settings works.

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

to

==========
MD Settings: reduce complexity and overhead of Languages singleton

This weird private-singleton + multiple-instance-observer element
pattern didn't really pan out.

TL;DR: Remove <settings-languages-singleton> and put all the logic in
<settings-languages>, create one of those in the languages page,
and pass the model around with simple data binding.

Currently, we do this:

  <settings-languages languages="{{languages}}"></settings-languages>

in any subpage that needs access to the languages model. While it's
cool that they don't have to rely on ancestors for data binding,
the logic to make this work is confusing.

It's easier to just have one element at the top (i.e. the singleton) and
use normal data binding to provide the languages model to subpages.
This reduces complexity (eliminates custom-rolled notify/listener logic)
and matches how the rest of MD Settings works.

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

Committed: https://crrev.com/75252ef73a6e34b53a5ee8ffdf30e1ffe0b9a300
Cr-Commit-Position: refs/heads/master@{#413019}
==========

[commit-bot: I haz the power, 2016-08-19 01:52:09]

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