ntp4: most visited dragging onto apps page

ntp4: most visited dragging onto apps page

You can now:
- drag a most visited tile onto an apps page to get an app for it
- drag a most visited tile onto the trash (cool animations)
- blacklisting a most visited tile with the [x] looks better
- you don't see the trash icon show up for things that can't be trashed (as opposed to being able to drag onto trash but having it do nothing)

BUG=none
TEST=manual

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95856

[Rick Byers, 2011-08-08 17:26:56]

[Tried sending this this morning but just realized rietveld had an error -
trying again]

Looks good, just a few minor suggestions.

[Evan Stade, 2011-08-08 18:26:58]

http://codereview.chromium.org/7592001/diff/1004/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/7592001/diff/1004/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:342: * @return {boolean} True if the
app can be uninstalled.
On 2011/08/08 15:24:15, Rick Byers wrote:
> Remove - doesn't return anything

Done.

http://codereview.chromium.org/7592001/diff/1004/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/most_visited_page.css (right):

http://codereview.chromium.org/7592001/diff/1004/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/most_visited_page.css:152: * Override opacity of
the tile to 1, so that we new tile animation
On 2011/08/08 15:24:15, Rick Byers wrote:
> s/we/the

Done.

http://codereview.chromium.org/7592001/diff/1004/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/most_visited_page.css:154:
.tile.restoring.restoring {
On 2011/08/08 15:24:15, Rick Byers wrote:
> I assume the second .restoring is necessary to give this rule higher
specificity
> (and not a typo or something), right?

yes, it's to override .tile.dragging

http://codereview.chromium.org/7592001/diff/1004/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/most_visited_page.js (right):

http://codereview.chromium.org/7592001/diff/1004/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/most_visited_page.js:220:
this.parentNode.classList.add('restoring');
On 2011/08/08 15:24:15, Rick Byers wrote:
> I'm confused by the name 'restoring' in this situation.  Could it simply be
> 'removing' or something like that?  Alternately add a comment why a most
visited
> page being blacklisted is considered to be 'restoring'.

you're right, finishing-drag might make more sense. Originally I had it as
restoring because we were waiting for new data to re-populate the tile with.

http://codereview.chromium.org/7592001/diff/1004/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/recently_closed.css (right):

http://codereview.chromium.org/7592001/diff/1004/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/recently_closed.css:19:
#footer.showing-trash--mode #recently-closed-menu-button {
On 2011/08/08 15:24:15, Rick Byers wrote:
> s/--/-

Done.

http://codereview.chromium.org/7592001/diff/1004/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/tile_page.js (right):

http://codereview.chromium.org/7592001/diff/1004/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/tile_page.js:179:
this.dragClone.firstChild.style.webkitTransform = 'scale(0)';
On 2011/08/08 15:24:15, Rick Byers wrote:
> shouldn't you really leave this presentation detail up to CSS and just apply a
> class here?

Done.

[Rick Byers, 2011-08-08 18:37:03]

LGTM with one little suggestion

http://codereview.chromium.org/7592001/diff/9001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/tile_page.js (right):

http://codereview.chromium.org/7592001/diff/9001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/tile_page.js:923: this.addDragData(null,
this.tileElements_.length - 1);
Seems a little ugly that you have to pass null as the dataTransfer here.  You
could fix this by having two public methods (addDragData just for the case of a
currentlyDraggingTile, and addOutsideData for the other case).  Or you could
just add a comment to the addDragData function definition below saying that
dataTransfer is permitted to be null ONLY WHEN currentlyDraggingTile is set.

[Evan Stade, 2011-08-08 20:42:49]

http://codereview.chromium.org/7592001/diff/9001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/tile_page.js (right):

http://codereview.chromium.org/7592001/diff/9001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/tile_page.js:923: this.addDragData(null,
this.tileElements_.length - 1);
On 2011/08/08 18:37:03, Rick Byers wrote:
> Seems a little ugly that you have to pass null as the dataTransfer here.  You
> could fix this by having two public methods (addDragData just for the case of
a
> currentlyDraggingTile, and addOutsideData for the other case).  Or you could
> just add a comment to the addDragData function definition below saying that
> dataTransfer is permitted to be null ONLY WHEN currentlyDraggingTile is set.

Done.