Remove local copies of SuggestionsSource.

Remove local copies of SuggestionsSource.

SuggestionsSource's lifetime is tied to the NewTabPage meaning that if objects
take local copies of SuggestionsSource they are unable to determine if it has
been destroyed.

BUG=649299
Committed: https://crrev.com/9b95dd08e45fbf96dca7c89b1db78a13abdc8f8e
Cr-Commit-Position: refs/heads/master@{#421165}

[PEConn, 2016-09-23 08:44:53]

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

[PEConn, 2016-09-23 08:44:53]

PTAL

[PEConn, 2016-09-23 08:44:58]

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

[commit-bot: I haz the power, 2016-09-23 08:45:17]

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, 2016-09-23 08:59:48]

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

[commit-bot: I haz the power, 2016-09-23 08:59:49]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Michael van Ouwerkerk, 2016-09-23 09:34:09]

lgtm with nits

Nice fix!

https://codereview.chromium.org/2366443005/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/2366443005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:284:
* own copy.
I think it would be more idiomatic to say one keeps a "reference", to
distinguish from keeping a copy (or clone) of the whole object. There was always
just once SuggestionsSource instance per ntp, with multiple references to it...
even after #destroy was called.

https://codereview.chromium.org/2366443005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:286:
SuggestionsSource getSuggestionsSource();
Maybe annotate with @Nullable ?

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

https://codereview.chromium.org/2366443005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:409:
// It is possible for this method to be called once the NewTabPage has been
destroyed.
Could you maybe refer to {@code NewTabPage#destroy} to more clearly distinguish
it from the garbage collection type of destruction? Also, maybe replace
instances of "once" with "after" because "once" kind of implies causality.

[PEConn, 2016-09-23 16:10:51]

https://codereview.chromium.org/2366443005/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/2366443005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:284:
* own copy.
On 2016/09/23 09:34:09, Michael van Ouwerkerk wrote:
> I think it would be more idiomatic to say one keeps a "reference", to
> distinguish from keeping a copy (or clone) of the whole object. There was
always
> just once SuggestionsSource instance per ntp, with multiple references to
it...
> even after #destroy was called.

Done.

https://codereview.chromium.org/2366443005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:286:
SuggestionsSource getSuggestionsSource();
On 2016/09/23 09:34:09, Michael van Ouwerkerk wrote:
> Maybe annotate with @Nullable ?

Done.

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

https://codereview.chromium.org/2366443005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:409:
// It is possible for this method to be called once the NewTabPage has been
destroyed.
On 2016/09/23 09:34:09, Michael van Ouwerkerk wrote:
> Could you maybe refer to {@code NewTabPage#destroy} to more clearly
distinguish
> it from the garbage collection type of destruction? Also, maybe replace
> instances of "once" with "after" because "once" kind of implies causality.

Done.

[PEConn, 2016-09-23 16:10:56]

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

[commit-bot: I haz the power, 2016-09-23 16:11:25]

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, 2016-09-23 16:29:40]

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

[commit-bot: I haz the power, 2016-09-23 16:29:41]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[PEConn, 2016-09-23 16:36:55]

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

[commit-bot: I haz the power, 2016-09-23 16:37:20]

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, 2016-09-23 17:15:45]

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

[commit-bot: I haz the power, 2016-09-23 17:15:46]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[PEConn, 2016-09-26 10:18:06]

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

[commit-bot: I haz the power, 2016-09-26 10:18:24]

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, 2016-09-26 11:32:36]

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

[commit-bot: I haz the power, 2016-09-26 11:32:37]

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...)

[PEConn, 2016-09-26 13:30:43]

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

[commit-bot: I haz the power, 2016-09-26 13:30:58]

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

[PEConn, 2016-09-26 14:55:37]

peconn@chromium.org changed reviewers:
+ tedchoc@chromium.org

[PEConn, 2016-09-26 14:55:38]

tedchoc@chromium.org: Please review changes in 

chrome/android/BUILD.gn
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/OWNERS
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java

[Michael van Ouwerkerk, 2016-09-26 15:12:32]

https://codereview.chromium.org/2366443005/diff/100001/chrome/android/javates...
File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/OWNERS
(right):

https://codereview.chromium.org/2366443005/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/OWNERS:2:
file://chrome/android/java/src/org/chromium/chrome/browser/ntp/OWNERS
tedchoc is also in that file, so it's ok to remove him from this one.

[PEConn, 2016-09-26 15:14:30]

https://codereview.chromium.org/2366443005/diff/100001/chrome/android/javates...
File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/OWNERS
(right):

https://codereview.chromium.org/2366443005/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/OWNERS:2:
file://chrome/android/java/src/org/chromium/chrome/browser/ntp/OWNERS
On 2016/09/26 15:12:32, Michael van Ouwerkerk wrote:
> tedchoc is also in that file, so it's ok to remove him from this one.

Done.

[Ted C, 2016-09-26 21:04:17]

lgtm

https://codereview.chromium.org/2366443005/diff/120001/chrome/android/javates...
File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/OWNERS
(right):

https://codereview.chromium.org/2366443005/diff/120001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/OWNERS:1:
file://chrome/android/java/src/org/chromium/chrome/browser/ntp/OWNERS
Have you seen examples of this working?  I recently tried doing this here:
https://codereview.chromium.org/2360323002, but in a follow up CL they
complained it didn't work: https://codereview.chromium.org/2352513002

Didn't have time to poke around more, but want to make sure I'm not setting a
bad precedent -- although eventually someone would fix it if we added it enough
times...right? :-)

[Michael van Ouwerkerk, 2016-09-27 09:04:57]

https://codereview.chromium.org/2366443005/diff/120001/chrome/android/javates...
File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/OWNERS
(right):

https://codereview.chromium.org/2366443005/diff/120001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/OWNERS:1:
file://chrome/android/java/src/org/chromium/chrome/browser/ntp/OWNERS
On 2016/09/26 21:04:16, Ted C wrote:
> Have you seen examples of this working?  I recently tried doing this here:
> https://codereview.chromium.org/2360323002, but in a follow up CL they
> complained it didn't work: https://codereview.chromium.org/2352513002
> 
> Didn't have time to poke around more, but want to make sure I'm not setting a
> bad precedent -- although eventually someone would fix it if we added it
enough
> times...right? :-)

We use this a lot for push messaging and for notifications:
https://cs.chromium.org/search/?q=f:notifications.*owners+%5Efile&sq=package:...
https://cs.chromium.org/search/?q=f:push.*owners+%5Efile&sq=package:chromium&...

Not sure why it didn't work for that case.

[PEConn, 2016-09-27 09:49:35]

The CQ bit was checked by peconn@chromium.org

[PEConn, 2016-09-27 09:49:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/2366443005/#ps120001
(title: "Added ntp OWNERS to ntp tests.")

[commit-bot: I haz the power, 2016-09-27 09:49:51]

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, 2016-09-27 10:48:24]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-27 10:50:18]

Description was changed from

==========
Remove local copies of SuggestionsSource.

SuggestionsSource's lifetime is tied to the NewTabPage meaning that if objects
take local copies of SuggestionsSource they are unable to determine if it has
been destroyed.

BUG=649299
==========

to

==========
Remove local copies of SuggestionsSource.

SuggestionsSource's lifetime is tied to the NewTabPage meaning that if objects
take local copies of SuggestionsSource they are unable to determine if it has
been destroyed.

BUG=649299

Committed: https://crrev.com/9b95dd08e45fbf96dca7c89b1db78a13abdc8f8e
Cr-Commit-Position: refs/heads/master@{#421165}
==========

[commit-bot: I haz the power, 2016-09-27 10:50:19]

Patchset 7 (id:??) landed as
https://crrev.com/9b95dd08e45fbf96dca7c89b1db78a13abdc8f8e
Cr-Commit-Position: refs/heads/master@{#421165}