[NTP Snippets] NTPSnippetsService: notify about new suggestions only in changed categories

[NTP Snippets] NTPSnippetsService: notify about new suggestions only in changed categories

BUG=657038
Committed: https://crrev.com/534523ba36742819e3712ae9054705b431e20d37
Cr-Commit-Position: refs/heads/master@{#426492}

[Marc Treib, 2016-10-20 13:04:58]

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

[Marc Treib, 2016-10-20 13:04:58]

PTAL!

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

https://codereview.chromium.org/2438543003/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_service.cc:744:
ClearCachedSuggestions(category);
The bug was triggered from here: ClearCachedSuggestions would call
NotifyNewSuggestions, which would call
ContentSuggestionsService::OnNewSuggestions for all categories, even those that
had already been marked NOT_PROVIDED in a previous iteration. And then
ContentSuggestionsService::OnNewSuggestions would add the category again.

[dgn, 2016-10-20 14:07:33]

Thanks! lgtm

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

https://codereview.chromium.org/2438543003/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_service.cc:584: // TODO(sfiera):
notify only when a category changed above.
todo not needed anymore?

[Marc Treib, 2016-10-20 14:29:50]

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

https://codereview.chromium.org/2438543003/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/ntp_snippets_service.cc:584: // TODO(sfiera):
notify only when a category changed above.
On 2016/10/20 14:07:33, dgn wrote:
> todo not needed anymore?

I think it's still needed: We still notify about all categories, even if not all
of them were changed above.
Fixing this might be as simple as adding an "if (content->provided_by_server)",
but I didn't want to do that in the same CL.

[Marc Treib, 2016-10-20 14:29:56]

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

[commit-bot: I haz the power, 2016-10-20 14:30:11]

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-20 15:43:56]

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

[commit-bot: I haz the power, 2016-10-20 15:43:58]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2016-10-20 16:09:22]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2016-10-20 16:09:39]

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 16:20:10]

Committed patchset #1 (id:1)

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

Description was changed from

==========
[NTP Snippets] NTPSnippetsService: notify about new suggestions only in changed
categories

BUG=657038
==========

to

==========
[NTP Snippets] NTPSnippetsService: notify about new suggestions only in changed
categories

BUG=657038

Committed: https://crrev.com/534523ba36742819e3712ae9054705b431e20d37
Cr-Commit-Position: refs/heads/master@{#426492}
==========

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

Patchset 1 (id:??) landed as
https://crrev.com/534523ba36742819e3712ae9054705b431e20d37
Cr-Commit-Position: refs/heads/master@{#426492}