NTP5: Fix page blacklisting and remove recently closed tabs when they're clicked. Fix the styling …

NTP5: Fix page blacklisting and remove recently closed tabs when they're clicked.  Fix the styling of the blacklist notification and pull it out of the tile page.

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

[jeremycho, 2012-09-04 19:49:49]

Hi Pedro - this is ready for review.

[jeremycho_google, 2012-09-05 03:48:39]

The double-favicon issue was due to the fact that in blacklist_(), both messages
'blacklistURLFromMostVisited' and 'getMostVisited' cause setMostVisitedPages to
be called.  This results in a race condition where formatThumbnail_ is called
twice and then the onerror function is called twice.  To fix this, the onerror
function reuses the favicon element if it exists.

For now I also added the renderGrid_ fix we discussed - let me know if you find
a better way to address this.

[pedro (no code reviews), 2012-09-05 17:36:38]

On 2012/09/05 03:48:39, jeremycho wrote:
> The double-favicon issue was due to the fact that in blacklist_(), both
messages
> 'blacklistURLFromMostVisited' and 'getMostVisited' cause setMostVisitedPages
to
> be called.  This results in a race condition where formatThumbnail_ is called
> twice and then the onerror function is called twice.  To fix this, the onerror
> function reuses the favicon element if it exists.
> 
> For now I also added the renderGrid_ fix we discussed - let me know if you
find
> a better way to address this.

Okay. I'll patch your changes and play around a little bit. One thing that I
just realized is that there was a function that used to control the position of
the notification (like the undo one), which is TilePage.positionNotification_().
If positioning the notification is not necessary anymore, we should delete that
function. I'll ping you with all my comments later.

I think I found a better way of fixing that problem, but I need to reason about
it carefully because the initialization sequence and the layout function are
confusing. The cardselected event is not earlier enough to do the first layout
necessary for each page. I'll use the cardcreated instead. But since this change
will require changing a few files I'll do it in another CL.

[pedro (no code reviews), 2012-09-05 18:56:44]

On 2012/09/05 17:36:38, pedrosimonetti1 wrote:
> On 2012/09/05 03:48:39, jeremycho wrote:
> > The double-favicon issue was due to the fact that in blacklist_(), both
> messages
> > 'blacklistURLFromMostVisited' and 'getMostVisited' cause setMostVisitedPages
> to
> > be called.  This results in a race condition where formatThumbnail_ is
called
> > twice and then the onerror function is called twice.  To fix this, the
onerror
> > function reuses the favicon element if it exists.
> > 
> > For now I also added the renderGrid_ fix we discussed - let me know if you
> find
> > a better way to address this.
> 
> Okay. I'll patch your changes and play around a little bit. One thing that I
> just realized is that there was a function that used to control the position
of
> the notification (like the undo one), which is
TilePage.positionNotification_().
> If positioning the notification is not necessary anymore, we should delete
that
> function. I'll ping you with all my comments later.
> 
> I think I found a better way of fixing that problem, but I need to reason
about
> it carefully because the initialization sequence and the layout function are
> confusing. The cardselected event is not earlier enough to do the first layout
> necessary for each page. I'll use the cardcreated instead. But since this
change
> will require changing a few files I'll do it in another CL.

Alright, here comes my comments. Please take a look at the
positionNotification_() too because it seems to be obsolete now.

[pedro (no code reviews), 2012-09-05 18:57:00]

http://codereview.chromium.org/10907065/diff/8001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp_search/recently_closed_page.js (right):

http://codereview.chromium.org/10907065/diff/8001/chrome/browser/resources/nt...
chrome/browser/resources/ntp_search/recently_closed_page.js:152: // the Recently
Closed page, fall back to Most Visited.
Most Visited --> first page

It happens to be the MostVisited right now, but that's not what the code is
actually doing.

http://codereview.chromium.org/10907065/diff/8001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp_search/thumbnail_page.js (right):

http://codereview.chromium.org/10907065/diff/8001/chrome/browser/resources/nt...
chrome/browser/resources/ntp_search/thumbnail_page.js:220: getValidData_:
function() {
I'd move this below refreshData, since getData, setData and refreshData comes
first when reading the code.

Also, I'd make this function return this.data_ here, and override this function
in MostVisited with the MV-specific logic in there.

http://codereview.chromium.org/10907065/diff/8001/chrome/browser/resources/nt...
chrome/browser/resources/ntp_search/thumbnail_page.js:250: var dataLength =
this.getValidData_().length;
nit: dataLength --> validDataLength

http://codereview.chromium.org/10907065/diff/8001/chrome/browser/resources/nt...
chrome/browser/resources/ntp_search/thumbnail_page.js:257: this.tiles_.pop();
Even though calling pop() will work, it feels wrong doing that, once we're
suppose to use addTile and removeTile functions. But removeTile is not quite
working as expected right now.

Either put a TODO here, or fix the removeTile function.

http://codereview.chromium.org/10907065/diff/8001/chrome/browser/resources/nt...
chrome/browser/resources/ntp_search/thumbnail_page.js:260: if (dataLength !=
tileCount && tileCount > 0)
Include a TODO here. I'll fix the problem in another CL.

[jeremycho, 2012-09-05 22:42:11]

The notification fade in/out on a card change looks a bit weird to me since it's
not scrolling away with the card, but let me know what you think.

Also fixed the last row titles not showing up.

http://codereview.chromium.org/10907065/diff/8001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp_search/recently_closed_page.js (right):

http://codereview.chromium.org/10907065/diff/8001/chrome/browser/resources/nt...
chrome/browser/resources/ntp_search/recently_closed_page.js:152: // the Recently
Closed page, fall back to Most Visited.
On 2012/09/05 18:57:00, pedrosimonetti1 wrote:
> Most Visited --> first page
> 
> It happens to be the MostVisited right now, but that's not what the code is
> actually doing.

Done.

http://codereview.chromium.org/10907065/diff/8001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp_search/thumbnail_page.js (right):

http://codereview.chromium.org/10907065/diff/8001/chrome/browser/resources/nt...
chrome/browser/resources/ntp_search/thumbnail_page.js:220: getValidData_:
function() {
On 2012/09/05 18:57:00, pedrosimonetti1 wrote:
> I'd move this below refreshData, since getData, setData and refreshData comes
> first when reading the code.
> 
> Also, I'd make this function return this.data_ here, and override this
function
> in MostVisited with the MV-specific logic in there.

Done.

http://codereview.chromium.org/10907065/diff/8001/chrome/browser/resources/nt...
chrome/browser/resources/ntp_search/thumbnail_page.js:250: var dataLength =
this.getValidData_().length;
On 2012/09/05 18:57:00, pedrosimonetti1 wrote:
> nit: dataLength --> validDataLength

Done.

http://codereview.chromium.org/10907065/diff/8001/chrome/browser/resources/nt...
chrome/browser/resources/ntp_search/thumbnail_page.js:257: this.tiles_.pop();
On 2012/09/05 18:57:00, pedrosimonetti1 wrote:
> Even though calling pop() will work, it feels wrong doing that, once we're
> suppose to use addTile and removeTile functions. But removeTile is not quite
> working as expected right now.
> 
> Either put a TODO here, or fix the removeTile function.

removeTile only seems necessary if we wanted to animate the repositioning of the
remaining tiles for Most Visited and Recently Closed (the currently NTP only
animates for Apps).

To call removeTile, we'd have to figure out which tile corresponds to the data
entry that's been deleted.

I'll leave a TODO for now to revisit once we've implemented Apps.

http://codereview.chromium.org/10907065/diff/8001/chrome/browser/resources/nt...
chrome/browser/resources/ntp_search/thumbnail_page.js:260: if (dataLength !=
tileCount && tileCount > 0)
On 2012/09/05 18:57:00, pedrosimonetti1 wrote:
> Include a TODO here. I'll fix the problem in another CL.

Done.

[pedro (no code reviews), 2012-09-06 00:28:49]

The code is looking better. As we discussed offline, I think that we should
remove the refreshData code. It would prevent showing the wrong order when
blacklisting the thumbnails, and would also simplify our code. Also, as you
noted yourself, the notification animation when a card changes looks weird, so
we should remove it.

http://codereview.chromium.org/10907065/diff/11003/chrome/browser/resources/n...
File chrome/browser/resources/ntp_search/new_tab.js (right):

http://codereview.chromium.org/10907065/diff/11003/chrome/browser/resources/n...
chrome/browser/resources/ntp_search/new_tab.js:329: window.setTimeout(function()
{
Do you really need this inside a setTimeout?

[jeremycho, 2012-09-06 04:40:08]

Removed the card change animation and fixed the bug caused by blacklisting all
thumbnails and then clicking restore all.

http://codereview.chromium.org/10907065/diff/11003/chrome/browser/resources/n...
File chrome/browser/resources/ntp_search/new_tab.js (right):

http://codereview.chromium.org/10907065/diff/11003/chrome/browser/resources/n...
chrome/browser/resources/ntp_search/new_tab.js:329: window.setTimeout(function()
{
On 2012/09/06 00:28:49, pedrosimonetti1 wrote:
> Do you really need this inside a setTimeout?

Without it, the fade-in transition isn't seen.

[jeremycho, 2012-09-06 05:10:02]

Hi Evan,

This isn't ready for review, but we wanted to get your input on Most Visited's
refreshData() before proceeding further.

Earlier we discussed whether set data can simply copy over the new data and you
mentioned cases where tiles could get reordered as a result.  The only scenario
Pedro and I could come up with was when the NTP is open in one window, and in
another window several pages are visited until their relative frequencies
change.  In this case, updating the order could be seen as the expected
behavior.  Are there other possibilities?

We've also been able to reproduce the following bug in ntp4:
1. Open an NTP
2. Continue to blacklist the first thumbnail until filler tiles are shown (i.e.
less than 8 thumbnails are present)
3. Open a new NTP
4. Blacklist the first thumbnail then click "restore all".  set data is called
with the complete list of pages, however refreshData only returns the number of
thumbnails shown after step (2).  As a result, filler tiles are still shown.

[Evan Stade, 2012-09-06 16:52:49]

On 2012/09/06 05:10:02, jeremycho1 wrote:
> Hi Evan,
> 
> This isn't ready for review, but we wanted to get your input on Most Visited's
> refreshData() before proceeding further.
> 
> Earlier we discussed whether set data can simply copy over the new data and
you
> mentioned cases where tiles could get reordered as a result.  The only
scenario
> Pedro and I could come up with was when the NTP is open in one window, and in
> another window several pages are visited until their relative frequencies
> change.  In this case, updating the order could be seen as the expected
> behavior.  Are there other possibilities?

wouldn't blacklisting be one common way that order could change? i.e. the
elements after the blacklisted tile will now be higher in the order. But we
don't want them to jiggle around; when you blacklist something you want that
tile to be replaced by something else while the rest of the UI stays where it
is.

> 
> We've also been able to reproduce the following bug in ntp4:
> 1. Open an NTP
> 2. Continue to blacklist the first thumbnail until filler tiles are shown
(i.e.
> less than 8 thumbnails are present)
> 3. Open a new NTP
> 4. Blacklist the first thumbnail then click "restore all".  set data is called
> with the complete list of pages, however refreshData only returns the number
of
> thumbnails shown after step (2).  As a result, filler tiles are still shown.

great, I hope you filed a bug.

[pedro (no code reviews), 2012-09-10 20:59:40]

lgtm

Hey Jeremy, as we discussed offline, we can address the refreshData in another
CL once that will require confirming the behavior with the designers, and
they've been busy with other stuff. So, let's move forward with the blacklisting
fix which is more important right now.

[jeremycho, 2012-09-11 02:27:31]

Hi Evan - this is ready for review.

[Evan Stade, 2012-09-11 16:48:46]

http://codereview.chromium.org/10907065/diff/14001/chrome/browser/resources/n...
File chrome/browser/resources/ntp_search/most_visited_page.js (right):

http://codereview.chromium.org/10907065/diff/14001/chrome/browser/resources/n...
chrome/browser/resources/ntp_search/most_visited_page.js:221: * Merges new Most
Visited data into the old while minimizing re-ordering.
would be easier to review this if you didn't move the function.

http://codereview.chromium.org/10907065/diff/14001/chrome/browser/resources/n...
chrome/browser/resources/ntp_search/most_visited_page.js:229: refreshData_:
function(oldData, newData, maxTileCount) {
can you enable most_visited_browsertest for the search ntp

http://codereview.chromium.org/10907065/diff/14001/chrome/browser/resources/n...
File chrome/browser/resources/ntp_search/recently_closed_page.js (right):

http://codereview.chromium.org/10907065/diff/14001/chrome/browser/resources/n...
chrome/browser/resources/ntp_search/recently_closed_page.js:158: else
if {
  ...
} else {
  ...
}

[jeremycho, 2012-09-12 04:11:50]

After discussing with UX, we decided that it was more desirable to always order
by frequency, even if thumbnails are shuffled in the process.  I've gone ahead
and removed refreshData.

http://codereview.chromium.org/10907065/diff/14001/chrome/browser/resources/n...
File chrome/browser/resources/ntp_search/most_visited_page.js (right):

http://codereview.chromium.org/10907065/diff/14001/chrome/browser/resources/n...
chrome/browser/resources/ntp_search/most_visited_page.js:221: * Merges new Most
Visited data into the old while minimizing re-ordering.
On 2012/09/11 16:48:47, Evan Stade wrote:
> would be easier to review this if you didn't move the function.
Deleted.

http://codereview.chromium.org/10907065/diff/14001/chrome/browser/resources/n...
chrome/browser/resources/ntp_search/most_visited_page.js:229: refreshData_:
function(oldData, newData, maxTileCount) {
On 2012/09/11 16:48:47, Evan Stade wrote:
> can you enable most_visited_browsertest for the search ntp

Now that refreshData_ is removed, that test isn't needed.

http://codereview.chromium.org/10907065/diff/14001/chrome/browser/resources/n...
File chrome/browser/resources/ntp_search/recently_closed_page.js (right):

http://codereview.chromium.org/10907065/diff/14001/chrome/browser/resources/n...
chrome/browser/resources/ntp_search/recently_closed_page.js:158: else
On 2012/09/11 16:48:47, Evan Stade wrote:
> if {
>   ...
> } else {
>   ...
> }

Done.

[Evan Stade, 2012-09-12 08:54:36]

what does it look like when you blacklist a thumbnail? Can you create a
screencast? The reason for refreshData is to make it look better when you delete
something. Until such time that you implement a spiffy animation for deleting to
replace the current one, I think you should leave it as is.

[jeremycho, 2012-09-12 17:03:20]

On 2012/09/12 08:54:36, Evan Stade wrote:
> what does it look like when you blacklist a thumbnail? Can you create a
> screencast? The reason for refreshData is to make it look better when you
delete
> something. Until such time that you implement a spiffy animation for deleting
to
> replace the current one, I think you should leave it as is.

Right now when you blacklist a thumbnail, it gets removed and the page is
immediately updated with the set of thumbnails passed to setMostVisitedPages,
without any animations.  AFAIK, we aren't planning on using the current NTP's
thumbnail-replace animation on the current NTP, only one to re-shuffle the
thumbnails, which lessens the need to preserve ordering.

Pedro's planning to work on the animation implementation, so we were hoping to
get this in first (this CL also makes blacklisting actually work, which would be
nice to have for the dogfood).

[Evan Stade, 2012-09-13 07:50:52]

lgtm

http://codereview.chromium.org/10907065/diff/8006/chrome/browser/resources/nt...
File chrome/browser/resources/ntp_search/thumbnail_page.js (right):

http://codereview.chromium.org/10907065/diff/8006/chrome/browser/resources/nt...
chrome/browser/resources/ntp_search/thumbnail_page.js:233: if (tileCount <
dataLength)
if one condition in an if/else block has curlies, all of them must.

[commit-bot: I haz the power, 2012-09-13 19:14:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremycho@chromium.org/10907065/1013

[commit-bot: I haz the power, 2012-09-13 21:42:30]

Change committed as 156641