📰 The Status card reports disabled sync states

[NTP Snippets] The Status card reports disabled sync states

When there are no snippets to display, the Status card now
shows an appropriate message and action for the following
contexts:

- User is signed out
- User has disabled sync
- User has disabled history sync
- User has set a custom passphrase

The detection of those states is done by the newly added
NTPSnippetsStatusService.

Preview: https://goo.gl/photos/gpDA8g6Mm2jW56e87

BUG=612508
Committed: https://crrev.com/bea29e22d8b64035f822e48b120691e751108539
Cr-Commit-Position: refs/heads/master@{#402537}

[dgn, 2016-06-13 16:58:46]

Description was changed from

==========
[NTP Snippets] The Status card reports disabled sync states

BUG=612508

implement methods for disabling the card, joining the bridge to the adapter is
missing
==========

to

==========
[NTP Snippets] The Status card reports disabled sync states

BUG=612508

implement methods for disabling the card, joining the bridge to the adapter is
missing
==========

[dgn, 2016-06-13 17:21:43]

Description was changed from

==========
[NTP Snippets] The Status card reports disabled sync states

BUG=612508

implement methods for disabling the card, joining the bridge to the adapter is
missing
==========

to

==========
[NTP Snippets] The Status card reports disabled sync states

Preview: https://goo.gl/photos/Zj1SKNLn3J6WyvM39

BUG=612508

implement methods for disabling the card, joining the bridge to the adapter is
missing
==========

[dgn, 2016-06-13 17:21:56]

Description was changed from

==========
[NTP Snippets] The Status card reports disabled sync states

Preview: https://goo.gl/photos/Zj1SKNLn3J6WyvM39

BUG=612508

implement methods for disabling the card, joining the bridge to the adapter is
missing
==========

to

==========
[NTP Snippets] The Status card reports disabled sync states

Preview: https://goo.gl/photos/Zj1SKNLn3J6WyvM39

BUG=612508
==========

[dgn, 2016-06-13 17:27:14]

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

[dgn, 2016-06-13 17:27:15]

PTAL

https://codereview.chromium.org/2061803002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(left):

https://codereview.chromium.org/2061803002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:136:
loadSnippets(listSnippets);
I chose to reload the list of snippets to update the StatusCard. Another option
would be to register a proper StateObserver so that the adapter/bridge can know
when the state changes and update the card. WDYT?

https://codereview.chromium.org/2061803002/diff/20001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2061803002/diff/20001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:1916: Turn on sync
+nepper: There is a string below for the button that does the same thing for
bookmarks. There it says "Open settings", because we redirect the user to the
sync settings page. Should this one say the same thing?

https://codereview.chromium.org/2061803002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (left):

https://codereview.chromium.org/2061803002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:664: if (ready() &&
(snippets_.empty() || fetch_after_load_)) {
redundant, if we transition to ready, we have the code to start a fetch in
EnterState(). Updated the code there to address empty snippets instead of this
bit.

[dgn, 2016-06-13 18:46:14]

Actually, a bunch of states need to be handled: custom passphrase, sync/history
sync, sign in.. I'll add them in this patch.

[Bernhard Bauer, 2016-06-14 12:10:40]

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

https://codereview.chromium.org/2061803002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:30:
private StatusListItem mListItem;
Do you actually need this as a member, or could you make it a final local
variable so you can reference it in the closure?

https://codereview.chromium.org/2061803002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:43:
.setText(mListItem.mHeaderStringId);
Would it make sense to push the code that sets the text into CardViewHolder, so
it can be used for snippet cards as well?

Also, you could cache the views so you don't have to get them every time the
ViewHolder is rebound. (FWIU, this is the main rationale for using the
ViewHolder pattern; see e.g.
https://www.javacodegeeks.com/2013/09/android-viewholder-pattern-example.html).

https://codereview.chromium.org/2061803002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:72:
/* TODO(dgn): When we can report more status, this probably should return
multiple states,
Nit: We actually use // style even for multiline comments.

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

https://codereview.chromium.org/2061803002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsConfig.java:23:
@IntDef({DISABLED_REASON_NONE, DISABLED_REASON_EXPLICITLY_DISABLED,
Ideally, we'd generate these from the C++ values. I think +peconn was at some
point looking into adding @IntDef to the generated Java?

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

https://codereview.chromium.org/2061803002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.h:54: // 3 -
HISTORY_SYNC_STATE_UNKNOWN
I think it's fine to just refer to the definition of DisabledReason for the
values.

[dgn, 2016-06-15 16:46:29]

PTAL

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

https://codereview.chromium.org/2061803002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:30:
private StatusListItem mListItem;
On 2016/06/14 12:10:40, Bernhard Bauer wrote:
> Do you actually need this as a member, or could you make it a final local
> variable so you can reference it in the closure?

Removed.

https://codereview.chromium.org/2061803002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:43:
.setText(mListItem.mHeaderStringId);
On 2016/06/14 12:10:40, Bernhard Bauer wrote:
> Would it make sense to push the code that sets the text into CardViewHolder,
so
> it can be used for snippet cards as well?
> 
> Also, you could cache the views so you don't have to get them every time the
> ViewHolder is rebound. (FWIU, this is the main rationale for using the
> ViewHolder pattern; see e.g.
>
https://www.javacodegeeks.com/2013/09/android-viewholder-pattern-example.html).

Done.

https://codereview.chromium.org/2061803002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:72:
/* TODO(dgn): When we can report more status, this probably should return
multiple states,
On 2016/06/14 12:10:39, Bernhard Bauer wrote:
> Nit: We actually use // style even for multiline comments.

Removed.

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

https://codereview.chromium.org/2061803002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsConfig.java:23:
@IntDef({DISABLED_REASON_NONE, DISABLED_REASON_EXPLICITLY_DISABLED,
On 2016/06/14 12:10:40, Bernhard Bauer wrote:
> Ideally, we'd generate these from the C++ values. I think +peconn was at some
> point looking into adding @IntDef to the generated Java?

Done.

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

https://codereview.chromium.org/2061803002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.h:54: // 3 -
HISTORY_SYNC_STATE_UNKNOWN
On 2016/06/14 12:10:40, Bernhard Bauer wrote:
> I think it's fine to just refer to the definition of DisabledReason for the
> values.

Done.

[dgn, 2016-06-15 16:56:59]

Description was changed from

==========
[NTP Snippets] The Status card reports disabled sync states

Preview: https://goo.gl/photos/Zj1SKNLn3J6WyvM39

BUG=612508
==========

to

==========
[NTP Snippets] The Status card reports disabled sync states

Preview: https://goo.gl/photos/gpDA8g6Mm2jW56e87

BUG=612508
==========

[dgn, 2016-06-15 16:57:19]

Updated the preview link with a video.

[Bernhard Bauer, 2016-06-15 17:08:56]

lgtm

https://codereview.chromium.org/2061803002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java
(right):

https://codereview.chromium.org/2061803002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:91:
if (!SigninManager.get(context).isSignInAllowed()) {
We should probably at some point extract this into a utility method.

https://codereview.chromium.org/2061803002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:174:
// So let's just use HistorySyncDisabled as fallback.
Suggestion: if we add a spinner add some point (e.g. to show while we're
fetching new snippets), we could use that here.

[dgn, 2016-06-15 17:45:59]

Thanks!

https://codereview.chromium.org/2061803002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java
(right):

https://codereview.chromium.org/2061803002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:91:
if (!SigninManager.get(context).isSignInAllowed()) {
On 2016/06/15 17:08:56, Bernhard Bauer wrote:
> We should probably at some point extract this into a utility method.

Done.

https://codereview.chromium.org/2061803002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:174:
// So let's just use HistorySyncDisabled as fallback.
On 2016/06/15 17:08:56, Bernhard Bauer wrote:
> Suggestion: if we add a spinner add some point (e.g. to show while we're
> fetching new snippets), we could use that here.

Good idea. Added that to the comment.

https://codereview.chromium.org/2061803002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SignInPreference.java
(left):

https://codereview.chromium.org/2061803002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SignInPreference.java:135:
if (ChromeSigninController.get(getContext()).isSignedIn()) return false;
isSignedIn() is checked  already in isSignInAllowed(), so I skipped it.

[dgn, 2016-06-15 17:46:03]

The CQ bit was checked by dgn@chromium.org

[dgn, 2016-06-15 17:46:03]

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/2061803002/#ps60001
(title: "Address comments, small changes for clarity")

[commit-bot: I haz the power, 2016-06-15 17:47:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2061803002/60001

[commit-bot: I haz the power, 2016-06-15 17:56:03]

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

[commit-bot: I haz the power, 2016-06-15 17:56:04]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[dgn, 2016-06-15 22:38:00]

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

[dgn, 2016-06-15 22:38:47]

noyau@: PTAL at the changes to /ios


Looking into tests issues with the signin manager.

[dgn, 2016-06-16 13:25:27]

PTAL

[Bernhard Bauer, 2016-06-16 14:03:36]

https://codereview.chromium.org/2061803002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java
(right):

https://codereview.chromium.org/2061803002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:165:
// So let's just use HistorySyncDisabled as fallback. If we add a spinner at
some
Make it a TODO()?

https://codereview.chromium.org/2061803002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/2061803002/diff/120001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_fetcher.h:118: void
setPersonalizationForTesting(Personalization personalization) {
Capitalize the beginning of the method name.

https://codereview.chromium.org/2061803002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/2061803002/diff/120001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service_unittest.cc:330:
fake_signin_manager_.reset(new FakeSigninManagerBase(
I would store this in a raw FakeSigninManagerBase* so you can access it below
without the static cast.

[dgn, 2016-06-16 14:34:10]

PTAL

https://codereview.chromium.org/2061803002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java
(right):

https://codereview.chromium.org/2061803002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusListItem.java:165:
// So let's just use HistorySyncDisabled as fallback. If we add a spinner at
some
On 2016/06/16 14:03:35, Bernhard Bauer wrote:
> Make it a TODO()?

Done.

https://codereview.chromium.org/2061803002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_fetcher.h (right):

https://codereview.chromium.org/2061803002/diff/120001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_fetcher.h:118: void
setPersonalizationForTesting(Personalization personalization) {
On 2016/06/16 14:03:35, Bernhard Bauer wrote:
> Capitalize the beginning of the method name.

Done.

https://codereview.chromium.org/2061803002/diff/120001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/2061803002/diff/120001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service_unittest.cc:330:
fake_signin_manager_.reset(new FakeSigninManagerBase(
On 2016/06/16 14:03:35, Bernhard Bauer wrote:
> I would store this in a raw FakeSigninManagerBase* so you can access it below
> without the static cast.

Done.

[Bernhard Bauer, 2016-06-16 16:23:55]

LGTM++

[dgn, 2016-06-20 11:27:34]

Friendly PTAL ping http://media.giphy.com/media/Z5y6DoZycMgXS/giphy.gif

[noyau (Ping after 24h), 2016-06-21 14:28:55]

https://codereview.chromium.org/2061803002/diff/140001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2061803002/diff/140001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:1932: </message>
Can you move those strings to components/ instead? We will need them on other
platforms, I don't see any reason to make them android only.

https://codereview.chromium.org/2061803002/diff/140001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/2061803002/diff/140001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.h:54: //
GENERATED_JAVA_ENUM_PACKAGE: org.chromium.chrome.browser.ntp.snippets
This is only true for android, please amend the comment to make this obvious. I
would argue that this comment should not be here in the first place. Is this
some kind of automated process generating this java code?

https://codereview.chromium.org/2061803002/diff/140001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.h:65:
PASSPHRASE_ENCRYPTION_ENABLED,
To be honest I don't like the idea of adding to this class a dependency on
signin. I was already reluctant when there was a dependency on sync, this makes
it too tightly coupled for my taste.

All this disabling  logic could be handled in a separate class. As it is coded
now I feel this is just increasing coupling for a very small gain codewise. 

Ideally the service should be limited to being a service storing and vending
snippets. The fetcher is the one getting the data to feed into the service. Can
you move this enabling/disabling code somewhere else? This would make the tests
clearer as well as of now I'm having difficulties figuring out which of those
disabled mode are tested.

[dgn, 2016-06-21 15:47:40]

https://codereview.chromium.org/2061803002/diff/140001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/2061803002/diff/140001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.h:54: //
GENERATED_JAVA_ENUM_PACKAGE: org.chromium.chrome.browser.ntp.snippets
On 2016/06/21 14:28:55, noyau wrote:
> This is only true for android, please amend the comment to make this obvious.
I
> would argue that this comment should not be here in the first place. Is this
> some kind of automated process generating this java code?

Yes, the java_cpp_enum I added to the BUILD.gn file does that. It runs only on
android builds.

https://codereview.chromium.org/2061803002/diff/140001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.h:65:
PASSPHRASE_ENCRYPTION_ENABLED,
On 2016/06/21 14:28:55, noyau wrote:
> To be honest I don't like the idea of adding to this class a dependency on
> signin. I was already reluctant when there was a dependency on sync, this
makes
> it too tightly coupled for my taste.
> 
> All this disabling  logic could be handled in a separate class. As it is coded
> now I feel this is just increasing coupling for a very small gain codewise. 
> 
> Ideally the service should be limited to being a service storing and vending
> snippets. The fetcher is the one getting the data to feed into the service.
Can
> you move this enabling/disabling code somewhere else? This would make the
tests
> clearer as well as of now I'm having difficulties figuring out which of those
> disabled mode are tested.

Very valid concern. We are working on some refactoring to the Java/XML side to
better prepare supporting different types of content. Some similar effort should
be made here too, in a way that would be compatible with offline pages for
example, that I think we started working on for Android.

So would it be okay to land the disabling as it is, and I'll send you a design
doc about the refactor and how the component would be with regards to new data
sources?

[dgn, 2016-06-24 19:58:41]

Description was changed from

==========
[NTP Snippets] The Status card reports disabled sync states

Preview: https://goo.gl/photos/gpDA8g6Mm2jW56e87

BUG=612508
==========

to

==========
[NTP Snippets] The Status card reports disabled sync states

When there are no snippets to display, the Status card now
shows an appropriate message and action for the following
contexts:

- User is signed out
- User has disabled sync
- User has disabled history sync
- User has set a custom passphrase

The detection of those states is done by the newly added
NTPSnippetsStatusService.

Preview: https://goo.gl/photos/gpDA8g6Mm2jW56e87

BUG=612508
==========

[dgn, 2016-06-24 20:04:29]

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

[dgn, 2016-06-24 20:04:30]

sdefresne@chromium.org: Please review changes in components/: 
I added a grd file for the ntp_snippets component, and updated the gyp files to
process the newly added files

bauerb@, noyau@: PTAL. There are significant changes since PS8, mostly to the
component code. Little difference in java code

[dgn, 2016-06-24 20:05:30]

Patchset #10 (id:180001) has been deleted

[dgn, 2016-06-24 20:05:43]

Patchset #9 (id:160001) has been deleted

[dgn, 2016-06-24 20:25:20]

https://codereview.chromium.org/2061803002/diff/140001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2061803002/diff/140001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:1932: </message>
On 2016/06/21 14:28:55, noyau wrote:
> Can you move those strings to components/ instead? We will need them on other
> platforms, I don't see any reason to make them android only.

Done. Not all the required strings are there, some are reused from other places
in our Android code (e.g. "Reload"). I suppose other platforms will have them
too.

https://codereview.chromium.org/2061803002/diff/140001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/2061803002/diff/140001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.h:65:
PASSPHRASE_ENCRYPTION_ENABLED,
On 2016/06/21 15:47:39, dgn wrote:
> On 2016/06/21 14:28:55, noyau wrote:
> > To be honest I don't like the idea of adding to this class a dependency on
> > signin. I was already reluctant when there was a dependency on sync, this
> makes
> > it too tightly coupled for my taste.
> > 
> > All this disabling  logic could be handled in a separate class. As it is
coded
> > now I feel this is just increasing coupling for a very small gain codewise. 
> > 
> > Ideally the service should be limited to being a service storing and vending
> > snippets. The fetcher is the one getting the data to feed into the service.
> Can
> > you move this enabling/disabling code somewhere else? This would make the
> tests
> > clearer as well as of now I'm having difficulties figuring out which of
those
> > disabled mode are tested.
> 
> Very valid concern. We are working on some refactoring to the Java/XML side to
> better prepare supporting different types of content. Some similar effort
should
> be made here too, in a way that would be compatible with offline pages for
> example, that I think we started working on for Android.
> 
> So would it be okay to land the disabling as it is, and I'll send you a design
> doc about the refactor and how the component would be with regards to new data
> sources?

Moved the dependencies to NTPSnippetStatusService. That will be updated again
when markusheintz@'s refactor of the component happens.

[sdefresne, 2016-06-27 12:23:26]

It looks like those strings are not yet used. Am I right?

components/ lgtm

[dgn, 2016-06-27 16:11:47]

On 2016/06/27 12:23:26, sdefresne wrote:
> It looks like those strings are not yet used. Am I right?
> 
> components/ lgtm

Thanks! The strings are used in Android: StatusListItem.java

noyau@ PTAL

[noyau (Ping after 24h), 2016-06-28 13:20:25]

lgtm

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.h:234: void
UpdateStateForStatus(DisabledReason disabled_reason);
Needs a comment.

[Bernhard Bauer, 2016-06-28 13:47:39]

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.h:310: // Sent when the state of
the service is changing. Something changed in its
Can you add empty lines before the comments? It gets worse the more methods
there are in this interface :)

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service_unittest.cc:309:
NTPSnippetsFetcher* snippets_fetcher = new NTPSnippetsFetcher(
FWIW, I would probably put this into a unique_ptr right away. You could also use
base::MakeUnique<T>() instead of base::WrapUnique(new T).

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service_unittest.cc:319: enabled,
pref_service(), nullptr, std::string("fr"), &scheduler_,
std::string has an implicit constructor, so you don't need to call it
explicitly.

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_status_service.cc (right):

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_status_service.cc:23:
disabled_reason_change_callback_.Reset();
This isn't really necessary -- no one is going to access this after the
destructor has run, and then the member will be destroyed anyway.

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_status_service.cc:24:
sync_service_observer_.Remove(sync_service_);
? The point of ScopedObserver is that it will unregister the observers on
destruction. If you remove the source, that will _not_ happen 😃

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_test_utils.cc (right):

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_test_utils.cc:54:
mock_sync_service_.reset(new testing::NiceMock<MockSyncService>);
This mock doesn't really seem to verify its interactions. Would it be easier /
more straightforward to just create a subclass of SyncService that can be
configured to return a constant value for each of those methods?

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_test_utils.h (right):

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_test_utils.h:20: class
AccountTrackerService;
Nit: Sort alphabetically

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
File components/ntp_snippets_strings.grdp (right):

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets_strings.grdp:5: <message
name="IDS_SNIPPETS_DISABLED_SIGNED_OUT_INSTRUCTIONS" desc="Body of the card
explaining the status of the ntp suggested content, when the user is signed
out." formatter_data="android_java">
Nit: "NTP" (also below)

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets_strings.grdp:6: To get personalized content suggested by
Google, sign in to Chrome.
If this message mentions Chrome, it should be in ...chromium_strings.grdp (with
a counterpart in ...google_chrome_strings.grdp). But then again we're pretty bad
about this on Android 😕

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets_strings.grdp:12: To get personalized content suggested
by Google, turn on sync.
I assume these strings have been signed off? In particular, lowercase "sync" is
fine?

[markusheintz_, 2016-06-28 14:22:52]

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

[markusheintz_, 2016-06-28 14:22:53]

https://codereview.chromium.org/2061803002/diff/140001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/2061803002/diff/140001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.h:65:
PASSPHRASE_ENCRYPTION_ENABLED,
On 2016/06/24 20:25:20, dgn wrote:
> On 2016/06/21 15:47:39, dgn wrote:
> > On 2016/06/21 14:28:55, noyau wrote:
> > > To be honest I don't like the idea of adding to this class a dependency on
> > > signin. I was already reluctant when there was a dependency on sync, this
> > makes
> > > it too tightly coupled for my taste.
> > > 
> > > All this disabling  logic could be handled in a separate class. As it is
> coded
> > > now I feel this is just increasing coupling for a very small gain
codewise. 
> > > 
> > > Ideally the service should be limited to being a service storing and
vending
> > > snippets. The fetcher is the one getting the data to feed into the
service.
> > Can
> > > you move this enabling/disabling code somewhere else? This would make the
> > tests
> > > clearer as well as of now I'm having difficulties figuring out which of
> those
> > > disabled mode are tested.
> > 
> > Very valid concern. We are working on some refactoring to the Java/XML side
to
> > better prepare supporting different types of content. Some similar effort
> should
> > be made here too, in a way that would be compatible with offline pages for
> > example, that I think we started working on for Android.
> > 
> > So would it be okay to land the disabling as it is, and I'll send you a
design
> > doc about the refactor and how the component would be with regards to new
data
> > sources?
> 
> Moved the dependencies to NTPSnippetStatusService. That will be updated again
> when markusheintz@'s refactor of the component happens.

I agree with Eric that we need to split this into multiple pieces.
I will discuss this with all involved parties.

[dgn, 2016-06-28 16:38:57]

PTAL

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.h (right):

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.h:234: void
UpdateStateForStatus(DisabledReason disabled_reason);
On 2016/06/28 13:20:25, noyau wrote:
> Needs a comment.

Done.

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.h:310: // Sent when the state of
the service is changing. Something changed in its
On 2016/06/28 13:47:39, Bernhard Bauer wrote:
> Can you add empty lines before the comments? It gets worse the more methods
> there are in this interface :)

Done.

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service_unittest.cc:309:
NTPSnippetsFetcher* snippets_fetcher = new NTPSnippetsFetcher(
On 2016/06/28 13:47:39, Bernhard Bauer wrote:
> FWIW, I would probably put this into a unique_ptr right away. You could also
use
> base::MakeUnique<T>() instead of base::WrapUnique(new T).

Done.

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service_unittest.cc:319: enabled,
pref_service(), nullptr, std::string("fr"), &scheduler_,
On 2016/06/28 13:47:39, Bernhard Bauer wrote:
> std::string has an implicit constructor, so you don't need to call it
> explicitly.

Done.

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_status_service.cc (right):

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_status_service.cc:23:
disabled_reason_change_callback_.Reset();
On 2016/06/28 13:47:39, Bernhard Bauer wrote:
> This isn't really necessary -- no one is going to access this after the
> destructor has run, and then the member will be destroyed anyway.

Done.

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_status_service.cc:24:
sync_service_observer_.Remove(sync_service_);
On 2016/06/28 13:47:39, Bernhard Bauer wrote:
> ? The point of ScopedObserver is that it will unregister the observers on
> destruction. If you remove the source, that will _not_ happen 😃

Moved over from NTPSnippetService but there it was done in shutdown, not
destructor. Removed.

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_test_utils.cc (right):

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_test_utils.cc:54:
mock_sync_service_.reset(new testing::NiceMock<MockSyncService>);
On 2016/06/28 13:47:39, Bernhard Bauer wrote:
> This mock doesn't really seem to verify its interactions. Would it be easier /
> more straightforward to just create a subclass of SyncService that can be
> configured to return a constant value for each of those methods?

Probably more staightforward, yes. Done.

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_test_utils.h (right):

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_test_utils.h:20: class
AccountTrackerService;
On 2016/06/28 13:47:39, Bernhard Bauer wrote:
> Nit: Sort alphabetically

Done.

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
File components/ntp_snippets_strings.grdp (right):

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets_strings.grdp:5: <message
name="IDS_SNIPPETS_DISABLED_SIGNED_OUT_INSTRUCTIONS" desc="Body of the card
explaining the status of the ntp suggested content, when the user is signed
out." formatter_data="android_java">
On 2016/06/28 13:47:39, Bernhard Bauer wrote:
> Nit: "NTP" (also below)

Done.

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets_strings.grdp:6: To get personalized content suggested by
Google, sign in to Chrome.
On 2016/06/28 13:47:39, Bernhard Bauer wrote:
> If this message mentions Chrome, it should be in ...chromium_strings.grdp
(with
> a counterpart in ...google_chrome_strings.grdp). But then again we're pretty
bad
> about this on Android 😕

Acknowledged.

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets_strings.grdp:12: To get personalized content suggested
by Google, turn on sync.
On 2016/06/28 13:47:39, Bernhard Bauer wrote:
> I assume these strings have been signed off? In particular, lowercase "sync"
is
> fine?

Rachel said that the ones from the slides are the validated strings. There are
already some cases of lowercase "sync" usage:
https://cs.chromium.org/chromium/src/components/sync_ui_strings.grdp?q=f:grd+...

[Bernhard Bauer, 2016-06-28 16:47:25]

LGTM!

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_status_service.cc (right):

https://codereview.chromium.org/2061803002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_status_service.cc:24:
sync_service_observer_.Remove(sync_service_);
On 2016/06/28 16:38:57, dgn wrote:
> On 2016/06/28 13:47:39, Bernhard Bauer wrote:
> > ? The point of ScopedObserver is that it will unregister the observers on
> > destruction. If you remove the source, that will _not_ happen 😃
> 
> Moved over from NTPSnippetService but there it was done in shutdown, not
> destructor. Removed.

Ah yes, ScopedObserver::Remove will actually remove the observer, but if we now
continue observing, that's indeed not necessary.

[dgn, 2016-06-28 17:09:44]

The CQ bit was checked by dgn@chromium.org

[dgn, 2016-06-28 17:09:45]

The patchset sent to the CQ was uploaded after l-g-t-m from noyau@chromium.org,
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2061803002/#ps240001
(title: "yolo: another big bag of changes")

[commit-bot: I haz the power, 2016-06-28 17:10:19]

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

[dgn, 2016-06-28 17:29:19]

The CQ bit was unchecked by dgn@chromium.org

[dgn, 2016-06-28 17:44:35]

The CQ bit was checked by dgn@chromium.org

[commit-bot: I haz the power, 2016-06-28 17:45:10]

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-06-28 20:39:36]

Description was changed from

==========
[NTP Snippets] The Status card reports disabled sync states

When there are no snippets to display, the Status card now
shows an appropriate message and action for the following
contexts:

- User is signed out
- User has disabled sync
- User has disabled history sync
- User has set a custom passphrase

The detection of those states is done by the newly added
NTPSnippetsStatusService.

Preview: https://goo.gl/photos/gpDA8g6Mm2jW56e87

BUG=612508
==========

to

==========
[NTP Snippets] The Status card reports disabled sync states

When there are no snippets to display, the Status card now
shows an appropriate message and action for the following
contexts:

- User is signed out
- User has disabled sync
- User has disabled history sync
- User has set a custom passphrase

The detection of those states is done by the newly added
NTPSnippetsStatusService.

Preview: https://goo.gl/photos/gpDA8g6Mm2jW56e87

BUG=612508
==========

[commit-bot: I haz the power, 2016-06-28 20:39:38]

Committed patchset #11 (id:240001)

[commit-bot: I haz the power, 2016-06-28 20:41:43]

Description was changed from

==========
[NTP Snippets] The Status card reports disabled sync states

When there are no snippets to display, the Status card now
shows an appropriate message and action for the following
contexts:

- User is signed out
- User has disabled sync
- User has disabled history sync
- User has set a custom passphrase

The detection of those states is done by the newly added
NTPSnippetsStatusService.

Preview: https://goo.gl/photos/gpDA8g6Mm2jW56e87

BUG=612508
==========

to

==========
[NTP Snippets] The Status card reports disabled sync states

When there are no snippets to display, the Status card now
shows an appropriate message and action for the following
contexts:

- User is signed out
- User has disabled sync
- User has disabled history sync
- User has set a custom passphrase

The detection of those states is done by the newly added
NTPSnippetsStatusService.

Preview: https://goo.gl/photos/gpDA8g6Mm2jW56e87

BUG=612508

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

[commit-bot: I haz the power, 2016-06-28 20:41:45]

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