📰 Empty state handling for the Bookmarks section

[NTP Client] Empty state handling for the Bookmarks section

- Added a show_if_empty field to CategoryInfo. A category with
this flag enabled will be always shown on the NTP, but if it's
disabled and we have no suggestion for this category when
initialising the NTP, we won't show it at all.

- If the section has a MORE button, when the last suggestion is
dismissed, we show a different Empty State status card that
has no action and leave the MORE button to provide the action.

- Added strings an a new status card for Bookmarks and updated
the ones for articles.

Preview: https://goo.gl/photos/CE8bFvEtuYQNhW1m9
BUG=610648
Committed: https://crrev.com/78a053aeef3074fc62eaa5dbed1d2c02efb4f7e1
Cr-Commit-Position: refs/heads/master@{#413141}

[dgn, 2016-08-16 21:10:26]

Description was changed from

==========
[NTP Client] Empty state handling for the Bookmarks section

- If a section is considered non permanent (e.g. BOOKMARKS) and
never got any suggestion, it does not show at all on the NTP

- If the section has a MORE button, instead of showing a status
card when the last suggestion is dismissed, the MORE button
stays. If we never showed suggestions, we will show a status
card like before.

Preview: https://goo.gl/photos/v3a3xG8sGwxPGCQN8
BUG=610648
==========

to

==========
[NTP Client] Empty state handling for the Bookmarks section

- If a section is considered non permanent (e.g. BOOKMARKS) and
never got any suggestion, it does not show at all on the NTP

- If the section has a MORE button, instead of showing a status
card when the last suggestion is dismissed, the MORE button
stays. If we never showed suggestions, we will show a status
card like before.

Preview: https://goo.gl/photos/v3a3xG8sGwxPGCQN8
BUG=610648
==========

[dgn, 2016-08-16 21:10:32]

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

[dgn, 2016-08-16 21:10:43]

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

[dgn, 2016-08-16 21:10:44]

PTAL

[commit-bot: I haz the power, 2016-08-16 21:11:10]

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-08-16 21:47:09]

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

[commit-bot: I haz the power, 2016-08-16 21:47:10]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-08-16 22:56:49]

https://codereview.chromium.org/2249903003/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/2249903003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:373:
ItemGroup getGroup(int itemPosition) {
Add @VisibleForTesting?

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

https://codereview.chromium.org/2249903003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:62:
if (mSuggestions.isEmpty() && !mIsPersistentSection && !mHadSuggestions) {
Really, we should probably remove the whole section in this case, but maybe
that's something for the refactoring.

https://codereview.chromium.org/2249903003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:69:
if (mSuggestions.isEmpty() && !(mHadSuggestions && mMoreButton != null)) {
The logic here is a bit hard to understand. Maybe if we put the more-button case
first, like this:

  if ((mMoreButton != null) && (!mSuggestions.isEmpty() || mHadSuggestions)) {
    items.add(mMoreButton);
  } else if (mSuggestions.isEmpty()) {
    items.add(mStatus);
    items.add(mProgressIndicator);
  }

it would be clearer?

And actually, `!mSuggestions.isEmpty()` implies `mHadSuggestions`, so we could 
remove the `mSuggestions.isEmpty()` in the first branch.

https://codereview.chromium.org/2249903003/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
(right):

https://codereview.chromium.org/2249903003/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:441:
// PT1: Test ARTICLES
Write out "Part"?

https://codereview.chromium.org/2249903003/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:454:
// When we dismiss them, we should have the status card coming back,
Nit: Period at the end of the sentence.

[dgn, 2016-08-17 17:12:27]

Description was changed from

==========
[NTP Client] Empty state handling for the Bookmarks section

- If a section is considered non permanent (e.g. BOOKMARKS) and
never got any suggestion, it does not show at all on the NTP

- If the section has a MORE button, instead of showing a status
card when the last suggestion is dismissed, the MORE button
stays. If we never showed suggestions, we will show a status
card like before.

Preview: https://goo.gl/photos/v3a3xG8sGwxPGCQN8
BUG=610648
==========

to

==========
[NTP Client] Empty state handling for the Bookmarks section

- If a section is considered non permanent (e.g. BOOKMARKS) and
never got any suggestion, it does not show at all on the NTP

- If the section has a MORE button, instead of showing a status
card when the last suggestion is dismissed, the MORE button
stays. If we never showed suggestions, we will show a status
card like before.

Preview: https://goo.gl/photos/CE8bFvEtuYQNhW1m9
BUG=610648
==========

[dgn, 2016-08-17 17:15:28]

Description was changed from

==========
[NTP Client] Empty state handling for the Bookmarks section

- If a section is considered non permanent (e.g. BOOKMARKS) and
never got any suggestion, it does not show at all on the NTP

- If the section has a MORE button, instead of showing a status
card when the last suggestion is dismissed, the MORE button
stays. If we never showed suggestions, we will show a status
card like before.

Preview: https://goo.gl/photos/CE8bFvEtuYQNhW1m9
BUG=610648
==========

to

==========
[NTP Client] Empty state handling for the Bookmarks section

- Added a show_if_empty field to CategoryInfo. A category with
this flag enabled will be always shown on the NTP, but if it's
disabled and we have no suggestion for this category when
initialising the NTP, we won't show it at all.

- If the section has a MORE button, when the last suggestion is
dismissed, we show a different Empty State status card that
has no action and leave the MORE button to provide the action.

- Added strings an a new status card for Bookmarks.

Preview: https://goo.gl/photos/CE8bFvEtuYQNhW1m9
BUG=610648
==========

[dgn, 2016-08-17 17:15:39]

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

[commit-bot: I haz the power, 2016-08-17 17:15:57]

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

[dgn, 2016-08-17 17:17:21]

PTAL

+pke@: FYI, I'm adding a new field to CategoryInfo.

https://codereview.chromium.org/2249903003/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/2249903003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:373:
ItemGroup getGroup(int itemPosition) {
On 2016/08/16 22:56:48, Bernhard Bauer wrote:
> Add @VisibleForTesting?

Done. That is purely for documentation purposes though, we don't need it for
junit tests.

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

https://codereview.chromium.org/2249903003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:62:
if (mSuggestions.isEmpty() && !mIsPersistentSection && !mHadSuggestions) {
On 2016/08/16 22:56:48, Bernhard Bauer wrote:
> Really, we should probably remove the whole section in this case, but maybe
> that's something for the refactoring.

Okay, did that by checking the category info before instantiating the section.

https://codereview.chromium.org/2249903003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:69:
if (mSuggestions.isEmpty() && !(mHadSuggestions && mMoreButton != null)) {
On 2016/08/16 22:56:48, Bernhard Bauer wrote:
> The logic here is a bit hard to understand. Maybe if we put the more-button
case
> first, like this:
> 
>   if ((mMoreButton != null) && (!mSuggestions.isEmpty() || mHadSuggestions)) {
>     items.add(mMoreButton);
>   } else if (mSuggestions.isEmpty()) {
>     items.add(mStatus);
>     items.add(mProgressIndicator);
>   }
> 
> it would be clearer?
> 
> And actually, `!mSuggestions.isEmpty()` implies `mHadSuggestions`, so we could

> remove the `mSuggestions.isEmpty()` in the first branch.

moving the hideIfEmpty check out simplifies that whole thing and removes the
need for mHadSuggestions.

https://codereview.chromium.org/2249903003/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
(right):

https://codereview.chromium.org/2249903003/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:441:
// PT1: Test ARTICLES
On 2016/08/16 22:56:48, Bernhard Bauer wrote:
> Write out "Part"?

Done.

https://codereview.chromium.org/2249903003/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:454:
// When we dismiss them, we should have the status card coming back,
On 2016/08/16 22:56:48, Bernhard Bauer wrote:
> Nit: Period at the end of the sentence.

Done.

[commit-bot: I haz the power, 2016-08-17 17:26:30]

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

[commit-bot: I haz the power, 2016-08-17 17:26:31]

Dry run: Exceeded global retry quota

[dgn, 2016-08-17 17:39:07]

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

[commit-bot: I haz the power, 2016-08-17 17:39:39]

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-08-17 18:18:17]

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

[commit-bot: I haz the power, 2016-08-17 18:18:19]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-08-17 20:38:03]

https://codereview.chromium.org/2249903003/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/2249903003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:373:
ItemGroup getGroup(int itemPosition) {
On 2016/08/17 17:17:21, dgn wrote:
> On 2016/08/16 22:56:48, Bernhard Bauer wrote:
> > Add @VisibleForTesting?
> 
> Done. That is purely for documentation purposes though, we don't need it for
> junit tests.

Yup, but I like being able to state that it shouldn't be used in production code
:)

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

https://codereview.chromium.org/2249903003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusItem.java:33:
public interface StatusActionDelegate { void onButtonTapped(); }
You could maybe remove the "Status" from here, as that's implied by the
containing class.

https://codereview.chromium.org/2249903003/diff/40001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2249903003/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:96: /*
has_more_button = */ true,
Nit: I think the more common style is to leave the equals sign out.

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

https://codereview.chromium.org/2249903003/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:289: /* show_if_empty = */
true);
Here as well.

https://codereview.chromium.org/2249903003/diff/40001/components/ntp_snippets...
File components/ntp_snippets_strings.grdp (right):

https://codereview.chromium.org/2249903003/diff/40001/components/ntp_snippets...
components/ntp_snippets_strings.grdp:11: <message
name="IDS_NTP_STATUS_CARD_DONE_FOR_NOW" desc="Title of the card explaining to
the user that they can expect to see bookmarks in this part of the New Tab Page
in the future." formatter_data="android_java">
Is the description correct here?

[dgn, 2016-08-18 09:52:50]

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

[dgn, 2016-08-18 09:52:57]

PTAL

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

https://codereview.chromium.org/2249903003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusItem.java:33:
public interface StatusActionDelegate { void onButtonTapped(); }
On 2016/08/17 20:38:03, Bernhard Bauer wrote:
> You could maybe remove the "Status" from here, as that's implied by the
> containing class.

Done.

https://codereview.chromium.org/2249903003/diff/40001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2249903003/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:96: /*
has_more_button = */ true,
On 2016/08/17 20:38:03, Bernhard Bauer wrote:
> Nit: I think the more common style is to leave the equals sign out.

Done.

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

https://codereview.chromium.org/2249903003/diff/40001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:289: /* show_if_empty = */
true);
On 2016/08/17 20:38:03, Bernhard Bauer wrote:
> Here as well.

Done.

https://codereview.chromium.org/2249903003/diff/40001/components/ntp_snippets...
File components/ntp_snippets_strings.grdp (right):

https://codereview.chromium.org/2249903003/diff/40001/components/ntp_snippets...
components/ntp_snippets_strings.grdp:11: <message
name="IDS_NTP_STATUS_CARD_DONE_FOR_NOW" desc="Title of the card explaining to
the user that they can expect to see bookmarks in this part of the New Tab Page
in the future." formatter_data="android_java">
On 2016/08/17 20:38:03, Bernhard Bauer wrote:
> Is the description correct here?

Changed the name of the string. I went with making the name and description have
more info about the usage context, rather than being generic and closer to the
value of the string. The wording gets changed every now and then so...

New strings: https://goo.gl/photos/Dk9km48pv6dwPWv56

[commit-bot: I haz the power, 2016-08-18 09:53:04]

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

[dgn, 2016-08-18 09:53:48]

Description was changed from

==========
[NTP Client] Empty state handling for the Bookmarks section

- Added a show_if_empty field to CategoryInfo. A category with
this flag enabled will be always shown on the NTP, but if it's
disabled and we have no suggestion for this category when
initialising the NTP, we won't show it at all.

- If the section has a MORE button, when the last suggestion is
dismissed, we show a different Empty State status card that
has no action and leave the MORE button to provide the action.

- Added strings an a new status card for Bookmarks.

Preview: https://goo.gl/photos/CE8bFvEtuYQNhW1m9
BUG=610648
==========

to

==========
[NTP Client] Empty state handling for the Bookmarks section

- Added a show_if_empty field to CategoryInfo. A category with
this flag enabled will be always shown on the NTP, but if it's
disabled and we have no suggestion for this category when
initialising the NTP, we won't show it at all.

- If the section has a MORE button, when the last suggestion is
dismissed, we show a different Empty State status card that
has no action and leave the MORE button to provide the action.

- Added strings an a new status card for Bookmarks and updated
the ones for articles.

Preview: https://goo.gl/photos/CE8bFvEtuYQNhW1m9
BUG=610648
==========

[commit-bot: I haz the power, 2016-08-18 10:58:34]

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

[commit-bot: I haz the power, 2016-08-18 10:58:36]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-08-18 18:42:33]

lgtm

[dgn, 2016-08-19 09:30:30]

The CQ bit was checked by dgn@chromium.org

[commit-bot: I haz the power, 2016-08-19 09:30:44]

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-08-19 09:36:23]

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

[commit-bot: I haz the power, 2016-08-19 09:36:24]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dgn, 2016-08-19 10:58:45]

The CQ bit was checked by dgn@chromium.org

[dgn, 2016-08-19 10:58:46]

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/2249903003/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-19 10:59:12]

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-08-19 11:23:29]

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

[commit-bot: I haz the power, 2016-08-19 11:23:30]

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

[dgn, 2016-08-19 13:32:55]

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

[commit-bot: I haz the power, 2016-08-19 13:33:16]

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-08-19 14:24:11]

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

[commit-bot: I haz the power, 2016-08-19 14:24:12]

Dry run: This issue passed the CQ dry run.

[dgn, 2016-08-19 14:32:46]

The CQ bit was checked by dgn@chromium.org

[dgn, 2016-08-19 14:32:47]

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/2249903003/#ps140001
(title: "fix instrumentation test")

[commit-bot: I haz the power, 2016-08-19 14:32:58]

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-08-19 14:37:28]

Description was changed from

==========
[NTP Client] Empty state handling for the Bookmarks section

- Added a show_if_empty field to CategoryInfo. A category with
this flag enabled will be always shown on the NTP, but if it's
disabled and we have no suggestion for this category when
initialising the NTP, we won't show it at all.

- If the section has a MORE button, when the last suggestion is
dismissed, we show a different Empty State status card that
has no action and leave the MORE button to provide the action.

- Added strings an a new status card for Bookmarks and updated
the ones for articles.

Preview: https://goo.gl/photos/CE8bFvEtuYQNhW1m9
BUG=610648
==========

to

==========
[NTP Client] Empty state handling for the Bookmarks section

- Added a show_if_empty field to CategoryInfo. A category with
this flag enabled will be always shown on the NTP, but if it's
disabled and we have no suggestion for this category when
initialising the NTP, we won't show it at all.

- If the section has a MORE button, when the last suggestion is
dismissed, we show a different Empty State status card that
has no action and leave the MORE button to provide the action.

- Added strings an a new status card for Bookmarks and updated
the ones for articles.

Preview: https://goo.gl/photos/CE8bFvEtuYQNhW1m9
BUG=610648
==========

[commit-bot: I haz the power, 2016-08-19 14:37:32]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-19 14:40:07]

Description was changed from

==========
[NTP Client] Empty state handling for the Bookmarks section

- Added a show_if_empty field to CategoryInfo. A category with
this flag enabled will be always shown on the NTP, but if it's
disabled and we have no suggestion for this category when
initialising the NTP, we won't show it at all.

- If the section has a MORE button, when the last suggestion is
dismissed, we show a different Empty State status card that
has no action and leave the MORE button to provide the action.

- Added strings an a new status card for Bookmarks and updated
the ones for articles.

Preview: https://goo.gl/photos/CE8bFvEtuYQNhW1m9
BUG=610648
==========

to

==========
[NTP Client] Empty state handling for the Bookmarks section

- Added a show_if_empty field to CategoryInfo. A category with
this flag enabled will be always shown on the NTP, but if it's
disabled and we have no suggestion for this category when
initialising the NTP, we won't show it at all.

- If the section has a MORE button, when the last suggestion is
dismissed, we show a different Empty State status card that
has no action and leave the MORE button to provide the action.

- Added strings an a new status card for Bookmarks and updated
the ones for articles.

Preview: https://goo.gl/photos/CE8bFvEtuYQNhW1m9
BUG=610648

Committed: https://crrev.com/78a053aeef3074fc62eaa5dbed1d2c02efb4f7e1
Cr-Commit-Position: refs/heads/master@{#413141}
==========

[commit-bot: I haz the power, 2016-08-19 14:40:08]

Patchset 8 (id:??) landed as
https://crrev.com/78a053aeef3074fc62eaa5dbed1d2c02efb4f7e1
Cr-Commit-Position: refs/heads/master@{#413141}