[NTP Client] Make the SignInPromo update on SignInAllowed changes

[NTP Client] Make the SignInPromo update on SignInAllowed changes

Sign in is not allowed until the FRE is completed, but the sign in
promo is initialised and shown before that. Observing these changes
allows showing the promo if the user doesn't sign in during the FRE.

Adds a SelfRegistrable type that objects can implement, and when
sent to the NTPManager, their unregister() method will be called when
the page is destroyed.
Used to take care of the cleanup of the above observers.

BUG=656501
Committed: https://crrev.com/068e28dc61b5443839bb31a33bfcff7b695c7546
Cr-Commit-Position: refs/heads/master@{#426470}

[dgn, 2016-10-19 10:47:38]

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

[dgn, 2016-10-19 10:48:12]

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

[dgn, 2016-10-19 10:48:12]

PTAL

[commit-bot: I haz the power, 2016-10-19 10:48:26]

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

[Michael van Ouwerkerk, 2016-10-19 12:09:18]

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

https://codereview.chromium.org/2439443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:155:
private SelfRegistrable mSelfRegistrable;
Maybe rename to mSignInStateObserver or something like that?

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

https://codereview.chromium.org/2439443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:300:
* TODO(dgn): This currently supports setting only one. Extend add them to a
lists if
Then maybe call it "set" instead of "add" for the moment? Then you don't need
this TODO either.

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

https://codereview.chromium.org/2439443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SignInPromo.java:193:
SignInPromo.this.hide();
nit: could this just be "hide()"?

https://codereview.chromium.org/2439443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SignInPromo.java:199:
SignInPromo.this.maybeShow();
nit: could this just be "maybeShow()"?

[Bernhard Bauer, 2016-10-19 12:15:26]

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

[Bernhard Bauer, 2016-10-19 12:15:27]

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

https://codereview.chromium.org/2439443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:175: *
be unregistered when the {@link NewTabPage} is destroyed.
If this interface is for being notified about the NewTabPage being destroyed,
why not make that explicit and call it DestructionObserver?

https://codereview.chromium.org/2439443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:179:
void register();
Does this even need to be part of the interface? It's called from
addSelfRegistrable(), so any caller of that method could take care of
registration themselves.

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

https://codereview.chromium.org/2439443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:301:
* needed somewhere else.
Yeah, I would maybe call the method set...

[commit-bot: I haz the power, 2016-10-19 12:35:37]

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

[commit-bot: I haz the power, 2016-10-19 12:35:38]

Dry run: This issue passed the CQ dry run.

[dgn, 2016-10-19 17:02:04]

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

[commit-bot: I haz the power, 2016-10-19 17:02:49]

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

[dgn, 2016-10-19 17:03:41]

PTAL

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

https://codereview.chromium.org/2439443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:155:
private SelfRegistrable mSelfRegistrable;
On 2016/10/19 12:09:18, Michael van Ouwerkerk wrote:
> Maybe rename to mSignInStateObserver or something like that?

renamed to mDestructionObserver. The NTP does not know it has anything to do
with sign in anyway.

https://codereview.chromium.org/2439443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:175: *
be unregistered when the {@link NewTabPage} is destroyed.
On 2016/10/19 12:15:27, Bernhard Bauer wrote:
> If this interface is for being notified about the NewTabPage being destroyed,
> why not make that explicit and call it DestructionObserver?

Good point. Done.

https://codereview.chromium.org/2439443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:179:
void register();
On 2016/10/19 12:15:27, Bernhard Bauer wrote:
> Does this even need to be part of the interface? It's called from
> addSelfRegistrable(), so any caller of that method could take care of
> registration themselves.

Done.

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

https://codereview.chromium.org/2439443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:300:
* TODO(dgn): This currently supports setting only one. Extend add them to a
lists if
On 2016/10/19 12:09:18, Michael van Ouwerkerk wrote:
> Then maybe call it "set" instead of "add" for the moment? Then you don't need
> this TODO either.

Done.

https://codereview.chromium.org/2439443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:301:
* needed somewhere else.
On 2016/10/19 12:15:27, Bernhard Bauer wrote:
> Yeah, I would maybe call the method set...

Done.

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

https://codereview.chromium.org/2439443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SignInPromo.java:193:
SignInPromo.this.hide();
On 2016/10/19 12:09:18, Michael van Ouwerkerk wrote:
> nit: could this just be "hide()"?

Done.

https://codereview.chromium.org/2439443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SignInPromo.java:199:
SignInPromo.this.maybeShow();
On 2016/10/19 12:09:18, Michael van Ouwerkerk wrote:
> nit: could this just be "maybeShow()"?

Done.

https://codereview.chromium.org/2439443002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SignInPromo.java
(right):

https://codereview.chromium.org/2439443002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SignInPromo.java:169:
mDestroyed = true;
not really necessary as remove*Observer() would just do nothing if the observers
are not registered, but it would be logically wrong...

[commit-bot: I haz the power, 2016-10-19 17:43:51]

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

[commit-bot: I haz the power, 2016-10-19 17:43:52]

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

[Bernhard Bauer, 2016-10-20 10:42:09]

https://codereview.chromium.org/2439443002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SignInPromo.java
(right):

https://codereview.chromium.org/2439443002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SignInPromo.java:169:
mDestroyed = true;
On 2016/10/19 17:03:41, dgn wrote:
> not really necessary as remove*Observer() would just do nothing if the
observers
> are not registered, but it would be logically wrong...

Hm, I have a slightly bigger philosophical issue with the fact that we call this
method when the sign in promo is dismissed, so onDestroy() isn't quite accurate,
but maybe that's not that much of a big deal.

https://codereview.chromium.org/2439443002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java
(right):

https://codereview.chromium.org/2439443002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java:356:
public void addSelfRegistrable(SelfRegistrable selfRegistrable) {}
Does this need to be updated?

[dgn, 2016-10-20 12:17:13]

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

[commit-bot: I haz the power, 2016-10-20 12:17:32]

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

[dgn, 2016-10-20 12:17:49]

https://codereview.chromium.org/2439443002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SignInPromo.java
(right):

https://codereview.chromium.org/2439443002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SignInPromo.java:169:
mDestroyed = true;
On 2016/10/20 10:42:09, Bernhard Bauer wrote:
> On 2016/10/19 17:03:41, dgn wrote:
> > not really necessary as remove*Observer() would just do nothing if the
> observers
> > are not registered, but it would be logically wrong...
> 
> Hm, I have a slightly bigger philosophical issue with the fact that we call
this
> method when the sign in promo is dismissed, so onDestroy() isn't quite
accurate,
> but maybe that's not that much of a big deal.

Done.

https://codereview.chromium.org/2439443002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java
(right):

https://codereview.chromium.org/2439443002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java:356:
public void addSelfRegistrable(SelfRegistrable selfRegistrable) {}
On 2016/10/20 10:42:09, Bernhard Bauer wrote:
> Does this need to be updated?

Done.

[Bernhard Bauer, 2016-10-20 12:48:50]

Philosophical correctness is best correctness! LGTM.

[commit-bot: I haz the power, 2016-10-20 12:53:27]

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

[commit-bot: I haz the power, 2016-10-20 12:53:28]

Dry run: This issue passed the CQ dry run.

[dgn, 2016-10-20 13:57:32]

The CQ bit was checked by dgn@chromium.org

[commit-bot: I haz the power, 2016-10-20 13:57: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-10-20 14:06:36]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-21 13:18:05]

Description was changed from

==========
[NTP Client] Make the SignInPromo update on SignInAllowed changes

Sign in is not allowed until the FRE is completed, but the sign in
promo is initialised and shown before that. Observing these changes
allows showing the promo if the user doesn't sign in during the FRE.

Adds a SelfRegistrable type that objects can implement, and when
sent to the NTPManager, their unregister() method will be called when
the page is destroyed.
Used to take care of the cleanup of the above observers.

BUG=656501
==========

to

==========
[NTP Client] Make the SignInPromo update on SignInAllowed changes

Sign in is not allowed until the FRE is completed, but the sign in
promo is initialised and shown before that. Observing these changes
allows showing the promo if the user doesn't sign in during the FRE.

Adds a SelfRegistrable type that objects can implement, and when
sent to the NTPManager, their unregister() method will be called when
the page is destroyed.
Used to take care of the cleanup of the above observers.

BUG=656501

Committed: https://crrev.com/068e28dc61b5443839bb31a33bfcff7b695c7546
Cr-Commit-Position: refs/heads/master@{#426470}
==========

[commit-bot: I haz the power, 2016-10-21 13:18:06]

Patchset 4 (id:??) landed as
https://crrev.com/068e28dc61b5443839bb31a33bfcff7b695c7546
Cr-Commit-Position: refs/heads/master@{#426470}