Issue 467363003: Guest mode causes certain settings to be hidden.

Issue 467363003: Guest mode causes certain settings to be hidden. (Closed)

Created:
6 years, 4 months ago by Mike Lerman

Modified:
6 years, 4 months ago

Reviewers:
Evan Stade

CC:
chromium-reviews, dbeam+watch-options_chromium.org, arv+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Project:
chromium

Visibility:
Public.

More Reviews

Description

Guest mode causes certain settings to be hidden. (specifically, everything except the omnibox's search engine) Remove the settings search box. Remove the enableResetProfileSettings JS variable, in place of "isGuest". BUG=402153 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291096

Patch Set 1 #

Total comments: 9

Patch Set 2 : Rebase #

Patch Set 3 : Estade's comments #

Total comments: 2

Patch Set 4 : Change visibility to CSS controlled #

Created: 6 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+34 lines, -11 lines)			Patch
M	chrome/browser/resources/options/browser_options.css	View	1 2 3	1 chunk	+7 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/options/browser_options.html	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/options/browser_options.js	View	1 2 3	1 chunk	+0 lines, -2 lines	0 comments	Download
M	chrome/browser/resources/options/content_settings.js	View	1 2 3	1 chunk	+5 lines, -2 lines	0 comments	Download
M	chrome/browser/resources/options/options.html	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/options/search_page.js	View	1 2	2 chunks	+8 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/webui/options/browser_options_handler.cc	View	1	3 chunks	+7 lines, -5 lines	0 comments	Download
M	chrome/browser/ui/webui/options/startup_pages_handler.cc	View	1 2 3	1 chunk	+5 lines, -0 lines	0 comments	Download

Messages

Total messages: 17 (0 generated)

Expand Messages | Collapse Messages

Mike Lerman

Hi Evan, Can you please review this CL? The goal is to remove most elements ...

6 years, 4 months ago (2014-08-14 19:47:54 UTC) #1

Evan Stade

On 2014/08/14 19:47:54, Mike Lerman wrote: > Hi Evan, > > Can you please review ...

6 years, 4 months ago (2014-08-14 20:09:17 UTC) #2

Mike Lerman

On 2014/08/14 20:09:17, Evan Stade wrote: > On 2014/08/14 19:47:54, Mike Lerman wrote: > > ...

6 years, 4 months ago (2014-08-14 20:12:13 UTC) #3

Mike Lerman

Hi Evan, This CL: - hides the search box in guest mode - Hides the ...

6 years, 4 months ago (2014-08-19 13:02:56 UTC) #4

Evan Stade

https://codereview.chromium.org/467363003/diff/40001/chrome/browser/resources/options/browser_options.js File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/467363003/diff/40001/chrome/browser/resources/options/browser_options.js#newcode215 chrome/browser/resources/options/browser_options.js:215: if (loadTimeData.getBoolean('profileIsGuest')) { I think a somewhat cleaner approach ...

6 years, 4 months ago (2014-08-19 17:49:14 UTC) #5

Mike Lerman

Thanks Evan. https://codereview.chromium.org/467363003/diff/40001/chrome/browser/resources/options/browser_options.js File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/467363003/diff/40001/chrome/browser/resources/options/browser_options.js#newcode215 chrome/browser/resources/options/browser_options.js:215: if (loadTimeData.getBoolean('profileIsGuest')) { On 2014/08/19 17:49:13, Evan ...

6 years, 4 months ago (2014-08-19 20:34:12 UTC) #6

Thanks Evan.

https://codereview.chromium.org/467363003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/467363003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:215: if
(loadTimeData.getBoolean('profileIsGuest')) {
On 2014/08/19 17:49:13, Evan Stade wrote:
> I think a somewhat cleaner approach would be to add an attribute to the <html>
> tag, i.e. something like:
> 
> <html i18n-values="guestMode=profileIsGuest">
> 
> Then add css to hide appropriate things, e.g.
> 
> html[guestMode='true'] .appearance-section {
>   display: none;
> }

I've always taken the approach the "logic" should never exist in CSS or HTML
files. CSS is for styling, HTML for overall layout, logic for JS or back end. I
think which options sections qualify should or should not be visible for a Guest
profile is "business logic", controlled in JS rather than HTML. But if you feel
that using styles makes for cleaner code, I always lean towards readability over
architectural niceness.

https://codereview.chromium.org/467363003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/search_page.js (right):

https://codereview.chromium.org/467363003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/search_page.js:190: // Guest users should never
have active search
On 2014/08/19 17:49:13, Evan Stade wrote:
> nit: final punctuation

Done.

https://codereview.chromium.org/467363003/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/startup_pages_handler.cc (right):

https://codereview.chromium.org/467363003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/startup_pages_handler.cc:52: // these values.
On 2014/08/19 17:49:13, Evan Stade wrote:
> nit: I don't think this comment is necessary

Acknowledged.

https://codereview.chromium.org/467363003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/startup_pages_handler.cc:53: if
(Profile::FromWebUI(web_ui())->IsOffTheRecord())
On 2014/08/19 17:49:13, Evan Stade wrote:
> would it be possible to not even create the StartupPagesHandler in the OTR
case?

Done, removed this call here and added a check in options_ui.

Evan Stade

6 years, 4 months ago (2014-08-19 20:41:15 UTC) #7

Mike Lerman

Thanks, Evan. I moved the visibility control to being CSS based. I'm hanging to i18n-values ...

6 years, 4 months ago (2014-08-20 15:58:16 UTC) #8