Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1019403002:
Fetch and actually set prefs (using chrome.send for now). (Closed)
|
Created:
5 years, 9 months ago by Jeremy Klein Modified:
5 years, 9 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, 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, tbuckley_google.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFetch and actually set prefs (using chrome.send for now).
Required wiring up the options handler for now, but that can all go
away once settingsPrivate is ready.
BUG=
Committed: https://crrev.com/9ace848b1e29993c394e79404466f60c142c7bba
Cr-Commit-Position: refs/heads/master@{#321506}
Patch Set 1 #Patch Set 2 : Revert unintended change. #
Total comments: 4
Patch Set 3 : fix cros-only #
Total comments: 4
Patch Set 4 : unintended changes #
Total comments: 11
Patch Set 5 : a11y #Patch Set 6 : imports! #Patch Set 7 : Address comments from stevenjb #
Total comments: 27
Patch Set 8 : asserts -> assert #Patch Set 9 : src -> href source -> src. *facepalm* #
Total comments: 28
Patch Set 10 : Address comments from michaelpg #Patch Set 11 : dbeam's comments #Patch Set 12 : One more round of michael comments. #
Total comments: 7
Patch Set 13 : comments from orenb #Patch Set 14 : dan comments #
Total comments: 1
Messages
Total messages: 41 (9 generated)
jhawkins@chromium.org changed reviewers: + jhawkins@chromium.org
https://codereview.chromium.org/1019403002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/md_settings_ui.h (right): https://codereview.chromium.org/1019403002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_settings_ui.h:18: // Overridden from OptionsPageUIHandlerHost: // OptionsPageUIHandlerHost implementation. https://codereview.chromium.org/1019403002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/core_options_handler.cc (right): https://codereview.chromium.org/1019403002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/core_options_handler.cc:371: // NOTREACHED(); Ship it
jlklein@chromium.org changed reviewers: + khorimoto@chromium.org, michaelpg@chromium.org
Note that this doesn't add the cr-settings-* UI elements yet or the Preference object. It just uses chrome.send to fetch and set prefs. A lot of the structure of the logic in prefs.js will stay the same once settingsPrivate is done, but it will be safer and simpler :-). https://codereview.chromium.org/1019403002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/md_settings_ui.h (right): https://codereview.chromium.org/1019403002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_settings_ui.h:18: // Overridden from OptionsPageUIHandlerHost: On 2015/03/19 22:23:34, James Hawkins wrote: > // OptionsPageUIHandlerHost implementation. Done. https://codereview.chromium.org/1019403002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/core_options_handler.cc (right): https://codereview.chromium.org/1019403002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/core_options_handler.cc:371: // NOTREACHED(); On 2015/03/19 22:23:34, James Hawkins wrote: > Ship it Haha, removed. Just trying things out :-)
https://codereview.chromium.org/1019403002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.html (right): https://codereview.chromium.org/1019403002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.html:2: <script src="chrome://resources/js/assert.js"></script> Using a <script> instead of a <link rel="import"> will NOT dedupe the request, right? So, if we include the same script multiple times, bad stuff will happen? We should probably figure out a centralized way to do this instead of doing it on an individual element. https://codereview.chromium.org/1019403002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/md_settings_ui.cc (right): https://codereview.chromium.org/1019403002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_settings_ui.cc:40: nit: Remove extra newline.
stevenjb@chromium.org changed reviewers: + stevenjb@chromium.org
https://codereview.chromium.org/1019403002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1019403002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:57: * Accessibility preferences state. This isn't accessibility specific, is it? https://codereview.chromium.org/1019403002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:83: var prefsToFetch = [callbackName].concat(PREFS_TO_FECTH); Nit: This confused me, it requires knowing that this is actually a list of args to pass to fetchPrefs. Maybe do the concat after setting prefsToFetch, or rename the variable. https://codereview.chromium.org/1019403002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:121: } So, does this allow us to use '.' in the pref keys, allowing us to use the pref names directly without creating new constants? https://codereview.chromium.org/1019403002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/md_settings_ui.h (right): https://codereview.chromium.org/1019403002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_settings_ui.h:17: ~MdSettingsUI() override; WS https://codereview.chromium.org/1019403002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_settings_ui.h:23: options::CoreOptionsHandler* core_handler_; WS
https://codereview.chromium.org/1019403002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.html (right): https://codereview.chromium.org/1019403002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.html:2: <script src="chrome://resources/js/assert.js"></script> On 2015/03/19 22:42:11, Kyle Horimoto wrote: > Using a <script> instead of a <link rel="import"> will NOT dedupe the request, > right? So, if we include the same script multiple times, bad stuff will happen? > We should probably figure out a centralized way to do this instead of doing it > on an individual element. I'm glad you brought this up because I actually meant to ask about it! In these cases, it isn't actually a big deal to duplicate the scripts, but I do agree that it would be cool to take advantage of the deduping that imports give us. This will basically mean wrapping assert.js and cr.js in assert.html and cr.html with only a script tag. I'm actually cool with this and will go ahead and do it as long as there aren't any strong objections. There is precedent for this in files like this: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/polyme... I think it's a nice pattern to follow for dependency management. https://codereview.chromium.org/1019403002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/md_settings_ui.cc (right): https://codereview.chromium.org/1019403002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_settings_ui.cc:40: On 2015/03/19 22:42:11, Kyle Horimoto wrote: > nit: Remove extra newline. Done. https://codereview.chromium.org/1019403002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1019403002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:57: * Accessibility preferences state. On 2015/03/19 22:51:18, stevenjb wrote: > This isn't accessibility specific, is it? Ah good catch. Left over from old stuff :-). https://codereview.chromium.org/1019403002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:83: var prefsToFetch = [callbackName].concat(PREFS_TO_FECTH); On 2015/03/19 22:51:18, stevenjb wrote: > Nit: This confused me, it requires knowing that this is actually a list of args > to pass to fetchPrefs. Maybe do the concat after setting prefsToFetch, or rename > the variable. Done. https://codereview.chromium.org/1019403002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:121: } On 2015/03/19 22:51:18, stevenjb wrote: > So, does this allow us to use '.' in the pref keys, allowing us to use the pref > names directly without creating new constants? Yep :-). We're constructing the real object here dynamically. https://codereview.chromium.org/1019403002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/md_settings_ui.h (right): https://codereview.chromium.org/1019403002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_settings_ui.h:17: ~MdSettingsUI() override; On 2015/03/19 22:51:18, stevenjb wrote: > WS Done. https://codereview.chromium.org/1019403002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/md_settings_ui.h:23: options::CoreOptionsHandler* core_handler_; On 2015/03/19 22:51:18, stevenjb wrote: > WS Done.
Nice. LGTM. https://codereview.chromium.org/1019403002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1019403002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/prefs/prefs.js:121: } On 2015/03/19 23:04:22, Jeremy Klein wrote: > On 2015/03/19 22:51:18, stevenjb wrote: > > So, does this allow us to use '.' in the pref keys, allowing us to use the > pref > > names directly without creating new constants? > > Yep :-). We're constructing the real object here dynamically. Nice. That makes exporting the whitelisted properties in settingsPrivate even less interesting.
https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:43: 'settings.a11y.large_cursor_enabled', nit: alphabetize. https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:57: * Object containing all prefereces. preferences https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:59: * @attribute settings existential: this element is called prefs, but everything inside it is called settings. what are we really? why are we here? https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:71: observer.open(this.propertyChangeCallback_.bind(this, 'settings')); Why is this here? propertyChangeCallback_ says it is "Called when a property of a pref changes". https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:108: let value = dict[prefName]; "value" confused me because later it represents a pref, not a pref value. How about something more generic, like "obj" or "dict"? https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:139: rawPref.hasOwnProperty('disabled'); so this assumes we don't have settings called "foo.bar.value" and "foo.bar.disabled", right? https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:161: switch (typeof newValue) { is a type of undefined possible? https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:183: * @param {string} propertyPath The path before the property names. From the perspective of propertyChangeCallback_ this comment makes sense, but in general shouldn't this be the full path of the setting? https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:207: chrome.send(setFn, [propertyPath, value]); I suspect this will blow up if you try to call setIntegerPref for a preference registered as a double. Have you tested it for that case? https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:212: * @param {number} value The new value of the pref. Array https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_settings_ui.cc (right): https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_settings_ui.cc:28: #if defined(OS_CHROMEOS) nit: don't indent #directives
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:27: * @const {!Array<string>} nit: you don't really need types if it's a literal declaration (pretty sure this is just redundant and the compiler already knows) https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:84: if (cr.isChromeOS) why not just do this via #if defined(OS_CHROMEOS) in the C++? https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:118: for (let i in pathPieces) { this is not guaranteed to be in order https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:206: var setFn = (value % 1 == 0) ? 'setIntegerPref' : 'setDoublePref'; () not needed, also did this already exist? because you can certainly set a double pref with an integer... https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:216: chrome.send('setListPref', [propertyPath, JSON.stringify(value)]); why are you stringifying? that already happens when passed over IPC https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_settings_ui.cc (right): https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_settings_ui.cc:28: #if defined(OS_CHROMEOS) 0-indent #ifs, e.g. // TODO(jlklein): #if defined(OS_CHROMEOS) core_handler_ = ...; #else core_handler_ = ...; #endif https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_settings_ui.cc:36: DCHECK(handler.get()); what are you DCHECK()ing? https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_settings_ui.cc:37: if (handler->IsEnabled()) { no curlies https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_settings_ui.cc:68: void MdSettingsUI::OnFinishedLoading() {} remove or explain why you're changing the behavior https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_settings_ui.h (right): https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_settings_ui.h:19: // OptionsPageUIHandlerHost implementation. just // OptionsPageUIHandlerHost:
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:206: var setFn = (value % 1 == 0) ? 'setIntegerPref' : 'setDoublePref'; On 2015/03/19 23:47:09, Dan Beam wrote: > () not needed, also did this already exist? because you can certainly set a > double pref with an integer... It looked to me like calling setIntegerPref for a preference registered as a double would fail in debug builds: https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/pref_se... This would happen if setNumberPref_ is called for a pref registered as type double with a value that happens to be a round number.
https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:43: 'settings.a11y.large_cursor_enabled', On 2015/03/19 23:39:07, michaelpg wrote: > nit: alphabetize. Done. https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:57: * Object containing all prefereces. On 2015/03/19 23:39:07, michaelpg wrote: > preferences Done. https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:59: * @attribute settings On 2015/03/19 23:39:07, michaelpg wrote: > existential: this element is called prefs, but everything inside it is called > settings. what are we really? why are we here? *sigh*... Yeah, you're right. I could rename this to prefs, but that also seems confusing because the ui will have prefs.pref.foo.bar. data? model? Got any other suggestions for clearing this up? https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:71: observer.open(this.propertyChangeCallback_.bind(this, 'settings')); On 2015/03/19 23:39:07, michaelpg wrote: > Why is this here? propertyChangeCallback_ says it is "Called when a property of > a pref changes". Yeah this is left over. Removed. https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:108: let value = dict[prefName]; On 2015/03/19 23:39:08, michaelpg wrote: > "value" confused me because later it represents a pref, not a pref value. How > about something more generic, like "obj" or "dict"? Going with prefObj. https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:139: rawPref.hasOwnProperty('disabled'); On 2015/03/19 23:39:08, michaelpg wrote: > so this assumes we don't have settings called "foo.bar.value" and > "foo.bar.disabled", right? Yeah. When the settingsPrivate stuff is finalized, we'll have more details to look at here in the prefObject so there will be a much lower chance of false positives. https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:161: switch (typeof newValue) { On 2015/03/19 23:39:07, michaelpg wrote: > is a type of undefined possible? I don't think it's possible for a new value. Do you see any way that the UI could set something to undefined? https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:183: * @param {string} propertyPath The path before the property names. On 2015/03/19 23:39:07, michaelpg wrote: > From the perspective of propertyChangeCallback_ this comment makes sense, but in > general shouldn't this be the full path of the setting? Done. https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:207: chrome.send(setFn, [propertyPath, value]); On 2015/03/19 23:39:07, michaelpg wrote: > I suspect this will blow up if you try to call setIntegerPref for a preference > registered as a double. Have you tested it for that case? Hmm, good point. I haven't tried that. I'd rather not worry about this problem now because settingsPrivate will just have a "setNumericPref" anyway and we won't need to worry about it. +stevenjb@chromium.org and +orenb@chromium.org to confirm. https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:212: * @param {number} value The new value of the pref. On 2015/03/19 23:39:08, michaelpg wrote: > Array Done. https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_settings_ui.cc (right): https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_settings_ui.cc:28: #if defined(OS_CHROMEOS) On 2015/03/19 23:39:08, michaelpg wrote: > nit: don't indent #directives Done.
https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:27: * @const {!Array<string>} On 2015/03/19 23:47:09, Dan Beam wrote: > nit: you don't really need types if it's a literal declaration (pretty sure this > is just redundant and the compiler already knows) Interesting. I think I'd rather be explicit for now for consistency, but it's good to know that the compiler's type inference is at least good enough for this :-). https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:84: if (cr.isChromeOS) On 2015/03/19 23:47:09, Dan Beam wrote: > why not just do this via > > #if defined(OS_CHROMEOS) > > in the C++? We will have something like this in the full settingsPrivate API in getAllPrefs, but I didn't want to mess with the existing coreOptionsHandler since this stuff is temporary until settingsPrivate is done. https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:118: for (let i in pathPieces) { On 2015/03/19 23:47:09, Dan Beam wrote: > this is not guaranteed to be in order I thought it was if it's just for an array. If not, that's pretty weak... https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:206: var setFn = (value % 1 == 0) ? 'setIntegerPref' : 'setDoublePref'; On 2015/03/19 23:47:09, Dan Beam wrote: > () not needed, Done. >also did this already exist? because you can certainly set a > double pref with an integer... Well right now the html explicitly decides whether a pref is a double or integer pref: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... This won't matter with settingsPrivate since we'll just have one setNumericPref function. https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:216: chrome.send('setListPref', [propertyPath, JSON.stringify(value)]); On 2015/03/19 23:47:09, Dan Beam wrote: > why are you stringifying? that already happens when passed over IPC Got this from https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... Is it not needed there either? Removed here. https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_settings_ui.cc (right): https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_settings_ui.cc:28: #if defined(OS_CHROMEOS) On 2015/03/19 23:47:09, Dan Beam wrote: > 0-indent #ifs, e.g. > > // TODO(jlklein): > #if defined(OS_CHROMEOS) > core_handler_ = ...; > #else > core_handler_ = ...; > #endif Ah I see. Misunderstood Michael's comment here. Done. https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_settings_ui.cc:36: DCHECK(handler.get()); On 2015/03/19 23:47:09, Dan Beam wrote: > what are you DCHECK()ing? Caught me copy-pasta-ing. Looks like this isn't needed here. Got it from: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... Where it needed to verify the handler's existance/non-nullness. https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_settings_ui.cc:37: if (handler->IsEnabled()) { On 2015/03/19 23:47:09, Dan Beam wrote: > no curlies Done. https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_settings_ui.cc:68: void MdSettingsUI::OnFinishedLoading() {} On 2015/03/19 23:47:09, Dan Beam wrote: > remove or explain why you're changing the behavior Nothin' to see here, just me bein a C++ n00b thinking I needed to override this. Removed here and in the .h. https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_settings_ui.h (right): https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_settings_ui.h:19: // OptionsPageUIHandlerHost implementation. On 2015/03/19 23:47:09, Dan Beam wrote: > just > > // OptionsPageUIHandlerHost: Done.
lgtm
lgtm https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:59: * @attribute settings On 2015/03/20 00:03:15, Jeremy Klein wrote: > On 2015/03/19 23:39:07, michaelpg wrote: > > existential: this element is called prefs, but everything inside it is called > > settings. what are we really? why are we here? > > *sigh*... Yeah, you're right. I could rename this to prefs, but that also seems > confusing because the ui will have prefs.pref.foo.bar. data? model? > > Got any other suggestions for clearing this up? I just hoped we'd have better definitions because of how random the current settings page seems. But it doesn't help that we're also having to coexist with the existing settings. Let's not worry about it now. If it gets really confusing we can define better terms later. https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:108: let value = dict[prefName]; On 2015/03/20 00:03:15, Jeremy Klein wrote: > On 2015/03/19 23:39:08, michaelpg wrote: > > "value" confused me because later it represents a pref, not a pref value. How > > about something more generic, like "obj" or "dict"? > > Going with prefObj. Cool, I think this is much clearer. https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:161: switch (typeof newValue) { On 2015/03/20 00:03:14, Jeremy Klein wrote: > On 2015/03/19 23:39:07, michaelpg wrote: > > is a type of undefined possible? > > I don't think it's possible for a new value. Do you see any way that the UI > could set something to undefined? I'm sure it will happen eventually. I'm just worried it'd be hard to debug. Could we maybe have an assert or a console.error as a default case?
https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:59: * @attribute settings On 2015/03/20 00:52:54, michaelpg wrote: > On 2015/03/20 00:03:15, Jeremy Klein wrote: > > On 2015/03/19 23:39:07, michaelpg wrote: > > > existential: this element is called prefs, but everything inside it is > called > > > settings. what are we really? why are we here? > > > > *sigh*... Yeah, you're right. I could rename this to prefs, but that also > seems > > confusing because the ui will have prefs.pref.foo.bar. data? model? > > > > Got any other suggestions for clearing this up? > > I just hoped we'd have better definitions because of how random the current > settings page seems. But it doesn't help that we're also having to coexist with > the existing settings. > > Let's not worry about it now. If it gets really confusing we can define better > terms later. SGTM https://codereview.chromium.org/1019403002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:161: switch (typeof newValue) { On 2015/03/20 00:52:54, michaelpg wrote: > On 2015/03/20 00:03:14, Jeremy Klein wrote: > > On 2015/03/19 23:39:07, michaelpg wrote: > > > is a type of undefined possible? > > > > I don't think it's possible for a new value. Do you see any way that the UI > > could set something to undefined? > > I'm sure it will happen eventually. I'm just worried it'd be hard to debug. > Could we maybe have an assert or a console.error as a default case? Added assert.
lgtm
orenb@chromium.org changed reviewers: + orenb@chromium.org
https://codereview.chromium.org/1019403002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1019403002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:29: var PREFS_TO_FECTH = [ spelling: FETCH https://codereview.chromium.org/1019403002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:78: // *Sigh* We need to export the function name to global scope. In a comment, explain why (it's temporary b/c options handler will call the function of this name in the global scope, right?) https://codereview.chromium.org/1019403002/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_settings_ui.cc (right): https://codereview.chromium.org/1019403002/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_settings_ui.cc:60: DCHECK(!profile->IsOffTheRecord() || profile->IsGuestSession()); should this be !profile->IsGuestSession() ?
https://codereview.chromium.org/1019403002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1019403002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:29: var PREFS_TO_FECTH = [ On 2015/03/20 01:06:53, Oren Blasberg wrote: > spelling: FETCH Done. https://codereview.chromium.org/1019403002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:78: // *Sigh* We need to export the function name to global scope. On 2015/03/20 01:06:53, Oren Blasberg wrote: > In a comment, explain why (it's temporary b/c options handler will call the > function of this name in the global scope, right?) Done. https://codereview.chromium.org/1019403002/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_settings_ui.cc (right): https://codereview.chromium.org/1019403002/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_settings_ui.cc:60: DCHECK(!profile->IsOffTheRecord() || profile->IsGuestSession()); On 2015/03/20 01:06:53, Oren Blasberg wrote: > should this be !profile->IsGuestSession() ? Nah guest sessions are ok for settings.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:118: for (let i in pathPieces) { On 2015/03/20 00:45:38, Jeremy Klein wrote: > On 2015/03/19 23:47:09, Dan Beam wrote: > > this is not guaranteed to be in order > > I thought it was if it's just for an array. If not, that's pretty weak... it's pretty weak: http://stackoverflow.com/questions/280713/elements-order-in-a-for-in-loop just use for (;;) or forEach
https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:118: for (let i in pathPieces) { On 2015/03/20 01:11:56, Dan Beam wrote: > On 2015/03/20 00:45:38, Jeremy Klein wrote: > > On 2015/03/19 23:47:09, Dan Beam wrote: > > > this is not guaranteed to be in order > > > > I thought it was if it's just for an array. If not, that's pretty weak... > > it's pretty weak: > http://stackoverflow.com/questions/280713/elements-order-in-a-for-in-loop > > just use for (;;) or forEach Nah, seems OK: "In short: Use an array if order is important to you." pathPieces is an array.
https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:206: var setFn = (value % 1 == 0) ? 'setIntegerPref' : 'setDoublePref'; On 2015/03/20 00:45:38, Jeremy Klein wrote: > On 2015/03/19 23:47:09, Dan Beam wrote: > > () not needed, > Done. > > >also did this already exist? because you can certainly set a > > double pref with an integer... > > Well right now the html explicitly decides whether a pref is a double or integer > pref: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > This won't matter with settingsPrivate since we'll just have one setNumericPref > function. JS can only have/send doubles. it's up to the C++ to set the right type, probably based on the pref name. ultimately i hope this distinction dies or is less subtle.
https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:118: for (let i in pathPieces) { On 2015/03/20 01:13:18, Jeremy Klein wrote: > On 2015/03/20 01:11:56, Dan Beam wrote: > > On 2015/03/20 00:45:38, Jeremy Klein wrote: > > > On 2015/03/19 23:47:09, Dan Beam wrote: > > > > this is not guaranteed to be in order > > > > > > I thought it was if it's just for an array. If not, that's pretty weak... > > > > it's pretty weak: > > http://stackoverflow.com/questions/280713/elements-order-in-a-for-in-loop > > > > just use for (;;) or forEach > > Nah, seems OK: > > "In short: Use an array if order is important to you." > pathPieces is an array. here be dragons
https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:118: for (let i in pathPieces) { On 2015/03/20 01:13:18, Jeremy Klein wrote: > On 2015/03/20 01:11:56, Dan Beam wrote: > > On 2015/03/20 00:45:38, Jeremy Klein wrote: > > > On 2015/03/19 23:47:09, Dan Beam wrote: > > > > this is not guaranteed to be in order > > > > > > I thought it was if it's just for an array. If not, that's pretty weak... > > > > it's pretty weak: > > http://stackoverflow.com/questions/280713/elements-order-in-a-for-in-loop > > > > just use for (;;) or forEach > > Nah, seems OK: > > "In short: Use an array if order is important to you." > pathPieces is an array. Switched in case someone messes with Array.prototype. https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:206: var setFn = (value % 1 == 0) ? 'setIntegerPref' : 'setDoublePref'; On 2015/03/20 01:23:53, Dan Beam wrote: > On 2015/03/20 00:45:38, Jeremy Klein wrote: > > On 2015/03/19 23:47:09, Dan Beam wrote: > > > () not needed, > > Done. > > > > >also did this already exist? because you can certainly set a > > > double pref with an integer... > > > > Well right now the html explicitly decides whether a pref is a double or > integer > > pref: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > > > This won't matter with settingsPrivate since we'll just have one > setNumericPref > > function. > > JS can only have/send doubles. it's up to the C++ to set the right type, > probably based on the pref name. > > ultimately i hope this distinction dies or is less subtle. Yep, what I meant is that with the settingsPrivate API the distinction will go away as far as the JS is concerned for exactly the reason you mentioned. For now, we just have to deal with this :-/
The CQ bit was checked by jlklein@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, khorimoto@chromium.org, michaelpg@chromium.org, jhawkins@chromium.org Link to the patchset: https://codereview.chromium.org/1019403002/#ps260001 (title: "dan comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1019403002/260001
why no BUG=?
On 2015/03/20 01:29:47, Dan Beam wrote: > why no BUG=? +tbuckley@, were you still going to file and triage the bugs we came up with from that meeting?
https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1019403002/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:206: var setFn = (value % 1 == 0) ? 'setIntegerPref' : 'setDoublePref'; On 2015/03/20 01:23:53, Dan Beam wrote: > On 2015/03/20 00:45:38, Jeremy Klein wrote: > > On 2015/03/19 23:47:09, Dan Beam wrote: > > > () not needed, > > Done. > > > > >also did this already exist? because you can certainly set a > > > double pref with an integer... > > > > Well right now the html explicitly decides whether a pref is a double or > integer > > pref: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > > > This won't matter with settingsPrivate since we'll just have one > setNumericPref > > function. > > JS can only have/send doubles. it's up to the C++ to set the right type, > probably based on the pref name. > > ultimately i hope this distinction dies or is less subtle. The good news is we don't have any double prefs set in Settings. If we add one then this will just start hitting NOTREACHED(). (although I don't know what happens in release builds when a pref value has the wrong type) https://codereview.chromium.org/1019403002/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_settings_ui.cc (right): https://codereview.chromium.org/1019403002/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_settings_ui.cc:60: DCHECK(!profile->IsOffTheRecord() || profile->IsGuestSession()); On 2015/03/20 01:11:05, Jeremy Klein wrote: > On 2015/03/20 01:06:53, Oren Blasberg wrote: > > should this be !profile->IsGuestSession() ? > > Nah guest sessions are ok for settings. Yep. Incognito profiles are OK if you're in guest mode on Chrome OS, but on desktop incognito opening settings should redirect you to a on-the-record window. There were some changes around this recently.
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/9ace848b1e29993c394e79404466f60c142c7bba Cr-Commit-Position: refs/heads/master@{#321506}
Message was sent while issue was closed.
https://codereview.chromium.org/1019403002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/prefs/prefs.js (right): https://codereview.chromium.org/1019403002/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/prefs/prefs.js:107: for (let prefName in dict) { p.s. Chrome on iOS may not support `let` (arv@ reminded me)
Message was sent while issue was closed.
On 2015/03/20 19:34:05, Dan Beam wrote: > https://codereview.chromium.org/1019403002/diff/260001/chrome/browser/resourc... > File chrome/browser/resources/settings/prefs/prefs.js (right): > > https://codereview.chromium.org/1019403002/diff/260001/chrome/browser/resourc... > chrome/browser/resources/settings/prefs/prefs.js:107: for (let prefName in dict) > { > p.s. Chrome on iOS may not support `let` (arv@ reminded me) Well this page won't run on ios (for the time-being), but is there an easy place to look up chrome iOS support for sure? I can't seem to find iOS support on chromestatus.com, https://kangax.github.io/compat-table/es6/, caniuse, etc. It's easy enough to just be safe and use var, but it would be really nice to start using es6 stuff.
Message was sent while issue was closed.
On 2015/03/20 21:11:37, Jeremy Klein wrote: > On 2015/03/20 19:34:05, Dan Beam wrote: > > > https://codereview.chromium.org/1019403002/diff/260001/chrome/browser/resourc... > > File chrome/browser/resources/settings/prefs/prefs.js (right): > > > > > https://codereview.chromium.org/1019403002/diff/260001/chrome/browser/resourc... > > chrome/browser/resources/settings/prefs/prefs.js:107: for (let prefName in > dict) > > { > > p.s. Chrome on iOS may not support `let` (arv@ reminded me) > > Well this page won't run on ios (for the time-being), but is there an easy place > to look up chrome iOS support for sure? I can't seem to find iOS support on > http://chromestatus.com, https://kangax.github.io/compat-table/es6/, caniuse, etc. It's > easy enough to just be safe and use var, but it would be really nice to start > using es6 stuff. We really do want to take advantage of the fact that this code will only ever run on Chrome. I think it is 99% safe to also assume that Chrome Settings UI will never run on iOS due to their restrictions.
Message was sent while issue was closed.
On 2015/03/20 22:38:37, stevenjb wrote: > On 2015/03/20 21:11:37, Jeremy Klein wrote: > > On 2015/03/20 19:34:05, Dan Beam wrote: > > > > > > https://codereview.chromium.org/1019403002/diff/260001/chrome/browser/resourc... > > > File chrome/browser/resources/settings/prefs/prefs.js (right): > > > > > > > > > https://codereview.chromium.org/1019403002/diff/260001/chrome/browser/resourc... > > > chrome/browser/resources/settings/prefs/prefs.js:107: for (let prefName in > > dict) > > > { > > > p.s. Chrome on iOS may not support `let` (arv@ reminded me) > > > > Well this page won't run on ios (for the time-being), but is there an easy > place > > to look up chrome iOS support for sure? I can't seem to find iOS support on > > http://chromestatus.com, https://kangax.github.io/compat-table/es6/, caniuse, > etc. It's > > easy enough to just be safe and use var, but it would be really nice to start > > using es6 stuff. > > We really do want to take advantage of the fact that this code will only ever > run on Chrome. I think it is 99% safe to also assume that Chrome Settings UI > will never run on iOS due to their restrictions. chrome://history runs on iOS (pretty sure), and may eventually be redone in the same way as settings (and maybe using similar shared code) |
