Remove code duplication in PageManager

Remove code duplication in PageManager

BUG=313244
R=dbeam@chromium.org

Committed: https://crrev.com/c1b047635dd175317ec54702a9a03e1af823a9b7
Cr-Commit-Position: refs/heads/master@{#291577}

[michaelpg, 2014-08-20 22:37:05]

PTAL. No rush.

[Dan Beam, 2014-08-20 23:54:03]

+30 lines, -29 lines, +1 method and loss of context while reading code => :(

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

https://codereview.chromium.org/488083005/diff/40001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:186: if
(page.willHidePage && page.name != pageName &&
did you ever add a noop func for willHidePage/didShowPage?

https://codereview.chromium.org/488083005/diff/40001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:703: * @param
{function} callback The callback function. Should take a
{function(cr.ui.pageManager.Page)} and remove "Should take a..."

https://codereview.chromium.org/488083005/diff/40001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:706: callForPages_:
function(includeRootPages, callback) {
forEachPage_

https://codereview.chromium.org/488083005/diff/40001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:707: if
(includeRootPages) {
i think this is much simpler:

var pages = includeRootPages ? this.registeredPages : 
                               this.registeredOverlayPages;
for (var name in pages) {
  callback.call(this, pages[name]);
}

https://codereview.chromium.org/488083005/diff/40001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:715:
callback(this.registeredOverlayPages[pageNames[i]]);
use callback.call(this, ...); to remove all the .bind(this) cruft

[michaelpg, 2014-08-21 00:29:18]

(I accidentally the patch set, so I re-uploaded it and added your comments with
my responses.)

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

https://codereview.chromium.org/488083005/diff/120001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:186: if
(page.willHidePage && page.name != pageName &&
On 2014/08/20 23:54:03, Dan Beam wrote:
> did you ever add a noop func for willHidePage/didShowPage?

nope, shall I do that in a separate CL?

https://codereview.chromium.org/488083005/diff/120001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:703: * @param
{function} callback The callback function. Should take a
On 2014/08/20 23:54:03, Dan Beam wrote:
> {function(cr.ui.pageManager.Page)} and remove "Should take a..."

Done.

https://codereview.chromium.org/488083005/diff/120001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:706: callForPages_:
function(includeRootPages, callback) {
On 2014/08/20 23:54:03, Dan Beam wrote:
> forEachPage_

Done.

https://codereview.chromium.org/488083005/diff/120001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:707: if
(includeRootPages) {
(includeRootPages) {
On 2014/08/20 23:54:03, Dan Beam wrote:
> i think this is much simpler:
> 
> var pages = includeRootPages ? this.registeredPages : 
>                                this.registeredOverlayPages;
> for (var name in pages) {
>   callback.call(this, pages[name]);
> }

The callback should be called for this.registeredPages *and*
this.registeredOverlayPages, unless (!includeRootPages). I didn't want
forEachPage_ to have to be called twice for the same callback, is that what
you're saying?

The original code concats the two objects' key arrays and then has to check
which object each key belongs to inside the for loop. That seems less than
ideal. But I realize that since we only have two root pages (and for arbitrary
consumers of this code, number of overlays is probably >> number of root pages),
if we change the original code to check this.registeredOverlayPages first we
only get at most 2 hash misses.

So... how does this look?

https://codereview.chromium.org/488083005/diff/120001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:715:
callback(this.registeredOverlayPages[pageNames[i]]);
On 2014/08/20 23:54:03, Dan Beam wrote:
> use callback.call(this, ...); to remove all the .bind(this) cruft

Done.

[Dan Beam, 2014-08-21 18:53:51]

lgtm

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

https://codereview.chromium.org/488083005/diff/140001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:181: var
isRootPageLocked =
nit: isRootPageLocked could be created inside forEachPage_

https://codereview.chromium.org/488083005/diff/140001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:713: }.bind(this));
instead of }.bind(this) just pass |this| as the second arg
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

[michaelpg, 2014-08-22 00:25:21]

On 2014/08/20 23:54:03, Dan Beam wrote:
> +30 lines, -29 lines, +1 method and loss of context while reading code => :(
> 

10 of those +lines (now 9) came from declaring and annotating the helper
function, which was the point of the TODO. 

What did you mean by "loss of context"?

[Dan Beam, 2014-08-22 00:39:25]

On 2014/08/22 00:25:21, michaelpg wrote:
> On 2014/08/20 23:54:03, Dan Beam wrote:
> > +30 lines, -29 lines, +1 method and loss of context while reading code => :(
> > 
> 
> 10 of those +lines (now 9) came from declaring and annotating the helper
> function, which was the point of the TODO. 
> 
> What did you mean by "loss of context"?

code
code
functionCall(); => have to read what functionCall() does
ok code again

[michaelpg, 2014-08-22 04:33:27]

Understood, but seems hard to avoid when the function makes sense as a
private member and private functions come after public ones.

What would help make things clearer for next time?


On Thu, Aug 21, 2014 at 5:39 PM, <dbeam@chromium.org> wrote:

> On 2014/08/22 00:25:21, michaelpg wrote:
>
>> On 2014/08/20 23:54:03, Dan Beam wrote:
>> > +30 lines, -29 lines, +1 method and loss of context while reading code
>> => :(
>> >
>>
>
>  10 of those +lines (now 9) came from declaring and annotating the helper
>> function, which was the point of the TODO.
>>
>
>  What did you mean by "loss of context"?
>>
>
> code
> code
> functionCall(); => have to read what functionCall() does
> ok code again
>
> https://codereview.chromium.org/488083005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[michaelpg, 2014-08-22 22:37:33]

Thanks.

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

https://codereview.chromium.org/488083005/diff/140001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:181: var
isRootPageLocked =
On 2014/08/21 18:53:51, Dan Beam wrote:
> nit: isRootPageLocked could be created inside forEachPage_

since it's specific to this function (e.g. targetPage) I'd rather not, this
keeps forEachPage_ general.

https://codereview.chromium.org/488083005/diff/140001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:704: */
add @private

https://codereview.chromium.org/488083005/diff/140001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/page_manager/page_manager.js:713: }.bind(this));
On 2014/08/21 18:53:51, Dan Beam wrote:
> instead of }.bind(this) just pass |this| as the second arg
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

Done.

[michaelpg, 2014-08-22 22:37:39]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-08-22 22:38:04]

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

[commit-bot: I haz the power, 2014-08-23 21:18:53]

Committed patchset #3 (180001) as 50bf523b2b43c932f9824b200f411960e54ea12a

[michaelpg, 2014-08-27 00:37:21]

Patchset #4 (id:200001) has been deleted

[commit-bot: I haz the power, 2014-09-10 02:30:48]

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