NTP5: Apps regression fix.

NTP5: Regression fix.

A regression fix was introduced while refactoring the Tile logic in: https://codereview.chromium.org/11438009/

The App insertion animation got broke, and this CL fixes the problem. It also fixes one corner-case bug of the insertion animation.

This CL depends on https://codereview.chromium.org/11564026/.

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

[pedro (no code reviews), 2012-12-14 09:28:10]

Hi Dan, please take a look at this CL. It fixes a regression bug. Thanks.

[Dan Beam, 2012-12-14 18:28:12]

https://codereview.chromium.org/11566026/diff/1/chrome/browser/resources/ntp_...
File chrome/browser/resources/ntp_search/apps_page.js (right):

https://codereview.chromium.org/11566026/diff/1/chrome/browser/resources/ntp_...
chrome/browser/resources/ntp_search/apps_page.js:188: * @param {Object} opt_data
The data representing the app.
@param {Object=}

https://codereview.chromium.org/11566026/diff/1/chrome/browser/resources/ntp_...
File chrome/browser/resources/ntp_search/most_visited_page.js (right):

https://codereview.chromium.org/11566026/diff/1/chrome/browser/resources/ntp_...
chrome/browser/resources/ntp_search/most_visited_page.js:13: * @param {Object}
opt_data The data representing the most visited page.
@param {Object=}

https://codereview.chromium.org/11566026/diff/1/chrome/browser/resources/ntp_...
File chrome/browser/resources/ntp_search/thumbnail_page.js (right):

https://codereview.chromium.org/11566026/diff/1/chrome/browser/resources/ntp_...
chrome/browser/resources/ntp_search/thumbnail_page.js:13: * @param {Object}
opt_data The data representing the thumbnail.
* @param {Object=}

https://codereview.chromium.org/11566026/diff/1/chrome/browser/resources/ntp_...
File chrome/browser/resources/ntp_search/tile_page.js (right):

https://codereview.chromium.org/11566026/diff/1/chrome/browser/resources/ntp_...
chrome/browser/resources/ntp_search/tile_page.js:858: // restored tile
temporarily and there will be no rearrangement of tiles.
I appreciate this comment but I'm still not quite sure what it means?  Is there
a way to sum this up in less words and better for a new reader of the code?

[pedro (no code reviews), 2012-12-14 18:54:10]

https://codereview.chromium.org/11566026/diff/1/chrome/browser/resources/ntp_...
File chrome/browser/resources/ntp_search/apps_page.js (right):

https://codereview.chromium.org/11566026/diff/1/chrome/browser/resources/ntp_...
chrome/browser/resources/ntp_search/apps_page.js:188: * @param {Object} opt_data
The data representing the app.
On 2012/12/14 18:28:13, Dan Beam wrote:
> @param {Object=}

Done.

https://codereview.chromium.org/11566026/diff/1/chrome/browser/resources/ntp_...
File chrome/browser/resources/ntp_search/most_visited_page.js (right):

https://codereview.chromium.org/11566026/diff/1/chrome/browser/resources/ntp_...
chrome/browser/resources/ntp_search/most_visited_page.js:13: * @param {Object}
opt_data The data representing the most visited page.
On 2012/12/14 18:28:13, Dan Beam wrote:
> @param {Object=}

Done.

https://codereview.chromium.org/11566026/diff/1/chrome/browser/resources/ntp_...
File chrome/browser/resources/ntp_search/thumbnail_page.js (right):

https://codereview.chromium.org/11566026/diff/1/chrome/browser/resources/ntp_...
chrome/browser/resources/ntp_search/thumbnail_page.js:13: * @param {Object}
opt_data The data representing the thumbnail.
On 2012/12/14 18:28:13, Dan Beam wrote:
> * @param {Object=}

Done.

https://codereview.chromium.org/11566026/diff/1/chrome/browser/resources/ntp_...
File chrome/browser/resources/ntp_search/tile_page.js (right):

https://codereview.chromium.org/11566026/diff/1/chrome/browser/resources/ntp_...
chrome/browser/resources/ntp_search/tile_page.js:858: // restored tile
temporarily and there will be no rearrangement of tiles.
On 2012/12/14 18:28:13, Dan Beam wrote:
> I appreciate this comment but I'm still not quite sure what it means?  Is
there
> a way to sum this up in less words and better for a new reader of the code?

I've rewritten this comment, let me know if it is better now. It's kinda hard to
describe because it is a very visual thing. I guess it makes more sense once you
see what the animation is doing (one tile is appearing with a "expand/scale"
animation while the others are being rearranged in order to give room for that
tile. But there's a corner case when the tile being added is on the very last
position of the grid, on a row that is not yet rendered.

[pedro (no code reviews), 2012-12-14 22:35:00]

Hey Dan, please take another look.

I think I found a better solution for that part of the code. It is more
readable, does not require breaking the code flow (does not use |return|), and
it solves the corner-case bug.

[Dan Beam, 2012-12-14 22:49:46]

lgtm

https://codereview.chromium.org/11566026/diff/9001/chrome/browser/resources/n...
File chrome/browser/resources/ntp_search/tile_page.js (right):

https://codereview.chromium.org/11566026/diff/9001/chrome/browser/resources/n...
chrome/browser/resources/ntp_search/tile_page.js:849: var tileCells =
this.querySelectorAll('.tile-cell');
FYI: if you use getElementsByClassName() you won't need to double query
selector, as the collection updates itself as things with that class name get
added.

[pedro (no code reviews), 2012-12-14 23:42:56]

Thanks for the tip. I won't submit this right now because it depends on
https://codereview.chromium.org/11564026/, so I'm still waiting for the CQ to
submit that CL first. :(

I've polished up the code a little bit more. Instead of adding a new element to
the |this.tiles_| array in order to trick renderGrid() to render extra tiles,
now I'm using a second parameter of renderGrid() which allows expanding the grid
in this case by informing the new number of tiles to be rendered.

https://codereview.chromium.org/11566026/diff/9001/chrome/browser/resources/n...
File chrome/browser/resources/ntp_search/tile_page.js (right):

https://codereview.chromium.org/11566026/diff/9001/chrome/browser/resources/n...
chrome/browser/resources/ntp_search/tile_page.js:849: var tileCells =
this.querySelectorAll('.tile-cell');
On 2012/12/14 22:49:46, Dan Beam wrote:
> FYI: if you use getElementsByClassName() you won't need to double query
> selector, as the collection updates itself as things with that class name get
> added.

Done. Thanks for the explanation. I wonder why querySelectorAll() is not
auto-updated like getElementsBySomething().

[commit-bot: I haz the power, 2012-12-16 00:35:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/11566026/1...

[commit-bot: I haz the power, 2012-12-16 04:52:39]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2012-12-16 06:33:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/11566026/1...

[commit-bot: I haz the power, 2012-12-16 06:33:54]

Change committed as 173376

[Dan Beam, 2012-12-17 19:08:56]

https://codereview.chromium.org/11566026/diff/9001/chrome/browser/resources/n...
File chrome/browser/resources/ntp_search/tile_page.js (right):

https://codereview.chromium.org/11566026/diff/9001/chrome/browser/resources/n...
chrome/browser/resources/ntp_search/tile_page.js:849: var tileCells =
this.querySelectorAll('.tile-cell');
On 2012/12/14 23:42:57, pedrosimonetti wrote:
> On 2012/12/14 22:49:46, Dan Beam wrote:
> > FYI: if you use getElementsByClassName() you won't need to double query
> > selector, as the collection updates itself as things with that class name
get
> > added.
> 
> Done. Thanks for the explanation. I wonder why querySelectorAll() is not
> auto-updated like getElementsBySomething().

This is by design in the new API so mutations events don't need to keep track of
and update live collections.  Also, many web authors just didn't understand that
getElementsBy{Class,Tag}Name() collections change when modifying the DOM.