🏠 Extract the ContentSuggestionManager interface from NTP

[Home] Extract the Suggestions delegates from NtpManager

Extracts SuggestionsNavigationDelegate and SuggestionsUiDelegate
interfaces from the NewTabPageManager and moves them to the
suggestions package. Suggestions-related classes now depend on
these rather than on the NewTabPageManager, whose usage is to be
restricted to the new tab page itself.

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

[dgn, 2017-01-12 21:55:39]

Description was changed from

==========
[Home] Extract the ContentSuggestionManager interface from NTP

Extracts ContentSuggestionManager and its implementation from the
ntp package, and move it to the experimental activity.

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

to

==========
[Home] Extract the ContentSuggestionManager interface from NTP

Extracts ContentSuggestionManager and its implementation from the
ntp package, and move it to the experimental activity.

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

[dgn, 2017-01-12 21:55:39]

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

[dgn, 2017-01-12 21:56:26]

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

[commit-bot: I haz the power, 2017-01-12 21:57:02]

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

[dgn, 2017-01-12 21:59:02]

PTAL

https://codereview.chromium.org/2623993007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java
(right):

https://codereview.chromium.org/2623993007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java:28:
private static ChromeActivity sCallerActivity;
In the next patch, I will change the ContentSuggestionsActivity to be a
ChromeActivity, it should fix the issues I have with getting the browser state
(tabModel) without requiring this static here.

I also have a crash when navigating about SafeBrowsing not finding some native
method, I think it might address that too.

[commit-bot: I haz the power, 2017-01-12 23:24:20]

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

[commit-bot: I haz the power, 2017-01-12 23:24:21]

Dry run: This issue passed the CQ dry run.

[tschumann, 2017-01-13 09:05:54]

https://codereview.chromium.org/2623993007/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsManager.java
(right):

https://codereview.chromium.org/2623993007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsManager.java:23:
public interface ContentSuggestionsManager {
drive-by: I have the feeling we're putting too much functionality into this
interface.
I like the idea to have a proper wrapper around the snippetsbridge hiding the
lower-level bridge.

However, a lot of other functionality creeps in here that actually has nothing
to do with managing content suggestions(e.g. offline URLs, navigation, state
checks). It's IMO better to have clear and easy-to use interfaces for these
tasks as well (we can put them next to each other in one package to make
discoverability easier).

I'd also recommend against introducing another generic name like manager -- it's
really unclear what behind it.

Please don't mix these

[Bernhard Bauer, 2017-01-13 09:30:23]

https://codereview.chromium.org/2623993007/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsManager.java
(right):

https://codereview.chromium.org/2623993007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsManager.java:23:
public interface ContentSuggestionsManager {
On 2017/01/13 09:05:54, tschumann wrote:
> drive-by: I have the feeling we're putting too much functionality into this
> interface.
> I like the idea to have a proper wrapper around the snippetsbridge hiding the
> lower-level bridge.
> 
> However, a lot of other functionality creeps in here that actually has nothing
> to do with managing content suggestions(e.g. offline URLs, navigation, state
> checks). It's IMO better to have clear and easy-to use interfaces for these
> tasks as well (we can put them next to each other in one package to make
> discoverability easier).

I think this is mostly because of the confusing name :) I think I would go with
something like [Content]SuggestionsUiDelegate, as this allow the UI to delegate
anything that isn't UI.

Navigations, offline URLs etc are in fact things that are necessary for showing
a suggestions _UI_ -- we already have an interface for handling just suggestions
with the SuggestionsSource. This is quite similar to the model that //content/
uses, where the ContentClient interface contains a lot of different methods and
ChromeContentClient implements them (although I wouldn't necessarily use the
term "client" for this interface either).

That being said, this CL is necessary right now to make a lot of the
functionality in the standalone suggestions UI work; if we want to split the
interface up into smaller ones, we can still do that at a later point.

> I'd also recommend against introducing another generic name like manager --
it's
> really unclear what behind it.
> 
> Please don't mix these

[tschumann, 2017-01-13 09:38:37]

https://codereview.chromium.org/2623993007/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsManager.java
(right):

https://codereview.chromium.org/2623993007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsManager.java:23:
public interface ContentSuggestionsManager {
On 2017/01/13 09:30:23, Bernhard Bauer wrote:
> On 2017/01/13 09:05:54, tschumann wrote:
> > drive-by: I have the feeling we're putting too much functionality into this
> > interface.
> > I like the idea to have a proper wrapper around the snippetsbridge hiding
the
> > lower-level bridge.
> > 
> > However, a lot of other functionality creeps in here that actually has
nothing
> > to do with managing content suggestions(e.g. offline URLs, navigation, state
> > checks). It's IMO better to have clear and easy-to use interfaces for these
> > tasks as well (we can put them next to each other in one package to make
> > discoverability easier).
> 
> I think this is mostly because of the confusing name :) I think I would go
with
> something like [Content]SuggestionsUiDelegate, as this allow the UI to
delegate
> anything that isn't UI.
> 
> Navigations, offline URLs etc are in fact things that are necessary for
showing
> a suggestions _UI_ -- we already have an interface for handling just
suggestions
> with the SuggestionsSource. This is quite similar to the model that //content/
> uses, where the ContentClient interface contains a lot of different methods
and
> ChromeContentClient implements them (although I wouldn't necessarily use the
> term "client" for this interface either).
> 
> That being said, this CL is necessary right now to make a lot of the
> functionality in the standalone suggestions UI work; if we want to split the
> interface up into smaller ones, we can still do that at a later point.
> 
> > I'd also recommend against introducing another generic name like manager --
> it's
> > really unclear what behind it.
> > 
> > Please don't mix these
> 

A different name would certainly help. If all clients/consumers of this
interface need the whole functionality set, then keeping it all in one interface
might make sense. However, if it turns out that some parts only need certain
pieces, I'd vote for splitting it up.
It's a trade-off between convenience and clear, sharp interfaces in the end.

Leaving the discussion and timing up to you. If you can clarify the role with a
better name, then I'm certainly less concerned.
However, I wonder if some functionality should be carved out in classes
corresponding to SnippetsBridge (e.g. the Navigation part) and turning this
interface more into an access point delegating most functionality). But like I
said, just my 2 cents :-)

[dgn, 2017-01-13 11:07:54]

On 2017/01/13 09:38:37, tschumann wrote:
>
https://codereview.chromium.org/2623993007/diff/20001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsManager.java
> (right):
> 
>
https://codereview.chromium.org/2623993007/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsManager.java:23:
> public interface ContentSuggestionsManager {
> On 2017/01/13 09:30:23, Bernhard Bauer wrote:
> > On 2017/01/13 09:05:54, tschumann wrote:
> > > drive-by: I have the feeling we're putting too much functionality into
this
> > > interface.
> > > I like the idea to have a proper wrapper around the snippetsbridge hiding
> the
> > > lower-level bridge.
> > > 
> > > However, a lot of other functionality creeps in here that actually has
> nothing
> > > to do with managing content suggestions(e.g. offline URLs, navigation,
state
> > > checks). It's IMO better to have clear and easy-to use interfaces for
these
> > > tasks as well (we can put them next to each other in one package to make
> > > discoverability easier).
> > 
> > I think this is mostly because of the confusing name :) I think I would go
> with
> > something like [Content]SuggestionsUiDelegate, as this allow the UI to
> delegate
> > anything that isn't UI.
> > 
> > Navigations, offline URLs etc are in fact things that are necessary for
> showing
> > a suggestions _UI_ -- we already have an interface for handling just
> suggestions
> > with the SuggestionsSource. This is quite similar to the model that
//content/
> > uses, where the ContentClient interface contains a lot of different methods
> and
> > ChromeContentClient implements them (although I wouldn't necessarily use the
> > term "client" for this interface either).
> > 
> > That being said, this CL is necessary right now to make a lot of the
> > functionality in the standalone suggestions UI work; if we want to split the
> > interface up into smaller ones, we can still do that at a later point.
> > 
> > > I'd also recommend against introducing another generic name like manager
--
> > it's
> > > really unclear what behind it.
> > > 
> > > Please don't mix these
> > 
> 
> A different name would certainly help. If all clients/consumers of this
> interface need the whole functionality set, then keeping it all in one
interface
> might make sense. However, if it turns out that some parts only need certain
> pieces, I'd vote for splitting it up.
> It's a trade-off between convenience and clear, sharp interfaces in the end.
> 
> Leaving the discussion and timing up to you. If you can clarify the role with
a
> better name, then I'm certainly less concerned.
> However, I wonder if some functionality should be carved out in classes
> corresponding to SnippetsBridge (e.g. the Navigation part) and turning this
> interface more into an access point delegating most functionality). But like I
> said, just my 2 cents :-)

Thanks for your comments, I definitely agree with you that this interface does
too many things. This PatchSet is the first step of splitting things, moving
away from the humongous newtabpagemanager. I will try splitting this more. 

And thanks for the name suggestions!

[Bernhard Bauer, 2017-01-13 14:17:03]

https://codereview.chromium.org/2623993007/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java
(right):

https://codereview.chromium.org/2623993007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java:34:
sCallerActivity = activity;
This is pretty ugly, and I can imagine a couple of cases in which it could go
wrong, like multi-window (which might be an edge case), but also if the caller
activity is destroyed. I guess the final place for this is going to provide us
with these dependencies in a different way, but can we at least use a
WeakReference here so we don't hold on to the activity indefinitely if it is
destroyed, and then treat the rest as a TODO?

[dgn, 2017-01-13 21:24:56]

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

[commit-bot: I haz the power, 2017-01-13 21:25:44]

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

[dgn, 2017-01-13 22:26:57]

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

[commit-bot: I haz the power, 2017-01-13 22:27:45]

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-13 23:59:45]

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

[commit-bot: I haz the power, 2017-01-13 23:59:46]

Dry run: 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...)

[dgn, 2017-01-16 10:35:23]

PTAL

[Bernhard Bauer, 2017-01-16 11:08:37]

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java
(right):

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java:61:
// destroyed.
I think this is causing a FindBugs error during build.

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java:75:
snippetsBridge.destroy();
Can't we just destroy the SnippetsBridge in our own onDestroy method?

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsNavigationDelegateImpl.java
(right):

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsNavigationDelegateImpl.java:46:
private boolean mIsDestroyed;
Is the destroyed check necessary here if we don't call into native code?

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegate.java
(right):

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegate.java:21:
public interface SuggestionsUiDelegate {
I like how small this interface is becoming :)

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegateImpl.java
(right):

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegateImpl.java:66:
if (mFaviconHelper == null) mFaviconHelper = new FaviconHelper();
Could you just create that eagerly? If you want to create it lazily, extract it
into a method? (Maybe one that returns the FaviconHelper, so you can make it
return a fake in tests?)

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegateImpl.java:137:
if (mIsDestroyed) return null;
I think in the long term I would like to push the destroyed flag to the classes
that actually need it, because they call into native code so they need a
destruction method to deallocate their native parts, and rely on garbage
collection to clean up other classes. I wouldn't want to end up in a state where
every single object has a destroyed flag that makes every method a no-op :)

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegateImpl.java:167:
// Iterate over the destruction observers from last to first.
Why is the iteration order important?

[Michael van Ouwerkerk, 2017-01-16 15:31:14]

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

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

Drive-by review :-)

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:437:
SuggestionsNavigationDelegateImpl navigationDelegate =
Can this be of type SuggestionsNavigationDelegate?

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

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:54:
* @param uiDelegate the SuggestionsUiDelegate to use to interact with the rest
of the system.
Nit: the param type is defined below, so no need to repeat it in the docs. Maybe
just say "delegate"?

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsNavigationDelegateImpl.java
(right):

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsNavigationDelegateImpl.java:36:
public class SuggestionsNavigationDelegateImpl
Most of this file is a move from NewTabPage.java, correct? Could you get
Rietveld to show that by uploading the CL with a lower similarity threshold?

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegate.java
(right):

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegate.java:22:
// Dependency injection
Is your plan to inject these dependencies directly in a future CL? If so, please
add a TODO for that.

[Michael van Ouwerkerk, 2017-01-16 15:31:51]

Can you update the CL description with a bit more detail please?

[Michael van Ouwerkerk, 2017-01-18 14:02:44]

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:111:
private LargeIconBridge mLargeIconBridge;
This is now unused, it should be deleted from NewTabPage.java

[dgn, 2017-01-18 16:13:53]

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

[commit-bot: I haz the power, 2017-01-18 16:14:32]

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

[dgn, 2017-01-18 16:22:28]

PTAL

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:111:
private LargeIconBridge mLargeIconBridge;
On 2017/01/18 14:02:44, Michael van Ouwerkerk wrote:
> This is now unused, it should be deleted from NewTabPage.java

Done.

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

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:54:
* @param uiDelegate the SuggestionsUiDelegate to use to interact with the rest
of the system.
On 2017/01/16 15:31:15, Michael van Ouwerkerk wrote:
> Nit: the param type is defined below, so no need to repeat it in the docs.
Maybe
> just say "delegate"?

Done.

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java
(right):

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java:61:
// destroyed.
On 2017/01/16 11:08:37, Bernhard Bauer wrote:
> I think this is causing a FindBugs error during build.

added an assert. Will look at the actual issue better in a follow up

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java:75:
snippetsBridge.destroy();
On 2017/01/16 11:08:37, Bernhard Bauer wrote:
> Can't we just destroy the SnippetsBridge in our own onDestroy method?

Means then that we need to make it a member... I made snippets bridge a
destruction observer. I think we have a neat mechanism for that, might as well
use it.

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsNavigationDelegateImpl.java
(right):

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsNavigationDelegateImpl.java:46:
private boolean mIsDestroyed;
On 2017/01/16 11:08:37, Bernhard Bauer wrote:
> Is the destroyed check necessary here if we don't call into native code?

I don't think so... It was mostly moved from the NTPmanager implementation, but
I might have been unnecessary, so removing, let's see if we get bugs :D

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegate.java
(right):

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegate.java:22:
// Dependency injection
On 2017/01/16 15:31:15, Michael van Ouwerkerk wrote:
> Is your plan to inject these dependencies directly in a future CL? If so,
please
> add a TODO for that.

Done.

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegateImpl.java
(right):

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegateImpl.java:66:
if (mFaviconHelper == null) mFaviconHelper = new FaviconHelper();
On 2017/01/16 11:08:37, Bernhard Bauer wrote:
> Could you just create that eagerly? If you want to create it lazily, extract
it
> into a method? (Maybe one that returns the FaviconHelper, so you can make it
> return a fake in tests?)

extracted to a method.

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegateImpl.java:137:
if (mIsDestroyed) return null;
On 2017/01/16 11:08:37, Bernhard Bauer wrote:
> I think in the long term I would like to push the destroyed flag to the
classes
> that actually need it, because they call into native code so they need a
> destruction method to deallocate their native parts, and rely on garbage
> collection to clean up other classes. I wouldn't want to end up in a state
where
> every single object has a destroyed flag that makes every method a no-op :)

True, also relying on when this method returns null as a way to check when the
tab is destroyed means that it's not safe to just keep the reference to the
suggestion source. Removed that and updated the doc to reflect the change. The
code always assumes we have a reference anyway (except the one place where we
use it to check if we are cardUI mode.)

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegateImpl.java:167:
// Iterate over the destruction observers from last to first.
On 2017/01/16 11:08:37, Bernhard Bauer wrote:
> Why is the iteration order important?

Hum it's not. We're not supposed to ask objects to do things when we get told
that we are destroyed, so regardless of the destruction order, we shouldn't
trigger anything bad. Reverted to the regular loop

[Bernhard Bauer, 2017-01-18 16:58:14]

LGTM when Michael is happy.

https://codereview.chromium.org/2623993007/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/2623993007/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:284:
/* Adapter for {@link #mRecyclerView}. Will be {@code null} when using the old
UI */
Use // for non-Java comments.

https://codereview.chromium.org/2623993007/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegateImpl.java
(right):

https://codereview.chromium.org/2623993007/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegateImpl.java:197:

Nit: these empty lines seem unnecessary.

https://codereview.chromium.org/2623993007/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java
(right):

https://codereview.chromium.org/2623993007/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java:83:
/*contextMenuManager=*/null);
Could you add spaces around the variable name and equals sign? It looks a bit
cramped to me otherwise.

[dgn, 2017-01-18 18:00:27]

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java
(right):

https://codereview.chromium.org/2623993007/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java:75:
snippetsBridge.destroy();
Used a member and the old #destroy() method in the end.

https://codereview.chromium.org/2623993007/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/2623993007/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:284:
/* Adapter for {@link #mRecyclerView}. Will be {@code null} when using the old
UI */
On 2017/01/18 16:58:14, Bernhard Bauer wrote:
> Use // for non-Java comments.

removed, that was the old javadoc.

https://codereview.chromium.org/2623993007/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegateImpl.java
(right):

https://codereview.chromium.org/2623993007/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/SuggestionsUiDelegateImpl.java:197:

On 2017/01/18 16:58:14, Bernhard Bauer wrote:
> Nit: these empty lines seem unnecessary.

Done.

https://codereview.chromium.org/2623993007/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java
(right):

https://codereview.chromium.org/2623993007/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java:83:
/*contextMenuManager=*/null);
On 2017/01/18 16:58:14, Bernhard Bauer wrote:
> Could you add spaces around the variable name and equals sign? It looks a bit
> cramped to me otherwise.

Done.

[Michael van Ouwerkerk, 2017-01-18 18:23:19]

Could you please update the CL description? You ended up extracting some
delegates, not a ContentSuggestionManager.

[Michael van Ouwerkerk, 2017-01-18 19:35:55]

lgtm

[Michael van Ouwerkerk, 2017-01-18 19:36:04]

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

[commit-bot: I haz the power, 2017-01-18 19:36:49]

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-18 21:26:56]

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

[commit-bot: I haz the power, 2017-01-18 21:26:57]

Dry run: 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...)

[dgn, 2017-01-19 09:54:06]

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

[commit-bot: I haz the power, 2017-01-19 09:54:27]

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

[dgn, 2017-01-19 09:57:17]

Description was changed from

==========
[Home] Extract the ContentSuggestionManager interface from NTP

Extracts ContentSuggestionManager and its implementation from the
ntp package, and move it to the experimental activity.

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

to

==========
[Home] Extract the Suggestions delegates from NtpManager

Extracts SuggestionsNavigationDelegate and SuggestionsUiDelegate
interfaces from the NewTabPageManager and moves them to the
suggestions package. Suggestions-related classes now depend on
these rather than on the NewTabPageManager, whose usage is to be
restricted to the new tab page itself.

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

[dgn, 2017-01-19 09:57:28]

The CQ bit was unchecked by dgn@chromium.org

[dgn, 2017-01-19 09:57:28]

The CQ bit was checked by dgn@chromium.org

[dgn, 2017-01-19 09:57:29]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/2623993007/#ps220001
(title: "findbuuugs! ლ(ಠ益ಠლ)")

[commit-bot: I haz the power, 2017-01-19 09:57: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-01-19 10:56:11]

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

[commit-bot: I haz the power, 2017-01-19 10:56:13]

Failed to apply patch for
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:
While running git apply --index -p1;
  error: patch failed:
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:70
  error:
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:
patch does not apply

Patch:      
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
Index:
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
diff --git
a/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
index
2d7b839225148c61ac4488e8d628f803784059ae..62cf6d0168d45ddaa86380ce6d84d1c0e02e2d05
100644
---
a/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
+++
b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
@@ -8,7 +8,6 @@
 import org.chromium.base.Log;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.chrome.browser.ntp.NewTabPage.DestructionObserver;
-import org.chromium.chrome.browser.ntp.NewTabPageView.NewTabPageManager;
 import org.chromium.chrome.browser.ntp.snippets.CategoryInt;
 import
org.chromium.chrome.browser.ntp.snippets.CategoryStatus.CategoryStatusEnum;
 import org.chromium.chrome.browser.ntp.snippets.SectionHeader;
@@ -20,6 +19,7 @@
 import org.chromium.chrome.browser.offlinepages.OfflinePageBridge;
 import org.chromium.chrome.browser.offlinepages.OfflinePageItem;
 import org.chromium.chrome.browser.suggestions.SuggestionsRanker;
+import org.chromium.chrome.browser.suggestions.SuggestionsUiDelegate;
 
 import java.util.ArrayList;
 import java.util.Iterator;
@@ -61,7 +61,7 @@
         void dismissSection(SuggestionsSection section);
     }
 
-    public SuggestionsSection(Delegate delegate, NewTabPageManager manager,
+    public SuggestionsSection(Delegate delegate, SuggestionsUiDelegate
uiDelegate,
             SuggestionsRanker ranker, OfflinePageBridge offlinePageBridge,
             SuggestionsCategoryInfo info) {
         mDelegate = delegate;
@@ -70,24 +70,25 @@ public SuggestionsSection(Delegate delegate,
NewTabPageManager manager,
         mSuggestionsRanker = ranker;
 
         mHeader = new SectionHeader(info.getTitle());
-        mSuggestionsList = new SuggestionsList(manager, info);
+        mSuggestionsList = new SuggestionsList(uiDelegate, info);
         mStatus = StatusItem.createNoSuggestionsItem(info);
         mMoreButton = new ActionItem(this, ranker);
         mProgressIndicator = new ProgressItem();
         addChildren(mHeader, mSuggestionsList, mStatus, mMoreButton,
mProgressIndicator);
 
-        setupOfflinePageBridgeObserver(manager);
+        setupOfflinePageBridgeObserver(uiDelegate);
         refreshChildrenVisibility();
     }
 
     private static class SuggestionsList extends ChildNode implements
Iterable<SnippetArticle> {
         private final List<SnippetArticle> mSuggestions = new ArrayList<>();
-        private final NewTabPageManager mNewTabPageManager;
+        // TODO(crbug.com/677672): Replace by SuggestionSource when it handles
destruction.
+        private final SuggestionsUiDelegate mUiDelegate;
         private final SuggestionsCategoryInfo mCategoryInfo;
 
-        public SuggestionsList(NewTabPageManager newTabPageManager,
-                SuggestionsCategoryInfo categoryInfo) {
-            mNewTabPageManager = newTabPageManager;
+        public SuggestionsList(
+                SuggestionsUiDelegate uiDelegate, SuggestionsCategoryInfo
categoryInfo) {
+            mUiDelegate = uiDelegate;
             mCategoryInfo = categoryInfo;
         }
 
@@ -164,7 +165,7 @@ public SnippetArticle remove(int position) {
         @Override
         public void dismissItem(int position, Callback<String>
itemRemovedCallback) {
             checkIndex(position);
-            SuggestionsSource suggestionsSource =
mNewTabPageManager.getSuggestionsSource();
+            SuggestionsSource suggestionsSource =
mUiDelegate.getSuggestionsSource();
             if (suggestionsSource == null) {
                 // It is possible for this method to be called after the
NewTabPage has had
                 // destroy() called. This can happen when
@@ -189,7 +190,7 @@ public void updateSuggestionOfflineId(SnippetArticle
article, Long newId) {
         }
     }
 
-    private void setupOfflinePageBridgeObserver(NewTabPageManager manager) {
+    private void setupOfflinePageBridgeObserver(SuggestionsUiDelegate
uiDelegate) {
         final OfflinePageBridge.OfflinePageModelObserver observer =
                 new OfflinePageBridge.OfflinePageModelObserver() {
                     @Override
@@ -219,7 +220,7 @@ public void offlinePageDeleted(long offlineId, ClientId
clientId) {
 
         mOfflinePageBridge.addObserver(observer);
 
-        manager.addDestructionObserver(new DestructionObserver() {
+        uiDelegate.addDestructionObserver(new DestructionObserver() {
             @Override
             public void onDestroy() {
                 mIsNtpDestroyed = true;

[dgn, 2017-01-19 11:18:09]

The CQ bit was checked by dgn@chromium.org

[dgn, 2017-01-19 11:18:10]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/2623993007/#ps240001
(title: "aaaand rebase again")

[commit-bot: I haz the power, 2017-01-19 11:18:30]

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-19 12:38:04]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1484824689207660,
"parent_rev": "9de525a39fecbf821804f1d7c8e2a09dde3c8f98", "commit_rev":
"08b64649caebdddd433ac2576753af00132a71ea"}

[commit-bot: I haz the power, 2017-01-19 12:38:32]

Description was changed from

==========
[Home] Extract the Suggestions delegates from NtpManager

Extracts SuggestionsNavigationDelegate and SuggestionsUiDelegate
interfaces from the NewTabPageManager and moves them to the
suggestions package. Suggestions-related classes now depend on
these rather than on the NewTabPageManager, whose usage is to be
restricted to the new tab page itself.

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

to

==========
[Home] Extract the Suggestions delegates from NtpManager

Extracts SuggestionsNavigationDelegate and SuggestionsUiDelegate
interfaces from the NewTabPageManager and moves them to the
suggestions package. Suggestions-related classes now depend on
these rather than on the NewTabPageManager, whose usage is to be
restricted to the new tab page itself.

BUG=677672

Review-Url: https://codereview.chromium.org/2623993007
Cr-Commit-Position: refs/heads/master@{#444711}
Committed:
https://chromium.googlesource.com/chromium/src/+/08b64649caebdddd433ac2576753...
==========

[commit-bot: I haz the power, 2017-01-19 12:38:35]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/08b64649caebdddd433ac2576753...