Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
599723002:
Disable wallpaper button in Guest mode settings (Closed)
DescriptionFix wallpaper button in Guest mode settings
R=stevenjb@chromium.org
BUG=401640
Committed: https://crrev.com/e20e3b27eda108409b5b1005af1dd988264efec7
Cr-Commit-Position: refs/heads/master@{#299046}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Tweak UIAccountTweaks #
Total comments: 6
Patch Set 3 : a bit clearer? #
Total comments: 2
Patch Set 4 : #
Messages
Total messages: 15 (3 generated)
Minor fix for guest mode settings. Not user-visible.
lgtm
https://codereview.chromium.org/599723002/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/599723002/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/browser_options.js:1496: button.disabled |= !!managed; nit: Actually, I think that in general this kind of thing is more clear as: if (managed) button.disabled = true; But that is really just a personal preference.
https://codereview.chromium.org/599723002/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/599723002/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/browser_options.js:1496: button.disabled |= !!managed; On 2014/09/24 16:15:23, stevenjb wrote: > nit: Actually, I think that in general this kind of thing is more clear as: > if (managed) > button.disabled = true; > But that is really just a personal preference. +dbeam@ Actually it looks like this can be called again, if the policy changes after the page has loaded. We shouldn't enable the button if it's already been disabled by UIAccountTweaks. I've added a helper function there I'd like Dan to take a look at. I think some other controls should migrate to that function, too.
michaelpg@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/599723002/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/599723002/diff/20001/chrome/browser/resources... chrome/browser/resources/options/browser_options.js:1949: if (!cr.isChromeOS || !enabled) Putting !cr.isChromeOS in a method called simply 'setElementEnabled_' is pretty confusing. I'm not familiar enough with UIAccountTweaks to suggest the best thing to do here is, but unless we use this same pattern elsewhere I might suggest leaving the logic in setWallpaperManaged_. I would also suggest inverting this conditional. https://codereview.chromium.org/599723002/diff/20001/ui/webui/resources/js/ch... File ui/webui/resources/js/chromeos/ui_account_tweaks.js (right): https://codereview.chromium.org/599723002/diff/20001/ui/webui/resources/js/ch... ui/webui/resources/js/chromeos/ui_account_tweaks.js:64: UIAccountTweaks.enableElementIfPossible = function(element) { Again, this function name seems too generic for the logic. https://codereview.chromium.org/599723002/diff/20001/ui/webui/resources/js/ch... ui/webui/resources/js/chromeos/ui_account_tweaks.js:65: if (cr.isChromeOS) { Shouldn't this always be true since this is in js/chromeos?
Patchset #3 (id:40001) has been deleted
PTAL. https://codereview.chromium.org/599723002/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/599723002/diff/20001/chrome/browser/resources... chrome/browser/resources/options/browser_options.js:1949: if (!cr.isChromeOS || !enabled) On 2014/09/25 15:45:33, stevenjb wrote: > Putting !cr.isChromeOS in a method called simply 'setElementEnabled_' is pretty > confusing. I'm not familiar enough with UIAccountTweaks to suggest the best > thing to do here is, but unless we use this same pattern elsewhere I might > suggest leaving the logic in setWallpaperManaged_. > I would also suggest inverting this conditional. We really should use this every time we enable/disable an element on Chrome OS in case that element has a guest-visibility or public-account-visibility attribute. So I think we should move to using this function in more places, to prevent more bugs like this from arising. Some elements, e.g. in plugins.html, have a *-visibility attribute but are not Chrome OS-only. So I could rename this function, and then wherever we want to disable an element we would say: if (cr.isChromeOS) this.setElementEnabledIfAllowed(element, false); else element.disabled = true; https://codereview.chromium.org/599723002/diff/20001/ui/webui/resources/js/ch... File ui/webui/resources/js/chromeos/ui_account_tweaks.js (right): https://codereview.chromium.org/599723002/diff/20001/ui/webui/resources/js/ch... ui/webui/resources/js/chromeos/ui_account_tweaks.js:64: UIAccountTweaks.enableElementIfPossible = function(element) { On 2014/09/25 15:45:33, stevenjb wrote: > Again, this function name seems too generic for the logic. enableElementIfAllowed? enableElementIfEditable? tryToEnableElement? enableElementForSessionType? https://codereview.chromium.org/599723002/diff/20001/ui/webui/resources/js/ch... ui/webui/resources/js/chromeos/ui_account_tweaks.js:65: if (cr.isChromeOS) { Yes. I've done a sanity check anyway, and nothing is calling this function outside of chrome OS.
lgtm https://codereview.chromium.org/599723002/diff/1/chrome/browser/resources/opt... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/599723002/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/browser_options.js:1496: button.disabled |= !!managed; On 2014/09/24 22:29:23, michaelpg wrote: > On 2014/09/24 16:15:23, stevenjb wrote: > > nit: Actually, I think that in general this kind of thing is more clear as: > > if (managed) > > button.disabled = true; > > But that is really just a personal preference. > > +dbeam@ > > Actually it looks like this can be called again, if the policy changes after the > page has loaded. We shouldn't enable the button if it's already been disabled by > UIAccountTweaks. I've added a helper function there I'd like Dan to take a look > at. I think some other controls should migrate to that function, too. any version including: button.disabled = button.disabled || managed; is fine by me. no particular preference here. i'd say out of all the versions, stevenjb@'s is probably the easiest to read, but we do (in general) prefer to hit the same codepaths when possible (e.g. instead of if/else => ternary). https://codereview.chromium.org/599723002/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/browser_options.js:1496: button.disabled |= !!managed; why are we doing !! if |managed| should already be a boolean? https://codereview.chromium.org/599723002/diff/1/chrome/browser/resources/opt... chrome/browser/resources/options/browser_options.js:1504: event.value = {}; for example: event.value = managed ? {controlledBy: 'policy'} : {}; or event.value = {}; if (managed) event.value.controlledBy = 'policy'; both seem mildly better https://codereview.chromium.org/599723002/diff/60001/ui/webui/resources/js/ch... File ui/webui/resources/js/chromeos/ui_account_tweaks.js (right): https://codereview.chromium.org/599723002/diff/60001/ui/webui/resources/js/ch... ui/webui/resources/js/chromeos/ui_account_tweaks.js:72: element.getAttribute(sessionType +'-visibility') == 'disabled') { nit: + '
This looks better / clearer to me, thanks. LGTM
Thanks. https://codereview.chromium.org/599723002/diff/60001/ui/webui/resources/js/ch... File ui/webui/resources/js/chromeos/ui_account_tweaks.js (right): https://codereview.chromium.org/599723002/diff/60001/ui/webui/resources/js/ch... ui/webui/resources/js/chromeos/ui_account_tweaks.js:72: element.getAttribute(sessionType +'-visibility') == 'disabled') { On 2014/09/29 18:52:18, Dan Beam wrote: > nit: + ' Done.
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/599723002/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e20e3b27eda108409b5b1005af1dd988264efec7 Cr-Commit-Position: refs/heads/master@{#299046} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
