🍝🏠 Refactor MostVisited UI management, extract it for reuse in Home.

🍝🏠 Refactor MostVisited UI management, extract it for reuse in Home.

* Tile is MostVisitedItem stripped of logic, just a data holder.
* TileGroup implements MostVisitedURLsObserver and is extracted from NewTabPageView.
* TileGroup.Delegate replaces MostVisitedItemManager and bits of NewTabPageManager.
* TileGroupDelegateImpl is extracted from NewTabPageManagerImpl.
* TileGroup.Observer allows custom actions to be taken for certain events in TileGroup.
* TileGrid and NewTabPageView now have a TileGroup.

BUG=677672
Review-Url: https://codereview.chromium.org/2651673010
Cr-Commit-Position: refs/heads/master@{#448051}
Committed: https://chromium.googlesource.com/chromium/src/+/d33a67d4d751a32dd524911fc1b98b584d56f91e

[Michael van Ouwerkerk, 2017-01-27 11:42:42]

Description was changed from

==========
🍝 Refactor MostVisited UI management, extract it for reuse in Home.

BUG=
==========

to

==========
🍝📰🏠 Refactor MostVisited UI management, extract it for reuse in Home.

BUG=
==========

[Michael van Ouwerkerk, 2017-01-27 11:42:59]

Description was changed from

==========
🍝📰🏠 Refactor MostVisited UI management, extract it for reuse in Home.

BUG=
==========

to

==========
🍝🏠 Refactor MostVisited UI management, extract it for reuse in Home.

BUG=
==========

[Michael van Ouwerkerk, 2017-01-27 12:03:17]

mvanouwerkerk@chromium.org changed reviewers:
+ bauerb@chromium.org

[Michael van Ouwerkerk, 2017-01-27 12:03:18]

Bernhard, this is a bit rough, but maybe you'd like to take a look already?

Known issues:
* Some debug lines.
* Some todos.
* No tests checked so surely broken.

[Michael van Ouwerkerk, 2017-01-27 12:04:14]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 12:04:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 12:32:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 12:32:44]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Bernhard Bauer, 2017-01-27 14:08:44]

Nice! Some places where we might be able to do some cleanup:

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:327:
private class NewTabPageTileGroupDelegate extends TileGroupDelegateImpl {
Add a comment that the functionality this adds over the base class is pretty
much just logging?

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemViewType.java
(right):

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemViewType.java:29:
ItemViewType.TILE_GRID
I would put this after ABOVE_THE_FOLD, so the items are roughly in the order in
which they appear in the UI. We don't need to care about backwards compatibility
with the values.

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:49:
@Nullable
Nit: Empty line before the annotation?

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:91:
&& ChromeFeatureList.isEnabled(ChromeFeatureList.NTP_SUGGESTIONS_STANDALONE_UI))
{
I would make this a different feature. And actually, maybe we want to do the
feature check when we create the adapter? It seems a bit strange to pass in a
tileGroupDelegate if we might end up not using it. (FTR, that's why I based the
above-the-fold item on a non-null view.)

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java
(right):

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:358:
public class Tile implements OnCreateContextMenuListener, OnClickListener {
I would almost be tempted to make Tile a really dumb pure model class that
doesn't know how to create a view or load its icon...

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:387:
mTileType = MostVisitedTileType.NONE;
Initialize this where it's declared?

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:464:
public void setTileType(int type) {
Add an @IntDef for the type? Also, should we maybe rename this to icon type?

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:528:
Bitmap bitmap = BitmapFactory.decodeFile(getWhitelistIconPath());
IIRC (okay, I don't, I just looked it up) there was some discussion in the
initial CL where this landed about using an AsyncTask instead
(https://codereview.chromium.org/1772363002/diff/100001/chrome/android/java/sr...).
Would now be a good time to do that?

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java
(right):

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java:33:
* Implements {@link TileGroup.Delegate}.
Duh :)

[dgn, 2017-01-27 17:56:43]

dgn@chromium.org changed reviewers:
+ dgn@chromium.org

[dgn, 2017-01-27 17:56:44]

wow this is so much bigger than I expected. Real spaghetti.

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:327:
private class NewTabPageTileGroupDelegate extends TileGroupDelegateImpl {
On 2017/01/27 14:08:44, Bernhard Bauer wrote:
> Add a comment that the functionality this adds over the base class is pretty
> much just logging?

Won't we want that logging in chrome home too? Shouldn't this move to the
suggestions package as well?

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java
(right):

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java:63:
public void openMostVisitedItem(int windowDisposition, Tile item) {
I think this (and the switchToExistingTab method) would be at their place in the
NavigationDelegate. The caller (click or context menu) can make the UMA call
separately by itself.

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java:145:
private void recordOpenedMostVisitedItem(TileGroup.Tile item) {
move to MetricsRecorder?

[Michael van Ouwerkerk, 2017-01-31 16:35:23]

Description was changed from

==========
🍝🏠 Refactor MostVisited UI management, extract it for reuse in Home.

BUG=
==========

to

==========
🍝🏠 Refactor MostVisited UI management, extract it for reuse in Home.

BUG=677672
==========

[Michael van Ouwerkerk, 2017-01-31 16:37:20]

Please take another look. This is much cleaner, but the tests are still busted.

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:327:
private class NewTabPageTileGroupDelegate extends TileGroupDelegateImpl {
On 2017/01/27 14:08:44, Bernhard Bauer wrote:
> Add a comment that the functionality this adds over the base class is pretty
> much just logging?

Done.

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:327:
private class NewTabPageTileGroupDelegate extends TileGroupDelegateImpl {
On 2017/01/27 17:56:44, dgn wrote:
> On 2017/01/27 14:08:44, Bernhard Bauer wrote:
> > Add a comment that the functionality this adds over the base class is pretty
> > much just logging?
> 
> Won't we want that logging in chrome home too? Shouldn't this move to the
> suggestions package as well?

I think we'll need to add a similar wrapper for Home, with some different
histogram names. I've added a TODO for that in TileGroupDelegateImpl.

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemViewType.java
(right):

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemViewType.java:29:
ItemViewType.TILE_GRID
On 2017/01/27 14:08:44, Bernhard Bauer wrote:
> I would put this after ABOVE_THE_FOLD, so the items are roughly in the order
in
> which they appear in the UI. We don't need to care about backwards
compatibility
> with the values.

Done.

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:49:
@Nullable
On 2017/01/27 14:08:44, Bernhard Bauer wrote:
> Nit: Empty line before the annotation?

Done.

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:91:
&& ChromeFeatureList.isEnabled(ChromeFeatureList.NTP_SUGGESTIONS_STANDALONE_UI))
{
On 2017/01/27 14:08:44, Bernhard Bauer wrote:
> I would make this a different feature. And actually, maybe we want to do the
> feature check when we create the adapter? It seems a bit strange to pass in a
> tileGroupDelegate if we might end up not using it. (FTR, that's why I based
the
> above-the-fold item on a non-null view.)

Actually, we know we can always pass in null from NewTabPageView. Seems like the
simplest approach. Done.

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java
(right):

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:358:
public class Tile implements OnCreateContextMenuListener, OnClickListener {
On 2017/01/27 14:08:44, Bernhard Bauer wrote:
> I would almost be tempted to make Tile a really dumb pure model class that
> doesn't know how to create a view or load its icon...

Me too, and these classes should not have any View as a member. Done.

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:387:
mTileType = MostVisitedTileType.NONE;
On 2017/01/27 14:08:44, Bernhard Bauer wrote:
> Initialize this where it's declared?

Ah yes. (that was the old code, btw)

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:464:
public void setTileType(int type) {
On 2017/01/27 14:08:44, Bernhard Bauer wrote:
> Add an @IntDef for the type? Also, should we maybe rename this to icon type?

Yes I have a note for such things, but didn't want to add more to this already
large CL. I'd like to focus on the class design and layering in this CL, and
leave things like disruptive moves and renames for later where reasonable.

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:528:
Bitmap bitmap = BitmapFactory.decodeFile(getWhitelistIconPath());
On 2017/01/27 14:08:44, Bernhard Bauer wrote:
> IIRC (okay, I don't, I just looked it up) there was some discussion in the
> initial CL where this landed about using an AsyncTask instead
>
(https://codereview.chromium.org/1772363002/diff/100001/chrome/android/java/sr...).
> Would now be a good time to do that?

Now seems like a good time to add a TODO, I've added one.

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java
(right):

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java:33:
* Implements {@link TileGroup.Delegate}.
On 2017/01/27 14:08:44, Bernhard Bauer wrote:
> Duh :)

Yes it's the obvious thing... any ideas for being more informative here? I think
the comment accurately reflects what this does :-)

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java:63:
public void openMostVisitedItem(int windowDisposition, Tile item) {
On 2017/01/27 17:56:44, dgn wrote:
> I think this (and the switchToExistingTab method) would be at their place in
the
> NavigationDelegate. The caller (click or context menu) can make the UMA call
> separately by itself.

I've added a TODO for resolving all the metrics issues. It will be a smaller CL
that requires special attention to our metrics story for Home.

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java:145:
private void recordOpenedMostVisitedItem(TileGroup.Tile item) {
On 2017/01/27 17:56:44, dgn wrote:
> move to MetricsRecorder? 

I think we'll need separate histogram names for Home, so we'll have something
like a ChomeHomeMetricsRecorder or a SuggestionsUma class that mirrors
NewTabPageUma.

[Michael van Ouwerkerk, 2017-01-31 16:44:03]

Description was changed from

==========
🍝🏠 Refactor MostVisited UI management, extract it for reuse in Home.

BUG=677672
==========

to

==========
🍝🏠 Refactor MostVisited UI management, extract it for reuse in Home.

* Tile is MostVisitedItem stripped of logic, just a data holder.
* TileGroup implements MostVisitedURLsObserver and is extracted from
NewTabPageView.
* TileGroup.Delegate replaces MostVisitedItemManager and bits of
NewTabPageManager.
* TileGroupDelegateImpl is extracted from NewTabPageManagerImpl.
* TileGroup.Observer allows custom actions to be taken for certain events in
TileGroup.
* TileGrid and NewTabPageView now have a TileGroup.
* Partial binding now uses callbacks to pass information around.

BUG=677672
==========

[Michael van Ouwerkerk, 2017-01-31 17:40:29]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 17:41:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 17:43:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 17:43:05]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[dgn, 2017-01-31 18:21:35]

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/PartialUpdateId.java
(left):

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/PartialUpdateId.java:1:
// Copyright 2017 The Chromium Authors. All rights reserved.
conflicts incoming... there are 2 of these now, with a 3rd one in flight. Maybe
the refactor of partial upload from ints to callback could be a separate CL?

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/suggestions/Tile.java
(right):

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/Tile.java:84:
public void setIndex(int index) {
is this another of this occurrences of "index" being used as "thing that was the
index of the tile at some point but may be inaccurate because one got removed
and we didn't update the index of the subsequent ones, but we use that in UMA
anyway"? I've been using "rank" instead for SnippetArticle and adding to the
documentation caveats on the accuracy of the value.

I think at least the documentation should be clarified if we can indeed have
this index not match the real index of the tile.

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGrid.java
(right):

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGrid.java:21:
public class TileGrid extends Leaf implements TileGroup.Observer {
what if the user removes all the tiles? do we still leave the grid in? Shouldn't
that be an optionalleaf instead?

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGrid.java:57:
public void onTileCountChanged() {}
why no notification here?

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java
(right):

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:270:
// Look for an existing tile to reuse.
this is useful only to keep the drawable, right? how about creating a tile
anyway, implementing this in Tile#equals and just moving over the drawable?

Edit: it doesn't look like we check to not reload the icon anywhere. Am I
missing it?

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java
(right):

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java:56:
public void removeMostVisitedItem(Tile item) {
assert isdestroyed?

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java:87:
public void onLoadingComplete(Tile[] items) {
assert isDestroyed?

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java:157:
CommandLine.getInstance().getSwitchValue(ChromeSwitches.NTP_SWITCH_TO_EXISTING_TAB);
That flag was added for an M49 experiment. Can we remove it now?

https://bugs.chromium.org/p/chromium/issues/detail?id=565953

File a new bug or update the old one to prioritise this?

[Bernhard Bauer, 2017-02-01 13:26:20]

Thanks!

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java
(right):

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java:33:
* Implements {@link TileGroup.Delegate}.
On 2017/01/31 16:37:20, Michael van Ouwerkerk wrote:
> On 2017/01/27 14:08:44, Bernhard Bauer wrote:
> > Duh :)
> 
> Yes it's the obvious thing... any ideas for being more informative here? I
think
> the comment accurately reflects what this does :-)

Maybe explain why we have a separate class in the first place? "Abstracts away
the non-UI dependencies of a tile group" or something?

https://codereview.chromium.org/2651673010/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItemView.java
(right):

https://codereview.chromium.org/2651673010/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItemView.java:55:
* Sets the site url. This is used to identify the view.
Nit: capitalize URL

https://codereview.chromium.org/2651673010/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedLayout.java
(right):

https://codereview.chromium.org/2651673010/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedLayout.java:72:
* Sets a new icon on the child view with a matching url.
Capitalize URL here as well please.

https://codereview.chromium.org/2651673010/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageViewHolder.java
(right):

https://codereview.chromium.org/2651673010/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageViewHolder.java:19:
* @see org.chromium.chrome.browser.ntp.cards.InnerNode#notifyItemChanged(int,
Object)
I think it would be good to change notifyItemChanged() to accept a Callback and
not a general Object.

https://codereview.chromium.org/2651673010/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageViewHolder.java:21:
public abstract static class PartialBindCallback extends
Callback<NewTabPageViewHolder> {
Why not use Callback<NewTabPageViewHolder> straight away? Or, if we are using a
new class, we could give the method a better name than onResult.

[Michael van Ouwerkerk, 2017-02-01 15:17:35]

https://codereview.chromium.org/2651673010/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageViewHolder.java
(right):

https://codereview.chromium.org/2651673010/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageViewHolder.java:19:
* @see org.chromium.chrome.browser.ntp.cards.InnerNode#notifyItemChanged(int,
Object)
On 2017/02/01 13:26:20, Bernhard Bauer wrote:
> I think it would be good to change notifyItemChanged() to accept a Callback
and
> not a general Object.

Done. I moved the partial bind callback changes to
https://codereview.chromium.org/2665983005/

https://codereview.chromium.org/2651673010/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageViewHolder.java:21:
public abstract static class PartialBindCallback extends
Callback<NewTabPageViewHolder> {
On 2017/02/01 13:26:20, Bernhard Bauer wrote:
> Why not use Callback<NewTabPageViewHolder> straight away? Or, if we are using
a
> new class, we could give the method a better name than onResult.

Because of type erasure. You can't do this:
assert payload instanceof Callback<NewTabPageViewHolder>;

I kind of like the consistency of sticking to the Callback signature. It behaves
like a callback, it accepts an argument unlike e.g. a Runnable which has a
prettier method name.

I've moved the partial bind callback changes to
https://codereview.chromium.org/2665983005/

[Michael van Ouwerkerk, 2017-02-03 12:36:55]

Description was changed from

==========
🍝🏠 Refactor MostVisited UI management, extract it for reuse in Home.

* Tile is MostVisitedItem stripped of logic, just a data holder.
* TileGroup implements MostVisitedURLsObserver and is extracted from
NewTabPageView.
* TileGroup.Delegate replaces MostVisitedItemManager and bits of
NewTabPageManager.
* TileGroupDelegateImpl is extracted from NewTabPageManagerImpl.
* TileGroup.Observer allows custom actions to be taken for certain events in
TileGroup.
* TileGrid and NewTabPageView now have a TileGroup.
* Partial binding now uses callbacks to pass information around.

BUG=677672
==========

to

==========
🍝🏠 Refactor MostVisited UI management, extract it for reuse in Home.

* Tile is MostVisitedItem stripped of logic, just a data holder.
* TileGroup implements MostVisitedURLsObserver and is extracted from
NewTabPageView.
* TileGroup.Delegate replaces MostVisitedItemManager and bits of
NewTabPageManager.
* TileGroupDelegateImpl is extracted from NewTabPageManagerImpl.
* TileGroup.Observer allows custom actions to be taken for certain events in
TileGroup.
* TileGrid and NewTabPageView now have a TileGroup.

BUG=677672
==========

[Michael van Ouwerkerk, 2017-02-03 12:37:49]

Please take another look.

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java
(right):

https://codereview.chromium.org/2651673010/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java:33:
* Implements {@link TileGroup.Delegate}.
On 2017/02/01 13:26:20, Bernhard Bauer wrote:
> On 2017/01/31 16:37:20, Michael van Ouwerkerk wrote:
> > On 2017/01/27 14:08:44, Bernhard Bauer wrote:
> > > Duh :)
> > 
> > Yes it's the obvious thing... any ideas for being more informative here? I
> think
> > the comment accurately reflects what this does :-)
> 
> Maybe explain why we have a separate class in the first place? "Abstracts away
> the non-UI dependencies of a tile group" or something?

Done.

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/PartialUpdateId.java
(left):

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/PartialUpdateId.java:1:
// Copyright 2017 The Chromium Authors. All rights reserved.
On 2017/01/31 18:21:35, dgn wrote:
> conflicts incoming... there are 2 of these now, with a 3rd one in flight.
Maybe
> the refactor of partial upload from ints to callback could be a separate CL?

Done.

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/suggestions/Tile.java
(right):

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/Tile.java:84:
public void setIndex(int index) {
On 2017/01/31 18:21:35, dgn wrote:
> is this another of this occurrences of "index" being used as "thing that was
the
> index of the tile at some point but may be inaccurate because one got removed
> and we didn't update the index of the subsequent ones, but we use that in UMA
> anyway"? I've been using "rank" instead for SnippetArticle and adding to the
> documentation caveats on the accuracy of the value.
> 
> I think at least the documentation should be clarified if we can indeed have
> this index not match the real index of the tile.

These are always correct, the tiles are rebuilt / reused+reset when one is
removed.

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGrid.java
(right):

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGrid.java:21:
public class TileGrid extends Leaf implements TileGroup.Observer {
On 2017/01/31 18:21:35, dgn wrote:
> what if the user removes all the tiles? do we still leave the grid in?
Shouldn't
> that be an optionalleaf instead?

It was rendering with 0 height so the user wouldn't know, but with OptionalLeaf
it's cleaner. Done.

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGrid.java:57:
public void onTileCountChanged() {}
On 2017/01/31 18:21:35, dgn wrote:
> why no notification here?

Done.

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java
(right):

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroup.java:270:
// Look for an existing tile to reuse.
On 2017/01/31 18:21:35, dgn wrote:
> this is useful only to keep the drawable, right? how about creating a tile
> anyway, implementing this in Tile#equals and just moving over the drawable?
> 
> Edit: it doesn't look like we check to not reload the icon anywhere. Am I
> missing it?

We currently recreate the views and reload the icons every time the data
changes. I've added a TODO for optimizing this in a later CL, as it might be an
interesting change and I don't want to slow down this initial refactor.

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java
(right):

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java:56:
public void removeMostVisitedItem(Tile item) {
On 2017/01/31 18:21:35, dgn wrote:
> assert isdestroyed?

Done.

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java:87:
public void onLoadingComplete(Tile[] items) {
On 2017/01/31 18:21:35, dgn wrote:
> assert isDestroyed?

Done.

https://codereview.chromium.org/2651673010/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/TileGroupDelegateImpl.java:157:
CommandLine.getInstance().getSwitchValue(ChromeSwitches.NTP_SWITCH_TO_EXISTING_TAB);
On 2017/01/31 18:21:35, dgn wrote:
> That flag was added for an M49 experiment. Can we remove it now?
> 
> https://bugs.chromium.org/p/chromium/issues/detail?id=565953
> 
> File a new bug or update the old one to prioritise this?

I've updated the bug.

https://codereview.chromium.org/2651673010/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItemView.java
(right):

https://codereview.chromium.org/2651673010/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItemView.java:55:
* Sets the site url. This is used to identify the view.
On 2017/02/01 13:26:20, Bernhard Bauer wrote:
> Nit: capitalize URL

Done.

https://codereview.chromium.org/2651673010/diff/220001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedLayout.java
(right):

https://codereview.chromium.org/2651673010/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedLayout.java:72:
* Sets a new icon on the child view with a matching url.
On 2017/02/01 13:26:20, Bernhard Bauer wrote:
> Capitalize URL here as well please.

Done.

[Michael van Ouwerkerk, 2017-02-03 12:37:55]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 12:42:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bernhard Bauer, 2017-02-03 12:47:38]

lgtm

https://codereview.chromium.org/2651673010/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2651673010/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:50:
private final TileGrid mTileGrid;
Oh hey, do we even need this as a member?

[Michael van Ouwerkerk, 2017-02-03 14:18:10]

https://codereview.chromium.org/2651673010/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/2651673010/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:50:
private final TileGrid mTileGrid;
On 2017/02/03 12:47:38, Bernhard Bauer wrote:
> Oh hey, do we even need this as a member?

We don't! Done.

[dgn, 2017-02-03 14:34:33]

lgtm

[Michael van Ouwerkerk, 2017-02-03 14:42:05]

The CQ bit was checked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2017-02-03 14:42:06]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2651673010/#ps260001
(title: "Remove mTileGrid.")

[commit-bot: I haz the power, 2017-02-03 14:42:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 16:26:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-03 16:26:16]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Michael van Ouwerkerk, 2017-02-03 18:12:04]

The CQ bit was checked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2017-02-03 18:12:04]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/2651673010/#ps280001
(title: "Return a copy of mTiles.")

[commit-bot: I haz the power, 2017-02-03 18:12:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 18:15:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-03 18:15:22]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Michael van Ouwerkerk, 2017-02-03 18:21:44]

The CQ bit was checked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2017-02-03 18:21:45]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/2651673010/#ps300001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-02-03 18:22:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 19:38:21]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1486146104625720,
"parent_rev": "ae5a63adbd059abfce2a9479253be2e2035b2de4", "commit_rev":
"d33a67d4d751a32dd524911fc1b98b584d56f91e"}

[commit-bot: I haz the power, 2017-02-03 19:38:58]

Description was changed from

==========
🍝🏠 Refactor MostVisited UI management, extract it for reuse in Home.

* Tile is MostVisitedItem stripped of logic, just a data holder.
* TileGroup implements MostVisitedURLsObserver and is extracted from
NewTabPageView.
* TileGroup.Delegate replaces MostVisitedItemManager and bits of
NewTabPageManager.
* TileGroupDelegateImpl is extracted from NewTabPageManagerImpl.
* TileGroup.Observer allows custom actions to be taken for certain events in
TileGroup.
* TileGrid and NewTabPageView now have a TileGroup.

BUG=677672
==========

to

==========
🍝🏠 Refactor MostVisited UI management, extract it for reuse in Home.

* Tile is MostVisitedItem stripped of logic, just a data holder.
* TileGroup implements MostVisitedURLsObserver and is extracted from
NewTabPageView.
* TileGroup.Delegate replaces MostVisitedItemManager and bits of
NewTabPageManager.
* TileGroupDelegateImpl is extracted from NewTabPageManagerImpl.
* TileGroup.Observer allows custom actions to be taken for certain events in
TileGroup.
* TileGrid and NewTabPageView now have a TileGroup.

BUG=677672

Review-Url: https://codereview.chromium.org/2651673010
Cr-Commit-Position: refs/heads/master@{#448051}
Committed:
https://chromium.googlesource.com/chromium/src/+/d33a67d4d751a32dd524911fc1b9...
==========

[commit-bot: I haz the power, 2017-02-03 19:38:59]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/d33a67d4d751a32dd524911fc1b9...