Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1333473002:
Support lists in <cr-settings-pref> (Closed)
|
Created:
5 years, 3 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@PrefsTests Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds support for list prefs (including lists of dictionaries).
TEST=CrSettingsBrowserTest
BUG=532270
Committed: https://crrev.com/6d665fd58254221164550766b8f62e913decc4a8
Cr-Commit-Position: refs/heads/master@{#349604}
Patch Set 1 #Patch Set 2 : Now with 100% more correctness #Patch Set 3 : List prefs fo real. #Patch Set 4 : closuring #Patch Set 5 : rebase #
Total comments: 19
Patch Set 6 : check types #Patch Set 7 : #Patch Set 8 : remove wrappers #Patch Set 9 : #
Total comments: 6
Patch Set 10 : #
Total comments: 3
Patch Set 11 : remove redundant check #
Messages
Total messages: 21 (6 generated)
michaelpg@chromium.org changed reviewers: + dbeam@chromium.org
Add list support and tests cases, as a follow-up to testing cr-settings-prefs.
https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (left): https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:52: function ListPrefWrapper(prefObj) { Why wouldn't we just make a DictPrefWrapper? https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:79: PrefWrapper.valuesEquivalent = function(val1, val2) { var toString = Object.prototype.toString; if (toString.call(val1) != toString.call(val2)) return false; if (val1 == val2) return true; if (val1 === null || typeof val1 != 'object') return false; function len(val) { return (Array.isArray(val) ? val : Object.keys(val)).length; } if (len(val1) != len(val2)) return false; for (var key in val1) { if (!PrefWrapper.values equivalent(val1[key], val2[key])) return false; } return true; https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:80: if (val1 == val2) Eh probably don't do this without type checking first https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:96: if (typeof val1 == 'object') { What about {}, null? https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:122: if (typeof val == 'object') { null?
https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (left): https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:52: function ListPrefWrapper(prefObj) { On 2015/09/17 06:36:56, Dan Beam wrote: > Why wouldn't we just make a DictPrefWrapper? These wrappers don't make much sense to me so I'm not sure what a DictPrefWrapper would look like. If anything I would get rid of the polymorphic design entirely, or split it between FundamentalPrefWrapper and ComplexPrefWrapper. A ListPref could have Dictionaries, and a DictPref could have Lists. It would be too much to have a ListPrefWrapper containing DictPrefWrappers containing ListPrefWrappers. Having a single, generalized, recursive "equals" function is simpler IMO than having ListEqual, DictionaryEqual, etc. and having them call each other recursively. https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:79: PrefWrapper.valuesEquivalent = function(val1, val2) { On 2015/09/17 06:36:56, Dan Beam wrote: > var toString = Object.prototype.toString; > if (toString.call(val1) != toString.call(val2)) > return false; > > if (val1 == val2) > return true; > > if (val1 === null || typeof val1 != 'object') > return false; > > function len(val) { > return (Array.isArray(val) ? val : Object.keys(val)).length; > } > > if (len(val1) != len(val2)) > return false; > > for (var key in val1) { > if (!PrefWrapper.values equivalent(val1[key], val2[key])) > return false; > } > > return true; I assume 80% of the time we'll be comparing booleans or numbers. Is it worth converting those to strings? https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:80: if (val1 == val2) On 2015/09/17 06:36:56, Dan Beam wrote: > Eh probably don't do this without type checking first Acknowledged. https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:96: if (typeof val1 == 'object') { On 2015/09/17 06:36:56, Dan Beam wrote: > What about {}, null? {}: "just works", valuesEquivalent({}, {}) == true null, undefined: added checks https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:122: if (typeof val == 'object') { On 2015/09/17 06:36:56, Dan Beam wrote: > null? Done.
https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (left): https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:52: function ListPrefWrapper(prefObj) { On 2015/09/17 17:53:47, michaelpg wrote: > On 2015/09/17 06:36:56, Dan Beam wrote: > > Why wouldn't we just make a DictPrefWrapper? > > These wrappers don't make much sense to me so I'm not sure what a > DictPrefWrapper would look like. If anything I would get rid of the polymorphic > design entirely, or split it between FundamentalPrefWrapper and > ComplexPrefWrapper. > > A ListPref could have Dictionaries, and a DictPref could have Lists. that's not a problem > It would be > too much to have a ListPrefWrapper containing DictPrefWrappers containing > ListPrefWrappers. no, it wouldn't > > Having a single, generalized, recursive "equals" function is simpler IMO than > having ListEqual, DictionaryEqual, etc. and having them call each other > recursively. no, it's not as it has every type's equality method baked in plus a whole ton of if (i'm this type) forks. https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:93: return true; this is the ListPrefWrapper.equals implementation https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:96: if (typeof val1 == 'object') { On 2015/09/17 17:53:47, michaelpg wrote: > On 2015/09/17 06:36:56, Dan Beam wrote: > > What about {}, null? > > {}: "just works", valuesEquivalent({}, {}) == true > null, undefined: added checks no, I meant val1 == {}, val2 == null https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:105: return true; this is the DictPrefWrapper.equals implementation
michaelpg@chromium.org changed reviewers: + stevenjb@chromium.org
michaelpg@chromium.org changed required reviewers: + stevenjb@chromium.org
+stevenjb cuz dan is busy Here's a new tack: Get rid of PrefWrappers. * We don't need to "wrap" more than the pref's value, because we won't be changing other properties of the pref. * We only need special Copy and Equals functions for Objects and Arrays. These functions can call each other recursively without having to create new PrefWrappers. https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (left): https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:52: function ListPrefWrapper(prefObj) { On 2015/09/17 18:13:31, Dan Beam wrote: > On 2015/09/17 17:53:47, michaelpg wrote: > > On 2015/09/17 06:36:56, Dan Beam wrote: > > > Why wouldn't we just make a DictPrefWrapper? > > > > These wrappers don't make much sense to me so I'm not sure what a > > DictPrefWrapper would look like. If anything I would get rid of the > polymorphic > > design entirely, or split it between FundamentalPrefWrapper and > > ComplexPrefWrapper. > > > > A ListPref could have Dictionaries, and a DictPref could have Lists. > > that's not a problem > > > It would be > > too much to have a ListPrefWrapper containing DictPrefWrappers containing > > ListPrefWrappers. > > no, it wouldn't > > > > > Having a single, generalized, recursive "equals" function is simpler IMO than > > having ListEqual, DictionaryEqual, etc. and having them call each other > > recursively. > > no, it's not as it has every type's equality method baked in plus a whole ton of > if (i'm this type) forks. I'm happy to separate out the equality methods, but I maintain that having a class for each type (essentially, re-creating the JavaScript type system but without considering prototypes) is, if not infeasible, an excessive amount of work and introduces redundant time/space usage. Converting an arrays of objects of arrays to a series of ListValues and DictionaryValues means creating a ton of pointless Objects just so we can polymorphically test them. https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:80: if (val1 == val2) On 2015/09/17 17:53:47, michaelpg wrote: > On 2015/09/17 06:36:56, Dan Beam wrote: > > Eh probably don't do this without type checking first > > Acknowledged. Actually, in my new version I think checking "===" (strict equality, not loose equality) simplifies the branching and should make the function cheaper on average. https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:96: if (typeof val1 == 'object') { On 2015/09/17 18:13:31, Dan Beam wrote: > On 2015/09/17 17:53:47, michaelpg wrote: > > On 2015/09/17 06:36:56, Dan Beam wrote: > > > What about {}, null? > > > > {}: "just works", valuesEquivalent({}, {}) == true > > null, undefined: added checks > > no, I meant val1 == {}, val2 == null New version handles this case. https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:105: return true; On 2015/09/17 18:13:31, Dan Beam wrote: > this is the DictPrefWrapper.equals implementation the problem is I don't want a ListPrefWrapper to be composed of DictPrefWrappers, because a DictPrefWrapper should wrap a pref, not just a value.
https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1333473002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:80: if (val1 == val2) On 2015/09/17 22:54:45, michaelpg wrote: > On 2015/09/17 17:53:47, michaelpg wrote: > > On 2015/09/17 06:36:56, Dan Beam wrote: > > > Eh probably don't do this without type checking first > > > > Acknowledged. > > Actually, in my new version I think checking "===" (strict equality, not loose > equality) simplifies the branching and should make the function cheaper on > average. === is probably OK, yeah
https://codereview.chromium.org/1333473002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1333473002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:35: return true; needs moar \n https://codereview.chromium.org/1333473002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:71: var keys2 = Object.keys(obj2); do we care about non-own keys? https://codereview.chromium.org/1333473002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:95: return val; nit: if (!(val instanceof Object)) return val; return Array.isArray(val) ? deepCopyArray(val) : deepCopyObject(val);
https://codereview.chromium.org/1333473002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1333473002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:35: return true; On 2015/09/17 23:12:01, Dan Beam wrote: > needs moar \n Done. https://codereview.chromium.org/1333473002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:71: var keys2 = Object.keys(obj2); On 2015/09/17 23:12:01, Dan Beam wrote: > do we care about non-own keys? No, everything coming from the pref store should be POD, as described in the deepEqual header. https://codereview.chromium.org/1333473002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:95: return val; On 2015/09/17 23:12:01, Dan Beam wrote: > nit: > > if (!(val instanceof Object)) > return val; > > return Array.isArray(val) ? deepCopyArray(val) : deepCopyObject(val); Done.
lgtm https://codereview.chromium.org/1333473002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1333473002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:37: if (val1 == null || val2 == null) you should probably use === just to make sure
Patchset #11 (id:200001) has been deleted
https://codereview.chromium.org/1333473002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1333473002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:37: if (val1 == null || val2 == null) On 2015/09/18 01:54:16, Dan Beam wrote: > you should probably use === just to make sure actually this line is superfluous because null and undefined are neither Arrays nor Objects
https://codereview.chromium.org/1333473002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1333473002/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:37: if (val1 == null || val2 == null) On 2015/09/18 02:03:22, michaelpg wrote: > On 2015/09/18 01:54:16, Dan Beam wrote: > > you should probably use === just to make sure > > actually this line is superfluous because null and undefined are neither Arrays > nor Objects super
I personally find this easier to follow than the Wrapper solution, so LGTM
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/1333473002/#ps220001 (title: "remove redundant check")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1333473002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1333473002/220001
Message was sent while issue was closed.
Committed patchset #11 (id:220001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/6d665fd58254221164550766b8f62e913decc4a8 Cr-Commit-Position: refs/heads/master@{#349604} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
