Hide/Disable several (meaningless) options in Settings uber-page for Guest.

Hide/Disable several (meaningless) options in Settings uber-page for Guest.
For detailed  description of changes see individual issues / https://docs.google.com/a/google.com/document/d/1o9bKsCyvYnFAu2tilRqFbyIwR1dPs2XdLkdvSZ3u8eg/edit

BUG=chromium-os:26899, chromium-os:26898, chromium-os:26897, chromium-os:26896, chromium-os:26894, chromium-os:26893, chromium-os:26891
TEST=tested manually


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=127538

[James Hawkins, 2012-02-28 20:03:35]

+csilv to take a look at the search-hidden stuff.

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
File chrome/browser/resources/options2/advanced_options.html (right):

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
chrome/browser/resources/options2/advanced_options.html:53: <div
id="searchSuggestEnabledGroup" class="checkbox">
IDs use dash-form (even though this page hasn't been cleaned up yet).

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
File chrome/browser/resources/options2/browser_options.js (right):

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
chrome/browser/resources/options2/browser_options.js:115: if
(AccountsOptions.loggedInAsGuest())
Move this 'if' logic up.

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
chrome/browser/resources/options2/browser_options.js:200: if
(AccountsOptions.loggedInAsGuest()) {
Move this block up to the top.

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
File chrome/browser/resources/options2/chromeos/accounts_options.js (right):

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
chrome/browser/resources/options2/chromeos/accounts_options.js:164: * Disable
and mark page elements that does not make sense in Guest mode.
s/does/do/

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
chrome/browser/resources/options2/chromeos/accounts_options.js:164: * Disable
and mark page elements that does not make sense in Guest mode.
Don't use statements about things 'making sense' in comments.  Just document
what the method does.

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
chrome/browser/resources/options2/chromeos/accounts_options.js:166:
AccountsOptions.disableElementsForGuest = function(element) {
Document |element|.

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
chrome/browser/resources/options2/chromeos/accounts_options.js:182:
AccountsOptions.disableElementForGuest_ = function(element) {
Document method.

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
File chrome/browser/resources/options2/content_settings.js (right):

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
chrome/browser/resources/options2/content_settings.js:70: var
manageIntentsButton = $('manage-intents-button');
Not sure why you're adding this.  The intents page is being removed.

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
File chrome/browser/resources/options2/search_page.js (right):

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
chrome/browser/resources/options2/search_page.js:315: if (!node.hidden) {
No braces.

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/ui/we...
File chrome/browser/ui/webui/options2/chromeos/core_chromeos_options_handler2.cc
(right):

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/ui/we...
chrome/browser/ui/webui/options2/chromeos/core_chromeos_options_handler2.cc:216:
localized_strings->SetString("logged_in_as_guest",
Start of parameters must be aligned on the same column.

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/ui/we...
chrome/browser/ui/webui/options2/chromeos/core_chromeos_options_handler2.cc:218:
ASCIIToUTF16("true") : ASCIIToUTF16("false"));
Must indent 4 spaces.

[Denis Kuznetsov (DE-MUC), 2012-02-29 08:41:43]

PTAL

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
File chrome/browser/resources/options2/advanced_options.html (right):

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
chrome/browser/resources/options2/advanced_options.html:53: <div
id="searchSuggestEnabledGroup" class="checkbox">
On 2012/02/28 20:03:35, James Hawkins wrote:
> IDs use dash-form (even though this page hasn't been cleaned up yet).

Done.

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
File chrome/browser/resources/options2/browser_options.js (right):

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
chrome/browser/resources/options2/browser_options.js:115: if
(AccountsOptions.loggedInAsGuest())
On 2012/02/28 20:03:35, James Hawkins wrote:
> Move this 'if' logic up.

Done.

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
chrome/browser/resources/options2/browser_options.js:200: if
(AccountsOptions.loggedInAsGuest()) {
On 2012/02/28 20:03:35, James Hawkins wrote:
> Move this block up to the top.

Done.

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
File chrome/browser/resources/options2/chromeos/accounts_options.js (right):

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
chrome/browser/resources/options2/chromeos/accounts_options.js:164: * Disable
and mark page elements that does not make sense in Guest mode.
On 2012/02/28 20:03:35, James Hawkins wrote:
> s/does/do/

Done.

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
chrome/browser/resources/options2/chromeos/accounts_options.js:164: * Disable
and mark page elements that does not make sense in Guest mode.
On 2012/02/28 20:03:35, James Hawkins wrote:
> Don't use statements about things 'making sense' in comments.  Just document
> what the method does.

Done.

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
chrome/browser/resources/options2/chromeos/accounts_options.js:166:
AccountsOptions.disableElementsForGuest = function(element) {
On 2012/02/28 20:03:35, James Hawkins wrote:
> Document |element|.

Done.

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
chrome/browser/resources/options2/chromeos/accounts_options.js:182:
AccountsOptions.disableElementForGuest_ = function(element) {
On 2012/02/28 20:03:35, James Hawkins wrote:
> Document method.

Done.

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
File chrome/browser/resources/options2/content_settings.js (right):

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
chrome/browser/resources/options2/content_settings.js:70: var
manageIntentsButton = $('manage-intents-button');
On 2012/02/28 20:03:35, James Hawkins wrote:
> Not sure why you're adding this.  The intents page is being removed.
It is an artifact of manual merge.
Done.

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
File chrome/browser/resources/options2/search_page.js (right):

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/resou...
chrome/browser/resources/options2/search_page.js:315: if (!node.hidden) {
On 2012/02/28 20:03:35, James Hawkins wrote:
> No braces.

Done.

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/ui/we...
File chrome/browser/ui/webui/options2/chromeos/core_chromeos_options_handler2.cc
(right):

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/ui/we...
chrome/browser/ui/webui/options2/chromeos/core_chromeos_options_handler2.cc:216:
localized_strings->SetString("logged_in_as_guest",
On 2012/02/28 20:03:35, James Hawkins wrote:
> Start of parameters must be aligned on the same column.

Done.

https://chromiumcodereview.appspot.com/9464053/diff/3004/chrome/browser/ui/we...
chrome/browser/ui/webui/options2/chromeos/core_chromeos_options_handler2.cc:218:
ASCIIToUTF16("true") : ASCIIToUTF16("false"));
On 2012/02/28 20:03:35, James Hawkins wrote:
> Must indent 4 spaces.

Done.

[csilv, 2012-03-01 01:32:18]

I really like the cleanups.  LGTM w/ style nits.

https://chromiumcodereview.appspot.com/9464053/diff/10001/chrome/browser/reso...
File chrome/browser/resources/options2/advanced_options.html (right):

https://chromiumcodereview.appspot.com/9464053/diff/10001/chrome/browser/reso...
chrome/browser/resources/options2/advanced_options.html:60: <div 
id="dns-prefetching-enabled-group" class="checkbox">
nit, extra space before "id"

https://chromiumcodereview.appspot.com/9464053/diff/10001/chrome/browser/reso...
File chrome/browser/resources/options2/browser_options.html (right):

https://chromiumcodereview.appspot.com/9464053/diff/10001/chrome/browser/reso...
chrome/browser/resources/options2/browser_options.html:111:
i18n-content="toolbarShowHomeButton"></span>
please restore the indentation that was removed.  the 'for' tag isn't needed
because of the nesting, it seems like you were going to to make a change that
you decided against.

https://chromiumcodereview.appspot.com/9464053/diff/10001/chrome/browser/reso...
File chrome/browser/resources/options2/browser_options.js (right):

https://chromiumcodereview.appspot.com/9464053/diff/10001/chrome/browser/reso...
chrome/browser/resources/options2/browser_options.js:91: // // Appearance
section.
nit, extra //

Perhaps we should combine the if blocks?

https://chromiumcodereview.appspot.com/9464053/diff/10001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/chromeos/core_chromeos_options_handler2.cc
(right):

https://chromiumcodereview.appspot.com/9464053/diff/10001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/core_chromeos_options_handler2.cc:222:

nit, remove extra line between these methods.

[Denis Kuznetsov (DE-MUC), 2012-03-01 14:21:54]

James Hawkins, please take a look again -- had to merge with TOT (changes in
chrome/browser/resources/options2/browser_options.html for new Home Page
selection) + grayed out labels for disabled elements
(chrome/browser/resources/options2/browser_options.html as well as
chrome/browser/resources/options2/advanced_options.html).

https://chromiumcodereview.appspot.com/9464053/diff/10001/chrome/browser/reso...
File chrome/browser/resources/options2/advanced_options.html (right):

https://chromiumcodereview.appspot.com/9464053/diff/10001/chrome/browser/reso...
chrome/browser/resources/options2/advanced_options.html:60: <div 
id="dns-prefetching-enabled-group" class="checkbox">
On 2012/03/01 01:32:18, csilv wrote:
> nit, extra space before "id"

Done.

https://chromiumcodereview.appspot.com/9464053/diff/10001/chrome/browser/reso...
File chrome/browser/resources/options2/browser_options.html (right):

https://chromiumcodereview.appspot.com/9464053/diff/10001/chrome/browser/reso...
chrome/browser/resources/options2/browser_options.html:111:
i18n-content="toolbarShowHomeButton"></span>
On 2012/03/01 01:32:18, csilv wrote:
> please restore the indentation that was removed.  the 'for' tag isn't needed
> because of the nesting, it seems like you were going to to make a change that
> you decided against.

Done.

https://chromiumcodereview.appspot.com/9464053/diff/10001/chrome/browser/reso...
File chrome/browser/resources/options2/browser_options.js (right):

https://chromiumcodereview.appspot.com/9464053/diff/10001/chrome/browser/reso...
chrome/browser/resources/options2/browser_options.js:91: // // Appearance
section.
On 2012/03/01 01:32:18, csilv wrote:
> nit, extra //
> 
> Perhaps we should combine the if blocks?

Done.

I like the idea that all code is grouped just the way page looks like - makes it
easier to find.

https://chromiumcodereview.appspot.com/9464053/diff/10001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/chromeos/core_chromeos_options_handler2.cc
(right):

https://chromiumcodereview.appspot.com/9464053/diff/10001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/core_chromeos_options_handler2.cc:222:

On 2012/03/01 01:32:18, csilv wrote:
> nit, remove extra line between these methods.

Done.

[James Hawkins, 2012-03-01 20:00:19]

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/reso...
File chrome/browser/resources/options2/advanced_options.html (right):

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/reso...
chrome/browser/resources/options2/advanced_options.html:14: <select
id="timezone-select" class="control" i18n-options="timezoneList"
nit: 80 cols.

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/reso...
File chrome/browser/resources/options2/chromeos/accounts_options.js (right):

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/reso...
chrome/browser/resources/options2/chromeos/accounts_options.js:170: * disabled.
nit: Indent 4 spaces.

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/reso...
File chrome/browser/resources/options2/chromeos/accounts_options_page.css
(right):

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/reso...
chrome/browser/resources/options2/chromeos/accounts_options_page.css:93:
h3.guest-disabled {
Combine all of these:

h3.guest-disabled,
span.guest-disabled,
label.guest-disabled {
  color: greay;
}

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/reso...
chrome/browser/resources/options2/chromeos/accounts_options_page.css:104:
a.guest-disabled {
Blank line before this.

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/reso...
File chrome/browser/resources/options2/content_settings.js (right):

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/reso...
chrome/browser/resources/options2/content_settings.js:70: // Chrome-OS specific
part
nit: Period at end of sentence.

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/chromeos/core_chromeos_options_handler2.cc
(right):

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/core_chromeos_options_handler2.cc:217:
"logged_in_as_guest",
loggedInAsGuest.

[Denis Kuznetsov (DE-MUC), 2012-03-02 17:32:25]

James Hawkins, please take a look.
I had to merge, advanced_options are now part of browser_options.

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/reso...
File chrome/browser/resources/options2/advanced_options.html (right):

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/reso...
chrome/browser/resources/options2/advanced_options.html:14: <select
id="timezone-select" class="control" i18n-options="timezoneList"
On 2012/03/01 20:00:19, James Hawkins wrote:
> nit: 80 cols.

Done.

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/reso...
File chrome/browser/resources/options2/chromeos/accounts_options.js (right):

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/reso...
chrome/browser/resources/options2/chromeos/accounts_options.js:170: * disabled.
On 2012/03/01 20:00:19, James Hawkins wrote:
> nit: Indent 4 spaces.

Done.

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/reso...
File chrome/browser/resources/options2/chromeos/accounts_options_page.css
(right):

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/reso...
chrome/browser/resources/options2/chromeos/accounts_options_page.css:93:
h3.guest-disabled {
On 2012/03/01 20:00:19, James Hawkins wrote:
> Combine all of these:
> 
> h3.guest-disabled,
> span.guest-disabled,
> label.guest-disabled {
>   color: greay;
> }

Done.

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/reso...
chrome/browser/resources/options2/chromeos/accounts_options_page.css:104:
a.guest-disabled {
On 2012/03/01 20:00:19, James Hawkins wrote:
> Blank line before this.

Done.

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/reso...
File chrome/browser/resources/options2/content_settings.js (right):

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/reso...
chrome/browser/resources/options2/content_settings.js:70: // Chrome-OS specific
part
On 2012/03/01 20:00:19, James Hawkins wrote:
> nit: Period at end of sentence.

Done.

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/chromeos/core_chromeos_options_handler2.cc
(right):

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/core_chromeos_options_handler2.cc:217:
"logged_in_as_guest",
On 2012/03/01 20:00:19, James Hawkins wrote:
> loggedInAsGuest.
As I could see, Localized values are used both for actually localized values, as
well as for passing some constants from C++ side. 
I assume that underscore_style should be used for constants instead of
camelHumpStyle to prevent occasional name clash.

[James Hawkins, 2012-03-02 22:26:17]

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/chromeos/core_chromeos_options_handler2.cc
(right):

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/core_chromeos_options_handler2.cc:217:
"logged_in_as_guest",
On 2012/03/02 17:32:25, Denis Kuznetsov wrote:
> On 2012/03/01 20:00:19, James Hawkins wrote:
> > loggedInAsGuest.
> As I could see, Localized values are used both for actually localized values,
as
> well as for passing some constants from C++ side. 
> I assume that underscore_style should be used for constants instead of
> camelHumpStyle to prevent occasional name clash.

No, it is a JavaScript variable on injection, thus it should follow the variable
naming convention of JS here (like we do for localized strings).

[Denis Kuznetsov (DE-MUC), 2012-03-05 09:25:57]

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/chromeos/core_chromeos_options_handler2.cc
(right):

https://chromiumcodereview.appspot.com/9464053/diff/17001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/core_chromeos_options_handler2.cc:217:
"logged_in_as_guest",
On 2012/03/02 22:26:17, James Hawkins wrote:
> On 2012/03/02 17:32:25, Denis Kuznetsov wrote:
> > On 2012/03/01 20:00:19, James Hawkins wrote:
> > > loggedInAsGuest.
> > As I could see, Localized values are used both for actually localized
values,
> as
> > well as for passing some constants from C++ side. 
> > I assume that underscore_style should be used for constants instead of
> > camelHumpStyle to prevent occasional name clash.
> 
> No, it is a JavaScript variable on injection, thus it should follow the
variable
> naming convention of JS here (like we do for localized strings).

Done.

[James Hawkins, 2012-03-05 17:07:12]

https://chromiumcodereview.appspot.com/9464053/diff/30001/chrome/browser/reso...
File chrome/browser/resources/options2/browser_options.html (right):

https://chromiumcodereview.appspot.com/9464053/diff/30001/chrome/browser/reso...
chrome/browser/resources/options2/browser_options.html:116: <div
class="settings-row" id="home-page-select-container">
nit: IDs should be the first attribute.

https://chromiumcodereview.appspot.com/9464053/diff/30001/chrome/browser/reso...
chrome/browser/resources/options2/browser_options.html:289:
pref="cros.system.timezone"></select>
nit: Closing tag should be on a new line.

https://chromiumcodereview.appspot.com/9464053/diff/30001/chrome/browser/reso...
File chrome/browser/resources/options2/browser_options.js (right):

https://chromiumcodereview.appspot.com/9464053/diff/30001/chrome/browser/reso...
chrome/browser/resources/options2/browser_options.js:122: if
(AccountsOptions.loggedInAsGuest()) {
Combine this if with if (cr.isChromeOS).

https://chromiumcodereview.appspot.com/9464053/diff/30001/chrome/browser/reso...
File chrome/browser/resources/options2/chromeos/accounts_options.js (right):

https://chromiumcodereview.appspot.com/9464053/diff/30001/chrome/browser/reso...
chrome/browser/resources/options2/chromeos/accounts_options.js:164: * Disable
and mark page elements for Guest mode.
nit: Method documentation should be declarative (as opposed to imperative, used
here).

"Disables and marks page elements for Guest mode."

https://chromiumcodereview.appspot.com/9464053/diff/30001/chrome/browser/reso...
chrome/browser/resources/options2/chromeos/accounts_options.js:165: * It adds
guest-disabled css class to all elements within given subtree,
nit: Don't use pronouns (it) in comments; they're ambiguous.  It's also shorter
w/out:

"Adds guest-disabled..."

https://chromiumcodereview.appspot.com/9464053/diff/30001/chrome/browser/reso...
chrome/browser/resources/options2/chromeos/accounts_options.js:190: * It adds
guest-disabled css class, adds disabled attribute for appropriate
nit: s/It adds/Adds/

https://chromiumcodereview.appspot.com/9464053/diff/30001/chrome/browser/reso...
File chrome/browser/resources/options2/content_settings.js (right):

https://chromiumcodereview.appspot.com/9464053/diff/30001/chrome/browser/reso...
chrome/browser/resources/options2/content_settings.js:70: // Chrome-OS specific
part.
Optional nit: This comment is superfluous.

[Denis Kuznetsov (DE-MUC), 2012-03-12 10:32:15]

http://codereview.chromium.org/9464053/diff/30001/chrome/browser/resources/op...
File chrome/browser/resources/options2/browser_options.html (right):

http://codereview.chromium.org/9464053/diff/30001/chrome/browser/resources/op...
chrome/browser/resources/options2/browser_options.html:116: <div
class="settings-row" id="home-page-select-container">
On 2012/03/05 17:07:12, James Hawkins wrote:
> nit: IDs should be the first attribute.

Done.

http://codereview.chromium.org/9464053/diff/30001/chrome/browser/resources/op...
chrome/browser/resources/options2/browser_options.html:289:
pref="cros.system.timezone"></select>
On 2012/03/05 17:07:12, James Hawkins wrote:
> nit: Closing tag should be on a new line.

Done.

http://codereview.chromium.org/9464053/diff/30001/chrome/browser/resources/op...
File chrome/browser/resources/options2/browser_options.js (right):

http://codereview.chromium.org/9464053/diff/30001/chrome/browser/resources/op...
chrome/browser/resources/options2/browser_options.js:122: if
(AccountsOptions.loggedInAsGuest()) {
On 2012/03/05 17:07:12, James Hawkins wrote:
> Combine this if with if (cr.isChromeOS).

Done.

http://codereview.chromium.org/9464053/diff/30001/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/accounts_options.js (right):

http://codereview.chromium.org/9464053/diff/30001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/accounts_options.js:164: * Disable
and mark page elements for Guest mode.
On 2012/03/05 17:07:12, James Hawkins wrote:
> nit: Method documentation should be declarative (as opposed to imperative,
used
> here).
> 
> "Disables and marks page elements for Guest mode."

Done.

http://codereview.chromium.org/9464053/diff/30001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/accounts_options.js:165: * It adds
guest-disabled css class to all elements within given subtree,
On 2012/03/05 17:07:12, James Hawkins wrote:
> nit: Don't use pronouns (it) in comments; they're ambiguous.  It's also
shorter
> w/out:
> 
> "Adds guest-disabled..."

Done.

http://codereview.chromium.org/9464053/diff/30001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/accounts_options.js:190: * It adds
guest-disabled css class, adds disabled attribute for appropriate
On 2012/03/05 17:07:12, James Hawkins wrote:
> nit: s/It adds/Adds/

Done.

http://codereview.chromium.org/9464053/diff/30001/chrome/browser/resources/op...
File chrome/browser/resources/options2/content_settings.js (right):

http://codereview.chromium.org/9464053/diff/30001/chrome/browser/resources/op...
chrome/browser/resources/options2/content_settings.js:70: // Chrome-OS specific
part.
On 2012/03/05 17:07:12, James Hawkins wrote:
> Optional nit: This comment is superfluous.

Done.

[James Hawkins, 2012-03-13 20:43:11]

I really think we should be using a custom attribute instead of hard-coding the
sections that should be hidden for guest mode. e.g., 'owner-pref" or something. 
Then do a querySelectorAll and hide elements that have this pref if not owner.

[Denis Kuznetsov (DE-MUC), 2012-03-14 13:17:31]

On the one hand, I like the idea of declarative control for settings. On the
other hand, we have three types of users (owner, ordinary user, guest), as well
as three states for each setting (normally working, disabled, hidden) plus few
"high-level effects" like "when all settings in a section are hidden, section
title should also be hidden".

So I'd go for current implementation in R19, and think about declarative
approach in R20.

[James Hawkins, 2012-03-14 17:37:11]

This is just for elements that should be hidden for guests, right?  So this CL
requires one custom attribute, not the many you listed.  The CL will actually
end up much shorter than all of the JS required to do this manually.  We have
about a week and a half before the branch point, so please make the proposed
change in this CL (unless you have reasoning for why it's an inferior solution).

[Denis Kuznetsov (DE-MUC), 2012-03-15 16:37:55]

Done, please take a look.

[James Hawkins, 2012-03-15 17:21:32]

Thanks! LGTM (really good) with nits.

http://codereview.chromium.org/9464053/diff/46001/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/accounts_options.js (right):

http://codereview.chromium.org/9464053/diff/46001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/accounts_options.js:172: if
(cr.isChromeOS && AccountsOptions.loggedInAsGuest()) {
Optional nit: Save indentation and return early.

http://codereview.chromium.org/9464053/diff/46001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/accounts_options.js:174: for (var i =
0; i < elements.length; ++i) {
nit: i++

[commit-bot: I haz the power, 2012-03-15 18:01:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/9464053/50001

[commit-bot: I haz the power, 2012-03-15 18:01:34]

Can't apply patch for file
chrome/browser/resources/options2/browser_options.html.
While running patch -p1 --forward --force;
patching file chrome/browser/resources/options2/browser_options.html
Hunk #1 FAILED at 104.
Hunk #2 succeeded at 114 (offset 1 line).
Hunk #3 succeeded at 161 (offset 1 line).
Hunk #4 succeeded at 175 (offset 1 line).
Hunk #5 succeeded at 257 (offset 1 line).
Hunk #6 succeeded at 303 (offset 1 line).
Hunk #7 succeeded at 551 (offset 1 line).
1 out of 7 hunks FAILED -- saving rejects to file
chrome/browser/resources/options2/browser_options.html.rej

[commit-bot: I haz the power, 2012-03-16 14:58:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/9464053/50002

[commit-bot: I haz the power, 2012-03-16 15:35:25]

Try job failure for 9464053-50002 (retry) on linux_rel for step "browser_tests".
It's a second try, previously, step "update" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-03-16 19:40:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/9464053/46003

[commit-bot: I haz the power, 2012-03-16 19:40:45]

Can't apply patch for file
chrome/browser/resources/options2/browser_options.html.
While running patch -p1 --forward --force;
patching file chrome/browser/resources/options2/browser_options.html
Hunk #1 FAILED at 105.
Hunk #2 succeeded at 119 with fuzz 2 (offset 5 lines).
Hunk #3 succeeded at 166 (offset 5 lines).
Hunk #4 succeeded at 180 (offset 5 lines).
Hunk #5 succeeded at 262 (offset 5 lines).
Hunk #6 succeeded at 308 (offset 5 lines).
Hunk #7 succeeded at 556 (offset 5 lines).
1 out of 7 hunks FAILED -- saving rejects to file
chrome/browser/resources/options2/browser_options.html.rej

[commit-bot: I haz the power, 2012-03-19 15:29:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/9464053/54001

[commit-bot: I haz the power, 2012-03-19 17:19:11]

Try job failure for 9464053-54001 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-03-19 17:42:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/9464053/54001

[commit-bot: I haz the power, 2012-03-19 19:28:37]

Try job failure for 9464053-54001 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-03-19 19:41:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/9464053/62003

[commit-bot: I haz the power, 2012-03-19 21:24:50]

Change committed as 127538