Use PageManager in About page

Use PageManager in About page

This creates a PageManager.Observer class which delegates handling of
page history and title changes to an uber page observer:
- page_manager.js
- uber_page_manager_observer.js

The About page content is separated into its own file so in a future CL
it can be used as an overlay on another page.

The custom overlay implementation in help_base_page.js is no longer needed.

R=dbeam@chromium.org
BUG=313244
TBR=ben@chromium.org (for c/b/browser_resources.grd)

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

[michaelpg, 2014-08-04 21:41:18]

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/help/help_content.css (right):

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
chrome/browser/resources/help/help_content.css:7: align-items: center;
as requested by presubmit script. Oddly this moves the chromium icon in
about:help down 1 or 2 pixels, but I think it's centered better that way.

[stevenjb, 2014-08-05 21:40:22]

Nice. lgtm, but definitely wait for Dan's review :)

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/help/help.js (right):

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
chrome/browser/resources/help/help.js:31: PageManager.showPageByName(pageName,
true, {replaceState: true});
nit: Document 'true' with a comment or well named local. I realize we don't do
this consistently in JS, but we should.

[Dan Beam, 2014-08-06 18:30:55]

+estade@ as FYI

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/help/help_page.js (right):

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
chrome/browser/resources/help/help_page.js:148: if
($('more-info-container').className == 'visible')
nit: $('more-info-container').classList.contains('visible') if possible

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
chrome/browser/resources/help/help_page.js:329: * @param {boolean} endOfTheLine
Train has rolled into the station.
Whether the train has rolled into the station.

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/alert_overlay.js (right):

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/alert_overlay.js:11: * Encapsulated handling of
a generic alert.
nit: @extends {cr.ui.pageManager.Page}

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/options.js (right):

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/options.js:230: uber.onContentFrameLoaded();
why are these calls here and not at the bottom of the method (for example)?

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/uber/uber_page_manager_observer.js (right):

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
chrome/browser/resources/uber/uber_page_manager_observer.js:25: if
(page.pageDiv.hidden)
can't you ask if page.visible?

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
chrome/browser/resources/uber/uber_page_manager_observer.js:50:
historyFunction.call(uber, {}, path);
why are you using |uber| as the thisArg?

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/help_overlay_handler.cc (right):

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/help_overlay_handler.cc:28: if
(!::switches::SettingsWindowEnabled())
why are we getting here without SettingWindowEnabled() returning true?

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/help_overlay_handler.cc:31:
web_ui()->AddMessageHandler(help_handler);
if (::switches::SettingsWindowEnabled())
  web_ui()->AddMessageHandler(new HelpHandler());

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/help_overlay_handler.h (right):

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/help_overlay_handler.h:7: 
#include "base/basictypes.h"
#include "base/macros.h"

https://codereview.chromium.org/423533003/diff/20001/ui/webui/resources/js/cr...
File ui/webui/resources/js/cr/ui/page_manager/page_manager.js (right):

https://codereview.chromium.org/423533003/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:587:
this.observers_[i].updateTitle(page.title);
nit: curlies

https://codereview.chromium.org/423533003/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:623:
this.observers_[i].updateHistory(newPath, replace);
nit: curlies

https://codereview.chromium.org/423533003/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:714: * @interface
ident off

[michaelpg, 2014-08-06 21:58:19]

thanks, comments addressed.

my client is complaining that browser_resources.grd doesn't have an owner
reviewer -- does it actually need one? I thought it doesn't, but I can't find
that rule now.

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/help/help.js (right):

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
chrome/browser/resources/help/help.js:31: PageManager.showPageByName(pageName,
true, {replaceState: true});
On 2014/08/05 21:40:22, stevenjb wrote:
> nit: Document 'true' with a comment or well named local. I realize we don't do
> this consistently in JS, but we should.

Done.

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/help/help_page.js (right):

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
chrome/browser/resources/help/help_page.js:148: if
($('more-info-container').className == 'visible')
On 2014/08/06 18:30:54, Dan Beam wrote:
> nit: $('more-info-container').classList.contains('visible') if possible

Done.

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
chrome/browser/resources/help/help_page.js:329: * @param {boolean} endOfTheLine
Train has rolled into the station.
On 2014/08/06 18:30:54, Dan Beam wrote:
> Whether the train has rolled into the station.

Done.

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/alert_overlay.js (right):

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/alert_overlay.js:11: * Encapsulated handling of
a generic alert.
On 2014/08/06 18:30:54, Dan Beam wrote:
> nit: @extends {cr.ui.pageManager.Page}

Done.

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/options.js (right):

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/options.js:230: uber.onContentFrameLoaded();
On 2014/08/06 18:30:54, Dan Beam wrote:
> why are these calls here and not at the bottom of the method (for example)?

i've moved these around a bit:

PageManager.initialize has to happen before PageManager.showPageByName, and we
want to add the observer before we start navigating to our given path.

uber.onContentFrameLoaded should actually happen later I think, after
showPageByName has completed, so it registers the correct scroll status.

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/uber/uber_page_manager_observer.js (right):

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
chrome/browser/resources/uber/uber_page_manager_observer.js:25: if
(page.pageDiv.hidden)
On 2014/08/06 18:30:54, Dan Beam wrote:
> can't you ask if page.visible?

Done.

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/resources...
chrome/browser/resources/uber/uber_page_manager_observer.js:50:
historyFunction.call(uber, {}, path);
On 2014/08/06 18:30:54, Dan Beam wrote:
> why are you using |uber| as the thisArg?

just copypasta. can just be historyFunction({}, path)

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/help_overlay_handler.cc (right):

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/help_overlay_handler.cc:28: if
(!::switches::SettingsWindowEnabled())
On 2014/08/06 18:30:54, Dan Beam wrote:
> why are we getting here without SettingWindowEnabled() returning true?

This will be the next CL, but to answer your question, including the help
overlay page in settings means we have to register the localized values, so this
handler has to be added. It would be nice if we could conditionally include the
help overlay HTML so we wouldn't have to localize (or include) a page when we
won't show it -- is that possible?

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/help_overlay_handler.h (right):

https://codereview.chromium.org/423533003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/help_overlay_handler.h:7: 
On 2014/08/06 18:30:55, Dan Beam wrote:
> #include "base/basictypes.h"
> #include "base/macros.h"

Oops, these files should not be part of this CL, they aren't even being built.
This is Part 3, actually showing the overlay in Settings.

https://codereview.chromium.org/423533003/diff/20001/ui/webui/resources/js/cr...
File ui/webui/resources/js/cr/ui/page_manager/page_manager.js (right):

https://codereview.chromium.org/423533003/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:587:
this.observers_[i].updateTitle(page.title);
On 2014/08/06 18:30:55, Dan Beam wrote:
> nit: curlies

Done.

https://codereview.chromium.org/423533003/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:623:
this.observers_[i].updateHistory(newPath, replace);
On 2014/08/06 18:30:55, Dan Beam wrote:
> nit: curlies

Done.

https://codereview.chromium.org/423533003/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:714: * @interface
On 2014/08/06 18:30:55, Dan Beam wrote:
> ident off

Done.

[stevenjb, 2014-08-06 22:12:17]

We have .grd exceptions for ash/, but not for chrome/. If it is a small change
like this and has been reviewed by owners of the affected components, it is
reasonable to TBR it. I double checked those changes and they lgtm.

[Dan Beam, 2014-08-06 22:42:18]

lgtm

[michaelpg, 2014-08-06 23:20:07]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-08-06 23:23:02]

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

[michaelpg, 2014-08-07 01:03:55]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-08-07 01:06:07]

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

[commit-bot: I haz the power, 2014-08-07 07:29:10]

Change committed as 287989