Remove deleted offline page suggestions from opened NTPs

Remove deleted offline page suggestions from opened NTPs

Add OnSuggestionInvalidated methods to the ContentSuggestionsProvider,
the ContentSuggestionsService and its observer and to the SnippetsBridge.

Remove suggestions from the UI immediately when they're invalidated.

Invalidate offline page suggestions when the referenced offline page is
deleted, and also clear it from the dismissed_ids list.

Add new unit tests for the UI layer and the service layer.

BUG=628198
Committed: https://crrev.com/2a48f8508b52e518696495338ea4ed7f9b7e6060
Cr-Commit-Position: refs/heads/master@{#412813}

[Philipp Keck, 2016-08-12 18:12:35]

pke@google.com changed reviewers:
+ treib@chromium.org

[Philipp Keck, 2016-08-12 18:12:35]

PTAL

[Michael van Ouwerkerk, 2016-08-15 11:06:08]

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

[Michael van Ouwerkerk, 2016-08-15 11:06:09]

Some drive-by nits :-) (I'm not adding myself to the reviewer list)

https://codereview.chromium.org/2244793002/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/2244793002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:211:
SuggestionsSection section = mSections.get(category);
nit: it would be less verbose, and I think still clear, to merge this line with
the next without using a local variable:
mSections.get(category).removeSuggestionById(suggestionId);

https://codereview.chromium.org/2244793002/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/2244793002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:64:
public void removeSuggestionById(String suggestionId) {
This method is called |removeSuggestion| and the one above |dismissSuggestion|.
Is there a semantic difference between |remove| and |dismiss|? If not, maybe we
can use the same term for both.

[Michael van Ouwerkerk, 2016-08-15 11:06:17]

Description was changed from

==========
Remove deleted offline page suggestions from opened NTPs

Add OnSuggestionInvalidated methods to the ContentSuggestionsProvider,
the ContentSuggestionsService and its observer and to the SnippetsBridge.

Remove suggestions from the UI immediately when they're invalidated.

Invalidate offline page suggestions when the referenced offline page is
deleted, and also clear it from the dismissed_ids list.

Add new unit tests for the UI layer and the service layer.

BUG=628198
==========

to

==========
Remove deleted offline page suggestions from opened NTPs

Add OnSuggestionInvalidated methods to the ContentSuggestionsProvider,
the ContentSuggestionsService and its observer and to the SnippetsBridge.

Remove suggestions from the UI immediately when they're invalidated.

Invalidate offline page suggestions when the referenced offline page is
deleted, and also clear it from the dismissed_ids list.

Add new unit tests for the UI layer and the service layer.

BUG=628198
==========

[Michael van Ouwerkerk, 2016-08-15 11:06:17]

mvanouwerkerk@chromium.org changed reviewers:
- mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2016-08-15 11:06:45]

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

[commit-bot: I haz the power, 2016-08-15 11:06:51]

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-15 11:47:32]

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

[commit-bot: I haz the power, 2016-08-15 11:47:32]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2016-08-16 09:03:54]

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

https://codereview.chromium.org/2244793002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.cc:226:
FOR_EACH_OBSERVER(Observer, observers_,
Wouldn't we want to send the invalidation anyway? Even if the suggestion isn't
in the service's store anymore, it might still be shown in the UI.

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

https://codereview.chromium.org/2244793002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.h:185: // still existed.
Returns true if a suggestion was removed? (I find the current formulation a bit
confusing)

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

https://codereview.chromium.org/2244793002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service_unittest.cc:324: .Times(1);
As I've recently learned, ".Times(1)" is a no-op, so might as well leave it out.

https://codereview.chromium.org/2244793002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service_unittest.cc:326:
ExpectThatSuggestionsAre(articles_category, {11, 13});
Add a case with a suggestion that isn't known to the service?

https://codereview.chromium.org/2244793002/diff/1/components/ntp_snippets/off...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2244793002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:225:
observer()->OnSuggestionInvalidated(
I think we should send the invalidation in any case, even if it was dismissed -
there might still be *another* open NTP that has it. (super edge case, but
still)

Probably also if the category status is "bad" - since we don't clear the UI in
that case, the suggestion might also still be there.

[Philipp Keck, 2016-08-16 10:54:39]

pke@google.com changed reviewers:
+ bauerb@chromium.org

[Philipp Keck, 2016-08-16 10:54:40]

Thank you Michael and Marc.
Bernhard, PTAL at the Android part.

https://codereview.chromium.org/2244793002/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/2244793002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:211:
SuggestionsSection section = mSections.get(category);
On 2016/08/15 11:06:09, Michael van Ouwerkerk wrote:
> nit: it would be less verbose, and I think still clear, to merge this line
with
> the next without using a local variable:
> mSections.get(category).removeSuggestionById(suggestionId);

Done.

https://codereview.chromium.org/2244793002/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/2244793002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:64:
public void removeSuggestionById(String suggestionId) {
On 2016/08/15 11:06:09, Michael van Ouwerkerk wrote:
> This method is called |removeSuggestion| and the one above
|dismissSuggestion|.
> Is there a semantic difference between |remove| and |dismiss|? If not, maybe
we
> can use the same term for both.

Right. They should both be "removeSuggestion", as they don't actually perform
the dismissing anymore, that's done in the C++ backend. Renamed it and
"removeSuggestionById" now calls "removeSuggestion".

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

https://codereview.chromium.org/2244793002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.cc:226:
FOR_EACH_OBSERVER(Observer, observers_,
On 2016/08/16 09:03:53, Marc Treib wrote:
> Wouldn't we want to send the invalidation anyway? Even if the suggestion isn't
> in the service's store anymore, it might still be shown in the UI.

Yes, right. Done.

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

https://codereview.chromium.org/2244793002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.h:185: // still existed.
On 2016/08/16 09:03:54, Marc Treib wrote:
> Returns true if a suggestion was removed? (I find the current formulation a
bit
> confusing)

Done.

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

https://codereview.chromium.org/2244793002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service_unittest.cc:324: .Times(1);
On 2016/08/16 09:03:54, Marc Treib wrote:
> As I've recently learned, ".Times(1)" is a no-op, so might as well leave it
out.

Done (for the entire file).

https://codereview.chromium.org/2244793002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service_unittest.cc:326:
ExpectThatSuggestionsAre(articles_category, {11, 13});
On 2016/08/16 09:03:54, Marc Treib wrote:
> Add a case with a suggestion that isn't known to the service?

Done.

https://codereview.chromium.org/2244793002/diff/1/components/ntp_snippets/off...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2244793002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:225:
observer()->OnSuggestionInvalidated(
On 2016/08/16 09:03:54, Marc Treib wrote:
> I think we should send the invalidation in any case, even if it was dismissed
-
> there might still be *another* open NTP that has it. (super edge case, but
> still)

I will fix this in an upcoming CL (all this code is totally refactored there).
There is another detail worth noting: The OfflinePageSuggestionsProvider doesn't
know what it's forwarding here. As we only return the first 10 results from the
OfflinePageModel when delivering suggestions, we have a good chance of reporting
a suggestion as invalidated which has never existed in the first place. I will
also document this in the next CL.

> Probably also if the category status is "bad" - since we don't clear the UI in
> that case, the suggestion might also still be there.

If we switched to the NOT_PROVIDED status dynamically, you would be right - it's
the only case where the category becomes unavailable but the section is still
visible on the UI. But the OfflinePageSuggestionsProvider doesn't do that.
We could still forward it, though I don't see why we would need to.

Calling OnSuggestionInvalidated for categories that never existed is a bit
weird, but not a problem.

[Philipp Keck, 2016-08-16 11:21:18]

https://codereview.chromium.org/2244793002/diff/1/components/ntp_snippets/off...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2244793002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:225:
observer()->OnSuggestionInvalidated(
On 2016/08/16 10:54:40, Philipp Keck wrote:
> On 2016/08/16 09:03:54, Marc Treib wrote:
> > I think we should send the invalidation in any case, even if it was
dismissed
> -
> > there might still be *another* open NTP that has it. (super edge case, but
> > still)
> 
> I will fix this in an upcoming CL (all this code is totally refactored there).
> There is another detail worth noting: The OfflinePageSuggestionsProvider
doesn't
> know what it's forwarding here. As we only return the first 10 results from
the
> OfflinePageModel when delivering suggestions, we have a good chance of
reporting
> a suggestion as invalidated which has never existed in the first place. I will
> also document this in the next CL.
> 
> > Probably also if the category status is "bad" - since we don't clear the UI
in
> > that case, the suggestion might also still be there.
> 
> If we switched to the NOT_PROVIDED status dynamically, you would be right -
it's
> the only case where the category becomes unavailable but the section is still
> visible on the UI. But the OfflinePageSuggestionsProvider doesn't do that.
> We could still forward it, though I don't see why we would need to.
> 
> Calling OnSuggestionInvalidated for categories that never existed is a bit
> weird, but not a problem.

The former is now implemented here:
https://codereview.chromium.org/2251743002/diff/20001/components/ntp_snippets...

[Michael van Ouwerkerk, 2016-08-16 11:23:55]

java lgtm

[Marc Treib, 2016-08-16 11:40:10]

lgtm

C++ LGTM (normally I might have asked for TODOs, but since the next CL is
already queued up...)

https://codereview.chromium.org/2244793002/diff/1/components/ntp_snippets/off...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2244793002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:225:
observer()->OnSuggestionInvalidated(
On 2016/08/16 10:54:40, Philipp Keck wrote:
> On 2016/08/16 09:03:54, Marc Treib wrote:
> > I think we should send the invalidation in any case, even if it was
dismissed
> -
> > there might still be *another* open NTP that has it. (super edge case, but
> > still)
> 
> I will fix this in an upcoming CL (all this code is totally refactored there).
> There is another detail worth noting: The OfflinePageSuggestionsProvider
doesn't
> know what it's forwarding here. As we only return the first 10 results from
the
> OfflinePageModel when delivering suggestions, we have a good chance of
reporting
> a suggestion as invalidated which has never existed in the first place. I will
> also document this in the next CL.
> 
> > Probably also if the category status is "bad" - since we don't clear the UI
in
> > that case, the suggestion might also still be there.
> 
> If we switched to the NOT_PROVIDED status dynamically, you would be right -
it's
> the only case where the category becomes unavailable but the section is still
> visible on the UI. But the OfflinePageSuggestionsProvider doesn't do that.
> We could still forward it, though I don't see why we would need to.

Fair enough, but probably worth a comment?

> Calling OnSuggestionInvalidated for categories that never existed is a bit
> weird, but not a problem.

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

https://codereview.chromium.org/2244793002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service_unittest.cc:320: std::string
suggestion_id = CreateSuggestion(12).id();
nitty nit: Move this after the empty line?

https://codereview.chromium.org/2244793002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service_unittest.cc:328: std::string
unknown_id = CreateSuggestion(1234).id();
nit: Add a comment explaining why? Otherwise the next reader will probably
wonder.

[Philipp Keck, 2016-08-16 11:51:04]

https://codereview.chromium.org/2244793002/diff/1/components/ntp_snippets/off...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2244793002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:225:
observer()->OnSuggestionInvalidated(
On 2016/08/16 11:40:09, Marc Treib wrote:
> On 2016/08/16 10:54:40, Philipp Keck wrote:
> > On 2016/08/16 09:03:54, Marc Treib wrote:
> > > I think we should send the invalidation in any case, even if it was
> dismissed
> > -
> > > there might still be *another* open NTP that has it. (super edge case, but
> > > still)
> > 
> > I will fix this in an upcoming CL (all this code is totally refactored
there).
> > There is another detail worth noting: The OfflinePageSuggestionsProvider
> doesn't
> > know what it's forwarding here. As we only return the first 10 results from
> the
> > OfflinePageModel when delivering suggestions, we have a good chance of
> reporting
> > a suggestion as invalidated which has never existed in the first place. I
will
> > also document this in the next CL.
> > 
> > > Probably also if the category status is "bad" - since we don't clear the
UI
> in
> > > that case, the suggestion might also still be there.
> > 
> > If we switched to the NOT_PROVIDED status dynamically, you would be right -
> it's
> > the only case where the category becomes unavailable but the section is
still
> > visible on the UI. But the OfflinePageSuggestionsProvider doesn't do that.
> > We could still forward it, though I don't see why we would need to.
> 
> Fair enough, but probably worth a comment?
> 
> > Calling OnSuggestionInvalidated for categories that never existed is a bit
> > weird, but not a problem.
> 

Done.

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

https://codereview.chromium.org/2244793002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service_unittest.cc:320: std::string
suggestion_id = CreateSuggestion(12).id();
On 2016/08/16 11:40:10, Marc Treib wrote:
> nitty nit: Move this after the empty line?

Done.

https://codereview.chromium.org/2244793002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service_unittest.cc:328: std::string
unknown_id = CreateSuggestion(1234).id();
On 2016/08/16 11:40:09, Marc Treib wrote:
> nit: Add a comment explaining why? Otherwise the next reader will probably
> wonder.

Done.

[Bernhard Bauer, 2016-08-16 16:55:22]

lgtm

[Philipp Keck, 2016-08-16 17:00:41]

On 2016/08/16 16:55:22, Bernhard Bauer wrote:
> lgtm

As suggested by bauerb@ over there, I'm moving the method comment changes for
OnSuggestionInvalidated from https://codereview.chromium.org/2251743002/ to this
CL.

[Philipp Keck, 2016-08-18 08:14:33]

The CQ bit was checked by pke@google.com

[Philipp Keck, 2016-08-18 08:14:33]

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

[commit-bot: I haz the power, 2016-08-18 08:14:43]

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

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

[commit-bot: I haz the power, 2016-08-18 08:23:28]

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

[Philipp Keck, 2016-08-18 12:57:45]

The CQ bit was checked by pke@google.com

[Philipp Keck, 2016-08-18 12:57:46]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
bauerb@chromium.org, mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/2244793002/#ps140001
(title: "Adjust MockContentSuggestionsProvider")

[commit-bot: I haz the power, 2016-08-18 12:57:56]

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-18 13:34:07]

Description was changed from

==========
Remove deleted offline page suggestions from opened NTPs

Add OnSuggestionInvalidated methods to the ContentSuggestionsProvider,
the ContentSuggestionsService and its observer and to the SnippetsBridge.

Remove suggestions from the UI immediately when they're invalidated.

Invalidate offline page suggestions when the referenced offline page is
deleted, and also clear it from the dismissed_ids list.

Add new unit tests for the UI layer and the service layer.

BUG=628198
==========

to

==========
Remove deleted offline page suggestions from opened NTPs

Add OnSuggestionInvalidated methods to the ContentSuggestionsProvider,
the ContentSuggestionsService and its observer and to the SnippetsBridge.

Remove suggestions from the UI immediately when they're invalidated.

Invalidate offline page suggestions when the referenced offline page is
deleted, and also clear it from the dismissed_ids list.

Add new unit tests for the UI layer and the service layer.

BUG=628198
==========

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

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-18 13:35:59]

Description was changed from

==========
Remove deleted offline page suggestions from opened NTPs

Add OnSuggestionInvalidated methods to the ContentSuggestionsProvider,
the ContentSuggestionsService and its observer and to the SnippetsBridge.

Remove suggestions from the UI immediately when they're invalidated.

Invalidate offline page suggestions when the referenced offline page is
deleted, and also clear it from the dismissed_ids list.

Add new unit tests for the UI layer and the service layer.

BUG=628198
==========

to

==========
Remove deleted offline page suggestions from opened NTPs

Add OnSuggestionInvalidated methods to the ContentSuggestionsProvider,
the ContentSuggestionsService and its observer and to the SnippetsBridge.

Remove suggestions from the UI immediately when they're invalidated.

Invalidate offline page suggestions when the referenced offline page is
deleted, and also clear it from the dismissed_ids list.

Add new unit tests for the UI layer and the service layer.

BUG=628198

Committed: https://crrev.com/2a48f8508b52e518696495338ea4ed7f9b7e6060
Cr-Commit-Position: refs/heads/master@{#412813}
==========

[commit-bot: I haz the power, 2016-08-18 13:36:00]

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