Split OptionsPage into Page and PageManager

Split OptionsPage into Page and PageManager.

This is the first step toward consolidating OptionsPage with similar code
in HelpBasePage.

I know this looks huge, but it's mostly renames -- the only changes
that aren't trivial are in:
* c/b/resources/options/options_page.js
* ui/webui/resources/js/cr/ui/page_helper/page.js
* ui/webui/resources/js/cr/ui/page_helper/page_manager.js

==========

This CL moves parts of OptionsPage to the new PageManager and Page objects.
These changes are essentally renames that shouldn't affect functionality.

cr.ui.pageManager.PageManager maintains a list of pages & overlays and can
show and hide overlays, scroll around as needed, etc. Basically what the
OptionsPage "static" members do for overlays.

cr.ui.pageManager.Page is basically the same as the OptionsPage prototype,
the essential attributes of each page/overlay. These will be managed by
the PageManager. Overlays and root pages like BrowserOptions will now descend
from Page.

These may not be the best names, and I don't know if cr.ui as at all the best
place for these classes -- let me know if you object or have better ideas.
Thanks.

==========

Future work:

* Move some logic back to OptionsPage by way of a handler to make
Page and PageManager independent of OptionsPage (e.g., for uber events,
history, and so forth)
* Transition the About page to use Page and PageManager and remove the
old code.
* Separate out bubble logic from general page manipulation.

==========

R=dbeam@chromium.org,stevenjb@chromium.org,

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

[michaelpg, 2014-07-24 00:34:33]

Dan, could you take a look and let me know if there's anyone else you want on
the review?

Thanks,
Michael

https://codereview.chromium.org/410293004/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/alert_overlay.js (left):

https://codereview.chromium.org/410293004/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/alert_overlay.js:34: // Call base class
implementation to start preference initialization.
There are a lot of these comments, but OptionsPage.prototype.initializePage was
an empty function (as Page.prototype.initializePage is now).

[Dan Beam, 2014-07-28 20:58:00]

i think the general idea of moving cross-page functionality into a manager is
good, let's just keep honing the details

https://chromiumcodereview.appspot.com/410293004/diff/60001/chrome/browser/re...
File chrome/browser/resources/options/options_page.js (right):

https://chromiumcodereview.appspot.com/410293004/diff/60001/chrome/browser/re...
chrome/browser/resources/options/options_page.js:18: * Gets the default page (to
be shown on initial load).
@return

https://chromiumcodereview.appspot.com/410293004/diff/60001/chrome/browser/re...
chrome/browser/resources/options/options_page.js:18: * Gets the default page (to
be shown on initial load).
@override

https://chromiumcodereview.appspot.com/410293004/diff/60001/chrome/browser/re...
chrome/browser/resources/options/options_page.js:58: * Initializes the complete
options page.  This will cause all C++ handlers
nit: \s\s => \s

https://chromiumcodereview.appspot.com/410293004/diff/60001/chrome/browser/re...
chrome/browser/resources/options/options_page.js:64:
PageManager.initialize.call(PageManager);
can we call the super first?

https://chromiumcodereview.appspot.com/410293004/diff/60001/chrome/browser/re...
chrome/browser/resources/options/options_page.js:66: 
doc on all these static methods

https://chromiumcodereview.appspot.com/410293004/diff/60001/chrome/browser/re...
chrome/browser/resources/options/options_page.js:89: setIsSettingsApp:
function() {
nit: it makes very little sense to me for a set* method not to take an argument.
 maybe rename enableSettings() or

  setIsSettingsApp: function(isSettings) {
    document.documentElement.classList.toggle('settings-app', isSettings);
  },

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
File ui/webui/resources/js/cr/ui/page_manager/page.js (left):

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page.js:558:
OptionsPage.reverseButtonStripIfNecessary_ = function(overlay) {
this should probably stay on Page

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
File ui/webui/resources/js/cr/ui/page_manager/page.js (right):

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page.js:133: if (visible)
needs curlies

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page.js:134: // TODO(michaelpg@):
Remove dependency on uber (crbug.com/313244).
TODO(michaelpg)

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page.js:134: // TODO(michaelpg@):
Remove dependency on uber (crbug.com/313244).
indent off

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page.js:214:
uber.invokeMethodOnParent('stopInterceptingEvents');
probably easy enough to make fadeCompleted protected and @override in pages that
need this?

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page.js:226: onVisibilityChanged_:
function() {
why not put all this functionality in the manager?  you're just calling
PageManager.* and using Page's public API anyway...

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page.js:230:
PageManager.updateScrollPosition_();
why is this class calling a private method on another class?

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page.js:237: get nestingLevel() {
this should arguably only be done by the manager as well.  pages are allowed to
know about other pages' attributes (e.g. parent) but in general should be
unaware of their surroundings as much as possible.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page.js:262: isAncestorOfPage:
function(page) {
move to page manager

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
File ui/webui/resources/js/cr/ui/page_manager/page_manager.js (right):

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:41: return
options.BrowserOptions.getInstance();
this should probably be abstract (e.g. just assert(false, 'implement me') or
something)

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:67: * @private
this isn't private

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:186: //
TODO(michaelpg@): Remove dependency on uber (crbug.com/313244).
same thing regarding uber deps: you should be able to derive this class, or if
you hate deriving a concrete class (reasonable), perhaps set up an observer
interface that the uber-specific code can implement

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:226: * @return
{boolean} whether we showed an overlay.
all the methods that end with _ => @private

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:684: },
arguably these methods should be in public => protected => private order

[stevenjb, 2014-07-28 22:27:23]

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
File ui/webui/resources/js/cr/ui/page_manager/page.js (right):

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page.js:9: * Base class for page.
Let's put a better description here, i.e. what is a 'Page'.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
File ui/webui/resources/js/cr/ui/page_manager/page_manager.js (right):

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:4: 
We should provide some sort of top level comment describing the purpose of
PageManager.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:13: * TODO(michaelpg@):
Remove dependency on uber menu (crbug.com/313244).
TODO(michaelpg)

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:57:
this.showPageByName(pageName, true);
JS novice question: Could this be done by giving updateHistory in showPageByName
a default value of 'true' instead of having two public methods that do the same
thing?

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:73: opt_propertyBag =
opt_propertyBag || {};
JS novice question: Is this preferable to declaring opt_properyBag = {}?

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:97: } else {
nit: no else after return

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:155: continue;
nit: We use the same pattern 3 times here, maybe create a helper that takes
function(page) { ... } ?

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:186: //
TODO(michaelpg@): Remove dependency on uber (crbug.com/313244).
TODO(michaelpg)

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:217: //
TODO(michaelpg@): Remove dependency on uber (crbug.com/313244).
TODO(michaelpg)

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:246: if
(overlay.didShowPage) overlay.didShowPage();
nit: two lines? (Or is that a C++ only thing?)

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:313: if
(overlay.didClosePage) overlay.didClosePage();
two lines?

[michaelpg, 2014-07-29 01:08:31]

I've moved some methods around and fixed a bunch of comment/public/private
stuff. I don't want to make too many changes to the original OptionsPage code in
addition to this big refactor, but I cleaned up the things that were pointed
out.

One thing to note: OptionsPage is just an Object, it isn't derived from
PageManager. The idea is for it to use PageManager and in a future CL add an
observer interface for OptionsPage to implement.

Thanks!

https://chromiumcodereview.appspot.com/410293004/diff/60001/chrome/browser/re...
File chrome/browser/resources/options/options_page.js (right):

https://chromiumcodereview.appspot.com/410293004/diff/60001/chrome/browser/re...
chrome/browser/resources/options/options_page.js:18: * Gets the default page (to
be shown on initial load).
On 2014/07/28 20:57:58, Dan Beam wrote:
> @return

Getting rid of this by making the default page a parameter of
PageManager.initialize().

https://chromiumcodereview.appspot.com/410293004/diff/60001/chrome/browser/re...
chrome/browser/resources/options/options_page.js:58: * Initializes the complete
options page.  This will cause all C++ handlers
On 2014/07/28 20:57:58, Dan Beam wrote:
> nit: \s\s => \s

Done.

https://chromiumcodereview.appspot.com/410293004/diff/60001/chrome/browser/re...
chrome/browser/resources/options/options_page.js:64:
PageManager.initialize.call(PageManager);
On 2014/07/28 20:57:58, Dan Beam wrote:
> can we call the super first?

It's not a super. I'm not sure. The original OptionsPage calls
coreOptionsInitialize before doing everything that PageManager.initialize now
does. I'm reluctant to change that in case the order in which handlers and
overlays are initialized is important.

https://chromiumcodereview.appspot.com/410293004/diff/60001/chrome/browser/re...
chrome/browser/resources/options/options_page.js:66: 
On 2014/07/28 20:57:58, Dan Beam wrote:
> doc on all these static methods

Done.

https://chromiumcodereview.appspot.com/410293004/diff/60001/chrome/browser/re...
chrome/browser/resources/options/options_page.js:89: setIsSettingsApp:
function() {
On 2014/07/28 20:57:58, Dan Beam wrote:
> nit: it makes very little sense to me for a set* method not to take an
argument.
>  maybe rename enableSettings() or
> 
>   setIsSettingsApp: function(isSettings) {
>     document.documentElement.classList.toggle('settings-app', isSettings);
>   },

Done.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
File ui/webui/resources/js/cr/ui/page_manager/page.js (left):

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page.js:558:
OptionsPage.reverseButtonStripIfNecessary_ = function(overlay) {
On 2014/07/28 20:57:59, Dan Beam wrote:
> this should probably stay on Page

Made it a member function of Page.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
File ui/webui/resources/js/cr/ui/page_manager/page.js (right):

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page.js:9: * Base class for page.
On 2014/07/28 22:27:22, stevenjb wrote:
> Let's put a better description here, i.e. what is a 'Page'.

How's this?

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page.js:133: if (visible)
On 2014/07/28 20:57:59, Dan Beam wrote:
> needs curlies

Done.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page.js:134: // TODO(michaelpg@):
Remove dependency on uber (crbug.com/313244).
On 2014/07/28 20:57:59, Dan Beam wrote:
> TODO(michaelpg)

Done.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page.js:134: // TODO(michaelpg@):
Remove dependency on uber (crbug.com/313244).
On 2014/07/28 20:57:59, Dan Beam wrote:
> indent off

Done.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page.js:214:
uber.invokeMethodOnParent('stopInterceptingEvents');
On 2014/07/28 20:57:59, Dan Beam wrote:
> probably easy enough to make fadeCompleted protected and @override in pages
that
> need this?

not sure I agree. A page that represents an overlay may want to be includable
from OptionsPage or <some page that doesn't use uber>. We should only need
OptionsPage to worry about uber.foo(), but fadeCompleted_ won't be called on
OptionsPage. I think the right solution will be to notify the parent page that
an overlay is being shown/hidden, which can be propagated if needed up to the
OptionsPage. e.g.:

// @private
Page.prototype.fadeCompleted_ = function() {
  this.pageDiv.hidden = true; // etc....
  this.onOverlayClosed();
};

// @protected
Page.prototype.onOverlayClosed = function() {
  if (this.parentPage)
    this.parentPage.onOverlayClosed();
};

// @override
OptionsPage.onOverlayClosed = function() {
  uber.invokeBlah(/* .. */);
}

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page.js:226: onVisibilityChanged_:
function() {
On 2014/07/28 20:57:59, Dan Beam wrote:
> why not put all this functionality in the manager?  you're just calling
> PageManager.* and using Page's public API anyway...

Done.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page.js:230:
PageManager.updateScrollPosition_();
On 2014/07/28 20:57:59, Dan Beam wrote:
> why is this class calling a private method on another class?

yeah that's icky. moved to PageManager.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page.js:237: get nestingLevel() {
On 2014/07/28 20:57:59, Dan Beam wrote:
> this should arguably only be done by the manager as well.  pages are allowed
to
> know about other pages' attributes (e.g. parent) but in general should be
> unaware of their surroundings as much as possible.

Done.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page.js:262: isAncestorOfPage:
function(page) {
On 2014/07/28 20:57:59, Dan Beam wrote:
> move to page manager

Done.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
File ui/webui/resources/js/cr/ui/page_manager/page_manager.js (right):

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:4: 
On 2014/07/28 22:27:23, stevenjb wrote:
> We should provide some sort of top level comment describing the purpose of
> PageManager.

How's this?

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:13: * TODO(michaelpg@):
Remove dependency on uber menu (crbug.com/313244).
On 2014/07/28 22:27:23, stevenjb wrote:
> TODO(michaelpg)

Done.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:41: return
options.BrowserOptions.getInstance();
On 2014/07/28 20:57:59, Dan Beam wrote:
> this should probably be abstract (e.g. just assert(false, 'implement me') or
> something)

I'd rather not derive from PageManager, instead OptionsPage should add an
observer to PageManager which will provide this information as well.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:57:
this.showPageByName(pageName, true);
On 2014/07/28 22:27:22, stevenjb wrote:
> JS novice question: Could this be done by giving updateHistory in
showPageByName
> a default value of 'true' instead of having two public methods that do the
same
> thing?

Done.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:67: * @private
On 2014/07/28 20:58:00, Dan Beam wrote:
> this isn't private

Done.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:73: opt_propertyBag =
opt_propertyBag || {};
On 2014/07/28 22:27:23, stevenjb wrote:
> JS novice question: Is this preferable to declaring opt_properyBag = {}?

Not sure what you are asking exactly:

function(foo = {}) {} // SyntaxError

function(foo) {
  foo = foo || {};
  return foo;
}

foo(window); // window
foo(); // {}

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:97: } else {
On 2014/07/28 22:27:23, stevenjb wrote:
> nit: no else after return

Done.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:155: continue;
On 2014/07/28 22:27:22, stevenjb wrote:
> nit: We use the same pattern 3 times here, maybe create a helper that takes
> function(page) { ... } ?

Sure. But I don't want to stray too far from the original OptionsPage in this
CL. I've added a TODO for this.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:186: //
TODO(michaelpg@): Remove dependency on uber (crbug.com/313244).
On 2014/07/28 20:58:00, Dan Beam wrote:
> same thing regarding uber deps: you should be able to derive this class, or if
> you hate deriving a concrete class (reasonable), perhaps set up an observer
> interface that the uber-specific code can implement

Yes, setting up an observer interface was my plan. I just wanted to do that
after committing the initial split -- this PageManager is still somewhat
OptionsPage specific, which I don't think is a bad thing since it's a clear step
in the direction of the ultimate goal.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:186: //
TODO(michaelpg@): Remove dependency on uber (crbug.com/313244).
On 2014/07/28 22:27:23, stevenjb wrote:
> TODO(michaelpg)

Done.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:217: //
TODO(michaelpg@): Remove dependency on uber (crbug.com/313244).
On 2014/07/28 22:27:22, stevenjb wrote:
> TODO(michaelpg)

Done.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:226: * @return
{boolean} whether we showed an overlay.
On 2014/07/28 20:57:59, Dan Beam wrote:
> all the methods that end with _ => @private

Done.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:246: if
(overlay.didShowPage) overlay.didShowPage();
On 2014/07/28 22:27:23, stevenjb wrote:
> nit: two lines? (Or is that a C++ only thing?)

This isn't my code, but I think two lines is the correct style.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:313: if
(overlay.didClosePage) overlay.didClosePage();
On 2014/07/28 22:27:22, stevenjb wrote:
> two lines?

Done.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:684: },
On 2014/07/28 20:58:00, Dan Beam wrote:
> arguably these methods should be in public => protected => private order

Done. And I tried to arrange related methods together as a secondary sort.

[Dan Beam, 2014-07-30 01:00:03]

very close, overall like it

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
File ui/webui/resources/js/cr/ui/page_manager/page_manager.js (right):

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:73: opt_propertyBag =
opt_propertyBag || {};
On 2014/07/28 22:27:23, stevenjb wrote:
> JS novice question: Is this preferable to declaring opt_properyBag = {}?

  opt_propertyBag = opt_propertyBag || {};

simplifies code like this:

  if (opt_propertyBag && opt_propertyBag.someProperty)

to

  if (opt_propertyBag.someProperty)

as |opt_propertyBag| is guaranteed to be an object after that line.

https://chromiumcodereview.appspot.com/410293004/diff/60001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:313: if
(overlay.didClosePage) overlay.didClosePage();
On 2014/07/29 01:08:30, michaelpg wrote:
> On 2014/07/28 22:27:22, stevenjb wrote:
> > two lines?
> 
> Done.

for future reference, 2 lines is more common but 1 line is allowed

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
File chrome/browser/resources/chromeos/bluetooth_pair_device.js (right):

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
chrome/browser/resources/chromeos/bluetooth_pair_device.js:49:
FakeBluetoothOverlayParent.getInstance());
indent off

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
File chrome/browser/resources/chromeos/fake_bluetooth_overlay_parent.js (right):

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
chrome/browser/resources/chromeos/fake_bluetooth_overlay_parent.js:29: },
in practice L22-29 is totally useless.  redefining initializePage (so this
method comes up first in the prototype chain) just to call the super's version
of this is useless.

additionally, a doc comment saying "Initializes * page" is also pretty useless
and should actually just be

  /** @override */

i've cleaned all these up here:
https://chromiumcodereview.appspot.com/426723004/

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js
(right):

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:169:
PageManager.showPageByName('bluetoothPairing', false);
how is this finding the unprefixed name (e.g. no options. or cr.ui.)?

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
File chrome/browser/resources/options/alert_overlay.js (right):

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
chrome/browser/resources/options/alert_overlay.js:57: if (this.okCallback !=
undefined) {
opt nit: no curlies, no comparison to undefined

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
File chrome/browser/resources/options/certificate_edit_ca_trust_overlay.js
(right):

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
chrome/browser/resources/options/certificate_edit_ca_trust_overlay.js:17:
'certificateEditCaTrustOverlay');
nit: this super call (and many others) could be less vertical lines

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
File chrome/browser/resources/options/certificate_import_error_overlay.js
(right):

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
chrome/browser/resources/options/certificate_import_error_overlay.js:6: 
nit: remove \n

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
File chrome/browser/resources/options/chromeos/change_picture_options.js
(right):

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
chrome/browser/resources/options/chromeos/change_picture_options.js:6: 
nit: remove \n

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
File chrome/browser/resources/options/factory_reset_overlay.js (right):

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
chrome/browser/resources/options/factory_reset_overlay.js:16:
loadTimeData.getString('factoryResetTitle'),
indent off

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
File chrome/browser/resources/options/geolocation_options.js (right):

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
chrome/browser/resources/options/geolocation_options.js:15: Page.call(this,
:(

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
File chrome/browser/resources/options/language_dictionary_overlay.js (right):

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
chrome/browser/resources/options/language_dictionary_overlay.js:17:
Page.call(this, 'editDictionary',
:'(

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
File chrome/browser/resources/options/options_page.js (right):

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
chrome/browser/resources/options/options_page.js:92: *     app launcher settings
"app".
no continuation indent

https://chromiumcodereview.appspot.com/410293004/diff/100001/chrome/browser/r...
chrome/browser/resources/options/options_page.js:101: *     for the app launcher
settings "app".
nor here

https://chromiumcodereview.appspot.com/410293004/diff/100001/ui/webui/resourc...
File ui/webui/resources/js/cr/ui/page_manager/page.js (right):

https://chromiumcodereview.appspot.com/410293004/diff/100001/ui/webui/resourc...
ui/webui/resources/js/cr/ui/page_manager/page.js:90:
reverseButtonStripIfNecessary: function() {
s/IfNecessary// as the manager now decides if this is necessary

https://chromiumcodereview.appspot.com/410293004/diff/100001/ui/webui/resourc...
File ui/webui/resources/js/cr/ui/page_manager/page_manager.js (right):

https://chromiumcodereview.appspot.com/410293004/diff/100001/ui/webui/resourc...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:34: * @type
{Array.Page}
!Object.<!Page> (you don't need to get crazy with the ! yet, though)

https://chromiumcodereview.appspot.com/410293004/diff/100001/ui/webui/resourc...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:48: * @param {Page}
defaultPage The page to be shown when no page is specified
cr.ui.page_manager.Page or Page? (be consistent either way)

https://chromiumcodereview.appspot.com/410293004/diff/100001/ui/webui/resourc...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:141: opt_updateHistory
= opt_updateHistory !== false;
it's weird to have an undefined argument end up being used like `true`

https://chromiumcodereview.appspot.com/410293004/diff/100001/ui/webui/resourc...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:184: //
TODO(michaelpg): Resolve code duplication.
seems unlikely that you'll be able to do this if some behavior depends
willHidePage() being called before any pages are actually hidden

https://chromiumcodereview.appspot.com/410293004/diff/100001/ui/webui/resourc...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:252: *     level 0.
don't 4 \s indent doc continuations (but you /do/ add 4 \s for @jsdoc tags)

https://chromiumcodereview.appspot.com/410293004/diff/100001/ui/webui/resourc...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:276: isAncestorOfPage:
function(potential_ancestor, potential_descendent) {
nit: jsVarsLikeThis

https://chromiumcodereview.appspot.com/410293004/diff/100001/ui/webui/resourc...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:445:
this.horizontalOffset = value;
nit: i'd make a private horizontalOffset_ and potentially make
setHorizontalOffset() an ES5 setter instead

[michaelpg, 2014-07-30 21:42:20]

Some more cleanup.

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
File chrome/browser/resources/chromeos/bluetooth_pair_device.js (right):

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/bluetooth_pair_device.js:49:
FakeBluetoothOverlayParent.getInstance());
On 2014/07/30 01:00:01, Dan Beam wrote:
> indent off

Done.

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
File chrome/browser/resources/chromeos/fake_bluetooth_overlay_parent.js (right):

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/fake_bluetooth_overlay_parent.js:29: },
On 2014/07/30 01:00:01, Dan Beam wrote:
> in practice L22-29 is totally useless.  redefining initializePage (so this
> method comes up first in the prototype chain) just to call the super's version
> of this is useless.
> 
> additionally, a doc comment saying "Initializes * page" is also pretty useless
> and should actually just be
> 
>   /** @override */
> 
> i've cleaned all these up here:
> https://chromiumcodereview.appspot.com/426723004/

Thanks.

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js
(right):

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:169:
PageManager.showPageByName('bluetoothPairing', false);
On 2014/07/30 01:00:01, Dan Beam wrote:
> how is this finding the unprefixed name (e.g. no options. or cr.ui.)?

good question. apparently this is unused code and is being removed:
https://crrev.com/419673012/

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
File chrome/browser/resources/options/alert_overlay.js (right):

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/alert_overlay.js:57: if (this.okCallback !=
undefined) {
On 2014/07/30 01:00:01, Dan Beam wrote:
> opt nit: no curlies, no comparison to undefined

would rather not touch this, the CL is big enough already

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
File chrome/browser/resources/options/certificate_edit_ca_trust_overlay.js
(right):

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/certificate_edit_ca_trust_overlay.js:17:
'certificateEditCaTrustOverlay');
On 2014/07/30 01:00:01, Dan Beam wrote:
> nit: this super call (and many others) could be less vertical lines

Done, for most if not all of these calls.

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
File chrome/browser/resources/options/certificate_import_error_overlay.js
(right):

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/certificate_import_error_overlay.js:6: 
On 2014/07/30 01:00:01, Dan Beam wrote:
> nit: remove \n

Done.

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/change_picture_options.js
(right):

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/change_picture_options.js:6: 
On 2014/07/30 01:00:01, Dan Beam wrote:
> nit: remove \n

Done.

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
File chrome/browser/resources/options/factory_reset_overlay.js (right):

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/factory_reset_overlay.js:16:
loadTimeData.getString('factoryResetTitle'),
On 2014/07/30 01:00:01, Dan Beam wrote:
> indent off

Done.

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
File chrome/browser/resources/options/geolocation_options.js (right):

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/geolocation_options.js:15: Page.call(this,
On 2014/07/30 01:00:02, Dan Beam wrote:
> :(

:)

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
File chrome/browser/resources/options/language_dictionary_overlay.js (right):

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/language_dictionary_overlay.js:17:
Page.call(this, 'editDictionary',
On 2014/07/30 01:00:02, Dan Beam wrote:
> :'(

:-D

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
File chrome/browser/resources/options/options_page.js (right):

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/options_page.js:92: *     app launcher settings
"app".
On 2014/07/30 01:00:02, Dan Beam wrote:
> no continuation indent

Done.

https://codereview.chromium.org/410293004/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/options_page.js:101: *     for the app launcher
settings "app".
On 2014/07/30 01:00:02, Dan Beam wrote:
> nor here

Done.

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

https://codereview.chromium.org/410293004/diff/100001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page.js:90:
reverseButtonStripIfNecessary: function() {
On 2014/07/30 01:00:02, Dan Beam wrote:
> s/IfNecessary// as the manager now decides if this is necessary

Done.

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

https://codereview.chromium.org/410293004/diff/100001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:34: * @type
{Array.Page}
On 2014/07/30 01:00:02, Dan Beam wrote:
> !Object.<!Page> (you don't need to get crazy with the ! yet, though)

or this?
    !Object.<string, !Page>

https://codereview.chromium.org/410293004/diff/100001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:48: * @param {Page}
defaultPage The page to be shown when no page is specified
On 2014/07/30 01:00:02, Dan Beam wrote:
> cr.ui.page_manager.Page or Page? (be consistent either way)

Done. (fully qualified just to help show that Page isn't defined before
PageManager is defined).

https://codereview.chromium.org/410293004/diff/100001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:141: opt_updateHistory
= opt_updateHistory !== false;
On 2014/07/30 01:00:02, Dan Beam wrote:
> it's weird to have an undefined argument end up being used like `true`

I think it's fair to have a "default" value of true. It simplifies a lot of code
that called PageManager.navigateToPage('foo') =>
PageManager.showPageByName('foo') without having to add ", true" everywhere.
Mind if I keep it in?

https://codereview.chromium.org/410293004/diff/100001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:184: //
TODO(michaelpg): Resolve code duplication.
On 2014/07/30 01:00:02, Dan Beam wrote:
> seems unlikely that you'll be able to do this if some behavior depends
> willHidePage() being called before any pages are actually hidden

Yeah, but we can at least make a helper function that takes a "function(page)"
parameter.

https://codereview.chromium.org/410293004/diff/100001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:252: *     level 0.
On 2014/07/30 01:00:02, Dan Beam wrote:
> don't 4 \s indent doc continuations (but you /do/ add 4 \s for @jsdoc tags)

Done.

https://codereview.chromium.org/410293004/diff/100001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:276: isAncestorOfPage:
function(potential_ancestor, potential_descendent) {
On 2014/07/30 01:00:02, Dan Beam wrote:
> nit: jsVarsLikeThis

nitDone = true;

https://codereview.chromium.org/410293004/diff/100001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:445:
this.horizontalOffset = value;
On 2014/07/30 01:00:02, Dan Beam wrote:
> nit: i'd make a private horizontalOffset_ and potentially make
> setHorizontalOffset() an ES5 setter instead

I'll keep this as is for now: we'll probably want to adjust how this works
anyway when we remove the navigation frame in settings in a window on CrOS.

[stevenjb, 2014-07-30 22:36:22]

lgtm

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

https://codereview.chromium.org/410293004/diff/60001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:73: opt_propertyBag =
opt_propertyBag || {};
On 2014/07/30 01:00:01, Dan Beam wrote:
> On 2014/07/28 22:27:23, stevenjb wrote:
> > JS novice question: Is this preferable to declaring opt_properyBag = {}?
> 
>   opt_propertyBag = opt_propertyBag || {};
> 
> simplifies code like this:
> 
>   if (opt_propertyBag && opt_propertyBag.someProperty)
> 
> to
> 
>   if (opt_propertyBag.someProperty)
> 
> as |opt_propertyBag| is guaranteed to be an object after that line.

Nevermind. I somehow thought you could provide default values to JS in the
declaration instead of doing it this way.

[Dan Beam, 2014-07-31 02:21:33]

lg to me except for question regarding updateTitle_

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

https://codereview.chromium.org/410293004/diff/100001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:34: * @type
{Array.Page}
On 2014/07/30 21:42:20, michaelpg wrote:
> On 2014/07/30 01:00:02, Dan Beam wrote:
> > !Object.<!Page> (you don't need to get crazy with the ! yet, though)
> 
> or this?
>     !Object.<string, !Page>

there are no other keys than string in JS

https://codereview.chromium.org/410293004/diff/200001/chrome/browser/resource...
File chrome/browser/resources/options/options_page.js (right):

https://codereview.chromium.org/410293004/diff/200001/chrome/browser/resource...
chrome/browser/resources/options/options_page.js:24:
PageManager.initialize(BrowserOptions.getInstance());
eventually:

  PageManager.addObserver(this);

and make OptionsPage a class that implements a PageObserver interface with the
general code of:

  onVisibilityChanged: function(page) {
    if (page.nestingLevel == 1) {
      if (page.visible)
        uber.stopInterceptingEvents();
      else
        uber.stopInterceptingEvents();
    }
  }

which removes your dependencies on uber :D

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

https://codereview.chromium.org/410293004/diff/200001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page.js:5:
cr.define('cr.ui.pageManager', function() {
js namespace names are all over the place, so i guess cr.ui.pageManager is as
good as any...

https://codereview.chromium.org/410293004/diff/200001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page.js:161:
PageManager.onPageVisibilityChanged(this);
all these PageManager.* calls should probably something like

  if (this.delegate_)
    this.delegate_.onVisibilityChanged(this);

where |this.delegate_| is probably a PageManager in disguise (maybe with another
fancy interface)

https://codereview.chromium.org/410293004/diff/200001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page.js:282: if
(PageManager.getNestingLevel(this) == 1) {
can this be implemented in PageManager.onPageVisibilityChanged?

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

https://codereview.chromium.org/410293004/diff/200001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:71: 
observers_: [],

addObserver: function(observer) {
  observers_.push(observer);
},

https://codereview.chromium.org/410293004/diff/200001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:296:
this.updateScrollPosition_();
for (var i = 0; i < this.observers_.length; ++i) {
  this.observers_.onVisibilityChanged(page);
}

https://codereview.chromium.org/410293004/diff/200001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:593: while (page) {
why did you change this implementation?

[michaelpg, 2014-07-31 03:21:17]

Thanks Dan. Responses below.

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

https://codereview.chromium.org/410293004/diff/100001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:34: * @type
{Array.Page}
On 2014/07/31 02:21:32, Dan Beam wrote:
> On 2014/07/30 21:42:20, michaelpg wrote:
> > On 2014/07/30 01:00:02, Dan Beam wrote:
> > > !Object.<!Page> (you don't need to get crazy with the ! yet, though)
> > 
> > or this?
> >     !Object.<string, !Page>
> 
> there are no other keys than string in JS

Still looks like the correct syntax for closure.
https://developers.google.com/closure/compiler/docs/js-for-compiler#types

https://codereview.chromium.org/410293004/diff/200001/chrome/browser/resource...
File chrome/browser/resources/options/options_page.js (right):

https://codereview.chromium.org/410293004/diff/200001/chrome/browser/resource...
chrome/browser/resources/options/options_page.js:24:
PageManager.initialize(BrowserOptions.getInstance());
On 2014/07/31 02:21:32, Dan Beam wrote:
> eventually:
> 
>   PageManager.addObserver(this);
> 
> and make OptionsPage a class that implements a PageObserver interface with the
> general code of:
> 
>   onVisibilityChanged: function(page) {
>     if (page.nestingLevel == 1) {
>       if (page.visible)
>         uber.stopInterceptingEvents();
>       else
>         uber.stopInterceptingEvents();
>     }
>   }
> 
> which removes your dependencies on uber :D

Yup, that's the goal!

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

https://codereview.chromium.org/410293004/diff/200001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page.js:5:
cr.define('cr.ui.pageManager', function() {
On 2014/07/31 02:21:32, Dan Beam wrote:
> js namespace names are all over the place, so i guess cr.ui.pageManager is as
> good as any...

it looked like camelCase was the more common thing. i could also not namespace
this, but I don't want to claim "cr.ui.Page" because that seems so general.

https://codereview.chromium.org/410293004/diff/200001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page.js:282: if
(PageManager.getNestingLevel(this) == 1) {
On 2014/07/31 02:21:32, Dan Beam wrote:
> can this be implemented in PageManager.onPageVisibilityChanged?

I think the point of this is to stop intercepting events during the transition.
onPageVisibilityChanged will be called when the transition starts, so it's too
soon.

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

https://codereview.chromium.org/410293004/diff/200001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:71: 
On 2014/07/31 02:21:33, Dan Beam wrote:
> observers_: [],
> 
> addObserver: function(observer) {
>   observers_.push(observer);
> },

Saving for next CL. Reason being I want to see everything fit together (i.e. the
about page) before committing the interface.

https://codereview.chromium.org/410293004/diff/200001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:593: while (page) {
On 2014/07/31 02:21:33, Dan Beam wrote:
> why did you change this implementation?

this is mostly the same implementation. the difference is when loading some
overlays, such as internet details, which for whatever reason have null titles,
the settings page title is changed to "chrome://settings".

we should get localized strings for those title-less overlays, but in the
meantime keeping the parent's title is better than nothing. (we still have to
change the title even if page.title == null, e.g. when triggering a lateral move
from one overlay to another with a different ancestor.)

https://codereview.chromium.org/410293004/diff/200001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:694:
elementContainsPoint_: function(e, x, y) {
note to self: remove unused code

[Dan Beam, 2014-07-31 03:30:38]

lgtm

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

https://codereview.chromium.org/410293004/diff/100001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:34: * @type
{Array.Page}
On 2014/07/31 03:21:16, michaelpg wrote:
> On 2014/07/31 02:21:32, Dan Beam wrote:
> > On 2014/07/30 21:42:20, michaelpg wrote:
> > > On 2014/07/30 01:00:02, Dan Beam wrote:
> > > > !Object.<!Page> (you don't need to get crazy with the ! yet, though)
> > > 
> > > or this?
> > >     !Object.<string, !Page>
> > 
> > there are no other keys than string in JS
> 
> Still looks like the correct syntax for closure.
> https://developers.google.com/closure/compiler/docs/js-for-compiler#types

both work and both are ignored, annoyingly enough

https://codereview.chromium.org/410293004/diff/200001/chrome/browser/resource...
File chrome/browser/resources/options/options_page.js (right):

https://codereview.chromium.org/410293004/diff/200001/chrome/browser/resource...
chrome/browser/resources/options/options_page.js:24:
PageManager.initialize(BrowserOptions.getInstance());
On 2014/07/31 03:21:16, michaelpg wrote:
> On 2014/07/31 02:21:32, Dan Beam wrote:
> > eventually:
> > 
> >   PageManager.addObserver(this);
> > 
> > and make OptionsPage a class that implements a PageObserver interface with
the
> > general code of:
> > 
> >   onVisibilityChanged: function(page) {
> >     if (page.nestingLevel == 1) {
> >       if (page.visible)
> >         uber.stopInterceptingEvents();
> >       else
> >         uber.stopInterceptingEvents();
> >     }
> >   }
> > 
> > which removes your dependencies on uber :D
> 
> Yup, that's the goal!

w00t

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

https://codereview.chromium.org/410293004/diff/200001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:71: 
On 2014/07/31 03:21:17, michaelpg wrote:
> On 2014/07/31 02:21:33, Dan Beam wrote:
> > observers_: [],
> > 
> > addObserver: function(observer) {
> >   observers_.push(observer);
> > },
> 
> Saving for next CL. Reason being I want to see everything fit together (i.e.
the
> about page) before committing the interface.

yes, meant next CL

https://codereview.chromium.org/410293004/diff/200001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:593: while (page) {
On 2014/07/31 03:21:17, michaelpg wrote:
> On 2014/07/31 02:21:33, Dan Beam wrote:
> > why did you change this implementation?
> 
> this is mostly the same implementation. the difference is when loading some
> overlays, such as internet details, which for whatever reason have null
titles,
> the settings page title is changed to "chrome://settings".
> 
> we should get localized strings for those title-less overlays, but in the
> meantime keeping the parent's title is better than nothing. (we still have to
> change the title even if page.title == null, e.g. when triggering a lateral
move
> from one overlay to another with a different ancestor.)

ok

https://codereview.chromium.org/410293004/diff/200001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:694:
elementContainsPoint_: function(e, x, y) {
On 2014/07/31 03:21:17, michaelpg wrote:
> note to self: remove unused code

+10

[michaelpg, 2014-07-31 03:51:22]

rebase, remove unused code

[michaelpg, 2014-07-31 03:51:23]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-07-31 03:55:48]

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

[michaelpg, 2014-07-31 04:17:57]

On 2014/07/31 03:55:48, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> 
https://chromium-status.appspot.com/cq/michaelpg@chromium.org/410293004/220001

Presubmit failed because of a comment in a javascript file mentioning
-webkit-box-direction.... *facepalm*

[michaelpg, 2014-07-31 05:11:40]

The CQ bit was unchecked by michaelpg@chromium.org

[michaelpg, 2014-07-31 05:38:20]

rebase correction

[michaelpg, 2014-07-31 05:38:21]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-07-31 05:41:08]

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

[commit-bot: I haz the power, 2014-07-31 11:21:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-07-31 11:25:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-31 11:25:39]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[michaelpg, 2014-07-31 12:41:32]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-07-31 12:44:32]

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

[commit-bot: I haz the power, 2014-07-31 16:46:19]

Change committed as 286820