[NTP4] Auto-deletion of empty apps panes.

[NTP4] Auto-deletion of empty apps panes.

R=estade@chromium.org
BUG=97762
TEST=NTP4 should continue functioning as before but delete empty apps pages when
the last app is moved/removed/uninstalled from a page.

[Dan Beam, 2011-11-30 20:45:41]

btw guys, this is really close, remaining functional issues are:

uninstall last from last page (idle doesn't delete empty pane)
uninstall last from middle page (CHECK() crash)
uninstall last from front page (CHECK() crash)

dragging the apps around generally works on both idle and active page, AFAIK

[Dan Beam, 2011-12-01 05:37:45]

By the way, if you're trying to actually apply this patch yourself with git cl
patch you'll also have to:
- roll WebKit past r101502 yourself (cloneNode().style fix)
- revert rbyers' r112387 (crbug.com/106012)

Can't wait til the upcoming merge conflicts I'm going to get from csilv's patch
to remove bookmarks pane (http://codereview.chromium.org/8760003/) and csharp's
page indices -> page ordinal patch (http://codereview.chromium.org/8198003/),
:/.

[Evan Stade, 2011-12-02 00:53:24]

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.js:27: }
vat is thees

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/page_list_view.js (right):

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:282: * @param {TilePage}
opt_refNode Optional reference node to insert in front
never end your sentences in a preposition. This should read "Optional reference
node of front in which to insert".

I kid.

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/tile_page.js (right):

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/tile_page.js:307: if (owningPage instanceof
ntp4.AppsPage && !opt_dontDelete)
don't do this, use inheritance or whatever (same applies to all the other places
you used instanceof)

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/card_slider.js (right):

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/card_slider.js:326: if (typeof
opt_callback == 'function') {
better to just assert this if you need to check it at all, but in general we
don't typecheck params

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/card_slider.js:339:
this.container_.addEventListener('webkitTransitionEnd', endHandler);
just attach once, don't add and remove every time

[Dan Beam, 2011-12-02 01:25:11]

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.js:27: }
On 2011/12/02 00:53:24, Evan Stade wrote:
> vat is thees

Done. (meant to only remove ; from after function decl(){} that don't need it)

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/page_list_view.js (right):

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:282: * @param {TilePage}
opt_refNode Optional reference node to insert in front
> I kid.

I read this part too late, damn it, lol.

"I need to read faster!" - Mitch Hedberg

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/tile_page.js (right):

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/tile_page.js:307: if (owningPage instanceof
ntp4.AppsPage && !opt_dontDelete)
On 2011/12/02 00:53:24, Evan Stade wrote:
> don't do this, use inheritance or whatever (same applies to all the other
places
> you used instanceof)

So do something like .removeIfEmpty() on TilePage and override for just
AppsPage?  That's cool with me.

Does this mean that I should also fix the other occurrences of the use of this
operator, like:

  if (typeof ntp4.MostVisitedPage != 'undefined' &&
      page instanceof ntp4.MostVisitedPage) {
    assert(this.tilePages.length == 1,
           'MostVisitedPage should be added as first tile page');

or

  var animate = page instanceof ntp4.AppsPage &&
                page.classList.contains('temporary');

to be something like

  if (page.shouldBeFirst())
    assert(this.tilePages.length == 1);

and

  var animate = page.shouldAnimateInsertion();

?

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/card_slider.js (right):

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/card_slider.js:326: if (typeof
opt_callback == 'function') {
On 2011/12/02 00:53:24, Evan Stade wrote:
> better to just assert this if you need to check it at all, but in general we
> don't typecheck params

It's optional, though.  So, we should only be trying to call it if they passed a
function as the third argument, so how else would you check if the third
argument is callable?

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/card_slider.js:339:
this.container_.addEventListener('webkitTransitionEnd', endHandler);
On 2011/12/02 00:53:24, Evan Stade wrote:
> just attach once, don't add and remove every time

Having generally ignored handlers adds overall memory bloat to the page, no? 
This handler is only necessary for this one time.

I guess you could push the callback into an array and do it on the next time
that event is called, but if you ever miss an event you'd call 2 callbacks at
once (but I guess you already would with this way as well).

[csilv, 2011-12-02 01:57:02]

http://codereview.chromium.org/8637001/diff/40001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/card_slider.js (right):

http://codereview.chromium.org/8637001/diff/40001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/card_slider.js:378: * @return {boolean}
Whether or not a transformation was necessary.
nit: move @return above @private

http://codereview.chromium.org/8637001/diff/42001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/page_list_view.js (right):

http://codereview.chromium.org/8637001/diff/42001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:245: */
add comment documentation for 'fromPage'

http://codereview.chromium.org/8637001/diff/42001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:624: * Re-order apps on this
invactive page when an active NTP gets re-ordered.
"inactive"

http://codereview.chromium.org/8637001/diff/42001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/app_launcher_handler.h (right):

http://codereview.chromium.org/8637001/diff/42001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:99: // Callback for the
"saveAppPageName" message.
"saveAppsPageName" in comment

[Evan Stade, 2011-12-02 02:39:48]

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/tile_page.js (right):

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/tile_page.js:307: if (owningPage instanceof
ntp4.AppsPage && !opt_dontDelete)
On 2011/12/02 01:25:11, Dan Beam wrote:
> On 2011/12/02 00:53:24, Evan Stade wrote:
> > don't do this, use inheritance or whatever (same applies to all the other
> places
> > you used instanceof)
> 
> So do something like .removeIfEmpty() on TilePage and override for just
> AppsPage?  That's cool with me.
> 
> Does this mean that I should also fix the other occurrences of the use of this
> operator, like:

no, I just meant in this context. Superclass is not supposed to know about
subclasses.

> 
>   if (typeof ntp4.MostVisitedPage != 'undefined' &&
>       page instanceof ntp4.MostVisitedPage) {
>     assert(this.tilePages.length == 1,
>            'MostVisitedPage should be added as first tile page');
> 
> or
> 
>   var animate = page instanceof ntp4.AppsPage &&
>                 page.classList.contains('temporary');
> 
> to be something like
> 
>   if (page.shouldBeFirst())
>     assert(this.tilePages.length == 1);
> 
> and
> 
>   var animate = page.shouldAnimateInsertion();
> 
> ?

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/card_slider.js (right):

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/card_slider.js:326: if (typeof
opt_callback == 'function') {
On 2011/12/02 01:25:11, Dan Beam wrote:
> On 2011/12/02 00:53:24, Evan Stade wrote:
> > better to just assert this if you need to check it at all, but in general we
> > don't typecheck params
> 
> It's optional, though.  So, we should only be trying to call it if they passed
a
> function as the third argument, so how else would you check if the third
> argument is callable?

oh ho ho. What's wrong with if (opt_callback)? The problem with typeof is that
it will hide bugs if people start passing in some other random data type.

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/card_slider.js:339:
this.container_.addEventListener('webkitTransitionEnd', endHandler);
On 2011/12/02 01:25:11, Dan Beam wrote:
> On 2011/12/02 00:53:24, Evan Stade wrote:
> > just attach once, don't add and remove every time
> 
> Having generally ignored handlers adds overall memory bloat to the page, no? 
> This handler is only necessary for this one time.

memory/cpu tradeoff, who wins? Code elegance. However I think this comment came
from before we had our little chat so just ignore it.

> 
> I guess you could push the callback into an array and do it on the next time
> that event is called, but if you ever miss an event you'd call 2 callbacks at
> once (but I guess you already would with this way as well).

[Dan Beam, 2011-12-05 18:05:09]

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/card_slider.js (right):

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/card_slider.js:326: if (typeof
opt_callback == 'function') {
On 2011/12/02 02:39:49, Evan Stade wrote:
> On 2011/12/02 01:25:11, Dan Beam wrote:
> > On 2011/12/02 00:53:24, Evan Stade wrote:
> > > better to just assert this if you need to check it at all, but in general
we
> > > don't typecheck params
> > 
> > It's optional, though.  So, we should only be trying to call it if they
passed
> a
> > function as the third argument, so how else would you check if the third
> > argument is callable?
> 
> oh ho ho. What's wrong with if (opt_callback)? The problem with typeof is that
> it will hide bugs if people start passing in some other random data type.

Done.

  if (typeof opt_callback == 'function')
    // do something with it
  else
    assert(typeof opt_callback == 'undefined');

Does this work for you?

P.S. This will be somewhere else, I ripped all this logic out of here and moved
them to an onWebkitTransitionEnd_ handler that fires a CARD_CHANGE_ENDED event.

http://codereview.chromium.org/8637001/diff/40001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/card_slider.js (right):

http://codereview.chromium.org/8637001/diff/40001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/card_slider.js:378: * @return {boolean}
Whether or not a transformation was necessary.
On 2011/12/02 01:57:02, csilv wrote:
> nit: move @return above @private

Done.

http://codereview.chromium.org/8637001/diff/42001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/page_list_view.js (right):

http://codereview.chromium.org/8637001/diff/42001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:245: */
On 2011/12/02 01:57:02, csilv wrote:
> add comment documentation for 'fromPage'

Done.

http://codereview.chromium.org/8637001/diff/42001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:624: * Re-order apps on this
invactive page when an active NTP gets re-ordered.
On 2011/12/02 01:57:02, csilv wrote:
> "inactive"

Done.

http://codereview.chromium.org/8637001/diff/42001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/app_launcher_handler.h (right):

http://codereview.chromium.org/8637001/diff/42001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:99: // Callback for the
"saveAppPageName" message.
On 2011/12/02 01:57:02, csilv wrote:
> "saveAppsPageName" in comment

Done.

[Dan Beam, 2011-12-05 18:07:12]

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/tile_page.js (right):

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/tile_page.js:307: if (owningPage instanceof
ntp4.AppsPage && !opt_dontDelete)
On 2011/12/02 02:39:49, Evan Stade wrote:
> On 2011/12/02 01:25:11, Dan Beam wrote:
> > On 2011/12/02 00:53:24, Evan Stade wrote:
> > > don't do this, use inheritance or whatever (same applies to all the other
> > places
> > > you used instanceof)
> > 
> > So do something like .removeIfEmpty() on TilePage and override for just
> > AppsPage?  That's cool with me.
> > 
> > Does this mean that I should also fix the other occurrences of the use of
this
> > operator, like:
> 
> no, I just meant in this context. Superclass is not supposed to know about
> subclasses.
> 
> > 
> >   if (typeof ntp4.MostVisitedPage != 'undefined' &&
> >       page instanceof ntp4.MostVisitedPage) {
> >     assert(this.tilePages.length == 1,
> >            'MostVisitedPage should be added as first tile page');
> > 
> > or
> > 
> >   var animate = page instanceof ntp4.AppsPage &&
> >                 page.classList.contains('temporary');
> > 
> > to be something like
> > 
> >   if (page.shouldBeFirst())
> >     assert(this.tilePages.length == 1);
> > 
> > and
> > 
> >   var animate = page.shouldAnimateInsertion();
> > 
> > ?
> 

Done. (still ironing out the issues with this small re-design)

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/card_slider.js (right):

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/card_slider.js:339:
this.container_.addEventListener('webkitTransitionEnd', endHandler);
On 2011/12/02 02:39:49, Evan Stade wrote:
> On 2011/12/02 01:25:11, Dan Beam wrote:
> > On 2011/12/02 00:53:24, Evan Stade wrote:
> > > just attach once, don't add and remove every time
> > 
> > Having generally ignored handlers adds overall memory bloat to the page, no?

> > This handler is only necessary for this one time.
> 
> memory/cpu tradeoff, who wins? Code elegance. However I think this comment
came
> from before we had our little chat so just ignore it.
> 
> > 
> > I guess you could push the callback into an array and do it on the next time
> > that event is called, but if you ever miss an event you'd call 2 callbacks
at
> > once (but I guess you already would with this way as well).
> 

Done. (gutted this to make its insides prettier)

[Dan Beam, 2011-12-05 20:08:09]

more relevant changes, stuff just isn't working again :|

[Dan Beam, 2011-12-08 00:21:06]

ptal, got things working again (passes all my tests)

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/card_slider.js (right):

http://codereview.chromium.org/8637001/diff/35001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/card_slider.js:326: if (typeof
opt_callback == 'function') {
On 2011/12/05 18:05:10, Dan Beam wrote:
> On 2011/12/02 02:39:49, Evan Stade wrote:
> > On 2011/12/02 01:25:11, Dan Beam wrote:
> > > On 2011/12/02 00:53:24, Evan Stade wrote:
> > > > better to just assert this if you need to check it at all, but in
general
> we
> > > > don't typecheck params
> > > 
> > > It's optional, though.  So, we should only be trying to call it if they
> passed
> > a
> > > function as the third argument, so how else would you check if the third
> > > argument is callable?
> > 
> > oh ho ho. What's wrong with if (opt_callback)? The problem with typeof is
that
> > it will hide bugs if people start passing in some other random data type.
> 
> Done.
> 
>   if (typeof opt_callback == 'function')
>     // do something with it
>   else
>     assert(typeof opt_callback == 'undefined');
> 
> Does this work for you?
> 
> P.S. This will be somewhere else, I ripped all this logic out of here and
moved
> them to an onWebkitTransitionEnd_ handler that fires a CARD_CHANGE_ENDED
event.

Actually, I ended up moving this to an array of callbacks to be done on the next
CARD_CHANGE_ENDED event (and add this to CardSlider for this case and the other
you mentioned [but I can't remember]).

[Evan Stade, 2011-12-08 02:34:54]

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/apps_page.js (left):

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.js:763: * TODO(estade): pass along an
index.
just remove the todo

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.js:778: chrome.send('setPageIndex',
[draggedTile.firstChild.appId, pageIndex]);
don't make this kind of stylistic change. You're not saving any lines and you're
arguably reducing readability. I wouldn't care one way or the other if you were
writing original code, but it's best to leave existing stuff alone.

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.js:639: addTileAt: function(contents,
index, animate) {
would rather you create an event 'tileAdded' that the TilePage fires on itself,
and AppsPage can listen to these events and do this loadIcon business.

the event seems more generally useful and its purpose is easier to understand
imo.

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.js:640: if (contents instanceof App &&
this.classList.contains('selected-card'))
when is the first half of this ever false? probably better to assert that clause

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/page_list_view.js (right):

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:266: app.id = '';
i think this can go inside the if (fromPage) case?

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:421: // applicable to apps. It
helps self-heal strange / un-expected data, i.e.
strange/unexepected data most likely comes from prefs files. If you mention that
fact it will be more motivating than "if something does something somehow"

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:444: if (prevIndex !=
this.shownPageIndex || prevPage != this.shownPage)
you should just always do this. this.shownPageIndex can be out of sync with the
browser's idea of shown page index (e.g. if you click around on a different NTP
after opening this one)

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:470: // app has just been
dropped on it or created somehow.
close paren

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:625: '|appsPage| is really an
AppsPage');
isn't the string an error message? Hence you would say "...is not really..."

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:627: var whenDone = function() {
some comment about whenDone is necessary

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:628: if (!opt_dontNotify) {
no curlies

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:634: };
newline

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:644: return false;
if (appsPage.tileCount != 0)
  return false;

...

this way the entire function body isn't indented

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:718: // If a page is now empty,
remove it and move backward one in our
comments are nice but this code is self-documenting/self-evident enough that I
don't think it needs a comment. The other thing you can do is

forEach removeAppsPageIfEmpty
forEach cleanupDrag

which is a flow that's even a little easier to follow imho

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:719: // array-like object as it
is a live NodeList and the length/indice
INDICE

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:730: * Ensure there is an apps
page at the given |index|. If index is smaller
smaller??

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:768: var direction = (index ==
this.appsPages.length - 1 ? -1 : 1);
no ()

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:783: if (index <
this.cardSlider.currentCard) {
no curlies

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:791: this.shownPageIndex -= 1;
this doesn't seem necessary. Wont the shownPageIndex be updated by
cardSlider.currentCard assignment?

[Dan Beam, 2011-12-08 02:40:54]

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/apps_page.js (left):

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.js:763: * TODO(estade): pass along an
index.
On 2011/12/08 02:34:56, Evan Stade wrote:
> just remove the todo

Done. (see newer patch set)

[Dan Beam, 2011-12-08 04:46:18]

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/apps_page.js (left):

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.js:778: chrome.send('setPageIndex',
[draggedTile.firstChild.appId, pageIndex]);
On 2011/12/08 02:34:56, Evan Stade wrote:
> don't make this kind of stylistic change. You're not saving any lines and
you're
> arguably reducing readability. I wouldn't care one way or the other if you
were
> writing original code, but it's best to leave existing stuff alone.

Done.

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.js:778: chrome.send('setPageIndex',
[draggedTile.firstChild.appId, pageIndex]);
On 2011/12/08 02:34:56, Evan Stade wrote:
> don't make this kind of stylistic change. You're not saving any lines and
you're
> arguably reducing readability. I wouldn't care one way or the other if you
were
> writing original code, but it's best to leave existing stuff alone.

OK, I didn't mean to do this I just re-wrote this code many times and it ended
up like this (slightly different from the original, wasn't sure if the one time
var was my change or not).  I'll revert these small hunks to make the diff
easier to read.

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.js:639: addTileAt: function(contents,
index, animate) {
On 2011/12/08 02:34:56, Evan Stade wrote:
> would rather you create an event 'tileAdded' that the TilePage fires on
itself,
> and AppsPage can listen to these events and do this loadIcon business.
> 
> the event seems more generally useful and its purpose is easier to understand
> imo.

Done.

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/apps_page.js:640: if (contents instanceof App &&
this.classList.contains('selected-card'))
On 2011/12/08 02:34:56, Evan Stade wrote:
> when is the first half of this ever false? probably better to assert that
clause

Done.

Yeah, I mistakenly thought we allowed non-Apps on an AppsPage (or could possibly
in the future).  My bad.

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/page_list_view.js (right):

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:266: app.id = '';
On 2011/12/08 02:34:56, Evan Stade wrote:
> i think this can go inside the if (fromPage) case?

Done.

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:421: // applicable to apps. It
helps self-heal strange / un-expected data, i.e.
On 2011/12/08 02:34:56, Evan Stade wrote:
> strange/unexepected data most likely comes from prefs files. If you mention
that
> fact it will be more motivating than "if something does something somehow"

Done. (changed comment)

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:444: if (prevIndex !=
this.shownPageIndex || prevPage != this.shownPage)
On 2011/12/08 02:34:56, Evan Stade wrote:
> you should just always do this. this.shownPageIndex can be out of sync with
the
> browser's idea of shown page index (e.g. if you click around on a different
NTP
> after opening this one)

Done.  OK, I just noticed it was often doing this more than necessary.

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:470: // app has just been
dropped on it or created somehow.
On 2011/12/08 02:34:56, Evan Stade wrote:
> close paren

Done.  http://xkcd.com/859/

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:561: if (prevPage !=
this.shownPage || prevIndex != this.shownPageIndex)
estade: I'm assuming you'll want me to remove the conditional around here as
well?

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:625: '|appsPage| is really an
AppsPage');
On 2011/12/08 02:34:56, Evan Stade wrote:
> isn't the string an error message? Hence you would say "...is not really..."

Done.

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:627: var whenDone = function() {
On 2011/12/08 02:34:56, Evan Stade wrote:
> some comment about whenDone is necessary

Done.

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:628: if (!opt_dontNotify) {
On 2011/12/08 02:34:56, Evan Stade wrote:
> no curlies

Done.

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:634: };
On 2011/12/08 02:34:56, Evan Stade wrote:
> newline

Done.

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:644: return false;
On 2011/12/08 02:34:56, Evan Stade wrote:
> if (appsPage.tileCount != 0)
>   return false;
> 
> ...
> 
> this way the entire function body isn't indented

Done.

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:718: // If a page is now empty,
remove it and move backward one in our
On 2011/12/08 02:34:56, Evan Stade wrote:
> comments are nice but this code is self-documenting/self-evident enough that I
> don't think it needs a comment.

Done.

> The other thing you can do is
> 
> forEach removeAppsPageIfEmpty
> forEach cleanupDrag
> 
> which is a flow that's even a little easier to follow imho

I originally did this but it was annoying as I had to subtract one from i every
time as it is a live list so I just combined the two.  That cool?

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:719: // array-like object as it
is a live NodeList and the length/indice
On 2011/12/08 02:34:56, Evan Stade wrote:
> INDICE

DONE. (well, removed whole comment...)

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:730: * Ensure there is an apps
page at the given |index|. If index is smaller
On 2011/12/08 02:34:56, Evan Stade wrote:
> smaller??

Done.

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:768: var direction = (index ==
this.appsPages.length - 1 ? -1 : 1);
On 2011/12/08 02:34:56, Evan Stade wrote:
> no ()

Done. (but I still don't agree that this is easier to read or safe)

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:783: if (index <
this.cardSlider.currentCard) {
On 2011/12/08 02:34:56, Evan Stade wrote:
> no curlies

Done.

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:791: this.shownPageIndex -= 1;
On 2011/12/08 02:34:56, Evan Stade wrote:
> this doesn't seem necessary. Wont the shownPageIndex be updated by
> cardSlider.currentCard assignment?

cardSlider.currentCard is an ES5 getter but all it's doing it just setting the
private member and not automagically switching anything.  Is this what you were
thinking?  Why do you think this *isn't* necessary?  I'm hesitant to change it
now as everything works, gar.  Let me try.

[Dan Beam, 2011-12-08 07:27:52]

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/page_list_view.js (right):

http://codereview.chromium.org/8637001/diff/65001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:791: this.shownPageIndex -= 1;
On 2011/12/08 04:46:18, Dan Beam wrote:
> On 2011/12/08 02:34:56, Evan Stade wrote:
> > this doesn't seem necessary. Wont the shownPageIndex be updated by
> > cardSlider.currentCard assignment?
> 
> cardSlider.currentCard is an ES5 getter but all it's doing it just setting the
> private member and not automagically switching anything.  Is this what you
were
> thinking?  Why do you think this *isn't* necessary?  I'm hesitant to change it
> now as everything works, gar.  Let me try.

Working on fixing this to be in cardSlider instead like we talked about.

[Dan Beam, 2011-12-13 07:57:11]

estade/csilv: I'm assuming I should continue with this CL and just rebase in
csharp's index -> ordinal changes, yeah?  It's not too hard to read yet, right?