Clean up cr.ui.pageManager.Page prototype

Clean up cr.ui.pageManager.Page prototype

BUG=313244
R=dbeam@chromium.org

Committed: https://crrev.com/2d952ff029c1cf6927c9569be7a47eeb11d8fba1
Cr-Commit-Position: refs/heads/master@{#295628}

[michaelpg, 2014-08-27 00:41:23]

Patchset #1 (id:1) has been deleted

[michaelpg, 2014-08-27 00:44:49]

Added these functions to the Page prototype, removed the PageManager checks and
changed some incorrect comments.

[Dan Beam, 2014-08-27 03:42:27]

lgtm w/questions

https://chromiumcodereview.appspot.com/514503002/diff/20001/chrome/browser/re...
File chrome/browser/resources/options/chromeos/change_picture_options.js
(right):

https://chromiumcodereview.appspot.com/514503002/diff/20001/chrome/browser/re...
chrome/browser/resources/options/chromeos/change_picture_options.js:129: * the
page was closed.
so wait, if these callbacks are mutually exclusive as
https://code.google.com/p/chromium/issues/detail?id=118923#c8 alleges, there's
no way both of these can be hit unless this page is an overlay in some contexts
and a root page in others, right?

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

https://chromiumcodereview.appspot.com/514503002/diff/20001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:193:
this.isAncestorOfPage(page, targetPage);
why wouldn't we just call didClosePage() here when .visible is now false?

[michaelpg, 2014-08-27 20:46:07]

Thanks Dan. See responses. TL;DR: didClosePage not always called when overlay
closes :-(

https://chromiumcodereview.appspot.com/514503002/diff/20001/chrome/browser/re...
File chrome/browser/resources/options/chromeos/change_picture_options.js
(right):

https://chromiumcodereview.appspot.com/514503002/diff/20001/chrome/browser/re...
chrome/browser/resources/options/chromeos/change_picture_options.js:129: * the
page was closed.
On 2014/08/27 03:42:27, Dan Beam wrote:
> so wait, if these callbacks are mutually exclusive as
> https://code.google.com/p/chromium/issues/detail?id=118923#c8 alleges, there's
> no way both of these can be hit unless this page is an overlay in some
contexts
> and a root page in others, right?

good question, i've reassessed my allegation. willHidePage will be called on
overlays like this whenever we show the search page. I'm not sure what I was
looking at when I thought overlays weren't notified about willHidePage.

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

https://chromiumcodereview.appspot.com/514503002/diff/20001/ui/webui/resource...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:193:
this.isAncestorOfPage(page, targetPage);
On 2014/08/27 03:42:27, Dan Beam wrote:
> why wouldn't we just call didClosePage() here when .visible is now false?

I think this whole "opening"/"closing" system needs an overhaul. Even I have
trouble wrapping my head around it, which is why I'm reluctant to make an
operational change without a dedicated CL (and time). But as it stands
didClosePage isn't necessarily called just when changing the visibility of a
page.

One example of a bug [I just found] that could be solved by fixing up this
logic: 408279

[Dan Beam, 2014-09-09 02:13:53]

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

https://codereview.chromium.org/514503002/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/page_manager/page.js:134: didClosePage: function()
{},
why hasn't this part of the patch landed?

[michaelpg, 2014-09-10 22:46:02]

On 2014/09/09 02:13:53, Dan Beam wrote:
>
https://codereview.chromium.org/514503002/diff/20001/ui/webui/resources/js/cr...
> File ui/webui/resources/js/cr/ui/page_manager/page.js (right):
> 
>
https://codereview.chromium.org/514503002/diff/20001/ui/webui/resources/js/cr...
> ui/webui/resources/js/cr/ui/page_manager/page.js:134: didClosePage: function()
> {},
> why hasn't this part of the patch landed?

I think the whole patch is good to go. I wanted to wait a day to let you see my
answers and then lost track of it!

[michaelpg, 2014-09-10 22:46:05]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-09-10 22:51:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/514503002/40001

[michaelpg, 2014-09-10 23:20:25]

The CQ bit was unchecked by michaelpg@chromium.org

[michaelpg, 2014-09-18 23:52:56]

Patchset #3 (id:60001) has been deleted

[michaelpg, 2014-09-18 23:56:52]

Patchset #3 (id:80001) has been deleted

[michaelpg, 2014-09-18 23:58:35]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-09-18 23:59:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/514503002/100001

[commit-bot: I haz the power, 2014-09-19 01:04:44]

Committed patchset #3 (id:100001) as de758f2f867475ba88bb0bbd9c98f6a2493b4a87

[commit-bot: I haz the power, 2014-09-19 01:05:17]

Patchset 3 (id:??) landed as
https://crrev.com/2d952ff029c1cf6927c9569be7a47eeb11d8fba1
Cr-Commit-Position: refs/heads/master@{#295628}