Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1287913005:
Refactor prefs.js for MD-Settings (Closed)
|
Created:
5 years, 4 months ago by michaelpg Modified:
5 years, 3 months ago CC:
chromium-reviews, khorimoto+watch-md-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, jhawkins+watch-md-settings_chromium.org, orenb+watch-md-settings_chromium.org, arv+watch_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. |
DescriptionRefactor prefs.js for MD-Settings
Motivations:
* Make easier to understand and extend.
* Eliminate potential infinite loops.
* Be more precise and identify error states.
* Remove extraneous set() calls (prevent excess Polymer notifications).
Assumptions:
* Elements should use Polymer.Base.set to update prefs
to ensure they propagate.
* Preferences should serialize to the same strings in
JS and the C++ JSON store.
BUG=512479
R=dbeam@chromium.org,stevenjb@chromium.org
TBR=stevenjb@chromium.org # because we talked extensively and this shouldn't wait another week
Committed: https://crrev.com/037f8fdb34fc72d62d3a78893dc143d870b0d7e5
Cr-Commit-Position: refs/heads/master@{#346747}
Patch Set 1 : #
Total comments: 11
Patch Set 2 : Draft Not For Review #
Total comments: 18
Patch Set 3 : Update after meeting #
Total comments: 57
Patch Set 4 : Feedback #
Total comments: 18
Patch Set 5 : dbeam comments #
Total comments: 19
Patch Set 6 : #Patch Set 7 : prefs fix #Patch Set 8 : fix #
Total comments: 7
Patch Set 9 : closure updates #
Total comments: 6
Patch Set 10 : pick up closure pass fix #Patch Set 11 : formatting #Patch Set 12 : don't not build on chromeos #Patch Set 13 : rebase #Patch Set 14 : whitespace again #Patch Set 15 : add return after switch for non-clang #Messages
Total messages: 52 (12 generated)
michaelpg@chromium.org changed reviewers: + stevenjb@chromium.org
michaelpg@chromium.org changed required reviewers: + stevenjb@chromium.org
Patchset #1 (id:1) has been deleted
PTAL. https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:45: this.committed_ = {}; Feel free to suggest a better name for "our belief of the state of Chrome's internal pref store".
tl;dr - we should create a factory method returns PrefObject wrappers. that factory method could be the only place with knowledge of the underlying pref type (e.g. if it's a list or not), which would make the code simpler, IMO https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:85: this.committed_[prefObj.key] = prefObj.value; make a getValue() or getLocalValue() that PrefObjects can specialize so these can be treated the same https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:91: // Check if the values are different. make a PrefObject#equals() so you can do this regardless of type https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:103: this.observeListPrefValue_(prefObj); make PrefObject#observe(callback) https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:110: tokens.forEach(function(token) { nit: maybe massage cr.exportPath() take a |target| argument? (an object to make sure a property path exists upon) https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:143: observeListPrefValue_: function(prefObj) { when is this called without observePref_ being triggered?
https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:45: this.committed_ = {}; On 2015/08/19 02:09:53, michaelpg wrote: > Feel free to suggest a better name for "our belief of the state of Chrome's > internal pref store". It looks like this is a cache of the most recent pref store update received from settingsPrivate, so maybe name it something like settingsCache_. https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:85: this.committed_[prefObj.key] = prefObj.value; On 2015/08/19 03:15:38, Dan Beam wrote: > make a getValue() or getLocalValue() that PrefObjects can specialize so these > can be treated the same Also, this assumes knowledge that prefObj.value is never an Object, which I guess is reasonable, but wasn't immediately clear to me. I tend to agree that a PrefObject wrapper might be useful for this. https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:110: tokens.forEach(function(token) { On 2015/08/19 03:15:38, Dan Beam wrote: > nit: maybe massage cr.exportPath() take a |target| argument? (an object to make > sure a property path exists upon) I see that prefs.html includes cr.html, but it doesn't appear to use it. I would slightly prefer to keep that dependency out of prefs and move it to any pages that use it (e.g. appearance). The reason for this is that I still hope at some point to be able to build an app that we can run on Chrome OS in kiosk mode for setting a subset of device level preferences (e.g. Internet and Display). There is still some componentization required to make this possible, but I would prefer not to make this task any more difficult. (I do concede that since we do not have any immediate plans to do this that we shouldn't make more work for ourselves at this point to support it, but between the choice of modifying cr.exportPath and a bit of code duplication, in this particular case I would lean towards code duplication). https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:161: // UI should only be able to change the value of a preference for now. It's not immediately clear that "value" refers to a specific property, e.g.: // UI should only be able to change the PrefObject.value property. (I think this is probably true "for ever" not just "for now"?)
https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:110: tokens.forEach(function(token) { On 2015/08/19 17:26:31, stevenjb wrote: > On 2015/08/19 03:15:38, Dan Beam wrote: > > nit: maybe massage cr.exportPath() take a |target| argument? (an object to > make > > sure a property path exists upon) > > I see that prefs.html includes cr.html, but it doesn't appear to use it. I would > slightly prefer to keep that dependency out of prefs and move it to any pages > that use it (e.g. appearance). > > The reason for this is that I still hope at some point to be able to build an > app that we can run on Chrome OS in kiosk mode for setting a subset of device > level preferences (e.g. Internet and Display). There is still some > componentization required to make this possible, but I would prefer not to make > this task any more difficult. > > (I do concede that since we do not have any immediate plans to do this that we > shouldn't make more work for ourselves at this point to support it, but between > the choice of modifying cr.exportPath and a bit of code duplication, in this > particular case I would lean towards code duplication). we could also make a generic merge() or mergeObjects() and call it from cr.exportPath() on window
On 2015/08/19 03:15:38, Dan Beam wrote:
> tl;dr - we should create a factory method returns PrefObject wrappers. that
> factory method could be the only place with knowledge of the underlying pref
> type (e.g. if it's a list or not), which would make the code simpler, IMO
>
Ok. So, create a Pref prototype, extend with ListPref, etc. and write a function
/** @param {Pref} */ makePrefObjectWrapper(pref)
Is that what you're going for?
>
https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/settings/prefs/prefs.js (right):
>
>
https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/prefs/prefs.js:85:
> this.committed_[prefObj.key] = prefObj.value;
> make a getValue() or getLocalValue() that PrefObjects can specialize so these
> can be treated the same
>
>
https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/prefs/prefs.js:91: // Check if the values
are
> different.
> make a PrefObject#equals() so you can do this regardless of type
>
>
https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/prefs/prefs.js:103:
> this.observeListPrefValue_(prefObj);
> make PrefObject#observe(callback)
>
>
https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/prefs/prefs.js:110:
> tokens.forEach(function(token) {
> nit: maybe massage cr.exportPath() take a |target| argument? (an object to
make
> sure a property path exists upon)
>
>
https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/prefs/prefs.js:143: observeListPrefValue_:
> function(prefObj) {
> when is this called without observePref_ being triggered?
On 2015/08/19 21:33:12, michaelpg wrote:
> On 2015/08/19 03:15:38, Dan Beam wrote:
> > tl;dr - we should create a factory method returns PrefObject wrappers. that
> > factory method could be the only place with knowledge of the underlying pref
> > type (e.g. if it's a list or not), which would make the code simpler, IMO
> >
>
> Ok. So, create a Pref prototype, extend with ListPref, etc. and write a
function
> /** @param {Pref} */ makePrefObjectWrapper(pref)
>
> Is that what you're going for?
yee
>
> >
>
https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource...
> > File chrome/browser/resources/settings/prefs/prefs.js (right):
> >
> >
>
https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource...
> > chrome/browser/resources/settings/prefs/prefs.js:85:
> > this.committed_[prefObj.key] = prefObj.value;
> > make a getValue() or getLocalValue() that PrefObjects can specialize so
these
> > can be treated the same
> >
> >
>
https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource...
> > chrome/browser/resources/settings/prefs/prefs.js:91: // Check if the values
> are
> > different.
> > make a PrefObject#equals() so you can do this regardless of type
> >
> >
>
https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource...
> > chrome/browser/resources/settings/prefs/prefs.js:103:
> > this.observeListPrefValue_(prefObj);
> > make PrefObject#observe(callback)
> >
> >
>
https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource...
> > chrome/browser/resources/settings/prefs/prefs.js:110:
> > tokens.forEach(function(token) {
> > nit: maybe massage cr.exportPath() take a |target| argument? (an object to
> make
> > sure a property path exists upon)
> >
> >
>
https://codereview.chromium.org/1287913005/diff/20001/chrome/browser/resource...
> > chrome/browser/resources/settings/prefs/prefs.js:143: observeListPrefValue_:
> > function(prefObj) {
> > when is this called without observePref_ being triggered?
i know you said not for review, but... I'm still confused as to what you're trying to accomplish. Are you trying to mirror C++'s model of prefs to JS? That's probably a bad idea, as we're required to have a model in C++, so anywhere else is duplicative and probably harmful. We should make the views dumber, not smarter, IMO. Do you understand the exact reason we're able to get in a looping state? Is it because 2 pages keep receiving updates from C++ that don't match their local state, and both sides think "I have a more updated value"? https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:30: * @param {!PrefObject} prefObj PrefObject -> PrefData https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:32: function Pref(prefObj) { prefObj -> data https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:32: function Pref(prefObj) { this.data_ = data; https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:34: this.update(prefObj); Object.observe(this.data_, this.dataChanged_.bind(this), ['update']); https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:38: Pref.prototype.equals = function(oldValue, newValue) { Pref.prototype.equals = function(otherPref) { return this.type == otherPref.type && this.value == otherPref.value; }; https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:50: if (this.equals(this.value, prefObj.value)) if (this.equals(prefObj)) https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:61: Pref.prototype.observe = function(prefObj) { Pref.prototype.observe_ = function() { Object.observe(this.data_, this.dataChanged_.bind(this), ['update']); }; https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:112: List.call(this, prefObj); Pref.call? https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:115: ListPref.__proto__ = Pref.prototype; nit: ListPref.prototype = { __proto__: Pref.prototype, ... }; https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:176: ListPref.prototype.arraysEqual_ = function(arr1, arr2) { maybe crazy question, but could ListPref contain an Array<Pref>? that way, we could just do: if (this.data_.length != otherPref.data_.length) return false; for (var i = 0; i < this.data_.length; ++i) { if (!this.data_[i].equals(otherPref.data_[i])) return false; } return true; https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:213: this.prefCache_ = {}; nit: this.prefs_ or this.state_ if this is long-lived or this.pending_ if they're only while inflight
I admit, I haven't examined or thought through the proposal careful. It might make sense to meet to discuss this early next week (in which case I will bone up), because I expect it will be important down the line. FWIW: For any networking changes, the UI never modifies the Polymer state (properties) directly. The UI only ever sends requests to Chrome to change state, and those changes are then reflected in the UI once received. The downside of this model is that there is potential for lag and the UI could feel unresponsive. The upside (which I believe outweighs the downside) is that the UI always reflects the actual state, and there is much less potential for feedback loops. In contrast, as I am implementing the policy indicators, I noticed the following in the current md-settings UI: If a cr-settings-checkbox is toggled but the Chrome model doesn't make the change (e.g. because the preference is policy restricted), the checkbox no longer reflects the correct state (which is prevented from changing by the model) until the page is refreshed. I don't think that we should rely on the UI controls (which should be disabled in this case, and I am currently implementing that) to match the model logic. i.e. if we do decide to continue to update the cached state when the UI changes, but the model change request fails, we should be sure to detect that and update any cached state. On Fri, Aug 21, 2015 at 11:13 AM, <dbeam@chromium.org> wrote: > i know you said not for review, but... > > I'm still confused as to what you're trying to accomplish. > > Are you trying to mirror C++'s model of prefs to JS? That's probably a bad > idea, as we're required to have a model in C++, so anywhere else is > duplicative > and probably harmful. We should make the views dumber, not smarter, IMO. > > Do you understand the exact reason we're able to get in a looping state? > Is it > because 2 pages keep receiving updates from C++ that don't match their > local > state, and both sides think "I have a more updated value"? > > > > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > File chrome/browser/resources/settings/prefs/prefs.js (right): > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/prefs/prefs.js:30: * @param > {!PrefObject} prefObj > PrefObject -> PrefData > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/prefs/prefs.js:32: function > Pref(prefObj) { > prefObj -> data > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/prefs/prefs.js:32: function > Pref(prefObj) { > this.data_ = data; > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/prefs/prefs.js:34: > this.update(prefObj); > Object.observe(this.data_, this.dataChanged_.bind(this), ['update']); > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/prefs/prefs.js:38: > Pref.prototype.equals = function(oldValue, newValue) { > Pref.prototype.equals = function(otherPref) { > return this.type == otherPref.type && this.value == otherPref.value; > }; > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/prefs/prefs.js:50: if > (this.equals(this.value, prefObj.value)) > if (this.equals(prefObj)) > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/prefs/prefs.js:61: > Pref.prototype.observe = function(prefObj) { > Pref.prototype.observe_ = function() { > Object.observe(this.data_, this.dataChanged_.bind(this), ['update']); > }; > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/prefs/prefs.js:112: List.call(this, > prefObj); > Pref.call? > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/prefs/prefs.js:115: ListPref.__proto__ > = Pref.prototype; > nit: > > ListPref.prototype = { > __proto__: Pref.prototype, > ... > }; > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/prefs/prefs.js:176: > ListPref.prototype.arraysEqual_ = function(arr1, arr2) { > maybe crazy question, but could ListPref contain an Array<Pref>? > > that way, we could just do: > > if (this.data_.length != otherPref.data_.length) > return false; > > for (var i = 0; i < this.data_.length; ++i) { > if (!this.data_[i].equals(otherPref.data_[i])) > return false; > } > > return true; > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/prefs/prefs.js:213: this.prefCache_ = > {}; > nit: this.prefs_ or this.state_ if this is long-lived > > or this.pending_ if they're only while inflight > > https://codereview.chromium.org/1287913005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Agreed: the ui state should always be informed by the C++ prefs model (this can
be indirect too: it's OK for an expand/collapse state to be dependent on a
settings checkbox, as long as the checkbox itself is dependent on the C++
model).
dbeam: Not mirror, no. Just save the latest values we got from settingsPrivate,
independently from what the UI is using.
Because the UI doesn't have perfect knowledge of the model, it's dangerous to
make assumptions. We observe C++ prefs to update Polymer properties, and we
observe Polymer properties to update C++ prefs. We assume it's OK to do C++ ->
Polymer -> C++ through the transitive property but it's not.
From the console window, try:
chrome.settingsPrivate.setPref('alternate_error_pages.enabled', false, '',
function() {}) |
chrome.settingsPrivate.setPref('alternate_error_pages.enabled', true, '',
function() {});
What happens:
* settingsPrivate sets pref to false. Sends prefChanged(false)
* settingsPrivate sets pref to true. Sends prefChanged(true)
* prefs.js receives prefChanged(false)
* prefs.js sets prefStore.pref.value = false, which is stale
* prefs.js observes that prefStore.pref.value changed and calls setPref(false)
***
* settingsPrivate sets pref BACK to false and sends prefChanged(false)
* prefs.js receives prefChanged(true)
* prefs.js sets prefStore.pref.value = true, which is now stale too
* prefs.js observes that prefStore.pref.value changed and calls setPref(true)
***
* settingsPrivate sets pref to true again and sends prefChanged(true)
The UI and C++ are out of sync and an infinite loop ensues over IPC.
What I want to do is prevent the *** lines: We shouldn't call setPref with the
value we just got from settingsPrivate. The existing code assumes this is safe
but if the value is stale (e.g. because it gets changed elsewhere in Chrome) we
can get stuck.
On 2015/08/21 17:35:24, stevenjb wrote:
> I admit, I haven't examined or thought through the proposal careful. It
> might make sense to meet to discuss this early next week (in which case I
> will bone up), because I expect it will be important down the line.
>
> FWIW: For any networking changes, the UI never modifies the Polymer state
> (properties) directly. The UI only ever sends requests to Chrome to change
> state, and those changes are then reflected in the UI once received. The
> downside of this model is that there is potential for lag and the UI could
> feel unresponsive. The upside (which I believe outweighs the downside) is
> that the UI always reflects the actual state, and there is much less
> potential for feedback loops.
>
> In contrast, as I am implementing the policy indicators, I noticed the
> following in the current md-settings UI:
>
> If a cr-settings-checkbox is toggled but the Chrome model doesn't make the
> change (e.g. because the preference is policy restricted), the checkbox no
> longer reflects the correct state (which is prevented from changing by the
> model) until the page is refreshed.
>
> I don't think that we should rely on the UI controls (which should be
> disabled in this case, and I am currently implementing that) to match the
> model logic. i.e. if we do decide to continue to update the cached state
> when the UI changes, but the model change request fails, we should be sure
> to detect that and update any cached state.
>
>
>
> On Fri, Aug 21, 2015 at 11:13 AM, <mailto:dbeam@chromium.org> wrote:
>
> > i know you said not for review, but...
> >
> > I'm still confused as to what you're trying to accomplish.
> >
> > Are you trying to mirror C++'s model of prefs to JS? That's probably a bad
> > idea, as we're required to have a model in C++, so anywhere else is
> > duplicative
> > and probably harmful. We should make the views dumber, not smarter, IMO.
> >
> > Do you understand the exact reason we're able to get in a looping state?
> > Is it
> > because 2 pages keep receiving updates from C++ that don't match their
> > local
> > state, and both sides think "I have a more updated value"?
> >
> >
> >
> >
> >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > File chrome/browser/resources/settings/prefs/prefs.js (right):
> >
> >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > chrome/browser/resources/settings/prefs/prefs.js:30: * @param
> > {!PrefObject} prefObj
> > PrefObject -> PrefData
> >
> >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > chrome/browser/resources/settings/prefs/prefs.js:32: function
> > Pref(prefObj) {
> > prefObj -> data
> >
> >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > chrome/browser/resources/settings/prefs/prefs.js:32: function
> > Pref(prefObj) {
> > this.data_ = data;
> >
> >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > chrome/browser/resources/settings/prefs/prefs.js:34:
> > this.update(prefObj);
> > Object.observe(this.data_, this.dataChanged_.bind(this), ['update']);
> >
> >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > chrome/browser/resources/settings/prefs/prefs.js:38:
> > Pref.prototype.equals = function(oldValue, newValue) {
> > Pref.prototype.equals = function(otherPref) {
> > return this.type == otherPref.type && this.value == otherPref.value;
> > };
> >
> >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > chrome/browser/resources/settings/prefs/prefs.js:50: if
> > (this.equals(this.value, prefObj.value))
> > if (this.equals(prefObj))
> >
> >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > chrome/browser/resources/settings/prefs/prefs.js:61:
> > Pref.prototype.observe = function(prefObj) {
> > Pref.prototype.observe_ = function() {
> > Object.observe(this.data_, this.dataChanged_.bind(this), ['update']);
> > };
> >
> >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > chrome/browser/resources/settings/prefs/prefs.js:112: List.call(this,
> > prefObj);
> > Pref.call?
> >
> >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > chrome/browser/resources/settings/prefs/prefs.js:115: ListPref.__proto__
> > = Pref.prototype;
> > nit:
> >
> > ListPref.prototype = {
> > __proto__: Pref.prototype,
> > ...
> > };
> >
> >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > chrome/browser/resources/settings/prefs/prefs.js:176:
> > ListPref.prototype.arraysEqual_ = function(arr1, arr2) {
> > maybe crazy question, but could ListPref contain an Array<Pref>?
> >
> > that way, we could just do:
> >
> > if (this.data_.length != otherPref.data_.length)
> > return false;
> >
> > for (var i = 0; i < this.data_.length; ++i) {
> > if (!this.data_[i].equals(otherPref.data_[i]))
> > return false;
> > }
> >
> > return true;
> >
> >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > chrome/browser/resources/settings/prefs/prefs.js:213: this.prefCache_ =
> > {};
> > nit: this.prefs_ or this.state_ if this is long-lived
> >
> > or this.pending_ if they're only while inflight
> >
> > https://codereview.chromium.org/1287913005/
> >
>
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2015/08/21 19:40:24, michaelpg wrote:
> Agreed: the ui state should always be informed by the C++ prefs model (this
can
> be indirect too: it's OK for an expand/collapse state to be dependent on a
> settings checkbox, as long as the checkbox itself is dependent on the C++
> model).
>
> dbeam: Not mirror, no. Just save the latest values we got from
settingsPrivate,
> independently from what the UI is using.
>
> Because the UI doesn't have perfect knowledge of the model, it's dangerous to
> make assumptions. We observe C++ prefs to update Polymer properties, and we
> observe Polymer properties to update C++ prefs. We assume it's OK to do C++ ->
> Polymer -> C++ through the transitive property but it's not.
>
> From the console window, try:
>
> chrome.settingsPrivate.setPref('alternate_error_pages.enabled', false, '',
> function() {}) |
> chrome.settingsPrivate.setPref('alternate_error_pages.enabled', true, '',
> function() {});
>
> What happens:
>
> * settingsPrivate sets pref to false. Sends prefChanged(false)
> * settingsPrivate sets pref to true. Sends prefChanged(true)
>
> * prefs.js receives prefChanged(false)
> * prefs.js sets prefStore.pref.value = false, which is stale
> * prefs.js observes that prefStore.pref.value changed and calls setPref(false)
^ we should only send updates from JS -> C++ when based on a user action. if we
can't currently determine the difference between those code paths, we should add
code to do this.
> ***
>
> * settingsPrivate sets pref BACK to false and sends prefChanged(false)
>
> * prefs.js receives prefChanged(true)
> * prefs.js sets prefStore.pref.value = true, which is now stale too
> * prefs.js observes that prefStore.pref.value changed and calls setPref(true)
> ***
>
> * settingsPrivate sets pref to true again and sends prefChanged(true)
>
> The UI and C++ are out of sync and an infinite loop ensues over IPC.
>
> What I want to do is prevent the *** lines: We shouldn't call setPref with the
> value we just got from settingsPrivate. The existing code assumes this is safe
> but if the value is stale (e.g. because it gets changed elsewhere in Chrome)
we
> can get stuck.
>
>
> On 2015/08/21 17:35:24, stevenjb wrote:
>
> > I admit, I haven't examined or thought through the proposal careful. It
> > might make sense to meet to discuss this early next week (in which case I
> > will bone up), because I expect it will be important down the line.
> >
> > FWIW: For any networking changes, the UI never modifies the Polymer state
> > (properties) directly. The UI only ever sends requests to Chrome to change
> > state, and those changes are then reflected in the UI once received. The
> > downside of this model is that there is potential for lag and the UI could
> > feel unresponsive. The upside (which I believe outweighs the downside) is
> > that the UI always reflects the actual state, and there is much less
> > potential for feedback loops.
> >
> > In contrast, as I am implementing the policy indicators, I noticed the
> > following in the current md-settings UI:
> >
> > If a cr-settings-checkbox is toggled but the Chrome model doesn't make the
> > change (e.g. because the preference is policy restricted), the checkbox no
> > longer reflects the correct state (which is prevented from changing by the
> > model) until the page is refreshed.
> >
> > I don't think that we should rely on the UI controls (which should be
> > disabled in this case, and I am currently implementing that) to match the
> > model logic. i.e. if we do decide to continue to update the cached state
> > when the UI changes, but the model change request fails, we should be sure
> > to detect that and update any cached state.
> >
> >
> >
> > On Fri, Aug 21, 2015 at 11:13 AM, <mailto:dbeam@chromium.org> wrote:
> >
> > > i know you said not for review, but...
> > >
> > > I'm still confused as to what you're trying to accomplish.
> > >
> > > Are you trying to mirror C++'s model of prefs to JS? That's probably a
bad
> > > idea, as we're required to have a model in C++, so anywhere else is
> > > duplicative
> > > and probably harmful. We should make the views dumber, not smarter, IMO.
> > >
> > > Do you understand the exact reason we're able to get in a looping state?
> > > Is it
> > > because 2 pages keep receiving updates from C++ that don't match their
> > > local
> > > state, and both sides think "I have a more updated value"?
> > >
> > >
> > >
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > > File chrome/browser/resources/settings/prefs/prefs.js (right):
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > > chrome/browser/resources/settings/prefs/prefs.js:30: * @param
> > > {!PrefObject} prefObj
> > > PrefObject -> PrefData
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > > chrome/browser/resources/settings/prefs/prefs.js:32: function
> > > Pref(prefObj) {
> > > prefObj -> data
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > > chrome/browser/resources/settings/prefs/prefs.js:32: function
> > > Pref(prefObj) {
> > > this.data_ = data;
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > > chrome/browser/resources/settings/prefs/prefs.js:34:
> > > this.update(prefObj);
> > > Object.observe(this.data_, this.dataChanged_.bind(this), ['update']);
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > > chrome/browser/resources/settings/prefs/prefs.js:38:
> > > Pref.prototype.equals = function(oldValue, newValue) {
> > > Pref.prototype.equals = function(otherPref) {
> > > return this.type == otherPref.type && this.value == otherPref.value;
> > > };
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > > chrome/browser/resources/settings/prefs/prefs.js:50: if
> > > (this.equals(this.value, prefObj.value))
> > > if (this.equals(prefObj))
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > > chrome/browser/resources/settings/prefs/prefs.js:61:
> > > Pref.prototype.observe = function(prefObj) {
> > > Pref.prototype.observe_ = function() {
> > > Object.observe(this.data_, this.dataChanged_.bind(this), ['update']);
> > > };
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > > chrome/browser/resources/settings/prefs/prefs.js:112: List.call(this,
> > > prefObj);
> > > Pref.call?
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > > chrome/browser/resources/settings/prefs/prefs.js:115: ListPref.__proto__
> > > = Pref.prototype;
> > > nit:
> > >
> > > ListPref.prototype = {
> > > __proto__: Pref.prototype,
> > > ...
> > > };
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > > chrome/browser/resources/settings/prefs/prefs.js:176:
> > > ListPref.prototype.arraysEqual_ = function(arr1, arr2) {
> > > maybe crazy question, but could ListPref contain an Array<Pref>?
> > >
> > > that way, we could just do:
> > >
> > > if (this.data_.length != otherPref.data_.length)
> > > return false;
> > >
> > > for (var i = 0; i < this.data_.length; ++i) {
> > > if (!this.data_[i].equals(otherPref.data_[i]))
> > > return false;
> > > }
> > >
> > > return true;
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource...
> > > chrome/browser/resources/settings/prefs/prefs.js:213: this.prefCache_ =
> > > {};
> > > nit: this.prefs_ or this.state_ if this is long-lived
> > >
> > > or this.pending_ if they're only while inflight
> > >
> > > https://codereview.chromium.org/1287913005/
> > >
> >
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Fri, Aug 21, 2015 at 2:50 PM, <dbeam@chromium.org> wrote: > On 2015/08/21 19:40:24, michaelpg wrote: > >> Agreed: the ui state should always be informed by the C++ prefs model >> (this >> > can > >> be indirect too: it's OK for an expand/collapse state to be dependent on a >> settings checkbox, as long as the checkbox itself is dependent on the C++ >> model). >> > > dbeam: Not mirror, no. Just save the latest values we got from >> > settingsPrivate, > >> independently from what the UI is using. >> > > Because the UI doesn't have perfect knowledge of the model, it's dangerous >> to >> make assumptions. We observe C++ prefs to update Polymer properties, and >> we >> observe Polymer properties to update C++ prefs. We assume it's OK to do >> C++ -> >> Polymer -> C++ through the transitive property but it's not. >> > > From the console window, try: >> > > chrome.settingsPrivate.setPref('alternate_error_pages.enabled', false, '', >> function() {}) | >> chrome.settingsPrivate.setPref('alternate_error_pages.enabled', true, '', >> function() {}); >> > > What happens: >> > > * settingsPrivate sets pref to false. Sends prefChanged(false) >> * settingsPrivate sets pref to true. Sends prefChanged(true) >> > > * prefs.js receives prefChanged(false) >> * prefs.js sets prefStore.pref.value = false, which is stale >> * prefs.js observes that prefStore.pref.value changed and calls >> setPref(false) >> > > ^ we should only send updates from JS -> C++ when based on a user action. > if we > can't currently determine the difference between those code paths, we > should add > code to do this. We do call Polymer.Base.set upon a non-user-action, so the UI updates when the pref changes. To differentiate between that and a user action, I thought we could maintain a cache of the latest values gotten from C++; that way when a change to a pref occurs, we know if it was changed by user action (doesn't match the latest value from C++) or by C++ (matches the latest value from C++). Then dbeam suggested breaking things out into classes to polymorphicize functions like "equals". So my WIP patch is an attempt to do that. If we can find a better way to differentiate, let's do it. I thought about having a flag like isUpdatingFromCpp_ but since changes to the prefs will propagate as Polymer changes, it seemed restrictive and difficult. Alternatively I can try to simplify the approach I have now. > > *** >> > > * settingsPrivate sets pref BACK to false and sends prefChanged(false) >> > > * prefs.js receives prefChanged(true) >> * prefs.js sets prefStore.pref.value = true, which is now stale too >> * prefs.js observes that prefStore.pref.value changed and calls >> setPref(true) >> *** >> > > * settingsPrivate sets pref to true again and sends prefChanged(true) >> > > The UI and C++ are out of sync and an infinite loop ensues over IPC. >> > > What I want to do is prevent the *** lines: We shouldn't call setPref with >> the >> value we just got from settingsPrivate. The existing code assumes this is >> safe >> but if the value is stale (e.g. because it gets changed elsewhere in >> Chrome) >> > we > >> can get stuck. >> > > > On 2015/08/21 17:35:24, stevenjb wrote: >> > > > I admit, I haven't examined or thought through the proposal careful. It >> > might make sense to meet to discuss this early next week (in which case >> I >> > will bone up), because I expect it will be important down the line. >> > >> > FWIW: For any networking changes, the UI never modifies the Polymer >> state >> > (properties) directly. The UI only ever sends requests to Chrome to >> change >> > state, and those changes are then reflected in the UI once received. The >> > downside of this model is that there is potential for lag and the UI >> could >> > feel unresponsive. The upside (which I believe outweighs the downside) >> is >> > that the UI always reflects the actual state, and there is much less >> > potential for feedback loops. >> > >> > In contrast, as I am implementing the policy indicators, I noticed the >> > following in the current md-settings UI: >> > >> > If a cr-settings-checkbox is toggled but the Chrome model doesn't make >> the >> > change (e.g. because the preference is policy restricted), the checkbox >> no >> > longer reflects the correct state (which is prevented from changing by >> the >> > model) until the page is refreshed. >> > >> > I don't think that we should rely on the UI controls (which should be >> > disabled in this case, and I am currently implementing that) to match >> the >> > model logic. i.e. if we do decide to continue to update the cached state >> > when the UI changes, but the model change request fails, we should be >> sure >> > to detect that and update any cached state. >> > >> > >> > >> > On Fri, Aug 21, 2015 at 11:13 AM, <mailto:dbeam@chromium.org> wrote: >> > >> > > i know you said not for review, but... >> > > >> > > I'm still confused as to what you're trying to accomplish. >> > > >> > > Are you trying to mirror C++'s model of prefs to JS? That's probably >> a >> > bad > >> > > idea, as we're required to have a model in C++, so anywhere else is >> > > duplicative >> > > and probably harmful. We should make the views dumber, not smarter, >> IMO. >> > > >> > > Do you understand the exact reason we're able to get in a looping >> state? >> > > Is it >> > > because 2 pages keep receiving updates from C++ that don't match their >> > > local >> > > state, and both sides think "I have a more updated value"? >> > > >> > > >> > > >> > > >> > > >> > > >> > >> > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > >> > > File chrome/browser/resources/settings/prefs/prefs.js (right): >> > > >> > > >> > > >> > >> > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > >> > > chrome/browser/resources/settings/prefs/prefs.js:30: * @param >> > > {!PrefObject} prefObj >> > > PrefObject -> PrefData >> > > >> > > >> > > >> > >> > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > >> > > chrome/browser/resources/settings/prefs/prefs.js:32: function >> > > Pref(prefObj) { >> > > prefObj -> data >> > > >> > > >> > > >> > >> > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > >> > > chrome/browser/resources/settings/prefs/prefs.js:32: function >> > > Pref(prefObj) { >> > > this.data_ = data; >> > > >> > > >> > > >> > >> > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > >> > > chrome/browser/resources/settings/prefs/prefs.js:34: >> > > this.update(prefObj); >> > > Object.observe(this.data_, this.dataChanged_.bind(this), ['update']); >> > > >> > > >> > > >> > >> > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > >> > > chrome/browser/resources/settings/prefs/prefs.js:38: >> > > Pref.prototype.equals = function(oldValue, newValue) { >> > > Pref.prototype.equals = function(otherPref) { >> > > return this.type == otherPref.type && this.value == otherPref.value; >> > > }; >> > > >> > > >> > > >> > >> > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > >> > > chrome/browser/resources/settings/prefs/prefs.js:50: if >> > > (this.equals(this.value, prefObj.value)) >> > > if (this.equals(prefObj)) >> > > >> > > >> > > >> > >> > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > >> > > chrome/browser/resources/settings/prefs/prefs.js:61: >> > > Pref.prototype.observe = function(prefObj) { >> > > Pref.prototype.observe_ = function() { >> > > Object.observe(this.data_, this.dataChanged_.bind(this), >> ['update']); >> > > }; >> > > >> > > >> > > >> > >> > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > >> > > chrome/browser/resources/settings/prefs/prefs.js:112: List.call(this, >> > > prefObj); >> > > Pref.call? >> > > >> > > >> > > >> > >> > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > >> > > chrome/browser/resources/settings/prefs/prefs.js:115: >> ListPref.__proto__ >> > > = Pref.prototype; >> > > nit: >> > > >> > > ListPref.prototype = { >> > > __proto__: Pref.prototype, >> > > ... >> > > }; >> > > >> > > >> > > >> > >> > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > >> > > chrome/browser/resources/settings/prefs/prefs.js:176: >> > > ListPref.prototype.arraysEqual_ = function(arr1, arr2) { >> > > maybe crazy question, but could ListPref contain an Array<Pref>? >> > > >> > > that way, we could just do: >> > > >> > > if (this.data_.length != otherPref.data_.length) >> > > return false; >> > > >> > > for (var i = 0; i < this.data_.length; ++i) { >> > > if (!this.data_[i].equals(otherPref.data_[i])) >> > > return false; >> > > } >> > > >> > > return true; >> > > >> > > >> > > >> > >> > > > https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... > >> > > chrome/browser/resources/settings/prefs/prefs.js:213: this.prefCache_ >> = >> > > {}; >> > > nit: this.prefs_ or this.state_ if this is long-lived >> > > >> > > or this.pending_ if they're only while inflight >> > > >> > > https://codereview.chromium.org/1287913005/ >> > > >> > >> > To unsubscribe from this group and stop receiving emails from it, send >> an >> email >> > to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > https://codereview.chromium.org/1287913005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #3 (id:60001) has been deleted
michaelpg@chromium.org changed required reviewers: - stevenjb@chromium.org
PTAL! These are the high level changes we discussed but the specifics can definitely be tweaked. https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:30: * @param {!PrefObject} prefObj On 2015/08/21 17:13:19, Dan Beam wrote: > PrefObject -> PrefData Why? PrefObject is defined in the settings_private.js externs and is the type that this function should be passed. https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:34: this.update(prefObj); On 2015/08/21 17:13:18, Dan Beam wrote: > Object.observe(this.data_, this.dataChanged_.bind(this), ['update']); Object.observe is being deprecated https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:38: Pref.prototype.equals = function(oldValue, newValue) { On 2015/08/21 17:13:19, Dan Beam wrote: > Pref.prototype.equals = function(otherPref) { > return this.type == otherPref.type && this.value == otherPref.value; > }; It's comparing PrefState and PrefObject, so I think it makes more sense to compare the values themselves. Updated to make that explicit. https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:112: List.call(this, prefObj); On 2015/08/21 17:13:19, Dan Beam wrote: > Pref.call? Changed. https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:115: ListPref.__proto__ = Pref.prototype; On 2015/08/21 17:13:19, Dan Beam wrote: > nit: > > ListPref.prototype = { > __proto__: Pref.prototype, > ... > }; OK. I'll take a closer look at this tomorrow. https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:176: ListPref.prototype.arraysEqual_ = function(arr1, arr2) { On 2015/08/21 17:13:19, Dan Beam wrote: > maybe crazy question, but could ListPref contain an Array<Pref>? > > that way, we could just do: > > if (this.data_.length != otherPref.data_.length) > return false; > > for (var i = 0; i < this.data_.length; ++i) { > if (!this.data_[i].equals(otherPref.data_[i])) > return false; > } > > return true; Take a look at the new CL and let me know what you think -- we're comparing apples and oranges :-\ https://codereview.chromium.org/1287913005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:213: this.prefCache_ = {}; On 2015/08/21 17:13:18, Dan Beam wrote: > nit: this.prefs_ or this.state_ if this is long-lived > > or this.pending_ if they're only while inflight -> this.settingsPrivateState_
https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/checkbox/checkbox.js (right): https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/checkbox/checkbox.js:71: this.set('pref.value', this.getNewValue_(this.checked)); what's the functional difference? this notifies observers? https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:31: * @param {!PrefObject} prefObj PrefObject -> PrefData https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:33: function PrefState(prefObj) { PrefState -> Pref, PrefWrapper https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:42: PrefState.prototype.equalsValue = function(value) { PrefState.prototype.equals = function(other) { return this.value == other.value; }; https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:57: ListPrefState.__proto__ = PrefState.prototype; ListPrefState.prototype = { __proto__: PrefState.prototype, ... less ClassName.prototype cruft ... }; https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:73: if (!arr1 || !arr2) empty arrays are truthy, btw is it valid to get here without an array? https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:82: if (val1 instanceof Array && !this.arraysEqual_(val1, val2)) nit: Array.isArray(val1) https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:87: return true; why not return arr1.join() == arr2.join(); if we don't need to support nesting, or: return JSON.stringify(arr1) == JSON.stringify(arr2); if we do? https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:182: if (!success) { if (success) return; ... https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:190: }.bind(this)); chrome.settingsPrivate.getPref(key, function(pref) { this.updatePrefs_([pref]); }.bind(this)); https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:215: * @return {string} all _ -> @private https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:217: getPrefKeyFromPath_: function(path) { i have no idea what this does, maybe give some concrete examples? https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:218: var tokens = path.split('.'); path.split('.').slice(1); https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:232: doc https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:267: prefState = new PrefState(prefObj); nit: ternary var prefState = prefObj.type == chrome.settingsPrivate.PrefType.LIST ? new ListPrefState(prefObj) : new PrefState(prefObj);
https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_private/settings_private_api.cc (right): https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/settings_private/settings_private_api.cc:42: return RespondNow(Error("Error setting pref *", parameters->name)); Since we went to the trouble to define the different failure types, we should set Error to "not_found", etc. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/checkbox/checkbox.js (right): https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/checkbox/checkbox.js:71: this.set('pref.value', this.getNewValue_(this.checked)); On 2015/08/26 16:51:33, Dan Beam wrote: > what's the functional difference? this notifies observers? Correct. https://www.polymer-project.org/1.0/docs/devguide/properties.html#observing-p... https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:31: * @param {!PrefObject} prefObj On 2015/08/26 16:51:33, Dan Beam wrote: > PrefObject -> PrefData What is PrefData? PrefObject is from settingsPrivate (and will soon need to be chrome.settingsPrivate.PrefObject) https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:73: if (!arr1 || !arr2) On 2015/08/26 16:51:33, Dan Beam wrote: > empty arrays are truthy, btw > > is it valid to get here without an array? PrefObject.value should never be null, so I believe we can/should assume that PrefState (or PrefObject) .value is also never null. That way we don't have to worry about whether null == []. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:87: return true; On 2015/08/26 16:51:33, Dan Beam wrote: > why not > > return arr1.join() == arr2.join(); > > if we don't need to support nesting, or: > > return JSON.stringify(arr1) == JSON.stringify(arr2); > > if we do? The C++ coder in me says "Eww". :) I know it's pretty unlikely, but wouldn't the first equate ["A","","B"] with ["A,","B"]? JSON.stringify should be robust since we rely on it for prefs serialization, but I might be a little concerned with the performance (but probably it doesn't matter). https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:145: if (!prefState.equalsValue(prefObj.value)) { nit: Early exit would better match the comment. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:252: nit: Add comment that notification will occur here.
https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:31: * @param {!PrefObject} prefObj On 2015/08/26 17:32:18, stevenjb wrote: > On 2015/08/26 16:51:33, Dan Beam wrote: > > PrefObject -> PrefData > > What is PrefData? PrefObject is from settingsPrivate (and will soon need to be > chrome.settingsPrivate.PrefObject) it's my proposition for a better name
https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:31: * @param {!PrefObject} prefObj On 2015/08/26 17:38:01, Dan Beam wrote: > On 2015/08/26 17:32:18, stevenjb wrote: > > On 2015/08/26 16:51:33, Dan Beam wrote: > > > PrefObject -> PrefData > > > > What is PrefData? PrefObject is from settingsPrivate (and will soon need to be > > chrome.settingsPrivate.PrefObject) > > it's my proposition for a better name My point is that't you're proposing a change to an existing API which means new externs and changes to C++ code also :P https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
On 2015/08/26 18:07:09, stevenjb wrote: > https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... > File chrome/browser/resources/settings/prefs/prefs.js (right): > > https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... > chrome/browser/resources/settings/prefs/prefs.js:31: * @param {!PrefObject} > prefObj > On 2015/08/26 17:38:01, Dan Beam wrote: > > On 2015/08/26 17:32:18, stevenjb wrote: > > > On 2015/08/26 16:51:33, Dan Beam wrote: > > > > PrefObject -> PrefData > > > > > > What is PrefData? PrefObject is from settingsPrivate (and will soon need to > be > > > chrome.settingsPrivate.PrefObject) > > > > it's my proposition for a better name > > My point is that't you're proposing a change to an existing API which means new > externs and changes to C++ code also :P > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... good thing it's settings*Private* then
On 2015/08/26 18:23:16, Dan Beam wrote: > On 2015/08/26 18:07:09, stevenjb wrote: > > > https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... > > File chrome/browser/resources/settings/prefs/prefs.js (right): > > > > > https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... > > chrome/browser/resources/settings/prefs/prefs.js:31: * @param {!PrefObject} > > prefObj > > On 2015/08/26 17:38:01, Dan Beam wrote: > > > On 2015/08/26 17:32:18, stevenjb wrote: > > > > On 2015/08/26 16:51:33, Dan Beam wrote: > > > > > PrefObject -> PrefData > > > > > > > > What is PrefData? PrefObject is from settingsPrivate (and will soon need > to > > be > > > > chrome.settingsPrivate.PrefObject) > > > > > > it's my proposition for a better name > > > > My point is that't you're proposing a change to an existing API which means > new > > externs and changes to C++ code also :P > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > > good thing it's settings*Private* then Sure, but, is s/Object/Data really that much more clear and worth the effort? They seem equally generic to me.
On 2015/08/26 18:26:50, stevenjb wrote: > On 2015/08/26 18:23:16, Dan Beam wrote: > > On 2015/08/26 18:07:09, stevenjb wrote: > > > > > > https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... > > > File chrome/browser/resources/settings/prefs/prefs.js (right): > > > > > > > > > https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... > > > chrome/browser/resources/settings/prefs/prefs.js:31: * @param {!PrefObject} > > > prefObj > > > On 2015/08/26 17:38:01, Dan Beam wrote: > > > > On 2015/08/26 17:32:18, stevenjb wrote: > > > > > On 2015/08/26 16:51:33, Dan Beam wrote: > > > > > > PrefObject -> PrefData > > > > > > > > > > What is PrefData? PrefObject is from settingsPrivate (and will soon need > > to > > > be > > > > > chrome.settingsPrivate.PrefObject) > > > > > > > > it's my proposition for a better name > > > > > > My point is that't you're proposing a change to an existing API which means > > new > > > externs and changes to C++ code also :P > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > > > > good thing it's settings*Private* then > > Sure, but, is s/Object/Data really that much more clear and worth the effort? > They seem equally generic to me. everything in JS is an object, more or less (including classes). PrefObject sounds like a class to me. Pref, PrefData, or PrefState all sound more struct-like (e.g. a vanilla built-in) tl;dr - if it's the whopping amount of effort we're worried about https://codereview.chromium.org/1315173003/
It's not the effort it's the (imho unnecessary) churn. For me, Object implies a Dictionary, whereas Data suggests it could be anything, so if we really want to change it I would personally suggest PrefContainer or PrefDictionary, but I still don't think it's worth changing. We could also typedef it in the .js. I will leave it to Michael to make the call :) On Wed, Aug 26, 2015 at 11:45 AM, <dbeam@chromium.org> wrote: > On 2015/08/26 18:26:50, stevenjb wrote: > >> On 2015/08/26 18:23:16, Dan Beam wrote: >> > On 2015/08/26 18:07:09, stevenjb wrote: >> > > >> > >> > > > https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... > >> > > File chrome/browser/resources/settings/prefs/prefs.js (right): >> > > >> > > >> > >> > > > https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... > >> > > chrome/browser/resources/settings/prefs/prefs.js:31: * @param >> > {!PrefObject} > >> > > prefObj >> > > On 2015/08/26 17:38:01, Dan Beam wrote: >> > > > On 2015/08/26 17:32:18, stevenjb wrote: >> > > > > On 2015/08/26 16:51:33, Dan Beam wrote: >> > > > > > PrefObject -> PrefData >> > > > > >> > > > > What is PrefData? PrefObject is from settingsPrivate (and will >> soon >> > need > >> > to >> > > be >> > > > > chrome.settingsPrivate.PrefObject) >> > > > >> > > > it's my proposition for a better name >> > > >> > > My point is that't you're proposing a change to an existing API which >> > means > >> > new >> > > externs and changes to C++ code also :P >> > > >> > >> > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > >> > >> > good thing it's settings*Private* then >> > > Sure, but, is s/Object/Data really that much more clear and worth the >> effort? >> They seem equally generic to me. >> > > everything in JS is an object, more or less (including classes). > PrefObject > sounds like a class to me. Pref, PrefData, or PrefState all sound more > struct-like (e.g. a vanilla built-in) > > tl;dr - if it's the whopping amount of effort we're worried about > https://codereview.chromium.org/1315173003/ > > https://codereview.chromium.org/1287913005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #4 (id:100001) has been deleted
PTAL. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_private/settings_private_api.cc (right): https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/settings_private/settings_private_api.cc:42: return RespondNow(Error("Error setting pref *", parameters->name)); On 2015/08/26 17:32:18, stevenjb wrote: > Since we went to the trouble to define the different failure types, we should > set Error to "not_found", etc. Done. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/checkbox/checkbox.js (right): https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/checkbox/checkbox.js:71: this.set('pref.value', this.getNewValue_(this.checked)); On 2015/08/26 16:51:33, Dan Beam wrote: > what's the functional difference? this notifies observers? To expand on steven's answer: Polymer doesn't use Object.observe. Top-level element properties are actually setters which use the prototype's "properties" object. Beyond the top level, Polymer doesn't own the data, so it only knows of a change if a change event is fired. Data binding fires these events automatically. When we manually change sub-properties, we have to trigger an event using notifyPath or set (a convenience function combining "operator=" and notifyPath). https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:31: * @param {!PrefObject} prefObj On 2015/08/26 18:07:09, stevenjb wrote: > On 2015/08/26 17:38:01, Dan Beam wrote: > > On 2015/08/26 17:32:18, stevenjb wrote: > > > On 2015/08/26 16:51:33, Dan Beam wrote: > > > > PrefObject -> PrefData > > > > > > What is PrefData? PrefObject is from settingsPrivate (and will soon need to > be > > > chrome.settingsPrivate.PrefObject) > > > > it's my proposition for a better name > > My point is that't you're proposing a change to an existing API which means new > externs and changes to C++ code also :P > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... I don't see how PrefObject provides more information that PrefData. I would be comfortable changing PrefObject to Pref in a later CL if that's more natural but otherwise I'm inclined to leave it be. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:33: function PrefState(prefObj) { On 2015/08/26 16:51:33, Dan Beam wrote: > PrefState -> Pref, PrefWrapper Done: PrefState -> PrefWrapper. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:42: PrefState.prototype.equalsValue = function(value) { On 2015/08/26 16:51:33, Dan Beam wrote: > PrefState.prototype.equals = function(other) { > return this.value == other.value; > }; As I mentioned in a previous comment, PrefState (PrefWrapper) is being compared with a settingsPrivate.PrefObject. It would be misleading for a function called "equals" to compare a property of two different types, wouldn't it? https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:57: ListPrefState.__proto__ = PrefState.prototype; On 2015/08/26 16:51:33, Dan Beam wrote: > ListPrefState.prototype = { > __proto__: PrefState.prototype, > > ... less ClassName.prototype cruft ... > }; Done. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:73: if (!arr1 || !arr2) On 2015/08/26 16:51:33, Dan Beam wrote: > empty arrays are truthy, btw > > is it valid to get here without an array? Seems unlikely but it depends on how prefs are structured. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:82: if (val1 instanceof Array && !this.arraysEqual_(val1, val2)) On 2015/08/26 16:51:33, Dan Beam wrote: > nit: Array.isArray(val1) Done. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:87: return true; On 2015/08/26 17:32:18, stevenjb wrote: > On 2015/08/26 16:51:33, Dan Beam wrote: > > why not > > > > return arr1.join() == arr2.join(); > > > > if we don't need to support nesting, or: > > > > return JSON.stringify(arr1) == JSON.stringify(arr2); > > > > if we do? > > The C++ coder in me says "Eww". :) I know it's pretty unlikely, but wouldn't the > first equate ["A","","B"] with ["A,","B"]? Not sure what you're going for: the second isn't a valid array. But these two non-equivalent arrays would be joined to the same string: ["A", ",B"], ["A,", "B"] > JSON.stringify should be robust since > we rely on it for prefs serialization, but I might be a little concerned with > the performance Yeah, it's kind of slow. > (but probably it doesn't matter). Yeah, it kind of doesn't. > https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:87: return true; On 2015/08/26 16:51:33, Dan Beam wrote: > why not > > return arr1.join() == arr2.join(); > > if we don't need to support nesting, or: > > return JSON.stringify(arr1) == JSON.stringify(arr2); > > if we do? JSON.stringify doesn't guarantee ordering of object property keys, so arrays of dictionaries may not compare correctly. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:145: if (!prefState.equalsValue(prefObj.value)) { On 2015/08/26 17:32:18, stevenjb wrote: > nit: Early exit would better match the comment. Done. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:182: if (!success) { On 2015/08/26 16:51:33, Dan Beam wrote: > if (success) > return; > > ... Done. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:190: }.bind(this)); On 2015/08/26 16:51:33, Dan Beam wrote: > chrome.settingsPrivate.getPref(key, function(pref) { > this.updatePrefs_([pref]); > }.bind(this)); I don't think that's more readable but it's more compact, so, done. Note that if the callback is called without a pref object we'll throw uncaught exceptions further up the stack, but the API should also log an error in that case so we should be ok. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:215: * @return {string} On 2015/08/26 16:51:33, Dan Beam wrote: > all _ -> @private Done. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:217: getPrefKeyFromPath_: function(path) { On 2015/08/26 16:51:33, Dan Beam wrote: > i have no idea what this does, maybe give some concrete examples? Done. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:218: var tokens = path.split('.'); On 2015/08/26 16:51:33, Dan Beam wrote: > path.split('.').slice(1); Done. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:232: On 2015/08/26 16:51:33, Dan Beam wrote: > doc Done. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:252: On 2015/08/26 17:32:18, stevenjb wrote: > nit: Add comment that notification will occur here. Done. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:267: prefState = new PrefState(prefObj); On 2015/08/26 16:51:33, Dan Beam wrote: > nit: ternary > > var prefState = prefObj.type == chrome.settingsPrivate.PrefType.LIST ? > new ListPrefState(prefObj) : new PrefState(prefObj); If we add object prefs, we'd have to change this back.
https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:30: * @constructor this might be better as an interface if we can't find a sane default equals/equalsValue implementation https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:34: this.value = prefObj.value; why does this have to be public? https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:42: PrefState.prototype.equalsValue = function(value) { On 2015/08/26 23:32:15, michaelpg wrote: > On 2015/08/26 16:51:33, Dan Beam wrote: > > PrefState.prototype.equals = function(other) { > > return this.value == other.value; > > }; > > As I mentioned in a previous comment, PrefState (PrefWrapper) is being compared > with a settingsPrivate.PrefObject. It would be misleading for a function called > "equals" to compare a property of two different types, wouldn't it? construct a PrefWrapper() around the PrefState and compare https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:43: return this.value == value; how do we prevent PrefStates of different types from comparing against eachother? (i.e. an integer pref of 1 compared to a list of ['1'] would == eachother.) right now it seems like it's up to the comparison order. maybe === is appropriate in this case? do we have cases where the C++ gives back '1' when the UI has 1 (or other types of stringification or type variance)? https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:72: ListPrefState.prototype.arraysEqual_ = function(arr1, arr2) { could this be generalized to work for primitives as well? https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:73: if (!arr1 || !arr2) On 2015/08/26 23:32:15, michaelpg wrote: > On 2015/08/26 16:51:33, Dan Beam wrote: > > empty arrays are truthy, btw > > > > is it valid to get here without an array? > > Seems unlikely but it depends on how prefs are structured. this opens a lot of holes. 0 == [''] for example https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:87: return true; On 2015/08/26 23:32:15, michaelpg wrote: > On 2015/08/26 16:51:33, Dan Beam wrote: > > why not > > > > return arr1.join() == arr2.join(); > > > > if we don't need to support nesting, or: > > > > return JSON.stringify(arr1) == JSON.stringify(arr2); > > > > if we do? > > JSON.stringify doesn't guarantee ordering of object property keys, so arrays of > dictionaries may not compare correctly. how does your current code handle arrays of dictionaries? https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:267: prefState = new PrefState(prefObj); On 2015/08/26 23:32:15, michaelpg wrote: > On 2015/08/26 16:51:33, Dan Beam wrote: > > nit: ternary > > > > var prefState = prefObj.type == chrome.settingsPrivate.PrefType.LIST ? > > new ListPrefState(prefObj) : new PrefState(prefObj); > > If we add object prefs, we'd have to change this back. we're not yet. do it then https://codereview.chromium.org/1287913005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:114: value: function() { return {}; }, value: {}, // does this have issues across prototypes? https://codereview.chromium.org/1287913005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:144: return; \n https://codereview.chromium.org/1287913005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:151: return; \n https://codereview.chromium.org/1287913005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:189: return; \n https://codereview.chromium.org/1287913005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:206: this.setPrefWrapper_(newPrefObj); change the name of setPrefWrapper_ if... it's not a setter that takes a type of PrefWrapper https://codereview.chromium.org/1287913005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:228: for (let token of tokens) { nit: for (let i = 0; i < tokens.length; ++i) { let key = tokens.slice(0, i).join('.'); if (this.settingsPrivateState_[key] != undefined) return key; } https://codereview.chromium.org/1287913005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:253: let tokens = newPrefObj.key.split('.').slice(0, -1); tokens -> keyParts https://codereview.chromium.org/1287913005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:263: }, this); why not use? https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... https://codereview.chromium.org/1287913005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:271: * Creates a PrefWrapper object from a chrome.settingsPrivate pref and adds {create,make}PrefWrapper_? general nit: if the doc comment varies from the method name, one should probably win
ptal. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:30: * @constructor On 2015/08/28 00:08:41, Dan Beam wrote: > this might be better as an interface if we can't find a sane default > equals/equalsValue implementation ? the sane default is == (for all primitive types) https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:34: this.value = prefObj.value; On 2015/08/28 00:08:40, Dan Beam wrote: > why does this have to be public? Doesn't. Done. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:42: PrefState.prototype.equalsValue = function(value) { On 2015/08/28 00:08:40, Dan Beam wrote: > On 2015/08/26 23:32:15, michaelpg wrote: > > On 2015/08/26 16:51:33, Dan Beam wrote: > > > PrefState.prototype.equals = function(other) { > > > return this.value == other.value; > > > }; > > > > As I mentioned in a previous comment, PrefState (PrefWrapper) is being > compared > > with a settingsPrivate.PrefObject. It would be misleading for a function > called > > "equals" to compare a property of two different types, wouldn't it? > > construct a PrefWrapper() around the PrefState and compare Done. I didn't have that because it seemed like extra work for no reason. But I trust your judgment. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:43: return this.value == value; On 2015/08/28 00:08:40, Dan Beam wrote: > how do we prevent PrefStates of different types from comparing against > eachother? (i.e. an integer pref of 1 compared to a list of ['1'] would == > eachother.) right now it seems like it's up to the comparison order. > > maybe === is appropriate in this case? do we have cases where the C++ gives > back '1' when the UI has 1 (or other types of stringification or type variance)? Prefs are typed in their PrefRegistry as a specific base::Value subtype, so they should not be doing this. IOW, it is sane for PrefState to assume the types are the same. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:87: return true; On 2015/08/28 00:08:40, Dan Beam wrote: > On 2015/08/26 23:32:15, michaelpg wrote: > > On 2015/08/26 16:51:33, Dan Beam wrote: > > > why not > > > > > > return arr1.join() == arr2.join(); > > > > > > if we don't need to support nesting, or: > > > > > > return JSON.stringify(arr1) == JSON.stringify(arr2); > > > > > > if we do? > > > > JSON.stringify doesn't guarantee ordering of object property keys, so arrays > of > > dictionaries may not compare correctly. > > how does your current code handle arrays of dictionaries? It doesn't. So I'll remove this and we can restart this discussion later if we add it back in. https://codereview.chromium.org/1287913005/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:267: prefState = new PrefState(prefObj); On 2015/08/28 00:08:41, Dan Beam wrote: > On 2015/08/26 23:32:15, michaelpg wrote: > > On 2015/08/26 16:51:33, Dan Beam wrote: > > > nit: ternary > > > > > > var prefState = prefObj.type == chrome.settingsPrivate.PrefType.LIST ? > > > new ListPrefState(prefObj) : new PrefState(prefObj); > > > > If we add object prefs, we'd have to change this back. > > we're not yet. do it then Done. https://codereview.chromium.org/1287913005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:114: value: function() { return {}; }, On 2015/08/28 00:08:41, Dan Beam wrote: > value: {}, // does this have issues across prototypes? it has issues across objects using this prototype, yes. If we create two <cr-settings-prefs>, each will have a reference to the same object. https://www.polymer-project.org/1.0/docs/devguide/properties.html#configure-v... We have no reason to make multiple <cr-settings-prefs> instances, but I don't think we should ignore the possibility unless we also want to enforce a singleton policy. https://codereview.chromium.org/1287913005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:144: return; On 2015/08/28 00:08:41, Dan Beam wrote: > \n Done. https://codereview.chromium.org/1287913005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:151: return; On 2015/08/28 00:08:41, Dan Beam wrote: > \n Done. https://codereview.chromium.org/1287913005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:189: return; On 2015/08/28 00:08:41, Dan Beam wrote: > \n Done. https://codereview.chromium.org/1287913005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:206: this.setPrefWrapper_(newPrefObj); On 2015/08/28 00:08:41, Dan Beam wrote: > change the name of setPrefWrapper_ if... it's not a setter that takes a type of > PrefWrapper Done. https://codereview.chromium.org/1287913005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:228: for (let token of tokens) { On 2015/08/28 00:08:41, Dan Beam wrote: > nit: > > for (let i = 0; i < tokens.length; ++i) { > let key = tokens.slice(0, i).join('.'); > if (this.settingsPrivateState_[key] != undefined) > return key; > } Done, but for i from [1, tokens.length]. https://codereview.chromium.org/1287913005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:253: let tokens = newPrefObj.key.split('.').slice(0, -1); On 2015/08/28 00:08:41, Dan Beam wrote: > tokens -> keyParts Done. https://codereview.chromium.org/1287913005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:263: }, this); On 2015/08/28 00:08:41, Dan Beam wrote: > why not use? > https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... Done, thanks. https://codereview.chromium.org/1287913005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:271: * Creates a PrefWrapper object from a chrome.settingsPrivate pref and adds On 2015/08/28 00:08:41, Dan Beam wrote: > {create,make}PrefWrapper_? > > general nit: if the doc comment varies from the method name, one should probably > win Done.
lgtm w/nits + questions https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:111: settingsPrivateWrappers_: { nit: wrappers_ or prefWrappers_ https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:141: var prefWrapper = this.settingsPrivateWrappers_[key]; evidence that settingsPrivateWrappers_ should be named prefWrappers_ :) https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:142: if (!prefWrapper) out of curiosity, when should this happen? never? can we assert() if so? https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:220: * 'prefs.search.suggest_enabled.value', the key of the pref that changed is why can this just be return path.split('.').slice(1, -1); is there not always a "prefs." and ".value"? i don't love the automagic object property searching https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:249: this.set('prefs.' + newPrefObj.key + '.value', newPrefObj.value); it's unfortunate that this auto expanding object stuff doesn't happen for us in Polymer. but i guess then you wouldn't be able to create properties with dots in them. https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:267: return (prefObj.type == chrome.settingsPrivate.PrefType.LIST) ? nit: remove extra ()
https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:70: // Two arrays might have the same values, so don't just use "==". This comment doesn't make much sense to me. IIUC, Array/Object == tests that both are instances of the same object, so it should be clear that a special comparator is needed. i.e. the comment just confuses me :) Also, since this function is otherwise trivial and arraysEqual_ is called only once, maybe combine them? https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:111: settingsPrivateWrappers_: { On 2015/08/28 02:05:36, Dan Beam wrote: > nit: wrappers_ or prefWrappers_ FWIW I personally prefer the more explicit name to identify the purpose of the second dictionary. https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:142: if (!prefWrapper) On 2015/08/28 02:05:36, Dan Beam wrote: > out of curiosity, when should this happen? never? can we assert() if so? IIUC, this would happen if UI referenced a pref that doesn't exist in settingsPrivate. That being the case, while it might be a little annoying for development, it would prevent non-functional UI elements, so I agree that we should assert here. https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:220: * 'prefs.search.suggest_enabled.value', the key of the pref that changed is On 2015/08/28 02:05:36, Dan Beam wrote: > why can this just be > > return path.split('.').slice(1, -1); > > is there not always a "prefs." and ".value"? > > i don't love the automagic object property searching Until / unless we support dictionary objects, I think Dan is correct, and simpler is better. We already verify that the path exists in settingsPrivateWrappers where this gets called, so it would probably be simpler to eliminate this altogether.
https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:220: * 'prefs.search.suggest_enabled.value', the key of the pref that changed is On 2015/08/28 02:05:36, Dan Beam wrote: > why can this just be can't** > > return path.split('.').slice(1, -1); > > is there not always a "prefs." and ".value"? > > i don't love the automagic object property searching we could also do: var parts = path.split('.'); assert(parts.shift() == 'prefs'); assert(parts.pop() == 'value'); return parts.join('.');
https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:70: // Two arrays might have the same values, so don't just use "==". On 2015/08/28 17:35:20, stevenjb wrote: > This comment doesn't make much sense to me. IIUC, Array/Object == tests that > both are instances of the same object, so it should be clear that a special > comparator is needed. i.e. the comment just confuses me :) > Also, since this function is otherwise trivial and arraysEqual_ is called only > once, maybe combine them? Done. https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:111: settingsPrivateWrappers_: { On 2015/08/28 17:35:21, stevenjb wrote: > On 2015/08/28 02:05:36, Dan Beam wrote: > > nit: wrappers_ or prefWrappers_ > > FWIW I personally prefer the more explicit name to identify the purpose of the > second dictionary. Acknowledged. https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:111: settingsPrivateWrappers_: { On 2015/08/28 02:05:36, Dan Beam wrote: > nit: wrappers_ or prefWrappers_ Done. https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:141: var prefWrapper = this.settingsPrivateWrappers_[key]; On 2015/08/28 02:05:36, Dan Beam wrote: > evidence that settingsPrivateWrappers_ should be named prefWrappers_ :) Done. https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:142: if (!prefWrapper) On 2015/08/28 17:35:21, stevenjb wrote: > On 2015/08/28 02:05:36, Dan Beam wrote: > > out of curiosity, when should this happen? never? can we assert() if so? > > IIUC, this would happen if UI referenced a pref that doesn't exist in > settingsPrivate. That being the case, while it might be a little annoying for > development, it would prevent non-functional UI elements, so I agree that we > should assert here. <cr-settings-pref-tracker> detects this case more reliably, so an assert here would at best be redundant. https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:220: * 'prefs.search.suggest_enabled.value', the key of the pref that changed is On 2015/08/28 02:05:36, Dan Beam wrote: > why can this just be > > return path.split('.').slice(1, -1); > > is there not always a "prefs." and ".value"? > > i don't love the automagic object property searching There is always a "prefs." and a ".value", but after the ".value" there may be ".splices" or ".length" for arrays splices, or \.\d (".0", ".1", etc.) for changes to array elements. https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:249: this.set('prefs.' + newPrefObj.key + '.value', newPrefObj.value); On 2015/08/28 02:05:36, Dan Beam wrote: > it's unfortunate that this auto expanding object stuff doesn't happen for us in > Polymer. but i guess then you wouldn't be able to create properties with dots > in them. yup. in reality we should be transforming dots to some other separator, but we decided not to for a few reasons: this maintains the ability to grep for preference keys, and is easier for people modifying settings pages to understand (at the expense of those of us modifying the core prefs.js stuff). https://codereview.chromium.org/1287913005/diff/140001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:267: return (prefObj.type == chrome.settingsPrivate.PrefType.LIST) ? On 2015/08/28 02:05:36, Dan Beam wrote: > nit: remove extra () Done.
Patchset #8 (id:200001) has been deleted
Compare with Patset 5 ("dbeam comments").
https://codereview.chromium.org/1287913005/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/prefs/prefs.js (right):
https://codereview.chromium.org/1287913005/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/prefs/prefs.js:95: * @type
{Object<PrefWrapper>}
"ERROR - Bad type annotation. Unknown type PrefWrapper"
>_<
https://codereview.chromium.org/1287913005/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/prefs/prefs.js:244:
cr.exportPath(newPrefObj.key, newPrefObj, this.prefs);
"ERROR - cr.exportPath() should have exactly 1 argument: namespace name."
"ERROR - variable cr is undeclared"
"ERROR - Property exportPath never defined on cr"
How can all 3 of those errors be true?
https://codereview.chromium.org/1287913005/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:244: cr.exportPath(newPrefObj.key, newPrefObj, this.prefs); On 2015/08/28 20:43:05, michaelpg wrote: > "ERROR - cr.exportPath() should have exactly 1 argument: namespace name." > "ERROR - variable cr is undeclared" > "ERROR - Property exportPath never defined on cr" > > How can all 3 of those errors be true? this is wrong: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur... the other 2: add dependencies on cr.js in some compiled_resources.gyp
https://codereview.chromium.org/1287913005/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:244: cr.exportPath(newPrefObj.key, newPrefObj, this.prefs); On 2015/08/28 20:50:05, Dan Beam wrote: > On 2015/08/28 20:43:05, michaelpg wrote: > > "ERROR - cr.exportPath() should have exactly 1 argument: namespace name." > > "ERROR - variable cr is undeclared" > > "ERROR - Property exportPath never defined on cr" > > > > How can all 3 of those errors be true? > > this is wrong: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur... should we remove that check? or not use exportPath? > > the other 2: add dependencies on cr.js in some compiled_resources.gyp
https://codereview.chromium.org/1287913005/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:244: cr.exportPath(newPrefObj.key, newPrefObj, this.prefs); On 2015/08/28 20:58:10, michaelpg wrote: > On 2015/08/28 20:50:05, Dan Beam wrote: > > On 2015/08/28 20:43:05, michaelpg wrote: > > > "ERROR - cr.exportPath() should have exactly 1 argument: namespace name." > > > "ERROR - variable cr is undeclared" > > > "ERROR - Property exportPath never defined on cr" > > > > > > How can all 3 of those errors be true? > > > > this is wrong: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur... > > should we remove that check? or not use exportPath? we should fix it > > > > > the other 2: add dependencies on cr.js in some compiled_resources.gyp >
dbeam: PTAL -- had to move PrefWrappers outside the closure, and updated ChromePass.java to accept cr.exportPath usage. The GYP stuff will be uploaded with stevenjb's CL after this one lands. https://codereview.chromium.org/1287913005/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:95: * @type {Object<PrefWrapper>} On 2015/08/28 20:43:05, michaelpg wrote: > "ERROR - Bad type annotation. Unknown type PrefWrapper" > > >_< Done. https://codereview.chromium.org/1287913005/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:244: cr.exportPath(newPrefObj.key, newPrefObj, this.prefs); On 2015/08/28 21:06:56, Dan Beam wrote: > On 2015/08/28 20:58:10, michaelpg wrote: > > On 2015/08/28 20:50:05, Dan Beam wrote: > > > On 2015/08/28 20:43:05, michaelpg wrote: > > > > "ERROR - cr.exportPath() should have exactly 1 argument: namespace name." > > > > "ERROR - variable cr is undeclared" > > > > "ERROR - Property exportPath never defined on cr" > > > > > > > > How can all 3 of those errors be true? > > > > > > this is wrong: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur... > > > > should we remove that check? or not use exportPath? > > we should fix it > > > > > > > > > the other 2: add dependencies on cr.js in some compiled_resources.gyp > > Done.
On 2015/08/29 01:26:01, michaelpg wrote: > dbeam: PTAL -- had to move PrefWrappers outside the closure, and updated > ChromePass.java to accept cr.exportPath usage. you... double updated ChromePass.java? https://codereview.chromium.org/1319263002/
lgtm w/pass+runner revert https://codereview.chromium.org/1287913005/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:36: /** off https://codereview.chromium.org/1287913005/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:47: /** also off https://codereview.chromium.org/1287913005/diff/240001/third_party/closure_co... File third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java (right): https://codereview.chromium.org/1287913005/diff/240001/third_party/closure_co... third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java:263: crExportPathNode.getChildAtIndex(1).getString()); cr.exportPath(pathName); // throws without a 'literal'
https://codereview.chromium.org/1287913005/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1287913005/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:36: /** On 2015/08/29 01:49:30, Dan Beam wrote: > off ew, sorry https://codereview.chromium.org/1287913005/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:47: /** On 2015/08/29 01:49:30, Dan Beam wrote: > also off Done. https://codereview.chromium.org/1287913005/diff/240001/third_party/closure_co... File third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java (right): https://codereview.chromium.org/1287913005/diff/240001/third_party/closure_co... third_party/closure_compiler/runner/src/com/google/javascript/jscomp/ChromePass.java:263: crExportPathNode.getChildAtIndex(1).getString()); On 2015/08/29 01:49:30, Dan Beam wrote: > cr.exportPath(pathName); // throws without a 'literal' Acknowledged.
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/1287913005/#ps330001 (title: "whitespace again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1287913005/330001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1287913005/330001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) 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
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/1287913005/#ps350001 (title: "add return after switch for non-clang")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1287913005/350001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1287913005/350001
Message was sent while issue was closed.
Committed patchset #15 (id:350001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/037f8fdb34fc72d62d3a78893dc143d870b0d7e5 Cr-Commit-Position: refs/heads/master@{#346747} |
