Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2509163004:
[MD settings] content category policy (Closed)
|
Created:
4 years, 1 month ago by dschuyler Modified:
4 years ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] content category policy
This CL adds a controlled-by indicator to the top-level content settings
categories.
BUG=662596
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/bc70cf415de67da7837b1fd7a8cb6109bf102799
Cr-Commit-Position: refs/heads/master@{#440266}
Patch Set 1 : enum #
Total comments: 10
Patch Set 2 : disabled a browser test; review changes #Patch Set 3 : removed unfinished test changes #
Total comments: 14
Patch Set 4 : browser_tests; review changes #
Total comments: 13
Patch Set 5 : main toggle and sub-setting toogle show controlled by icon; review changes #
Total comments: 15
Patch Set 6 : review changes #Patch Set 7 : after pre-CLs #
Total comments: 20
Patch Set 8 : nits #
Messages
Total messages: 80 (57 generated)
Description was changed from ========== [MD settings] content category policy This CL adds a controlled-by indicator to the top-level content settings categories. BUG=662596 ========== to ========== [MD settings] content category policy This CL adds a controlled-by indicator to the top-level content settings categories. BUG=662596 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dschuyler@chromium.org changed reviewers: + dbeam@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== [MD settings] content category policy This CL adds a controlled-by indicator to the top-level content settings categories. BUG=662596 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] content category policy This CL adds a controlled-by indicator to the top-level content settings categories. BUG=662596 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dbeam@chromium.org changed reviewers: + tommycli@chromium.org - dbeam@chromium.org
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
+tommycli@
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
https://codereview.chromium.org/2509163004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_category.js (right): https://codereview.chromium.org/2509163004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.js:28: }, does this need an initial value? https://codereview.chromium.org/2509163004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.js:168: pref.controlledBy = chrome.settingsPrivate.ControlledBy.USER_POLICY; nit: ternary, maybe? pref.controlledBy = category.source == PolicySource.EXTENSION ? chrome.settingsPrivate.ControlledBy.EXTENSION : chrome.settingsPrivate.ControlledBy.USER_POLICY; https://codereview.chromium.org/2509163004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.js:188: var sliderSetting = setting; do we still need sliderSetting? https://codereview.chromium.org/2509163004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2509163004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:400: std::unique_ptr<base::DictionaryValue> category(new base::DictionaryValue); nit: can we just do stack construction instead? i.e. base::DictionaryValue category; site_settings::GetContentCategorySetting(map, content_type, &category); ResolveJavascriptCallback(*callback_id, category);
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2509163004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2509163004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:402: ResolveJavascriptCallback(*callback_id, *category.get()); I think you will have to update the unit test here: https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/site_se... To validate against the new DictionaryValue instead of the StringValue for the 3rd argument. (The second argument is created by the body of ResolveJavascriptCallback, so *category.get() here will become the 3rd argument)
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
This CL has a set of site settings browser tests disabled. I'd like to spend more time thinking on what these tests should be doing. https://codereview.chromium.org/2509163004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_category.js (right): https://codereview.chromium.org/2509163004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.js:28: }, On 2016/11/17 22:33:38, Dan Beam wrote: > does this need an initial value? If we want to default to showing a controlled by policy then it would need an initial value to say that. The current value (empty) will init to not showing the policy icon. The user will not be able to change the value (the policy is enforced) but they will not see the icon until the value is updated. I think there's a chance that if we default to showing the policy icon, it may flash/flicker on a slow machine. Since the security of the setting is still enforced, it's a split second UI issue. I could also explicitly set it to a non-controlled value. That may be more clear. https://codereview.chromium.org/2509163004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.js:168: pref.controlledBy = chrome.settingsPrivate.ControlledBy.USER_POLICY; On 2016/11/17 22:33:38, Dan Beam wrote: > nit: ternary, maybe? > > pref.controlledBy = category.source == PolicySource.EXTENSION ? > chrome.settingsPrivate.ControlledBy.EXTENSION : > chrome.settingsPrivate.ControlledBy.USER_POLICY; Done. https://codereview.chromium.org/2509163004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.js:188: var sliderSetting = setting; On 2016/11/17 22:33:38, Dan Beam wrote: > do we still need sliderSetting? I think it's used for the "Blocked" vs "<descriptive string>". https://codereview.chromium.org/2509163004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/site_settings_handler.cc (right): https://codereview.chromium.org/2509163004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:400: std::unique_ptr<base::DictionaryValue> category(new base::DictionaryValue); On 2016/11/17 22:33:38, Dan Beam wrote: > nit: can we just do stack construction instead? i.e. > > base::DictionaryValue category; > site_settings::GetContentCategorySetting(map, content_type, &category); > ResolveJavascriptCallback(*callback_id, category); Done. https://codereview.chromium.org/2509163004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler.cc:402: ResolveJavascriptCallback(*callback_id, *category.get()); On 2016/11/18 00:09:26, tommycli wrote: > I think you will have to update the unit test here: > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/site_se... > > To validate against the new DictionaryValue instead of the StringValue for the > 3rd argument. (The second argument is created by the body of > ResolveJavascriptCallback, so *category.get() here will become the 3rd argument) Thanks Tommy. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2509163004/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_category.js (right): https://codereview.chromium.org/2509163004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.js:95: onChangePermissionControl_: function(event) { can we just remove |event| here? https://codereview.chromium.org/2509163004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.js:170: chrome.settingsPrivate.ControlledBy.USER_POLICY; this needs an extra 2 \s https://codereview.chromium.org/2509163004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.js:172: // The fakePref_ must be replaced (rather than just value changs) so that changs -> changes https://codereview.chromium.org/2509163004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.js:172: // The fakePref_ must be replaced (rather than just value changs) so that note: this.set('fakePref_.enforcement', chrome.settingsPrivate.Enforcement.ENFORCED); accomplishes what you want because it does: this.fakePrefs_.enforcement = ...; this.notifyPath('fakePrefs_.enforcement'); combined under the covers anyways, what you have is fine, and it's more like the typical prefs mechanism (AFAIK) https://codereview.chromium.org/2509163004/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/site_settings_helper.cc (right): https://codereview.chromium.org/2509163004/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/site_settings_helper.cc:257: object->SetString(site_settings::kSource, provider); can we just set this only when provider != default?
https://codereview.chromium.org/2509163004/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/cr_settings_browsertest.js (right): https://codereview.chromium.org/2509163004/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/cr_settings_browsertest.js:671: // site_settings_category.registerTests(); why do we need to disable these again?
https://codereview.chromium.org/2509163004/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/settings/cr_settings_browsertest.js (right): https://codereview.chromium.org/2509163004/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/settings/cr_settings_browsertest.js:671: // site_settings_category.registerTests(); On 2016/11/22 00:46:13, Dan Beam wrote: > why do we need to disable these again? They were waiting for me to workout how to make them work again. There's no single catching point, just a series of small, 'what is this doing' things to figure out. I'm past that and I'm getting the code ready to put up another patch.
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Patchset #4 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2509163004/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_settings_category.js (right): https://codereview.chromium.org/2509163004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.js:95: onChangePermissionControl_: function(event) { On 2016/11/21 21:48:46, Dan Beam wrote: > can we just remove |event| here? Done. https://codereview.chromium.org/2509163004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.js:170: chrome.settingsPrivate.ControlledBy.USER_POLICY; On 2016/11/21 21:48:46, Dan Beam wrote: > this needs an extra 2 \s Done. https://codereview.chromium.org/2509163004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.js:172: // The fakePref_ must be replaced (rather than just value changs) so that On 2016/11/21 21:48:46, Dan Beam wrote: > changs -> changes Done. https://codereview.chromium.org/2509163004/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_settings_category.js:172: // The fakePref_ must be replaced (rather than just value changs) so that On 2016/11/21 21:48:46, Dan Beam wrote: > note: > > this.set('fakePref_.enforcement', > chrome.settingsPrivate.Enforcement.ENFORCED); > > accomplishes what you want because it does: > > this.fakePrefs_.enforcement = ...; > this.notifyPath('fakePrefs_.enforcement'); > > combined under the covers > > anyways, what you have is fine, and it's more like the typical prefs mechanism > (AFAIK) Acknowledged. https://codereview.chromium.org/2509163004/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/site_settings_helper.cc (right): https://codereview.chromium.org/2509163004/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/site_settings_helper.cc:257: object->SetString(site_settings::kSource, provider); On 2016/11/21 21:48:46, Dan Beam wrote: > can we just set this only when provider != default? Is that to reduce the size of the IPC call? There are other uses of the provider source in this api - I'd l ike to change them all together (or leave them as is). May I make that change in a separate CL (and change each call to follow the same pattern)? https://codereview.chromium.org/2509163004/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/settings/site_settings_category_tests.js (left): https://codereview.chromium.org/2509163004/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/site_settings_category_tests.js:203: }).then(function(arguments) { I changed from arguments because that shadows a javascript built in value.
https://codereview.chromium.org/2509163004/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/site_settings_helper.cc (right): https://codereview.chromium.org/2509163004/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/site_settings_helper.cc:257: object->SetString(site_settings::kSource, provider); On 2016/11/22 02:28:50, dschuyler wrote: > On 2016/11/21 21:48:46, Dan Beam wrote: > > can we just set this only when provider != default? > > Is that to reduce the size of the IPC call? no > There are other > uses of the provider source in this api can you give me an example? this looks like new code to me... > - I'd l ike to > change them all together (or leave them as is). what does "them all" mean? > May I make > that change in a separate CL (and change each call to follow > the same pattern)? https://codereview.chromium.org/2509163004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_category.html (right): https://codereview.chromium.org/2509163004/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.html:31: <settings-toggle-button id="toggle" pref="{{fakePref_}}"> why is this becoming a settings-toggle but others still use paper-toggle-button? https://codereview.chromium.org/2509163004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_category.js (right): https://codereview.chromium.org/2509163004/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:16: /** @private */ can this be @private {settings_private.PrefObject} or whatever this type is? https://codereview.chromium.org/2509163004/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:89: if (!this.category) why can't we just add category to the observer? as in: observers: [ ... 'onChangePermissionControl_(category, fakePref_.value)', ], so we don't have to filter out !this.category https://codereview.chromium.org/2509163004/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:179: // Early out if there is no actual change. can we do this inside of setFakePref_ instead? https://codereview.chromium.org/2509163004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2509163004/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:15: var PolicySource = { this name doesn't make much sense. policy is a specific thing, and extensions nor preferences are a source of policy. policy does *set* preferences often, and it also can *install* an extension. how about ContentSettingSource? or ContentSettingProvider? https://codereview.chromium.org/2509163004/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:42: var ContentCategorySetting; can this be like ContentTypeDefault or ContentSettingDefault? you're calling getDefaultValueForContentType to get these
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Patchset #5 (id:140001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2509163004/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/site_settings_helper.cc (right): https://codereview.chromium.org/2509163004/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/site_settings_helper.cc:257: object->SetString(site_settings::kSource, provider); On 2016/11/22 02:57:40, Dan Beam wrote: > On 2016/11/22 02:28:50, dschuyler wrote: > > On 2016/11/21 21:48:46, Dan Beam wrote: > > > can we just set this only when provider != default? > > > > Is that to reduce the size of the IPC call? > > no > > > There are other > > uses of the provider source in this api > > can you give me an example? this looks like new code to me... > > > - I'd l ike to > > change them all together (or leave them as is). > > what does "them all" mean? > > > May I make > > that change in a separate CL (and change each call to follow > > the same pattern)? Done. https://codereview.chromium.org/2509163004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_category.html (right): https://codereview.chromium.org/2509163004/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.html:31: <settings-toggle-button id="toggle" pref="{{fakePref_}}"> On 2016/11/22 02:57:40, Dan Beam wrote: > why is this becoming a settings-toggle but others still use paper-toggle-button? Ah, a user won't necessary know that the controlled by is implied and it should be explicit. I've changed the sub-settings of the tri-state to have controlled by indicators as well. https://codereview.chromium.org/2509163004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_category.js (right): https://codereview.chromium.org/2509163004/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:16: /** @private */ On 2016/11/22 02:57:40, Dan Beam wrote: > can this be @private {settings_private.PrefObject} or whatever this type is? Done. https://codereview.chromium.org/2509163004/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:89: if (!this.category) On 2016/11/22 02:57:40, Dan Beam wrote: > why can't we just add category to the observer? > > as in: > > observers: [ > ... > 'onChangePermissionControl_(category, fakePref_.value)', > ], > > so we don't have to filter out !this.category Nice, that allows for removing the on-change in the toggle as well. Done. https://codereview.chromium.org/2509163004/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:179: // Early out if there is no actual change. On 2016/11/22 02:57:40, Dan Beam wrote: > can we do this inside of setFakePref_ instead? Done. https://codereview.chromium.org/2509163004/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2509163004/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:15: var PolicySource = { On 2016/11/22 02:57:41, Dan Beam wrote: > this name doesn't make much sense. policy is a specific thing, and extensions > nor preferences are a source of policy. policy does *set* preferences often, > and it also can *install* an extension. > > how about ContentSettingSource? or ContentSettingProvider? Done. https://codereview.chromium.org/2509163004/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:42: var ContentCategorySetting; On 2016/11/22 02:57:41, Dan Beam wrote: > can this be like ContentTypeDefault or ContentSettingDefault? you're calling > getDefaultValueForContentType to get these Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
can you just add a getter like this?
get categoryEnabled() {
return assert(this.fakePref_).value;
},
or something so you don't have to churn the code so much?
https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_settings_category.js
(left):
https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_settings_category.js:55: },
ugh, why were specific options in the generic class? that doesn't make any
sense to me.
https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_settings_category.js
(right):
https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_settings_category.js:34: },
why is this needed?
https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_settings_category.js:37:
currentSetting_: {
can this be default_ or like categoryDefault_?
https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_settings_category.js:168:
Object.assign({'value': prefValue}, basePref));
why are you using Object.assign() rather than just
basePref.value = true;
...
this.fakePref_ = basePref;
https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/resourc...
File
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js
(right):
https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:16:
DEFAULT: 'default',
where is this ^ ever used?
https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc (right):
https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:61: const
base::DictionaryValue* default_value = NULL;
use nullptr instead of NULL in new code
https://codereview.chromium.org/2509163004/diff/160001/chrome/test/data/webui...
File chrome/test/data/webui/settings/site_settings_category_tests.js (left):
https://codereview.chromium.org/2509163004/diff/160001/chrome/test/data/webui...
chrome/test/data/webui/settings/site_settings_category_tests.js:190:
}).then(function(arguments) {
omg this is a horrible idea
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_category.js (left): https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:55: }, On 2016/11/29 05:17:14, Dan Beam wrote: > ugh, why were specific options in the generic class? that doesn't make any > sense to me. Not sure. https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_category.js (right): https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:34: }, On 2016/11/29 05:17:14, Dan Beam wrote: > why is this needed? This is a more generic way of handling the flashAskFirst_ and cookiesSessionOnly_ member variables. Having a sub-pref allows any category to have a tri-state value without adding a special variable for each one and hand-working it into the code. At the moment, Flash and Cookies are the only two with tri-states. I would not be surprised if one or two others became tri-states and this sub-pref will make supporting them easier. https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:37: currentSetting_: { On 2016/11/29 05:17:14, Dan Beam wrote: > can this be default_ or like categoryDefault_? How about priorDefaultContentSetting_? https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:168: Object.assign({'value': prefValue}, basePref)); On 2016/11/29 05:17:14, Dan Beam wrote: > why are you using Object.assign() rather than just > > basePref.value = true; > ... > this.fakePref_ = basePref; basePref is used twice, once here and again on line 184. If I set value directly it will (I believe) become the single instance of value that will be seen by fakePref and fakeSubPref. I'm looking to have two separate instances of value with a separate pref referencing each value. https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js (right): https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_prefs_browser_proxy.js:16: DEFAULT: 'default', On 2016/11/29 05:17:14, Dan Beam wrote: > where is this ^ ever used? It's a holdover from a prior patch in this CL, before we omitted sending the value from C++ it it were 'default'. I've made a note in the comment rather than defining the value. https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc (right): https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:61: const base::DictionaryValue* default_value = NULL; On 2016/11/29 05:17:14, Dan Beam wrote: > use nullptr instead of NULL in new code Done. https://codereview.chromium.org/2509163004/diff/160001/chrome/test/data/webui... File chrome/test/data/webui/settings/site_settings_category_tests.js (left): https://codereview.chromium.org/2509163004/diff/160001/chrome/test/data/webui... chrome/test/data/webui/settings/site_settings_category_tests.js:190: }).then(function(arguments) { On 2016/11/29 05:17:14, Dan Beam wrote: > omg this is a horrible idea They didn't know. Detecting this could be a handy addition to the presubmit checks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/site_settings_category.js (left): https://codereview.chromium.org/2509163004/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/site_settings_category.js:55: }, On 2016/11/29 22:28:12, dschuyler wrote: > On 2016/11/29 05:17:14, Dan Beam wrote: > > ugh, why were specific options in the generic class? that doesn't make any > > sense to me. > > Not sure. you reviewed one of these https://codereview.chromium.org/2316633002/diff/20001/chrome/browser/resource... tommycli@ added the other https://codereview.chromium.org/2280233002/diff/60001/chrome/browser/resource... can't we just do something like <content select=".extra-options"> or something?
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Patchset #7 (id:200001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This CL brought up a series of refactors that were addressed in CLs 2559813002, 2552883009, 2561723002, 2563893002, 2576613002, 2580653003, 2554403005. The remaining changes are much smaller.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/category_default_setting.js (right): https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/category_default_setting.js:25: priorDefaultContentSetting_: { nit: if this is not bound to anything in the HTML, this can just be a private member that's not a property. https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/category_default_setting.js:63: get categoryEnabled() { nit: Is this used outside of onChangePremissionControl_? If not, can we just assign it to a var within that scope? https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/category_default_setting.js:103: break; I see you added this clause (and below) in PS5. When can this happen? https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/category_default_setting.js:158: Object.assign({'value': prefValue}, basePref)); Here and below, does using https://www.polymer-project.org/1.0/docs/devguide/model-data#set-path work? or notifyPath if that doesn't work...
https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/category_default_setting.js (right): https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/category_default_setting.js:25: priorDefaultContentSetting_: { On 2016/12/20 22:07:53, tommycli wrote: > nit: if this is not bound to anything in the HTML, this can just be a private > member that's not a property. It could. I thought there was a discussion in a team meeting to prefer properties in place of private members. I looked to see if this got noted in the style guide and haven't found it. It sounds like it may be up to preference. https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/category_default_setting.js:63: get categoryEnabled() { On 2016/12/20 22:07:53, tommycli wrote: > nit: Is this used outside of onChangePremissionControl_? If not, can we just > assign it to a var within that scope? It's used in testing. Another option would be to use this.controlParams_.value directly. I think Dan suggested using an accessor in a previous patch within this CL. Either way is ok with me. https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/category_default_setting.js:103: break; On 2016/12/20 22:07:53, tommycli wrote: > I see you added this clause (and below) in PS5. When can this happen? It's happens during startup. It's possible to rearrange the code to remove these checks, but that results in less efficient code (i.e. more time to startup the page). The current code has an |if| in cookies and plugins which are tested rarely. An alternative involves updating the subControlParams_ before the controlParams_ which simply changes which values are undefined. https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/category_default_setting.js:158: Object.assign({'value': prefValue}, basePref)); On 2016/12/20 22:07:53, tommycli wrote: > Here and below, does using > https://www.polymer-project.org/1.0/docs/devguide/model-data#set-path work? or > notifyPath if that doesn't work... It would work for one element at a time. I could replace this with a series of set() or notifyPath calls (one for each member - or at least .enforcement, .controlledBy, and .value). That seems like a big hammer though. Do you think it would read clearer as .set calls (and is it just preference or is there a more meaningful difference)? Here's another link to those docs which highlights the approach used here: https://www.polymer-project.org/1.0/docs/devguide/model-data#override-dirty-c...
LGTM though i still recommend adding one comment (in two places) as described below. https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/category_default_setting.js (right): https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/category_default_setting.js:25: priorDefaultContentSetting_: { On 2016/12/20 23:22:41, dschuyler wrote: > On 2016/12/20 22:07:53, tommycli wrote: > > nit: if this is not bound to anything in the HTML, this can just be a private > > member that's not a property. > > It could. I thought there was a discussion in a team meeting to prefer > properties in place of private members. I looked to see if this got noted in the > style guide and haven't found it. It sounds like it may be up to preference. Aite. I'm okay with it as-is depending on personal preference, if you do prefer it this way. https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/category_default_setting.js:63: get categoryEnabled() { On 2016/12/20 23:22:41, dschuyler wrote: > On 2016/12/20 22:07:53, tommycli wrote: > > nit: Is this used outside of onChangePremissionControl_? If not, can we just > > assign it to a var within that scope? > > It's used in testing. Another option would be to use this.controlParams_.value > directly. I think Dan suggested using an accessor in a previous patch within > this CL. Either way is ok with me. Ack. I'm okay with deferring refactoring this if it's needed to support a test. https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/category_default_setting.js:103: break; On 2016/12/20 23:22:41, dschuyler wrote: > On 2016/12/20 22:07:53, tommycli wrote: > > I see you added this clause (and below) in PS5. When can this happen? > > It's happens during startup. It's possible to rearrange the code to remove these > checks, but that results in less efficient code (i.e. more time to startup the > page). The current code has an |if| in cookies and plugins which are tested > rarely. An alternative involves updating the subControlParams_ before the > controlParams_ which simply changes which values are undefined. Sounds good. In that case, I suggest adding a comment to explain why, since it wasn't obvious to me. https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/category_default_setting.js:158: Object.assign({'value': prefValue}, basePref)); On 2016/12/20 23:22:41, dschuyler wrote: > On 2016/12/20 22:07:53, tommycli wrote: > > Here and below, does using > > https://www.polymer-project.org/1.0/docs/devguide/model-data#set-path work? or > > notifyPath if that doesn't work... > > It would work for one element at a time. I could replace this with a series of > set() or notifyPath calls (one for each member - or at least .enforcement, > .controlledBy, and .value). That seems like a big hammer though. Do you think it > would read clearer as .set calls (and is it just preference or is there a more > meaningful difference)? > > Here's another link to those docs which highlights the approach used here: > https://www.polymer-project.org/1.0/docs/devguide/model-data#override-dirty-c... Thanks for the explanation. Looks good as-is then.
lgtm https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/category_default_setting.js (right): https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/category_default_setting.js:103: break; On 2016/12/20 23:26:42, tommycli wrote: > On 2016/12/20 23:22:41, dschuyler wrote: > > On 2016/12/20 22:07:53, tommycli wrote: > > > I see you added this clause (and below) in PS5. When can this happen? > > > > It's happens during startup. It's possible to rearrange the code to remove > these > > checks, but that results in less efficient code (i.e. more time to startup the > > page). The current code has an |if| in cookies and plugins which are tested > > rarely. An alternative involves updating the subControlParams_ before the > > controlParams_ which simply changes which values are undefined. > > Sounds good. In that case, I suggest adding a comment to explain why, since it > wasn't obvious to me. dschuyler@: are you saying that the less efficient way would be: observers: [ 'onChangePermissionsControl_(category, controlParams_.value, subControlParams_.value)', ], instead of 2 separate observers? https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/category_default_setting.js:137: this.priorDefaultContentSetting_.source == update.source) curlies https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/category_default_setting.js:143: 'type': 'BOOLEAN', nit: can you use chrome.settingsPrivate.PrefType.BOOLEAN https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/category_default_setting.js:144: }; nit: this probably fits on only 1 line
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/site_settings/category_default_setting.js (right): https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/category_default_setting.js:103: break; On 2016/12/21 03:06:27, Dan Beam wrote: > On 2016/12/20 23:26:42, tommycli wrote: > > On 2016/12/20 23:22:41, dschuyler wrote: > > > On 2016/12/20 22:07:53, tommycli wrote: > > > > I see you added this clause (and below) in PS5. When can this happen? > > > > > > It's happens during startup. It's possible to rearrange the code to remove > > these > > > checks, but that results in less efficient code (i.e. more time to startup > the > > > page). The current code has an |if| in cookies and plugins which are tested > > > rarely. An alternative involves updating the subControlParams_ before the > > > controlParams_ which simply changes which values are undefined. > > > > Sounds good. In that case, I suggest adding a comment to explain why, since it > > wasn't obvious to me. > > dschuyler@: are you saying that the less efficient way would be: > > observers: [ > 'onChangePermissionsControl_(category, controlParams_.value, > subControlParams_.value)', > ], > > instead of 2 separate observers? That wasn't my intention (to be saying that). I meant to be saying that an |if| test is much faster than a function call. Combining the observers is a good idea. It didn't work with the way I was only setting the subControlParams_ for Cookies and Flash. By changing the code to always set subControlParams_, then the combined observer will work, and then the |if| tests can be removed (and the extra call is not made). This looks cleaner too. Done. https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/category_default_setting.js:137: this.priorDefaultContentSetting_.source == update.source) On 2016/12/21 03:06:27, Dan Beam wrote: > curlies Done. https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/category_default_setting.js:143: 'type': 'BOOLEAN', On 2016/12/21 03:06:27, Dan Beam wrote: > nit: can you use chrome.settingsPrivate.PrefType.BOOLEAN Done. https://codereview.chromium.org/2509163004/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/site_settings/category_default_setting.js:144: }; On 2016/12/21 03:06:27, Dan Beam wrote: > nit: this probably fits on only 1 line True, but with the chrome.settingsPrivate.PrefType.BOOLEAN it doesn't :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2509163004/#ps240001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1482361113816250,
"parent_rev": "227d08c81e916c2cc0e6b58653666486e16f7fe4", "commit_rev":
"009a9856b2768afec6665851206a6a56d5c35b97"}
Message was sent while issue was closed.
Description was changed from ========== [MD settings] content category policy This CL adds a controlled-by indicator to the top-level content settings categories. BUG=662596 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] content category policy This CL adds a controlled-by indicator to the top-level content settings categories. BUG=662596 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2509163004 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== [MD settings] content category policy This CL adds a controlled-by indicator to the top-level content settings categories. BUG=662596 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2509163004 ========== to ========== [MD settings] content category policy This CL adds a controlled-by indicator to the top-level content settings categories. BUG=662596 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/bc70cf415de67da7837b1fd7a8cb6109bf102799 Cr-Commit-Position: refs/heads/master@{#440266} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/bc70cf415de67da7837b1fd7a8cb6109bf102799 Cr-Commit-Position: refs/heads/master@{#440266} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
