NTP5: Reuse insertion/removal animations for Apps.

NTP5: Reuse insertion/removal animations for Apps.

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


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

[pedro (no code reviews), 2012-12-05 08:55:39]

Evan and Dan, this is the second part of the animation fix for Apps.

[Dan Beam, 2012-12-05 18:40:44]

https://chromiumcodereview.appspot.com/11445009/diff/3001/chrome/browser/reso...
File chrome/browser/resources/ntp_search/new_tab.js (left):

https://chromiumcodereview.appspot.com/11445009/diff/3001/chrome/browser/reso...
chrome/browser/resources/ntp_search/new_tab.js:431: var state = page &&
page.getTileRepositioningState();
^ so why is all this logic being removed?

https://chromiumcodereview.appspot.com/11445009/diff/3001/chrome/browser/reso...
File chrome/browser/resources/ntp_search/tile_page.js (right):

https://chromiumcodereview.appspot.com/11445009/diff/3001/chrome/browser/reso...
chrome/browser/resources/ntp_search/tile_page.js:489: if (!opt_create)
why isn't this simply 2 methods? `set dataList` and createTiles()?

https://chromiumcodereview.appspot.com/11445009/diff/3001/chrome/browser/reso...
chrome/browser/resources/ntp_search/tile_page.js:857: var extraTile = extraCell
&& extraCell.firstChild;
IMO, these are slightly easier to understand for C++ devs (or anybody unaware of
|| or && returning the results of an expression in JS):

  var extraTile = extraCell ? extraCell.firstChild : null;

or

  var extraTile;
  if (extraCell && extraCell.firstChild)
    extraTile = extraCell.firstChild;

[pedro (no code reviews), 2012-12-05 20:03:55]

https://codereview.chromium.org/11445009/diff/3001/chrome/browser/resources/n...
File chrome/browser/resources/ntp_search/new_tab.js (left):

https://codereview.chromium.org/11445009/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp_search/new_tab.js:431: var state = page &&
page.getTileRepositioningState();
On 2012/12/05 18:40:44, Dan Beam wrote:
> ^ so why is all this logic being removed?

This logic was placed in the wrong spot. It has been moved to the proper place
inside apps_page.js.

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

https://codereview.chromium.org/11445009/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp_search/tile_page.js:489: if (!opt_create)
On 2012/12/05 18:40:44, Dan Beam wrote:
> why isn't this simply 2 methods? `set dataList` and createTiles()?

For simplicity reasons, I'm trying to share as much code as possible between
Most Visited and Apps, and the Most Visited logic requires the tiles to be
created any time dataList has changed. Apps Page, on the other hand does not
require that.

Please bear in mind that the NTP5 is going to be rewritten from scratch, and all
the code you're seeing now is going to be deleted. I wouldn't be too much
worried about the NTP5 code right now, because this is just for experiment
purposes.

https://codereview.chromium.org/11445009/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp_search/tile_page.js:857: var extraTile = extraCell
&& extraCell.firstChild;
On 2012/12/05 18:40:44, Dan Beam wrote:
> IMO, these are slightly easier to understand for C++ devs (or anybody unaware
of
> || or && returning the results of an expression in JS):
> 
>   var extraTile = extraCell ? extraCell.firstChild : null;
> 
> or
> 
>   var extraTile;
>   if (extraCell && extraCell.firstChild)
>     extraTile = extraCell.firstChild;

Done.

[Dan Beam, 2012-12-05 20:05:54]

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

https://codereview.chromium.org/11445009/diff/3001/chrome/browser/resources/n...
chrome/browser/resources/ntp_search/tile_page.js:489: if (!opt_create)
On 2012/12/05 20:03:55, pedrosimonetti wrote:
> On 2012/12/05 18:40:44, Dan Beam wrote:
> > why isn't this simply 2 methods? `set dataList` and createTiles()?
> 
> For simplicity reasons, I'm trying to share as much code as possible between
> Most Visited and Apps, and the Most Visited logic requires the tiles to be
> created any time dataList has changed. Apps Page, on the other hand does not
> require that.
> 
> Please bear in mind that the NTP5 is going to be rewritten from scratch, and
all
> the code you're seeing now is going to be deleted. I wouldn't be too much
> worried about the NTP5 code right now, because this is just for experiment
> purposes.

If only I had a $1 for every time I heard that ;)

[pedro (no code reviews), 2012-12-06 18:25:10]

Friendly ping.

[Evan Stade, 2012-12-06 20:52:03]

I gather that you two are having a conversation about how to proceed with
development. This code has continued to flip flop and I feel like there's been a
lot of effort spent on reviewing code, or bringing code up to committing
standards, which didn't need to be spent. A long time ago I strongly suggested
using a separate repo for development of search ntp, and I still think that's
the best idea. (src-internal is not the right place for it, however)

[Dan Beam, 2012-12-07 05:08:56]

https://codereview.chromium.org/11445009/diff/11001/chrome/browser/resources/...
File chrome/browser/resources/ntp_search/tile_page.js (right):

https://codereview.chromium.org/11445009/diff/11001/chrome/browser/resources/...
chrome/browser/resources/ntp_search/tile_page.js:496: opt_create = typeof
opt_create == 'undefined' ? true : opt_create;
nit: I generally think it's better to have `undefined` match the truthiness
(i.e. opt_dontCreate) so when you don't pass an argument the truthiness matches
(i.e. addEventListener('blah', func) is natural to me even though it takes
[true, false] as the third argument).  this also allows one to simply say:

  if (opt_dontCreate)
    return;

with no added preparation or sniffing of arguments.

[Dan Beam, 2012-12-07 05:09:08]

generally this seems fine to me, though...

[Dan Beam, 2012-12-07 05:11:26]

lgtm with little to no knowledge of what you're trying to accomplish nor having
ever seen the UI

https://codereview.chromium.org/11445009/diff/11001/chrome/browser/resources/...
File chrome/browser/resources/ntp_search/apps_page.js (right):

https://codereview.chromium.org/11445009/diff/11001/chrome/browser/resources/...
chrome/browser/resources/ntp_search/apps_page.js:645: this.renderGrid_();
I do wonder why adding a tile doesn't automatically render... is that a goal? to
have mutations not have side effects?

[pedro (no code reviews), 2012-12-08 02:57:29]

https://codereview.chromium.org/11445009/diff/11001/chrome/browser/resources/...
File chrome/browser/resources/ntp_search/apps_page.js (right):

https://codereview.chromium.org/11445009/diff/11001/chrome/browser/resources/...
chrome/browser/resources/ntp_search/apps_page.js:645: this.renderGrid_();
On 2012/12/07 05:11:26, Dan Beam wrote:
> I do wonder why adding a tile doesn't automatically render... is that a goal?
to
> have mutations not have side effects?

The goal was being able to add/remove multiple tiles and/or have transient tiles
(like in add/remove animations) without having to reconstruct the entire grid
multiple times (rows, cells and tiles).

But I agree that not having to call renderGrid_ would make the API simpler to
use, and I like the idea of simplifying things. So I included renderGrid_ calls
inside the addTileAt_ method, making the renderGrid_ call here not necessary.

https://codereview.chromium.org/11445009/diff/11001/chrome/browser/resources/...
File chrome/browser/resources/ntp_search/tile_page.js (right):

https://codereview.chromium.org/11445009/diff/11001/chrome/browser/resources/...
chrome/browser/resources/ntp_search/tile_page.js:496: opt_create = typeof
opt_create == 'undefined' ? true : opt_create;
On 2012/12/07 05:08:56, Dan Beam wrote:
> nit: I generally think it's better to have `undefined` match the truthiness
> (i.e. opt_dontCreate) so when you don't pass an argument the truthiness
matches
> (i.e. addEventListener('blah', func) is natural to me even though it takes
> [true, false] as the third argument).  this also allows one to simply say:
> 
>   if (opt_dontCreate)
>     return;
> 
> with no added preparation or sniffing of arguments.

I went ahead and implemented your previous suggestion, separating this into two
methods.

[Dan Beam, 2012-12-08 05:26:19]

https://chromiumcodereview.appspot.com/11445009/diff/15003/chrome/browser/res...
File chrome/browser/resources/ntp_search/tile_page.js (right):

https://chromiumcodereview.appspot.com/11445009/diff/15003/chrome/browser/res...
chrome/browser/resources/ntp_search/tile_page.js:475: * @protected
nit: updateGride_ -> updateGrid (_ is only for @private)

https://chromiumcodereview.appspot.com/11445009/diff/15003/chrome/browser/res...
chrome/browser/resources/ntp_search/tile_page.js:599: * @protected
same nit

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

https://codereview.chromium.org/11445009/diff/15003/chrome/browser/resources/...
File chrome/browser/resources/ntp_search/tile_page.js (right):

https://codereview.chromium.org/11445009/diff/15003/chrome/browser/resources/...
chrome/browser/resources/ntp_search/tile_page.js:475: * @protected
On 2012/12/08 05:26:19, Dan Beam wrote:
> nit: updateGride_ -> updateGrid (_ is only for @private)

Done.

https://codereview.chromium.org/11445009/diff/15003/chrome/browser/resources/...
chrome/browser/resources/ntp_search/tile_page.js:599: * @protected
On 2012/12/08 05:26:19, Dan Beam wrote:
> same nit

Done.

[commit-bot: I haz the power, 2012-12-12 08:21:46]

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

[commit-bot: I haz the power, 2012-12-12 10:51:31]

Change committed as 172579