New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 638802 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Remove singleton/observer element pattern from MD Settings

Project Member Reported by michae...@chromium.org, Aug 18 2016

Issue description

After data binding with IronMeta was lost in 0.8, we needed a way to bind the
same data in multiple places, especially data relating to a complicated model:
lots of pages might want to show or change this data, but only one element
should be responsible for complex logic for responding to model changes.
Biggest example was Preferences.

Two options were:
1. Have one element at the top of the tree handling the model and providing the
   data. Normal data binding propagates changes up and down the tree -- the
   downside is, you have to declare properties in every node in between the
   elements you want to share data.
2. Replace IronMeta with a Singleton/Observer pattern. Create the singleton
   element at startup; it owns the data. Any element that wants to bind to
   that data adds an observer element to its local DOM and binds to that --
   no dependencies on ancestors or descendants.

I implemented option 1 for Preferences and Languages, both very complicated
models that can't just be a Behavior shared by a bunch of elements. In
hindsight, the implementation is too obscure, fragile, and (possibly) less
efficient than plain old data binding. There's extra code for managing the
Observer relationships, and there are singleton elements created at startup
that could be created later. Furthermore, it would be a burden for anyone
other than myself to get up to speed on and maintain in the future.

I want to remove settings-languages-singleton and settings-prefs-singleton.
Languages data binding is easy because, unsurprisingly, everything that uses
the languages model is a descendant of the Languages page. Prefs is easy
because we mostly already do it the normal way -- the Languages singleton
is the only other user of the Prefs singleton, which is a pretty good sign
that the pattern wasn't useful.

Original design doc for context: https://docs.google.com/document/d/1yTA34r7KyFXMlXxkSsR2vSNzh6V1S8MuxOrkh5smKaI
The nail in the coffin is that we never came up with other use cases:
other complex models live in the browser process, and are handled from
JS with simple FooBrowserProxy classes.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 19 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/75252ef73a6e34b53a5ee8ffdf30e1ffe0b9a300

commit 75252ef73a6e34b53a5ee8ffdf30e1ffe0b9a300
Author: michaelpg <michaelpg@chromium.org>
Date: Fri Aug 19 01:47:39 2016

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

Review-Url: https://codereview.chromium.org/2252323002
Cr-Commit-Position: refs/heads/master@{#413019}

[modify] https://crrev.com/75252ef73a6e34b53a5ee8ffdf30e1ffe0b9a300/chrome/browser/resources/settings/languages_page/language_detail_page.html
[modify] https://crrev.com/75252ef73a6e34b53a5ee8ffdf30e1ffe0b9a300/chrome/browser/resources/settings/languages_page/language_detail_page.js
[modify] https://crrev.com/75252ef73a6e34b53a5ee8ffdf30e1ffe0b9a300/chrome/browser/resources/settings/languages_page/languages.html
[modify] https://crrev.com/75252ef73a6e34b53a5ee8ffdf30e1ffe0b9a300/chrome/browser/resources/settings/languages_page/languages.js
[modify] https://crrev.com/75252ef73a6e34b53a5ee8ffdf30e1ffe0b9a300/chrome/browser/resources/settings/languages_page/languages_page.html
[modify] https://crrev.com/75252ef73a6e34b53a5ee8ffdf30e1ffe0b9a300/chrome/browser/resources/settings/languages_page/languages_page.js
[modify] https://crrev.com/75252ef73a6e34b53a5ee8ffdf30e1ffe0b9a300/chrome/browser/resources/settings/languages_page/languages_types.js
[modify] https://crrev.com/75252ef73a6e34b53a5ee8ffdf30e1ffe0b9a300/chrome/browser/resources/settings/languages_page/manage_input_methods_page.html
[modify] https://crrev.com/75252ef73a6e34b53a5ee8ffdf30e1ffe0b9a300/chrome/browser/resources/settings/languages_page/manage_input_methods_page.js
[modify] https://crrev.com/75252ef73a6e34b53a5ee8ffdf30e1ffe0b9a300/chrome/browser/resources/settings/languages_page/manage_languages_page.html
[modify] https://crrev.com/75252ef73a6e34b53a5ee8ffdf30e1ffe0b9a300/chrome/browser/resources/settings/languages_page/manage_languages_page.js
[modify] https://crrev.com/75252ef73a6e34b53a5ee8ffdf30e1ffe0b9a300/chrome/test/data/webui/settings/cr_settings_browsertest.js
[modify] https://crrev.com/75252ef73a6e34b53a5ee8ffdf30e1ffe0b9a300/chrome/test/data/webui/settings/languages_page_browsertest.js
[rename] https://crrev.com/75252ef73a6e34b53a5ee8ffdf30e1ffe0b9a300/chrome/test/data/webui/settings/languages_tests.js

Project Member

Comment 2 by bugdroid1@chromium.org, Aug 19 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/190a0525f1b960e8f241819421cc94c0f0e0f38a

commit 190a0525f1b960e8f241819421cc94c0f0e0f38a
Author: michaelpg <michaelpg@chromium.org>
Date: Fri Aug 19 06:03:58 2016

MD Settings: reduce complexity and overhead of prefs singleton

Remove <settings-prefs-singleton> and put all the logic in <settings-prefs>.
No more custom listeners or fragile test structures.

This depends on removing <settings-languages-singleton>.

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

Review-Url: https://codereview.chromium.org/2254113002
Cr-Commit-Position: refs/heads/master@{#413062}

[modify] https://crrev.com/190a0525f1b960e8f241819421cc94c0f0e0f38a/chrome/browser/resources/settings/prefs/prefs.js
[modify] https://crrev.com/190a0525f1b960e8f241819421cc94c0f0e0f38a/chrome/test/data/webui/settings/fake_settings_private.js
[modify] https://crrev.com/190a0525f1b960e8f241819421cc94c0f0e0f38a/chrome/test/data/webui/settings/languages_tests.js
[modify] https://crrev.com/190a0525f1b960e8f241819421cc94c0f0e0f38a/chrome/test/data/webui/settings/passwords_and_forms_browsertest.js
[modify] https://crrev.com/190a0525f1b960e8f241819421cc94c0f0e0f38a/chrome/test/data/webui/settings/prefs_tests.js

Status: Fixed (was: Started)

Sign in to add a comment