📰 Let the backend trigger sign in related refreshes

[NTP Client] Let the backend trigger sign in related refreshes

The detection of when to trigger a refresh after a sign in had
a few issues:

- If the signin promo was dismissed, we would not do it
- We would not refresh on sign out because we ignore NOT_PROVIDED
state changes
- The order of the refresh between the UI and the backend was
not explictly defined, meaning we could start refreshing the UI too
early if the SignInManager changed.

BUG=657936
Committed: https://crrev.com/f5708892a89abfb09a81a1c8198f134c42d22010
Cr-Commit-Position: refs/heads/master@{#433830}

[dgn, 2016-11-21 15:27:22]

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

[dgn, 2016-11-21 15:27:23]

PTAL

[Bernhard Bauer, 2016-11-21 15:48:15]

Nice!

https://codereview.chromium.org/2519053002/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/2519053002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:300:
// TODO(dgn): We currently don't have any case where we enter this state.
assert false then?

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

https://codereview.chromium.org/2519053002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java:39:
/** Called when the observer should discard the suggestions is has a pull new
ones. */
Nit: the end of this sentence doesn't parse.

https://codereview.chromium.org/2519053002/diff/1/chrome/browser/ui/webui/sni...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/2519053002/diff/1/chrome/browser/ui/webui/sni...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:154: if
(!dom_loaded_) {
It might be worth moving this check into SendContentSuggestions().

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_provider.h:126: virtual void
OnSignInStateChanged(){};
Nit: space before braces, and no semicolon at the end.

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.cc:284: };
Nit: empty line between method bodies.

[Marc Treib, 2016-11-21 15:48:53]

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

https://codereview.chromium.org/2519053002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java:39:
/** Called when the observer should discard the suggestions is has a pull new
ones. */
s/a/and/

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_provider.h:126: virtual void
OnSignInStateChanged(){};
nit: no ";". Also, space between "()" and "{}"?

Alternatively: Should we require all Providers to implement this?

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.cc:285: void
ContentSuggestionsService::GoogleSignedOut(const std::string& account_id,
nit: empty line before

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.h:71: // state changes and
that personalised suggestions have to be shown or
Remove "that"?

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/ntp_snippets_status_service.cc (right):

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_status_service.cc:85: return
signin_manager_ && signin_manager_->IsAuthenticated();
Hm... a bit weird that this still needs the SigninManager, but relies on
external notifications that something changed. Could we add a "bool signed_in"
to OnSignInStateChanged and get rid of SigninManager here?

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/ntp_snippets_status_service.h (right):

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_status_service.h:49: void
OnSignInStateChanged();
Add a comment?

[dgn, 2016-11-21 16:22:08]

PTAL

https://codereview.chromium.org/2519053002/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/2519053002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:300:
// TODO(dgn): We currently don't have any case where we enter this state.
On 2016/11/21 15:48:15, Bernhard Bauer wrote:
> assert false then?

It can be changed by variations
(https://cs.chromium.org/chromium/src/components/ntp_snippets/remote/ntp_snipp...),
so I don't want to break that code path. 

But what should we do about this in the future? Show the sign in promo if it has
been dismissed?

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

https://codereview.chromium.org/2519053002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java:39:
/** Called when the observer should discard the suggestions is has a pull new
ones. */
On 2016/11/21 15:48:53, Marc Treib wrote:
> s/a/and/

Done.

https://codereview.chromium.org/2519053002/diff/1/chrome/browser/ui/webui/sni...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/2519053002/diff/1/chrome/browser/ui/webui/sni...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:154: if
(!dom_loaded_) {
On 2016/11/21 15:48:15, Bernhard Bauer wrote:
> It might be worth moving this check into SendContentSuggestions().

There are a bunch of calls below that don't check that.

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_provider.h:126: virtual void
OnSignInStateChanged(){};
On 2016/11/21 15:48:53, Marc Treib wrote:
> nit: no ";". Also, space between "()" and "{}"?
> 
> Alternatively: Should we require all Providers to implement this?

Done.

AFAIK only the remote provider cares about the signin state, right? The others
just rely on whatever local thing backs them. So that would just add a dozen of
files to the changelog for not much. But if you feel strongly about it I'll make
the change.

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.cc:284: };
On 2016/11/21 15:48:15, Bernhard Bauer wrote:
> Nit: empty line between method bodies.

Done.

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.cc:285: void
ContentSuggestionsService::GoogleSignedOut(const std::string& account_id,
On 2016/11/21 15:48:53, Marc Treib wrote:
> nit: empty line before

Done.

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.h:71: // state changes and
that personalised suggestions have to be shown or
On 2016/11/21 15:48:53, Marc Treib wrote:
> Remove "that"?

Done.

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/ntp_snippets_status_service.cc (right):

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_status_service.cc:85: return
signin_manager_ && signin_manager_->IsAuthenticated();
On 2016/11/21 15:48:53, Marc Treib wrote:
> Hm... a bit weird that this still needs the SigninManager, but relies on
> external notifications that something changed. Could we add a "bool signed_in"
> to OnSignInStateChanged and get rid of SigninManager here?

there are other cases where it need to pull the information, because it's
observing other things, for example preference changes. If that notification
comes before the signin state one, we might get into a bad state. Also, we don't
receive state notifications until after we ran the callback once, to properly
initialise the state. (Init() method.)

So because that bit might be delicate, I preferred staying as close as possible
to the previous implementation. That is a change that can be made independently
of the rest of the patch.

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/ntp_snippets_status_service.h (right):

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_status_service.h:49: void
OnSignInStateChanged();
On 2016/11/21 15:48:53, Marc Treib wrote:
> Add a comment?

Done.

[Marc Treib, 2016-11-21 16:52:30]

lgtm

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_provider.h:126: virtual void
OnSignInStateChanged(){};
On 2016/11/21 16:22:08, dgn wrote:
> On 2016/11/21 15:48:53, Marc Treib wrote:
> > nit: no ";". Also, space between "()" and "{}"?
> > 
> > Alternatively: Should we require all Providers to implement this?
> 
> Done.
> 
> AFAIK only the remote provider cares about the signin state, right? The others
> just rely on whatever local thing backs them. So that would just add a dozen
of
> files to the changelog for not much. But if you feel strongly about it I'll
make
> the change.

I guess it's conceivable that other providers might care, but at least so far
none of them does. So this is fine.

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/ntp_snippets_status_service.cc (right):

https://codereview.chromium.org/2519053002/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_status_service.cc:85: return
signin_manager_ && signin_manager_->IsAuthenticated();
On 2016/11/21 16:22:08, dgn wrote:
> On 2016/11/21 15:48:53, Marc Treib wrote:
> > Hm... a bit weird that this still needs the SigninManager, but relies on
> > external notifications that something changed. Could we add a "bool
signed_in"
> > to OnSignInStateChanged and get rid of SigninManager here?
> 
> there are other cases where it need to pull the information, because it's
> observing other things, for example preference changes. If that notification
> comes before the signin state one, we might get into a bad state. Also, we
don't
> receive state notifications until after we ran the callback once, to properly
> initialise the state. (Init() method.)
> 
> So because that bit might be delicate, I preferred staying as close as
possible
> to the previous implementation. That is a change that can be made
independently
> of the rest of the patch.

Okay, fair enough. Could you add a TODO though?

[Bernhard Bauer, 2016-11-21 16:58:15]

lgtm

https://codereview.chromium.org/2519053002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java
(right):

https://codereview.chromium.org/2519053002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java:39:
/** Called when the observer should discard the suggestions is has and pull new
ones. */
Nit: "it has".

[dgn, 2016-11-21 17:32:42]

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

[commit-bot: I haz the power, 2016-11-21 17:33:46]

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-11-21 17:36:49]

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

[commit-bot: I haz the power, 2016-11-21 17:36:50]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dgn, 2016-11-21 17:56:12]

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

[commit-bot: I haz the power, 2016-11-21 17:56:42]

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-11-21 18:07:19]

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

[commit-bot: I haz the power, 2016-11-21 18:07:20]

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-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[dgn, 2016-11-21 18:34:23]

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

[dgn, 2016-11-21 18:34:23]

noyau@chromium.org: PTAL. I updated the constructor for
ContentSuggestionsService

[dgn, 2016-11-21 18:34:35]

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

[commit-bot: I haz the power, 2016-11-21 18:35: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, 2016-11-21 19:31:36]

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

[commit-bot: I haz the power, 2016-11-21 19:31:37]

Dry run: This issue passed the CQ dry run.

[noyau (Ping after 24h), 2016-11-22 10:27:30]

ios lgtm

[dgn, 2016-11-22 10:32:30]

The CQ bit was checked by dgn@chromium.org

[dgn, 2016-11-22 10:32:30]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2519053002/#ps80001
(title: "Fix iOS build")

[dgn, 2016-11-22 10:32:36]

Thanks!

[commit-bot: I haz the power, 2016-11-22 10:32:48]

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-11-22 10:36:26]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1479810750336540,
"parent_rev": "70962b393b85add55a1f26f85aeee5e24af6f96b", "commit_rev":
"3f4d070d3e95170fa62a63096bbb14b2c584ea94"}

[commit-bot: I haz the power, 2016-11-22 10:36:54]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-22 10:38:13]

Description was changed from

==========
[NTP Client] Let the backend trigger sign in related refreshes

The detection of when to trigger a refresh after a sign in had
a few issues:

- If the signin promo was dismissed, we would not do it
- We would not refresh on sign out because we ignore NOT_PROVIDED
state changes
- The order of the refresh between the UI and the backend was
not explictly defined, meaning we could start refreshing the UI too
early if the SignInManager changed.

BUG=657936
==========

to

==========
[NTP Client] Let the backend trigger sign in related refreshes

The detection of when to trigger a refresh after a sign in had
a few issues:

- If the signin promo was dismissed, we would not do it
- We would not refresh on sign out because we ignore NOT_PROVIDED
state changes
- The order of the refresh between the UI and the backend was
not explictly defined, meaning we could start refreshing the UI too
early if the SignInManager changed.

BUG=657936

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

[commit-bot: I haz the power, 2016-11-22 10:38:14]

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