Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2708013003:
[MD settings] show icon when content settings are controlled by an extension (Closed)
|
Created:
3 years, 10 months ago by dschuyler Modified:
3 years, 9 months ago CC:
chromium-reviews, michaelpg+watch-elements_chromium.org, stevenjb+watch-md-settings_chromium.org, dbeam+watch-elements_chromium.org, oshima+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] show icon when content settings are controlled by an extension
This CL adds an policy/controlledBy icon to content settings when that
setting is controlled by an extension. It also disables the toggle
control for that content setting.
BUG=693301
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2708013003
Cr-Commit-Position: refs/heads/master@{#454756}
Committed: https://chromium.googlesource.com/chromium/src/+/2e192df5bae608faf8e02c2a26fc26e40552f2f8
Patch Set 1 : unit tests #Patch Set 2 : tooltip #Patch Set 3 : closure #
Total comments: 6
Patch Set 4 : isPrefEnforced #
Total comments: 2
Patch Set 5 : ignore-extensions #
Total comments: 15
Patch Set 6 : redo with CL 2696903005 #Patch Set 7 : unit test fix #Dependent Patchsets: Messages
Total messages: 77 (50 generated)
Description was changed from ========== [MD settings] show icon when content settings are controlled by an extension This CL adds an policy/controlledBy icon to content settings when that setting is controlled by an extension. It also disables the toggle control for that content setting. BUG=693301 ========== to ========== [MD settings] show icon when content settings are controlled by an extension This CL adds an policy/controlledBy icon to content settings when that setting is controlled by an extension. It also disables the toggle control for that content setting. BUG=693301 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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
dschuyler@chromium.org changed reviewers: + dbeam@chromium.org
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2708013003/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/policy/cr_policy_indicator_behavior.js (left): https://codereview.chromium.org/2708013003/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/policy/cr_policy_indicator_behavior.js:42: type != CrPolicyIndicatorType.EXTENSION; how does this affect other UIs like <settings-toggle-button> or <settings-checkbox>? https://codereview.chromium.org/2708013003/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (left): https://codereview.chromium.org/2708013003/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:17: pref.controlledBy != chrome.settingsPrivate.ControlledBy.EXTENSION; policy != extension
https://codereview.chromium.org/2708013003/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (left): https://codereview.chromium.org/2708013003/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:17: pref.controlledBy != chrome.settingsPrivate.ControlledBy.EXTENSION; On 2017/02/24 04:44:20, Dan Beam wrote: > policy != extension as in: this method is named "isPrefPolicyControlled". unless the extension is installed by policy (adminstrators can create a force list of extension IDs + sources), a preference that's controlled by an extension is ... controlled by an extension (not policy)
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/2708013003/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/policy/cr_policy_indicator_behavior.js (left): https://codereview.chromium.org/2708013003/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/policy/cr_policy_indicator_behavior.js:42: type != CrPolicyIndicatorType.EXTENSION; On 2017/02/24 04:44:19, Dan Beam wrote: > how does this affect other UIs like <settings-toggle-button> or > <settings-checkbox>? settings-checkbox is used in the People section import and clear browsing data. settings-toggle-button is used frequently. I haven't seen anything odd due to this change. On the settings-toggle-button, I can confirm that an extension controlled icon will appear if that is what the type is set to (which is the intent of the change, so that's good). https://codereview.chromium.org/2708013003/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (left): https://codereview.chromium.org/2708013003/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:17: pref.controlledBy != chrome.settingsPrivate.ControlledBy.EXTENSION; On 2017/02/24 04:44:20, Dan Beam wrote: > policy != extension Acknowledged. https://codereview.chromium.org/2708013003/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:17: pref.controlledBy != chrome.settingsPrivate.ControlledBy.EXTENSION; On 2017/02/24 04:45:45, Dan Beam wrote: > On 2017/02/24 04:44:20, Dan Beam wrote: > > policy != extension > > as in: this method is named "isPrefPolicyControlled". unless the extension is > installed by policy (adminstrators can create a force list of extension IDs + > sources), a preference that's controlled by an extension is ... controlled by an > extension (not policy) Ah, and this function isn't really checking whether the pref is policy controlled, it's testing whether it is enforced. And the places this function is being called are looking for whether a control should be disabled, where enforced is the key topic (rather than policy).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2708013003/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (right): https://codereview.chromium.org/2708013003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:41: return CrPolicyIndicatorType.EXTENSION; please show me screenshots of situations like a <controlled-*> or <settings-*> thing with a pref={{}} that also has a nearby <extension-controlled-indicator> without duplicate indications (i.e. no puzzle piece and tooltip AND a "[Name] is controlling this setting" inline UI right by eachother)
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> 1) add extension icons back as first-level citizens in cr-pref-*-indicators > 2) add a ignore-extensions attribute to the behavior that determines whether to show icons > 3) add this ignore-extensions to everything that we have a <extension-controlled-indicator> for Done. > and file a P2 not-beta-blocking bug about being able to disable extensions in more places crbug 696791. https://codereview.chromium.org/2708013003/diff/80001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (right): https://codereview.chromium.org/2708013003/diff/80001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:41: return CrPolicyIndicatorType.EXTENSION; On 2017/02/25 00:13:41, Dan Beam wrote: > please show me screenshots of situations like a <controlled-*> or <settings-*> > thing with a pref={{}} that also has a nearby <extension-controlled-indicator> > without duplicate indications (i.e. no puzzle piece and tooltip AND a "[Name] is > controlling this setting" inline UI right by eachother) Done off-line. https://codereview.chromium.org/2708013003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/appearance_page/appearance_page.html (right): https://codereview.chromium.org/2708013003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/appearance_page/appearance_page.html:118: pref="[[prefs.homepage_is_newtabpage]]" ignore-extensions> ignore-extensions on line 114 and 118 may not be strictly necessary right now. Afaik, we show the extension controlled info for the homepage in the onstartup section and not in the appearance section. So this is a calculated estimate on the right thing to do. https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/policy/cr_policy_indicator_behavior.js (left): https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/policy/cr_policy_indicator_behavior.js:42: type != CrPolicyIndicatorType.EXTENSION; This is now accounted for in cr_policy_pref_behavior.js so that all the ignore-extensions logic is in that file. https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (left): https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:15: isPrefPolicyControlled: function(pref) { This is renamed as isPrefEnforced. https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (right): https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:32: return pref.enforcement == chrome.settingsPrivate.Enforcement.ENFORCED; Lines 28 to 32 can be expressed in one line of logical operators if preferred.
On 2017/02/28 00:15:18, dschuyler wrote: > > 1) add extension icons back as first-level citizens in cr-pref-*-indicators > > 2) add a ignore-extensions attribute to the behavior that determines whether > to show icons > > 3) add this ignore-extensions to everything that we have a > <extension-controlled-indicator> for > > Done. > > > and file a P2 not-beta-blocking bug about being able to disable extensions in > more places > > crbug 696791. > > https://codereview.chromium.org/2708013003/diff/80001/ui/webui/resources/cr_e... > File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (right): > > https://codereview.chromium.org/2708013003/diff/80001/ui/webui/resources/cr_e... > ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:41: return > CrPolicyIndicatorType.EXTENSION; > On 2017/02/25 00:13:41, Dan Beam wrote: > > please show me screenshots of situations like a <controlled-*> or <settings-*> > > thing with a pref={{}} that also has a nearby <extension-controlled-indicator> > > without duplicate indications (i.e. no puzzle piece and tooltip AND a "[Name] > is > > controlling this setting" inline UI right by eachother) > > Done off-line. > > https://codereview.chromium.org/2708013003/diff/100001/chrome/browser/resourc... > File chrome/browser/resources/settings/appearance_page/appearance_page.html > (right): > > https://codereview.chromium.org/2708013003/diff/100001/chrome/browser/resourc... > chrome/browser/resources/settings/appearance_page/appearance_page.html:118: > pref="[[prefs.homepage_is_newtabpage]]" ignore-extensions> > ignore-extensions on line 114 and 118 may not be strictly necessary right now. > Afaik, we show the extension controlled info for the homepage in the onstartup > section and not in the appearance section. So this is a calculated estimate on > the right thing to do. > > https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... > File ui/webui/resources/cr_elements/policy/cr_policy_indicator_behavior.js > (left): > > https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... > ui/webui/resources/cr_elements/policy/cr_policy_indicator_behavior.js:42: type > != CrPolicyIndicatorType.EXTENSION; > This is now accounted for in cr_policy_pref_behavior.js so that all the > ignore-extensions logic is in that file. > > https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... > File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (left): > > https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... > ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:15: > isPrefPolicyControlled: function(pref) { > This is renamed as isPrefEnforced. > > https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... > File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (right): > > https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... > ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:32: return > pref.enforcement == chrome.settingsPrivate.Enforcement.ENFORCED; > Lines 28 to 32 can be expressed in one line of logical operators if preferred. CL 2696903005 and CL 2708013003 appear to be in the same space (controlled by indicators). Does it make sense to land one before the other particularly?
stevenjb@: what do you think about this? it's to avoid showing double indicators but fix a class of extension-controlled issues in that code I [accidentally] thought was unused ages ago: https://codereview.chromium.org/2479973002/diff/180001/chrome/browser/extensi... dschuyler@: do you know if this will have any performance implications? this code generally seems fine to me, but I'm worried about adding a bunch of properties in high occurrence elements at this point. please assuage my fears :) https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (right): https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:18: ignoreExtensions: Boolean, ignoreControllingExtensions? https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:32: return pref.enforcement == chrome.settingsPrivate.Enforcement.ENFORCED; On 2017/02/28 00:15:18, dschuyler wrote: > Lines 28 to 32 can be expressed in one line of logical operators if preferred. eh, this is fine, it's very clear
Description was changed from ========== [MD settings] show icon when content settings are controlled by an extension This CL adds an policy/controlledBy icon to content settings when that setting is controlled by an extension. It also disables the toggle control for that content setting. BUG=693301 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] show icon when content settings are controlled by an extension This CL adds an policy/controlledBy icon to content settings when that setting is controlled by an extension. It also disables the toggle control for that content setting. BUG=693301 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dbeam@chromium.org changed reviewers: + stevenjb@chromium.org
+stevenjb@ as a reviewer for realz now (was already on a watchlist, though)
https://codereview.chromium.org/2708013003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/appearance_page/appearance_page.html (right): https://codereview.chromium.org/2708013003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/appearance_page/appearance_page.html:118: pref="[[prefs.homepage_is_newtabpage]]" ignore-extensions> On 2017/02/28 00:15:18, dschuyler wrote: > ignore-extensions on line 114 and 118 may not be strictly necessary right now. > Afaik, we show the extension controlled info for the homepage in the onstartup > section and not in the appearance section. So this is a calculated estimate on > the right thing to do. I would rather error on the side of more icons than less, at least for now. https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (right): https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:13: * Showing that an extension is controlling a pref is sometimes done with a 'In those' https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:18: ignoreExtensions: Boolean, On 2017/02/28 02:30:03, Dan Beam wrote: > ignoreControllingExtensions? FWIW, I kind of prefer just 'ignore-extensions', I think the context is sufficient. https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:32: return pref.enforcement == chrome.settingsPrivate.Enforcement.ENFORCED; On 2017/02/28 02:30:03, Dan Beam wrote: > On 2017/02/28 00:15:18, dschuyler wrote: > > Lines 28 to 32 can be expressed in one line of logical operators if preferred. > > eh, this is fine, it's very clear Make sure to merge this with Michael's change (I just lgtm'd it). It uses some pseudo-inheretance to reduce redundant code. FWIW, I'm not sure that the new name is sufficiently clearer to merit the renaming churn.
https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (right): https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:18: ignoreExtensions: Boolean, On 2017/02/28 02:59:33, stevenjb wrote: > On 2017/02/28 02:30:03, Dan Beam wrote: > > ignoreControllingExtensions? > > FWIW, I kind of prefer just 'ignore-extensions', I think the context is > sufficient. the context in this file. but look at other invocations
https://codereview.chromium.org/2708013003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/appearance_page/appearance_page.html (right): https://codereview.chromium.org/2708013003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/appearance_page/appearance_page.html:118: pref="[[prefs.homepage_is_newtabpage]]" ignore-extensions> On 2017/02/28 02:59:33, stevenjb wrote: > On 2017/02/28 00:15:18, dschuyler wrote: > > ignore-extensions on line 114 and 118 may not be strictly necessary right now. > > Afaik, we show the extension controlled info for the homepage in the onstartup > > section and not in the appearance section. So this is a calculated estimate on > > the right thing to do. > > I would rather error on the side of more icons than less, at least for now. that's not your call, unless you changed to our designer ;)
https://codereview.chromium.org/2708013003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/appearance_page/appearance_page.html (right): https://codereview.chromium.org/2708013003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/appearance_page/appearance_page.html:118: pref="[[prefs.homepage_is_newtabpage]]" ignore-extensions> On 2017/02/28 19:51:36, Dan Beam wrote: > On 2017/02/28 02:59:33, stevenjb wrote: > > On 2017/02/28 00:15:18, dschuyler wrote: > > > ignore-extensions on line 114 and 118 may not be strictly necessary right > now. > > > Afaik, we show the extension controlled info for the homepage in the > onstartup > > > section and not in the appearance section. So this is a calculated estimate > on > > > the right thing to do. > > > > I would rather error on the side of more icons than less, at least for now. > > that's not your call, unless you changed to our designer ;) I was under the assumption that we didn't have design input yet, thus the original comment :P Obviously if the design doesn't have icons here then we shouldn't have icons here. If we are unsure, I would prefer to err on the side of more information instead of less. https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (right): https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:18: ignoreExtensions: Boolean, On 2017/02/28 03:02:11, Dan Beam wrote: > On 2017/02/28 02:59:33, stevenjb wrote: > > On 2017/02/28 02:30:03, Dan Beam wrote: > > > ignoreControllingExtensions? > > > > FWIW, I kind of prefer just 'ignore-extensions', I think the context is > > sufficient. > > the context in this file. but look at other invocations I was taking other invocations into consideration.
https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (right): https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:18: ignoreExtensions: Boolean, On 2017/02/28 19:56:28, stevenjb wrote: > On 2017/02/28 03:02:11, Dan Beam wrote: > > On 2017/02/28 02:59:33, stevenjb wrote: > > > On 2017/02/28 02:30:03, Dan Beam wrote: > > > > ignoreControllingExtensions? > > > > > > FWIW, I kind of prefer just 'ignore-extensions', I think the context is > > > sufficient. > > > > the context in this file. but look at other invocations > > I was taking other invocations into consideration. aight, just making sure you saw them
On 2017/02/28 20:00:38, Dan Beam wrote: > https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... > File ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js (right): > > https://codereview.chromium.org/2708013003/diff/100001/ui/webui/resources/cr_... > ui/webui/resources/cr_elements/policy/cr_policy_pref_behavior.js:18: > ignoreExtensions: Boolean, > On 2017/02/28 19:56:28, stevenjb wrote: > > On 2017/02/28 03:02:11, Dan Beam wrote: > > > On 2017/02/28 02:59:33, stevenjb wrote: > > > > On 2017/02/28 02:30:03, Dan Beam wrote: > > > > > ignoreControllingExtensions? > > > > > > > > FWIW, I kind of prefer just 'ignore-extensions', I think the context is > > > > sufficient. > > > > > > the context in this file. but look at other invocations > > > > I was taking other invocations into consideration. > > aight, just making sure you saw them Just an FYI, I'm looking for CL 2696903005 to land and then rework this CL into that new code. (So this CL may change quite a bit).
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 #6 (id:120001) 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...
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 #6 (id:140001) has been deleted
Patchset #6 (id:160001) 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...
On 2017/02/28 02:30:04, Dan Beam wrote: > stevenjb@: what do you think about this? it's to avoid showing double > indicators but fix a class of extension-controlled issues in that code I > [accidentally] thought was unused ages ago: > https://codereview.chromium.org/2479973002/diff/180001/chrome/browser/extensi... > > dschuyler@: do you know if this will have any performance implications? > > this code generally seems fine to me, but I'm worried about adding a bunch of > properties in high occurrence elements at this point. please assuage my fears > :) Using the Time to first paint method in the Performance dev tool and refreshing the page (so that the profile is gathered automatically) there doesn't appear to be a difference (i.e. any difference is smaller than the noise).
This was a redo on top of the other CL. Please take a fresh look. Do please make note of things that need to change even if they were mentioned before (It's getting hard to follow where the discussions landed and whether all the notes sill apply).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/01 23:21:48, stevenjb wrote: > lgtm I have to agree with Dan regarding the "ignore-extensions" name being confusing. I am working on another bug relating to this whole mechanism, and I thought "ignore-extensions" meant that the controlled-ness of the button/radio-button will completely ignore extension-controls (including its value and disabled state), when really the intention of that attribute is to just hide the extension indicator. I think something like "no-extension-indicator" would be much clearer.
On 2017/03/02 20:13:57, scottchen wrote: > On 2017/03/01 23:21:48, stevenjb wrote: > > lgtm > > I have to agree with Dan regarding the "ignore-extensions" name being confusing. > I am working on another bug relating to this whole mechanism, and I thought > "ignore-extensions" meant that the controlled-ness of the button/radio-button > will completely ignore extension-controls (including its value and disabled > state), when really the intention of that attribute is to just hide the > extension indicator. I think something like "no-extension-indicator" would be > much clearer. That makes a lot of sense to me Scott. I like the no-extension-indicator as well. Iiuc, that will allow isPrefEnforced() to be purely 'is this enforced' without consideration to whether it's an extension(?).
sgtm On Thu, Mar 2, 2017 at 1:17 PM, <dschuyler@chromium.org> wrote: > On 2017/03/02 20:13:57, scottchen wrote: > > On 2017/03/01 23:21:48, stevenjb wrote: > > > lgtm > > > > I have to agree with Dan regarding the "ignore-extensions" name being > confusing. > > I am working on another bug relating to this whole mechanism, and I > thought > > "ignore-extensions" meant that the controlled-ness of the > button/radio-button > > will completely ignore extension-controls (including its value and > disabled > > state), when really the intention of that attribute is to just hide the > > extension indicator. I think something like "no-extension-indicator" > would be > > much clearer. > > That makes a lot of sense to me Scott. I like the no-extension-indicator as > well. Iiuc, that will allow isPrefEnforced() to be purely 'is this > enforced' > without consideration to whether it's an extension(?). > > https://codereview.chromium.org/2708013003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/03/02 20:23:56, stevenjb wrote: > sgtm > > On Thu, Mar 2, 2017 at 1:17 PM, <mailto:dschuyler@chromium.org> wrote: > > > On 2017/03/02 20:13:57, scottchen wrote: > > > On 2017/03/01 23:21:48, stevenjb wrote: > > > > lgtm > > > > > > I have to agree with Dan regarding the "ignore-extensions" name being > > confusing. > > > I am working on another bug relating to this whole mechanism, and I > > thought > > > "ignore-extensions" meant that the controlled-ness of the > > button/radio-button > > > will completely ignore extension-controls (including its value and > > disabled > > > state), when really the intention of that attribute is to just hide the > > > extension indicator. I think something like "no-extension-indicator" > > would be > > > much clearer. > > > > That makes a lot of sense to me Scott. I like the no-extension-indicator as > > well. Iiuc, that will allow isPrefEnforced() to be purely 'is this > > enforced' > > without consideration to whether it's an extension(?). > > > > https://codereview.chromium.org/2708013003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. no-extension-incidator sgtm. scottchen@ can be our new official namerator :)
On 2017/03/02 20:48:51, Dan Beam wrote: > On 2017/03/02 20:23:56, stevenjb wrote: > > sgtm > > > > On Thu, Mar 2, 2017 at 1:17 PM, <mailto:dschuyler@chromium.org> wrote: > > > > > On 2017/03/02 20:13:57, scottchen wrote: > > > > On 2017/03/01 23:21:48, stevenjb wrote: > > > > > lgtm > > > > > > > > I have to agree with Dan regarding the "ignore-extensions" name being > > > confusing. > > > > I am working on another bug relating to this whole mechanism, and I > > > thought > > > > "ignore-extensions" meant that the controlled-ness of the > > > button/radio-button > > > > will completely ignore extension-controls (including its value and > > > disabled > > > > state), when really the intention of that attribute is to just hide the > > > > extension indicator. I think something like "no-extension-indicator" > > > would be > > > > much clearer. > > > > > > That makes a lot of sense to me Scott. I like the no-extension-indicator as > > > well. Iiuc, that will allow isPrefEnforced() to be purely 'is this > > > enforced' > > > without consideration to whether it's an extension(?). > > > > > > https://codereview.chromium.org/2708013003/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > no-extension-incidator sgtm. scottchen@ can be our new official namerator :) at your service.
On 2017/03/03 01:23:37, scottchen wrote: > On 2017/03/02 20:48:51, Dan Beam wrote: > > On 2017/03/02 20:23:56, stevenjb wrote: > > > sgtm > > > > > > On Thu, Mar 2, 2017 at 1:17 PM, <mailto:dschuyler@chromium.org> wrote: > > > > > > > On 2017/03/02 20:13:57, scottchen wrote: > > > > > On 2017/03/01 23:21:48, stevenjb wrote: > > > > > > lgtm > > > > > > > > > > I have to agree with Dan regarding the "ignore-extensions" name being > > > > confusing. > > > > > I am working on another bug relating to this whole mechanism, and I > > > > thought > > > > > "ignore-extensions" meant that the controlled-ness of the > > > > button/radio-button > > > > > will completely ignore extension-controls (including its value and > > > > disabled > > > > > state), when really the intention of that attribute is to just hide the > > > > > extension indicator. I think something like "no-extension-indicator" > > > > would be > > > > > much clearer. > > > > > > > > That makes a lot of sense to me Scott. I like the no-extension-indicator > as > > > > well. Iiuc, that will allow isPrefEnforced() to be purely 'is this > > > > enforced' > > > > without consideration to whether it's an extension(?). > > > > > > > > https://codereview.chromium.org/2708013003/ > > > > > > > > > > -- > > > You received this message because you are subscribed to the Google Groups > > > "Chromium-reviews" group. > > > To unsubscribe from this group and stop receiving emails from it, send an > > email > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > no-extension-incidator sgtm. scottchen@ can be our new official namerator :) > > at your service. The change to no-extension-indicator is in CL https://codereview.chromium.org/2727953005/
lgtm
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2708013003/#ps200001 (title: "unit test fix")
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": 200001, "attempt_start_ts": 1488594943612550,
"parent_rev": "c1bf1dcdd2a53f9b90ecb292abd3a1d53d8c7ef1", "commit_rev":
"2e192df5bae608faf8e02c2a26fc26e40552f2f8"}
Message was sent while issue was closed.
Description was changed from ========== [MD settings] show icon when content settings are controlled by an extension This CL adds an policy/controlledBy icon to content settings when that setting is controlled by an extension. It also disables the toggle control for that content setting. BUG=693301 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] show icon when content settings are controlled by an extension This CL adds an policy/controlledBy icon to content settings when that setting is controlled by an extension. It also disables the toggle control for that content setting. BUG=693301 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2708013003 Cr-Commit-Position: refs/heads/master@{#454756} Committed: https://chromium.googlesource.com/chromium/src/+/2e192df5bae608faf8e02c2a26fc... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001) as https://chromium.googlesource.com/chromium/src/+/2e192df5bae608faf8e02c2a26fc... |
