Ntp: show AllDismissedItem when all sections have been dismissed.

Ntp: show AllDismissedItem when all sections have been dismissed.

BUG=651058
Committed: https://crrev.com/52783d986c243bce2baeed48a07b6b15029d3312
Cr-Commit-Position: refs/heads/master@{#424710}

[Michael van Ouwerkerk, 2016-10-07 11:20:49]

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

[commit-bot: I haz the power, 2016-10-07 11:21:00]

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

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

[Michael van Ouwerkerk, 2016-10-07 11:21:41]

Bernhard, could you take a look please? The UI mocks have not been finalized
yet, but the logic should not be affected by that.

[commit-bot: I haz the power, 2016-10-07 11:23:15]

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

[commit-bot: I haz the power, 2016-10-07 11:23:16]

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

[Michael van Ouwerkerk, 2016-10-07 11:28:43]

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

[Michael van Ouwerkerk, 2016-10-07 11:28:44]

Marc, could you please take a look at the C++ parts for restoring dismissed
sections?

[Marc Treib, 2016-10-07 11:40:14]

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

https://codereview.chromium.org/2400783003/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.cc:160: for (const auto& it
: dismissed_providers_by_category_)
nit: Please give |it| another name, it's not an iterator.

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

https://codereview.chromium.org/2400783003/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.h:252:
dismissed_providers_by_category_;
What's the status of persisting dismissals? Seems like this wouldn't really work
together - we can't persist the provider pointers.

Generally, I'm not too happy with Yet Another data structure storing
categories/providers here... can we just go over all providers instead and
re-register everything?

[Bernhard Bauer, 2016-10-07 13:46:45]

LGTM once Marc is happy with the native side.

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

https://codereview.chromium.org/2400783003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/AllDismissedItem.java:65:
int messageId = R.string.ntp_all_dismissed_body_text_morning;
Move the assignment into an additional else branch at the end, so the compiler
checks that we pick one of the branches?

[Michael van Ouwerkerk, 2016-10-10 14:07:54]

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

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

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-10-10 15:23:06]

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

[commit-bot: I haz the power, 2016-10-10 15:23:07]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Michael van Ouwerkerk, 2016-10-10 16:19:32]

Thanks! Marc: please take another look.

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

https://codereview.chromium.org/2400783003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/AllDismissedItem.java:65:
int messageId = R.string.ntp_all_dismissed_body_text_morning;
On 2016/10/07 13:46:44, Bernhard Bauer wrote:
> Move the assignment into an additional else branch at the end, so the compiler
> checks that we pick one of the branches?

Done.

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

https://codereview.chromium.org/2400783003/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.cc:160: for (const auto& it
: dismissed_providers_by_category_)
On 2016/10/07 11:40:13, Marc Treib wrote:
> nit: Please give |it| another name, it's not an iterator.

Done.

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

https://codereview.chromium.org/2400783003/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.h:252:
dismissed_providers_by_category_;
On 2016/10/07 11:40:13, Marc Treib wrote:
> What's the status of persisting dismissals? Seems like this wouldn't really
work
> together - we can't persist the provider pointers.
> 
> Generally, I'm not too happy with Yet Another data structure storing
> categories/providers here... can we just go over all providers instead and
> re-register everything?

Nicolas is working on persisting section dismissals. As discussed over chat,
keeping a separate data structure centralized in the service seems slightly
preferable to the alternatives of introducing an additional provider state, only
partially undoing what happens in RegisterCategoryIfRequired, or additional
provider / observer plumbing.

[Michael van Ouwerkerk, 2016-10-10 16:22:08]

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

[Michael van Ouwerkerk, 2016-10-10 16:22:08]

Alexei, can you take a look please for histograms.xml?

[Marc Treib, 2016-10-11 07:35:58]

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

https://codereview.chromium.org/2400783003/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:160: for (const auto&
category_provider_pair : dismissed_providers_by_category_)
Braces if the body doesn't fit on one line.

https://codereview.chromium.org/2400783003/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:163:
dismissed_providers_by_category_.clear();
Should RegisterCategoryIfRequired remove the entry from
dismissed_providers_by_category_? That would make sure we don't have a provider
in both providers_by_category_ and dismissed_providers_by_category_. Right now
that'll happen if the status changes after the category has been dismissed.

https://codereview.chromium.org/2400783003/diff/40001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2400783003/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:252:
dismissed_providers_by_category_;
nit: Maybe move this next to provider_by_category_?

[Michael van Ouwerkerk, 2016-10-11 12:04:16]

Thanks Marc! Please take another look.

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

https://codereview.chromium.org/2400783003/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:160: for (const auto&
category_provider_pair : dismissed_providers_by_category_)
On 2016/10/11 07:35:58, Marc Treib wrote:
> Braces if the body doesn't fit on one line.

Done.

https://codereview.chromium.org/2400783003/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:163:
dismissed_providers_by_category_.clear();
On 2016/10/11 07:35:58, Marc Treib wrote:
> Should RegisterCategoryIfRequired remove the entry from
> dismissed_providers_by_category_? That would make sure we don't have a
provider
> in both providers_by_category_ and dismissed_providers_by_category_. Right now
> that'll happen if the status changes after the category has been dismissed.

Done.

https://codereview.chromium.org/2400783003/diff/40001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2400783003/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:252:
dismissed_providers_by_category_;
On 2016/10/11 07:35:58, Marc Treib wrote:
> nit: Maybe move this next to provider_by_category_?

Done.

[Michael van Ouwerkerk, 2016-10-11 12:04:22]

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

[commit-bot: I haz the power, 2016-10-11 12:04:31]

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-10-11 12:13:38]

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

[commit-bot: I haz the power, 2016-10-11 12:13:39]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Marc Treib, 2016-10-11 12:18:15]

LGTM with some more nits.

It would also be really nice to have some tests for the category dismissal and
un-dismissal. I guess dgn is working on some?

https://codereview.chromium.org/2400783003/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java
(right):

https://codereview.chromium.org/2400783003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:52:
public static final int ACTION_CLICKED_INTEREST = 8;
Unrelated, but: This is unused. Maybe remove it, or at least mark it as
deprecated or something?

https://codereview.chromium.org/2400783003/diff/60001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2400783003/diff/60001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:156:
std::find(categories_.begin(), categories_.end(), category));
I think we agreed to also clear suggestions_by_category_[category] here? It
should be rare that there's anything there at this point, but let's make sure
that our various data structures always stay consistent.

https://codereview.chromium.org/2400783003/diff/60001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:160: auto
dismissed_providers_by_category_copy = dismissed_providers_by_category_;
nit: Add a comment on why the copy is needed?

https://codereview.chromium.org/2400783003/diff/60001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:166:
dismissed_providers_by_category_.clear();
Now it should already be empty at this point, right? If so, maybe just DCHECK
that?

[Michael van Ouwerkerk, 2016-10-11 15:21:32]

Thanks! I've added a test for dismissing and restoring a category.

https://codereview.chromium.org/2400783003/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java
(right):

https://codereview.chromium.org/2400783003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:52:
public static final int ACTION_CLICKED_INTEREST = 8;
On 2016/10/11 12:18:14, Marc Treib wrote:
> Unrelated, but: This is unused. Maybe remove it, or at least mark it as
> deprecated or something?

I added a TODO for removing it and cleaning up histograms.xml accordingly.

https://codereview.chromium.org/2400783003/diff/60001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2400783003/diff/60001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:156:
std::find(categories_.begin(), categories_.end(), category));
On 2016/10/11 12:18:14, Marc Treib wrote:
> I think we agreed to also clear suggestions_by_category_[category] here? It
> should be rare that there's anything there at this point, but let's make sure
> that our various data structures always stay consistent.

Done.

https://codereview.chromium.org/2400783003/diff/60001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:160: auto
dismissed_providers_by_category_copy = dismissed_providers_by_category_;
On 2016/10/11 12:18:14, Marc Treib wrote:
> nit: Add a comment on why the copy is needed?

Done.

https://codereview.chromium.org/2400783003/diff/60001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:166:
dismissed_providers_by_category_.clear();
On 2016/10/11 12:18:15, Marc Treib wrote:
> Now it should already be empty at this point, right? If so, maybe just DCHECK
> that?

Done.

[Michael van Ouwerkerk, 2016-10-11 15:21:47]

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

[commit-bot: I haz the power, 2016-10-11 15:22:06]

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-10-11 15:24:26]

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

[commit-bot: I haz the power, 2016-10-11 15:24:28]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Bernhard Bauer, 2016-10-11 15:26:28]

https://codereview.chromium.org/2400783003/diff/80001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
(right):

https://codereview.chromium.org/2400783003/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:163:
if (descriptors.length == 0)
matcher.expect(NewTabPageItem.VIEW_TYPE_ALL_DISMISSED);
Nit: Use braces and separate lines here?

[Marc Treib, 2016-10-11 15:29:24]

lgtm

Thanks!

https://codereview.chromium.org/2400783003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2400783003/diff/80001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:169:
dismissed_providers_by_category_.clear();
nit: No need for this - we don't handle DCHECK failures.
(But you don't have to abort the commit for this)

[Michael van Ouwerkerk, 2016-10-11 15:37:32]

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

[Michael van Ouwerkerk, 2016-10-11 15:37:43]

https://codereview.chromium.org/2400783003/diff/80001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
(right):

https://codereview.chromium.org/2400783003/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:163:
if (descriptors.length == 0)
matcher.expect(NewTabPageItem.VIEW_TYPE_ALL_DISMISSED);
On 2016/10/11 15:26:28, Bernhard Bauer wrote:
> Nit: Use braces and separate lines here?

Done.

https://codereview.chromium.org/2400783003/diff/80001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2400783003/diff/80001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:169:
dismissed_providers_by_category_.clear();
On 2016/10/11 15:29:24, Marc Treib wrote:
> nit: No need for this - we don't handle DCHECK failures.
> (But you don't have to abort the commit for this)

Done.

[commit-bot: I haz the power, 2016-10-11 15:37:49]

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

[Alexei Svitkine (slow), 2016-10-11 15:43:08]

histograms.xml lgtm

[commit-bot: I haz the power, 2016-10-11 15:47:16]

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

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

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

[Michael van Ouwerkerk, 2016-10-12 09:31:56]

The CQ bit was checked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2016-10-12 09:31:56]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
treib@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2400783003/#ps120001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-10-12 09:32: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-10-12 11:04:06]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-12 11:06:31]

Description was changed from

==========
Ntp: show AllDismissedItem when all sections have been dismissed.

BUG=651058
==========

to

==========
Ntp: show AllDismissedItem when all sections have been dismissed.

BUG=651058

Committed: https://crrev.com/52783d986c243bce2baeed48a07b6b15029d3312
Cr-Commit-Position: refs/heads/master@{#424710}
==========

[commit-bot: I haz the power, 2016-10-12 11:06:32]

Patchset 7 (id:??) landed as
https://crrev.com/52783d986c243bce2baeed48a07b6b15029d3312
Cr-Commit-Position: refs/heads/master@{#424710}