[NTP Snippets] Clear the snippets when the user signs out

[NTP Snippets] Clear the snippets when the user signs out

Adds an onSnippetsCleared method to the SnippetsObserver interface,
that gets called when the service wants the snippets to be cleared.
(= when no suggestion hosts are returned.)

Also makes the NewTabPageAdapter stop unregistering its observer
when it received satisfying data for the first time. Instead it
uses a boolean to know whether it should refresh the displayed
snippets or not.

BUG=608913
Committed: https://crrev.com/cbea852c300a928cf29668e0d17e7f27ea596904
Cr-Commit-Position: refs/heads/master@{#394123}

[dgn, 2016-05-16 13:03:56]

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

[dgn, 2016-05-16 13:03:57]

PTAL

[Bernhard Bauer, 2016-05-17 09:12:48]

JUnit tests seem to be failing.

https://codereview.chromium.org/1980033002/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/1980033002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:42:
private boolean mIsExpectingSnippets;
Nit: "is expecting snippets" makes it sound like it's a bug if we get snippets
without expecting them. Maybe "wants snippets"? Or invert it and "ignore
snippets"?

https://codereview.chromium.org/1980033002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/1980033002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:147:
if (mNativeSnippetsBridge == 0) return;
If the native side has been destroyed, this wouldn't be called, no? You could
assert that it's non-zero instead.

https://codereview.chromium.org/1980033002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/1980033002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.h:201: // Sent every time the
service has its data sources cleared.
Maybe add a comment that explains why we do this?

[dgn, 2016-05-17 10:46:01]

PTAL

https://codereview.chromium.org/1980033002/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/1980033002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:42:
private boolean mIsExpectingSnippets;
On 2016/05/17 09:12:48, Bernhard Bauer wrote:
> Nit: "is expecting snippets" makes it sound like it's a bug if we get snippets
> without expecting them. Maybe "wants snippets"? Or invert it and "ignore
> snippets"?

Done.

https://codereview.chromium.org/1980033002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/1980033002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:147:
if (mNativeSnippetsBridge == 0) return;
On 2016/05/17 09:12:48, Bernhard Bauer wrote:
> If the native side has been destroyed, this wouldn't be called, no? You could
> assert that it's non-zero instead.

Removed the thing entirely. I'm not quite sure in which conditions we can run
into such a case with onSnippetsAvailable(). I think making
NTPSnippetsBridge::NTPSnippetsServiceLoaded() being called in SetObserver
instead of in the constructor made that impossible.

https://codereview.chromium.org/1980033002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/1980033002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.h:201: // Sent every time the
service has its data sources cleared.
On 2016/05/17 09:12:48, Bernhard Bauer wrote:
> Maybe add a comment that explains why we do this?

Added comments both here and in snippets_bridge.cc

[Bernhard Bauer, 2016-05-17 12:24:40]

lgtm

https://codereview.chromium.org/1980033002/diff/20001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/1980033002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:172: // The user signed out or
disabled sync. Since snippets rely on those, we
Nit: double "we".

[dgn, 2016-05-17 13:18:22]

Thanks :)

https://codereview.chromium.org/1980033002/diff/20001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/1980033002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:172: // The user signed out or
disabled sync. Since snippets rely on those, we
On 2016/05/17 12:24:39, Bernhard Bauer wrote:
> Nit: double "we".

Done.

[dgn, 2016-05-17 13:18:41]

The CQ bit was checked by dgn@chromium.org

[dgn, 2016-05-17 13:18:41]

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/1980033002/#ps40001
(title: "fix double we")

[commit-bot: I haz the power, 2016-05-17 13:18:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1980033002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1980033002/40001

[commit-bot: I haz the power, 2016-05-17 14:38:50]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-17 14:41:05]

Description was changed from

==========
[NTP Snippets] Clear the snippets when the user signs out

Adds an onSnippetsCleared method to the SnippetsObserver interface,
that gets called when the service wants the snippets to be cleared.
(= when no suggestion hosts are returned.)

Also makes the NewTabPageAdapter stop unregistering its observer
when it received satisfying data for the first time. Instead it
uses a boolean to know whether it should refresh the displayed
snippets or not.

BUG=608913
==========

to

==========
[NTP Snippets] Clear the snippets when the user signs out

Adds an onSnippetsCleared method to the SnippetsObserver interface,
that gets called when the service wants the snippets to be cleared.
(= when no suggestion hosts are returned.)

Also makes the NewTabPageAdapter stop unregistering its observer
when it received satisfying data for the first time. Instead it
uses a boolean to know whether it should refresh the displayed
snippets or not.

BUG=608913

Committed: https://crrev.com/cbea852c300a928cf29668e0d17e7f27ea596904
Cr-Commit-Position: refs/heads/master@{#394123}
==========

[commit-bot: I haz the power, 2016-05-17 14:41:06]

Patchset 3 (id:??) landed as
https://crrev.com/cbea852c300a928cf29668e0d17e7f27ea596904
Cr-Commit-Position: refs/heads/master@{#394123}

[Marc Treib, 2016-05-17 14:57:16]

treib@chromium.org changed reviewers:
+ jkrcal@chromium.org, treib@chromium.org

[Marc Treib, 2016-05-17 14:57:17]

https://codereview.chromium.org/1980033002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1980033002/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:334: if (hosts.empty()) {
Post-commit drive-by comment: After https://codereview.chromium.org/1922083004/
and https://codereview.chromium.org/1974483002/, we now support fetching without
host restrictions, so this might not be the best place to send the "cleared"
event.
(Relatedly, this method is also broken now, since it filters by hosts even if
host restriction is turned off.)

[jkrcal, 2016-05-17 15:11:11]

On 2016/05/17 14:57:17, Marc Treib wrote:
>
https://codereview.chromium.org/1980033002/diff/40001/components/ntp_snippets...
> File components/ntp_snippets/ntp_snippets_service.cc (right):
> 
>
https://codereview.chromium.org/1980033002/diff/40001/components/ntp_snippets...
> components/ntp_snippets/ntp_snippets_service.cc:334: if (hosts.empty()) {
> Post-commit drive-by comment: After
https://codereview.chromium.org/1922083004/
> and https://codereview.chromium.org/1974483002/, we now support fetching
without
> host restrictions, so this might not be the best place to send the "cleared"
> event.
> (Relatedly, this method is also broken now, since it filters by hosts even if
> host restriction is turned off.)

I am addressing the related comment in
https://codereview.chromium.org/1986173003/.