Integrate About page into Settings for Chrome OS settings in a window.

Integrate About page into Settings for Chrome OS settings in a window.

This is the last patch to get the About overlay showing in the Settings page
on Chrome OS when Settings in a window is enabled, so that we can hide the
uber navigation frame and truly display Settings in its own window.

BUG=379348
R=dbeam@chromium.org,ben@chromium.org

ben: please review:
    chrome/browser/browser_about_handler.cc
i hope i'm not doing anything blasphemous here!

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289613

[michaelpg, 2014-08-07 08:17:08]

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/browser_...
File chrome/browser/browser_about_handler.cc (right):

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/browser_...
chrome/browser/browser_about_handler.cc:64: host =
chrome::kChromeUISettingsFrameHost;
So, this makes chrome://settings load the settings frame standalone, but the URL
in the omnibox changes back to "chrome://settings-frame" when you update the
history state. This doesn't really matter since the page should be opened in a
window, but I'm not sure why we can't just make "chrome://settings" point to
what "chrome://settings-frame" normally points to.

[stevenjb, 2014-08-07 17:35:15]

One meta comment: In a followup we should move 	chrome/browser/ui/webui/help/ ->
	chrome/browser/ui/webui/about/ and rename the classes appropriately.

Otherwise, the cpp changes lgtm.

[michaelpg, 2014-08-07 19:33:49]

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/browser_...
File chrome/browser/browser_about_handler.cc (right):

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/browser_...
chrome/browser/browser_about_handler.cc:64: host =
chrome::kChromeUISettingsFrameHost;
On 2014/08/07 08:17:07, michaelpg wrote:
> So, this makes chrome://settings load the settings frame standalone, but the
URL
> in the omnibox changes back to "chrome://settings-frame" when you update the
> history state. This doesn't really matter since the page should be opened in a
> window, but I'm not sure why we can't just make "chrome://settings" point to
> what "chrome://settings-frame" normally points to.

I think I'm being dumb, since we're loading resources from
chrome://settings-frame in the options page we should keep the host as
kChromeUISettingsFrameHost as it is here. So ignore these comments I think.

[Dan Beam, 2014-08-07 22:36:17]

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/browser_...
File chrome/browser/browser_about_handler.cc (right):

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/browser_...
chrome/browser/browser_about_handler.cc:73: if (url->path().empty() ||
url->path() == std::string("/"))
why do you need std::string()?

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/resource...
File chrome/browser/resources/help/help_page.js (left):

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/resource...
chrome/browser/resources/help/help_page.js:117:
PageManager.showPageByName('channel-change-page', false);
?

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.css (right):

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.css:459: footer
button:nth-child(2) {
what are you actually targeting here?

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.css:461: margin-left: auto;
-webkit-margin-start: auto; maybe?

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/help_overlay_handler.cc (right):

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/help_overlay_handler.cc:24: IDS_ABOUT_TITLE);
  RegisterTitle(localized_strings, "aboutOverlay", IDS_ABOUT_TITLE);

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/options_ui.cc (right):

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/options_ui.cc:277:
AddOptionsPageUIHandler(localized_strings, new HelpOverlayHandler());
why isn't this added only on ChromeOS?

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/options_ui_browsertest.cc (right):

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/options_ui_browsertest.cc:158: #endif
#if defined(OS_CHROMEOS)
  bool should_navbar_exist = false;
#else
  bool should_navbar_exist = true;
#endif

https://codereview.chromium.org/449623003/diff/120001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/449623003/diff/120001/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:206: const char kDisableAboutInSettings[]      
  = "disable-about-in-settings";
line up = with others

[michaelpg, 2014-08-08 01:10:46]

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/browser_...
File chrome/browser/browser_about_handler.cc (right):

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/browser_...
chrome/browser/browser_about_handler.cc:73: if (url->path().empty() ||
url->path() == std::string("/"))
On 2014/08/07 22:36:16, Dan Beam wrote:
> why do you need std::string()?

Done.

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/resource...
File chrome/browser/resources/help/help_page.js (left):

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/resource...
chrome/browser/resources/help/help_page.js:117:
PageManager.showPageByName('channel-change-page', false);
On 2014/08/07 22:36:16, Dan Beam wrote:
> ?

the default for updateHistory is true. I don't see why this should be using
false.

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.css (right):

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.css:459: footer
button:nth-child(2) {
On 2014/08/07 22:36:16, Dan Beam wrote:
> what are you actually targeting here?

#version-button, the "Version [blah]" link which is the actual button to open
the overlay. see
https://folio.googleplex.com/tbuckley/chromeos/Settings/about/v2#%2Fsettings_...

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.css:461: margin-left: auto;
On 2014/08/07 22:36:16, Dan Beam wrote:
> -webkit-margin-start: auto; maybe?

Done.

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/help_overlay_handler.cc (right):

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/help_overlay_handler.cc:24: IDS_ABOUT_TITLE);
On 2014/08/07 22:36:16, Dan Beam wrote:
>   RegisterTitle(localized_strings, "aboutOverlay", IDS_ABOUT_TITLE);

Done.

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/options_ui.cc (right):

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/options_ui.cc:277:
AddOptionsPageUIHandler(localized_strings, new HelpOverlayHandler());
On 2014/08/07 22:36:16, Dan Beam wrote:
> why isn't this added only on ChromeOS?

Done. (The only other thing to mention is I intentionally didn't put it in
options/chromeos, because there's nothing chrome-os-specific about it other than
that we happen to only want to do this on chrome os right now.)

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/options_ui_browsertest.cc (right):

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/options_ui_browsertest.cc:158: #endif
On 2014/08/07 22:36:16, Dan Beam wrote:
> #if defined(OS_CHROMEOS)
>   bool should_navbar_exist = false;
> #else
>   bool should_navbar_exist = true;
> #endif

Done.

https://codereview.chromium.org/449623003/diff/120001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/449623003/diff/120001/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:206: const char kDisableAboutInSettings[]      
  = "disable-about-in-settings";
On 2014/08/07 22:36:16, Dan Beam wrote:
> line up = with others

Done, forgot to re-align when I changed it from "enable" to "disable"

[Dan Beam, 2014-08-08 02:39:13]

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/resource...
File chrome/browser/resources/help/help_page.js (left):

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/resource...
chrome/browser/resources/help/help_page.js:117:
PageManager.showPageByName('channel-change-page', false);
On 2014/08/08 01:10:46, michaelpg wrote:
> On 2014/08/07 22:36:16, Dan Beam wrote:
> > ?
> 
> the default for updateHistory is true. I don't see why this should be using
> false. 

sometimes when showing dialogs we *don't* want a history entry (e.g. a confirm
dialog for a contextual action that can't be undone)

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/options_ui.cc (right):

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/options_ui.cc:277:
AddOptionsPageUIHandler(localized_strings, new HelpOverlayHandler());
On 2014/08/08 01:10:46, michaelpg wrote:
> On 2014/08/07 22:36:16, Dan Beam wrote:
> > why isn't this added only on ChromeOS?
> 
> Done. (The only other thing to mention is I intentionally didn't put it in
> options/chromeos, because there's nothing chrome-os-specific about it other
than
> that we happen to only want to do this on chrome os right now.)

are there immediate plans to use any of the "about page in settings" code
outside of chromeos?

[stevenjb, 2014-08-08 16:41:32]

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/options_ui.cc (right):

https://codereview.chromium.org/449623003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/options_ui.cc:277:
AddOptionsPageUIHandler(localized_strings, new HelpOverlayHandler());
On 2014/08/08 02:39:13, Dan Beam wrote:
> On 2014/08/08 01:10:46, michaelpg wrote:
> > On 2014/08/07 22:36:16, Dan Beam wrote:
> > > why isn't this added only on ChromeOS?
> > 
> > Done. (The only other thing to mention is I intentionally didn't put it in
> > options/chromeos, because there's nothing chrome-os-specific about it other
> than
> > that we happen to only want to do this on chrome os right now.)
> 
> are there immediate plans to use any of the "about page in settings" code
> outside of chromeos?

The plan of record is to try settings-in-a-window (with About integrated as per
this CL) on Chrome OS, then if we like it to enable it on other platforms as
well, probably in 39 or 40.

[Ben Goodger (Google), 2014-08-08 22:59:36]

I don't see chrome_pages.cc in this CL?

[michaelpg, 2014-08-08 23:04:07]

On 2014/08/08 22:59:36, Ben Goodger (Google) wrote:
> I don't see chrome_pages.cc in this CL?

Ohh that's right, sorry, I had thought about modifying GetSettingsUrl() but
didn't.

[Ben Goodger (Google), 2014-08-11 18:30:12]

chrome_browser_about_handler lgtm

[michaelpg, 2014-08-11 20:39:33]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2014-08-11 20:39:38]

The CQ bit was unchecked by michaelpg@chromium.org

[michaelpg, 2014-08-11 20:41:09]

Thanks, Ben.

Dan, how is this looking? I had to make some changes to fix tests including
making the channel selector radio buttons more uniform with other settings
overlays.

[Dan Beam, 2014-08-13 02:50:47]

if you make smaller, more concentrated CLs you will get faster reviews

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/resource...
File chrome/browser/resources/help/channel_change_page.css (right):

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/resource...
chrome/browser/resources/help/channel_change_page.css:17: margin: 0 25px;
why are you making design changes in this CL?

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/resource...
File chrome/browser/resources/help/help_page.js (left):

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/resource...
chrome/browser/resources/help/help_page.js:117:
PageManager.showPageByName('channel-change-page', false);
i'd do this separately

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/resource...
File chrome/browser/resources/help/help_page.js (right):

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/resource...
chrome/browser/resources/help/help_page.js:18: title =
loadTimeData.getString('aboutTitle');
nit:

  var id = loadTimeData.valueExists('aboutOverlayTabTitle') ?
      'aboutOverlayTabTitle' : 'aboutTitle';
  Page.call(this, 'help', loadTimeData.getString(id), 'help-page');

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/resource...
File chrome/browser/resources/options/options_bundle.js (right):

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/resource...
chrome/browser/resources/options/options_bundle.js:53: var ChannelChangePage =
help.ChannelChangePage;
this is a silly practice and i'd avoid adding more if possible

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/help/help_handler.h (right):

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/help_handler.h:32: static void
GetLocalizedValues(base::DictionaryValue* localized_strings);
Y U NO FORWARD DECLARE DictionaryValue?

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/help/help_utils.cc (right):

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/help_utils.cc:12: #if defined(OS_CHROMEOS)
i doesn't make a ton of sense to me to rename from help_utils_chromeos =>
help_utils then add a ton of #ifdef soup.

either way, probably better in a separate CL.

https://codereview.chromium.org/449623003/diff/160001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/page_manager/page.js (right):

https://codereview.chromium.org/449623003/diff/160001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page.js:71: didShowPage: function() {},
i'd do this separately

https://codereview.chromium.org/449623003/diff/160001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/page_manager/page_manager.js (right):

https://codereview.chromium.org/449623003/diff/160001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:196: if (page.visible
&& name != pageName &&
why did you add this visibility check?

[michaelpg, 2014-08-13 08:12:11]

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/resource...
File chrome/browser/resources/help/channel_change_page.css (right):

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/resource...
chrome/browser/resources/help/channel_change_page.css:17: margin: 0 25px;
On 2014/08/13 02:50:46, Dan Beam wrote:
> why are you making design changes in this CL?

SettingsFormatWebUITest.RadioCheckboxStyleCheck requires an ancestor div have
the "radio" class for consistent formatting. But I'll just add an exemption to
that test instead.

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/resource...
File chrome/browser/resources/help/help_page.js (right):

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/resource...
chrome/browser/resources/help/help_page.js:18: title =
loadTimeData.getString('aboutTitle');
On 2014/08/13 02:50:46, Dan Beam wrote:
> nit:
> 
>   var id = loadTimeData.valueExists('aboutOverlayTabTitle') ?
>       'aboutOverlayTabTitle' : 'aboutTitle';
>   Page.call(this, 'help', loadTimeData.getString(id), 'help-page');

Done.

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/resource...
File chrome/browser/resources/options/options_bundle.js (right):

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/resource...
chrome/browser/resources/options/options_bundle.js:53: var ChannelChangePage =
help.ChannelChangePage;
On 2014/08/13 02:50:46, Dan Beam wrote:
> this is a silly practice and i'd avoid adding more if possible

Done.

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/help/help_handler.h (right):

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/help_handler.h:32: static void
GetLocalizedValues(base::DictionaryValue* localized_strings);
On 2014/08/13 02:50:46, Dan Beam wrote:
> Y U NO FORWARD DECLARE DictionaryValue?

let me know if this is overboard or correct

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/help/help_utils.cc (right):

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/help_utils.cc:12: #if defined(OS_CHROMEOS)
On 2014/08/13 02:50:46, Dan Beam wrote:
> i doesn't make a ton of sense to me to rename from help_utils_chromeos =>
> help_utils then add a ton of #ifdef soup.
> 
> either way, probably better in a separate CL.

ok. This might as well go in help_utils_chromes anyway at this point.

https://codereview.chromium.org/449623003/diff/160001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/page_manager/page.js (right):

https://codereview.chromium.org/449623003/diff/160001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page.js:71: didShowPage: function() {},
On 2014/08/13 02:50:46, Dan Beam wrote:
> i'd do this separately

ok, removed.

https://codereview.chromium.org/449623003/diff/160001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/page_manager/page_manager.js (right):

https://codereview.chromium.org/449623003/diff/160001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:196: if (page.visible
&& name != pageName &&
On 2014/08/13 02:50:46, Dan Beam wrote:
> why did you add this visibility check?

because this doesn't match the comment: "notify pages if they will be hidden",
not "notify pages if they will be hidden or if they are already hidden". this
leads to workarounds where implementers have to check if the page is actually
visible, e.g.:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

i think this was also related to adding the willHidePage function that other
classes were "overriding", but I don't really remember how anymore. i'll leave
it out of this CL.

[Dan Beam, 2014-08-13 17:03:18]

lgtm w/comments addressed

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/resource...
File chrome/browser/resources/help/channel_change_page.css (right):

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/resource...
chrome/browser/resources/help/channel_change_page.css:17: margin: 0 25px;
On 2014/08/13 08:12:11, michaelpg wrote:
> On 2014/08/13 02:50:46, Dan Beam wrote:
> > why are you making design changes in this CL?
> 
> SettingsFormatWebUITest.RadioCheckboxStyleCheck requires an ancestor div have
> the "radio" class for consistent formatting. But I'll just add an exemption to
> that test instead.

i'm cool with it if that's why, just wanted to understand :)

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/resource...
File chrome/browser/resources/help/help_page.js (left):

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/resource...
chrome/browser/resources/help/help_page.js:117:
PageManager.showPageByName('channel-change-page', false);
On 2014/08/13 02:50:46, Dan Beam wrote:
> i'd do this separately

ping

https://codereview.chromium.org/449623003/diff/220001/chrome/browser/resource...
File chrome/browser/resources/help/help_page.js (right):

https://codereview.chromium.org/449623003/diff/220001/chrome/browser/resource...
chrome/browser/resources/help/help_page.js:150: this.toggleMoreInfo_();
nit: i'd make a setMoreInfoVisible_ and use it here and in toggleMoreInfo_

https://codereview.chromium.org/449623003/diff/220001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/help/help_handler.h (right):

https://codereview.chromium.org/449623003/diff/220001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/help_handler.h:31: }
don't forward OVERRIDE methods

https://codereview.chromium.org/449623003/diff/220001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/help_overlay_handler.h (right):

https://codereview.chromium.org/449623003/diff/220001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/help_overlay_handler.h:14: }
not required

https://codereview.chromium.org/449623003/diff/220001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/settings_format_browsertest.js (right):

https://codereview.chromium.org/449623003/diff/220001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/settings_format_browsertest.js:30:
'channel-change-page-stable-option'],
eh, just change the CSS like you did

[michaelpg, 2014-08-13 20:17:51]

this should address everything.

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/resource...
File chrome/browser/resources/help/help_page.js (left):

https://codereview.chromium.org/449623003/diff/160001/chrome/browser/resource...
chrome/browser/resources/help/help_page.js:117:
PageManager.showPageByName('channel-change-page', false);
On 2014/08/13 17:03:18, Dan Beam wrote:
> On 2014/08/13 02:50:46, Dan Beam wrote:
> > i'd do this separately
> 
> ping

Done.

https://codereview.chromium.org/449623003/diff/220001/chrome/browser/resource...
File chrome/browser/resources/help/help_page.js (right):

https://codereview.chromium.org/449623003/diff/220001/chrome/browser/resource...
chrome/browser/resources/help/help_page.js:150: this.toggleMoreInfo_();
On 2014/08/13 17:03:18, Dan Beam wrote:
> nit: i'd make a setMoreInfoVisible_ and use it here and in toggleMoreInfo_

Done.

https://codereview.chromium.org/449623003/diff/220001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/help/help_handler.h (right):

https://codereview.chromium.org/449623003/diff/220001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/help_handler.h:31: }
On 2014/08/13 17:03:18, Dan Beam wrote:
> don't forward OVERRIDE methods

Done.

https://codereview.chromium.org/449623003/diff/220001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/help_overlay_handler.h (right):

https://codereview.chromium.org/449623003/diff/220001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/help_overlay_handler.h:14: }
On 2014/08/13 17:03:18, Dan Beam wrote:
> not required

Done.

https://codereview.chromium.org/449623003/diff/220001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/settings_format_browsertest.js (right):

https://codereview.chromium.org/449623003/diff/220001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/settings_format_browsertest.js:30:
'channel-change-page-stable-option'],
On 2014/08/13 17:03:18, Dan Beam wrote:
> eh, just change the CSS like you did

Done.

[michaelpg, 2014-08-13 21:06:38]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-08-13 21:08:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaelpg@chromium.org/449623003/260002

[Dan Beam, 2014-08-13 22:26:18]

slgtm

https://codereview.chromium.org/449623003/diff/260002/chrome/browser/resource...
File chrome/browser/resources/help/help_page.js (right):

https://codereview.chromium.org/449623003/diff/260002/chrome/browser/resource...
chrome/browser/resources/help/help_page.js:162: }
if (visible == moreInfo.classList.contains('visible'))
  return;

https://codereview.chromium.org/449623003/diff/260002/chrome/browser/resource...
chrome/browser/resources/help/help_page.js:167:
moreInfo.classList.remove('visible');
moreInfo.classList.toggle('visible', visible);

https://codereview.chromium.org/449623003/diff/260002/chrome/browser/resource...
chrome/browser/resources/help/help_page.js:183:
this.setMoreInfoVisible_(!visible);
nit:
      this.setMoreInfoVisible_(!moreInfo.classList.contains('visible'));

[michaelpg, 2014-08-13 22:29:07]

The CQ bit was unchecked by michaelpg@chromium.org

[michaelpg, 2014-08-14 07:00:54]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-08-14 07:05:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaelpg@chromium.org/449623003/290001

[michaelpg, 2014-08-14 11:42:49]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2014-08-14 11:43:56]

https://codereview.chromium.org/449623003/diff/260002/chrome/browser/resource...
File chrome/browser/resources/help/help_page.js (right):

https://codereview.chromium.org/449623003/diff/260002/chrome/browser/resource...
chrome/browser/resources/help/help_page.js:162: }
On 2014/08/13 22:26:18, Dan Beam wrote:
> if (visible == moreInfo.classList.contains('visible'))
>   return;

Done.

https://codereview.chromium.org/449623003/diff/260002/chrome/browser/resource...
chrome/browser/resources/help/help_page.js:167:
moreInfo.classList.remove('visible');
On 2014/08/13 22:26:18, Dan Beam wrote:
> moreInfo.classList.toggle('visible', visible);

Done.

https://codereview.chromium.org/449623003/diff/260002/chrome/browser/resource...
chrome/browser/resources/help/help_page.js:183:
this.setMoreInfoVisible_(!visible);
On 2014/08/13 22:26:18, Dan Beam wrote:
> nit:
>       this.setMoreInfoVisible_(!moreInfo.classList.contains('visible'));

Done.

[commit-bot: I haz the power, 2014-08-14 11:46:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaelpg@chromium.org/449623003/310001

[commit-bot: I haz the power, 2014-08-14 16:51:33]

Committed patchset #6 (310001) as 289613