Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1357183002:
MD-Settings: convert cr-settings-prefs to a singleton model (Closed)
|
Created:
5 years, 3 months ago by michaelpg Modified:
5 years, 3 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, jhawkins+watch-md-settings_chromium.org, arv+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, jlklein+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD-Settings: convert cr-settings-prefs to a singleton model
This CL adds <cr-settings-prefs-private>, a singleton element created at startup
and initialized on demand. <cr-settings-prefs> now mediates between the
singleton model and the outside world, providing access to the shared prefs
state.
This lets a component access the shared prefs by adding a <cr-settings-prefs> to
its local DOM, instead of having to pipe a prefs object from <cr-settings> all
the way down to any component that needs it. This also lets us create other
singletons to access the shared prefs.
The singleton can be re-initialized in between tests to start with a fresh set
of prefs. <cr-settings-prefs> elements created by tests are reset so they don't
interfere with the next test (because we can't just destroy a DOM element).
CrSettingsPrefsTest is updated to reset the singleton in between tests. It creates
200 elements for each test, so if the shared model starts generating extraneous
events the test is likely to explode exponentially. Currently, time added by each
additional <cr-settings-prefs> is slightly less than linear, which is good:
# | time from first setup to last teardown (ms) on Z620
1 | 87
2 | 89
4 | 101
8 | 118
16 | 154
32 | 228
64 | 433
128 | 762
256 | 1416
BUG=532270
R=dbeam@chromium.org
Committed: https://crrev.com/8ff101453f82f6c4f3a61aea938b4e3a4b0f2b07
Cr-Commit-Position: refs/heads/master@{#350411}
Patch Set 1 : #
Total comments: 1
Patch Set 2 : closure, typo #
Total comments: 1
Patch Set 3 : #
Total comments: 27
Patch Set 4 : changes #Patch Set 5 : missed one #Patch Set 6 : this -> CrSettingsPrefsInternal #Patch Set 7 : unnecessary parens #
Total comments: 8
Patch Set 8 : dbeam feedback #
Total comments: 3
Patch Set 9 : #Patch Set 10 : rebase #Patch Set 11 : clarify singleton/private/model language #
Dependent Patchsets: Messages
Total messages: 36 (17 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
PTAL https://codereview.chromium.org/1357183002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/prefs_tests.js (right): https://codereview.chromium.org/1357183002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:45: assertNotEquals(null, MockSettingsApi.listener_); Switched expects to asserts because test_api.js makes it hard to determine what test failed when an expect fails. Also, a failure is likely to involve all prefs, making the log spew is too high for expects.
https://codereview.chromium.org/1357183002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1357183002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:369: if (!deepEqual(this.get(newPrefObj.key, prefs), newPrefObj)) { silly typo, thanks Closure! this doesn't affect correctness but makes the tests 25% faster :-)
https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/pref_tracker/pref_tracker.js (right): https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/pref_tracker/pref_tracker.js:53: console.error('Pref not found. Parent control:' + is this console.error() instead of throw now because throwing from here wouldn't halt much? https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:155: this.prefsPrivate_.initialize(); nit: reverse these if possible https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:169: assert(!this.listening_); there seems to be a graceful fix for this: just unlisten() before any listen() https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:173: })); CrSettingsPrefs.initialized.then(function() { this.squelching_(function() { // this is easier to read, IMO }); }.bind(this)); https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:181: if (this.listening_) { is this if really necessary? https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:217: squelching_: function(fn) { runWhileIgnoringChanges_ IMO https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:218: this.squelch_ = true; this.ignoreChanges_ or this.ignorePrefChanges_ https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:221: this.squelch_ = false; instead of setting a flag, just stop listening during this time https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs_types.js (right): https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs_types.js:12: var CrSettingsPrefs = (function() { nit: var CrSettingsPrefs = function() { var CrSettingsPrefsInternal = { setInitialized: function() { /** @public {boolean} */ this.isIntialized = true; this.res_(); }, reset: function() { this.isInitialized = false; /** @public {!Promise} */ this.initialized = new Promise(function(res) { this.res_ = res; }.bind(this)); }, }; CrSettingsPrefsInternal.reset(); return CrSettingsPrefsInternal; }(); feel free to eradicate my perilous use of |this| in a static context https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs_types.js:42: resolveInitialized = resolve; why shouldn't this be a private static rather than a local? e.g. CrSettingsPrefs.resolve_ https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs_types.js:49: CrSettingsPrefs.reset(); maybe i'm being paranoid, but why can't we run reset() before this class is exposed? https://codereview.chromium.org/1357183002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/prefs_tests.js (right): https://codereview.chromium.org/1357183002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:45: assertNotEquals(null, MockSettingsApi.listener_); why again is this ^ necessary?
Feedback addressed, thanks -- PTAnotherL. https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/pref_tracker/pref_tracker.js (right): https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/pref_tracker/pref_tracker.js:53: console.error('Pref not found. Parent control:' + On 2015/09/21 22:25:20, Dan Beam wrote: > is this console.error() instead of throw now because throwing from here wouldn't > halt much? Yeah, it just said "Uncaught (in promise): blah", console.error('blah') seems cleaner to me. https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:155: this.prefsPrivate_.initialize(); On 2015/09/21 22:25:20, Dan Beam wrote: > nit: reverse these if possible Done. https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:169: assert(!this.listening_); On 2015/09/21 22:25:20, Dan Beam wrote: > there seems to be a graceful fix for this: just unlisten() before any listen() Done. https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:173: })); On 2015/09/21 22:25:20, Dan Beam wrote: > CrSettingsPrefs.initialized.then(function() { > this.squelching_(function() { > // this is easier to read, IMO > }); > }.bind(this)); Done. https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:181: if (this.listening_) { On 2015/09/21 22:25:20, Dan Beam wrote: > is this if really necessary? Done. https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:217: squelching_: function(fn) { On 2015/09/21 22:25:20, Dan Beam wrote: > runWhileIgnoringChanges_ IMO Done. https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:218: this.squelch_ = true; On 2015/09/21 22:25:20, Dan Beam wrote: > this.ignoreChanges_ or this.ignorePrefChanges_ Done. https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:221: this.squelch_ = false; On 2015/09/21 22:25:20, Dan Beam wrote: > instead of setting a flag, just stop listening during this time I'm not sure how that would work... the interaction between events gets complicated. I don't want to manually listen to and unlisten to prefs, so the flag is useful in prefsChanged_. And calling unlisten() followed by listen() would result in a new listener if we weren't listening before the squelch call. also: each call to listen creates a new event handler, which it adds to the DOM node as well as a private WeakMap; unlisten undoes all that. I'd want to consider what the overhead of doing that on every change would be. https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs_types.js (right): https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs_types.js:12: var CrSettingsPrefs = (function() { On 2015/09/21 22:25:21, Dan Beam wrote: > nit: > > var CrSettingsPrefs = function() { > var CrSettingsPrefsInternal = { > setInitialized: function() { > /** @public {boolean} */ @public is not documented... does it do something different from @type? > this.isIntialized = true; > this.res_(); > }, > reset: function() { > this.isInitialized = false; > /** @public {!Promise} */ > this.initialized = new Promise(function(res) { > this.res_ = res; > }.bind(this)); > }, > }; > > CrSettingsPrefsInternal.reset(); > > return CrSettingsPrefsInternal; > }(); Done. Much nicer. > > feel free to eradicate my perilous use of |this| in a static context I've done it both ways, patches 5 and 6, which is preferable? https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs_types.js:42: resolveInitialized = resolve; On 2015/09/21 22:25:21, Dan Beam wrote: > why shouldn't this be a private static rather than a local? e.g. > CrSettingsPrefs.resolve_ Done. But what do you have against environment locals? https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs_types.js:49: CrSettingsPrefs.reset(); On 2015/09/21 22:25:21, Dan Beam wrote: > maybe i'm being paranoid, but why can't we run reset() before this class is > exposed? Done. https://codereview.chromium.org/1357183002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/prefs_tests.js (right): https://codereview.chromium.org/1357183002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/prefs_tests.js:45: assertNotEquals(null, MockSettingsApi.listener_); On 2015/09/21 22:25:21, Dan Beam wrote: > why again is this ^ necessary? Necessary isn't the right word. Removed.
lgtm https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:221: this.squelch_ = false; On 2015/09/22 00:34:07, michaelpg wrote: > On 2015/09/21 22:25:20, Dan Beam wrote: > > instead of setting a flag, just stop listening during this time > > I'm not sure how that would work... the interaction between events gets > complicated. I don't want to manually listen to and unlisten to prefs, so the > flag is useful in prefsChanged_. And calling unlisten() followed by listen() > would result in a new listener if we weren't listening before the squelch call. any time I see "listen to events" followed by "but ignore sometimes", I think "why not just listen for events only sometimes". because there's currently no public API for "isListening" in Polymer[1], I guess you're off the hook (as it'd make the code just as bad to have a .listening_ flag, IMO) [1] https://github.com/Polymer/polymer/blob/22daec6f16453254a0077044077723f28d373... > > also: each call to listen creates a new event handler, which it adds to the DOM > node as well as a private WeakMap; unlisten undoes all that. I'd want to > consider what the overhead of doing that on every change would be. prove the difference or don't worry yet, imo https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs_types.js (right): https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs_types.js:12: var CrSettingsPrefs = (function() { On 2015/09/22 00:34:07, michaelpg wrote: > On 2015/09/21 22:25:21, Dan Beam wrote: > > nit: > > > > var CrSettingsPrefs = function() { > > var CrSettingsPrefsInternal = { > > setInitialized: function() { > > /** @public {boolean} */ > > @public is not documented... does it do something different from @type? > > > this.isIntialized = true; > > this.res_(); > > }, > > reset: function() { > > this.isInitialized = false; > > /** @public {!Promise} */ > > this.initialized = new Promise(function(res) { > > this.res_ = res; > > }.bind(this)); > > }, > > }; > > > > CrSettingsPrefsInternal.reset(); > > > > return CrSettingsPrefsInternal; > > }(); > > Done. Much nicer. > > > > > feel free to eradicate my perilous use of |this| in a static context > > I've done it both ways, patches 5 and 6, which is preferable? whatever you have currently is fine https://codereview.chromium.org/1357183002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs_types.js:42: resolveInitialized = resolve; On 2015/09/22 00:34:07, michaelpg wrote: > On 2015/09/21 22:25:21, Dan Beam wrote: > > why shouldn't this be a private static rather than a local? e.g. > > CrSettingsPrefs.resolve_ > > Done. But what do you have against environment locals? harder to mock/change when testing https://codereview.chromium.org/1357183002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1357183002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:213: runWhileIgnoringChanges_: function(fn) { assert(!this.ignoreChanges_, 'update the code to support nesting'); https://codereview.chromium.org/1357183002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:332: // Prefs are now initialized. nit: useless comment https://codereview.chromium.org/1357183002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs_types.js (right): https://codereview.chromium.org/1357183002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs_types.js:27: reset: function() { nit: resetForTesting, maybe? https://codereview.chromium.org/1357183002/diff/160001/chrome/test/data/webui... File chrome/test/data/webui/settings/prefs_tests.js (right): https://codereview.chromium.org/1357183002/diff/160001/chrome/test/data/webui... chrome/test/data/webui/settings/prefs_tests.js:206: for (var i = 0; i < 100; i++) { was 200 too slow?
Patchset #8 (id:180001) has been deleted
https://codereview.chromium.org/1357183002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1357183002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:213: runWhileIgnoringChanges_: function(fn) { On 2015/09/22 06:12:31, Dan Beam wrote: > assert(!this.ignoreChanges_, 'update the code to support nesting'); Done. https://codereview.chromium.org/1357183002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:332: // Prefs are now initialized. On 2015/09/22 06:12:31, Dan Beam wrote: > nit: useless comment Done. https://codereview.chromium.org/1357183002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs_types.js (right): https://codereview.chromium.org/1357183002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs_types.js:27: reset: function() { On 2015/09/22 06:12:31, Dan Beam wrote: > nit: resetForTesting, maybe? Done. https://codereview.chromium.org/1357183002/diff/160001/chrome/test/data/webui... File chrome/test/data/webui/settings/prefs_tests.js (right): https://codereview.chromium.org/1357183002/diff/160001/chrome/test/data/webui... chrome/test/data/webui/settings/prefs_tests.js:206: for (var i = 0; i < 100; i++) { On 2015/09/22 06:12:31, Dan Beam wrote: > was 200 too slow? no, just a questions of flakiness buffer. on my Z620 it doesn't timeout unless createdElements.length > 800 or so. mocha's default timeout is 2000ms per test case -- we could increase this for any given test or all tests if we want a larger buffer. I want to understand crbug.com/534718 better before changing timeouts. https://codereview.chromium.org/1357183002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs_types.js (right): https://codereview.chromium.org/1357183002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs_types.js:46: CrSettingsPrefsInternal.setup_(); Is calling this "private" method OK here or is this weird?
michaelpg@chromium.org changed reviewers: + stevenjb@chromium.org
michaelpg@chromium.org changed required reviewers: + dbeam@chromium.org
https://codereview.chromium.org/1357183002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs_types.js (right): https://codereview.chromium.org/1357183002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs_types.js:46: CrSettingsPrefsInternal.setup_(); On 2015/09/22 21:38:37, michaelpg wrote: > Is calling this "private" method OK here or is this weird? no, but CrSettingsPrefInternal.isInitialized in undefined in the new code, though, right?
https://codereview.chromium.org/1357183002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs_types.js (right): https://codereview.chromium.org/1357183002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs_types.js:46: CrSettingsPrefsInternal.setup_(); On 2015/09/22 21:58:35, Dan Beam wrote: > On 2015/09/22 21:38:37, michaelpg wrote: > > Is calling this "private" method OK here or is this weird? > > no, but CrSettingsPrefInternal.isInitialized in undefined in the new code, > though, right? uh, yeah.. I should move that back.
The CQ bit was checked by michaelpg@chromium.org
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/1357183002/#ps220001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1357183002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1357183002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
Patchset #11 (id:260001) has been deleted
The CQ bit was checked by michaelpg@chromium.org
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/1357183002/#ps280001 (title: "clarify singleton/private/model language")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1357183002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1357183002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1357183002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1357183002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1357183002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1357183002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1357183002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1357183002/280001
Message was sent while issue was closed.
Committed patchset #11 (id:280001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/8ff101453f82f6c4f3a61aea938b4e3a4b0f2b07 Cr-Commit-Position: refs/heads/master@{#350411} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
