[NTP::PhysicalWeb] Implement suggestion dismissal.

[NTP::PhysicalWeb] Implement suggestion dismissal.

After this CL Physical Web suggestions on the NTP can be dismissed.
The list is stored in prefs. It is pruned on fetches and when URL is
lost.

BUG=667766
Committed: https://crrev.com/2600e8e00ef42b3399872fe8fec5ba829793e337
Cr-Commit-Position: refs/heads/master@{#437573}

[vitaliii, 2016-12-07 12:43:18]

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

[commit-bot: I haz the power, 2016-12-07 12:43:35]

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

[vitaliii, 2016-12-07 12:43:49]

vitaliii@chromium.org changed reviewers:
+ jkrcal@chromium.org

[vitaliii, 2016-12-07 12:43:50]

Hi jkrcal@,

could you have a look?

[commit-bot: I haz the power, 2016-12-07 13:32:04]

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

[commit-bot: I haz the power, 2016-12-07 13:32:04]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2016-12-07 14:20:01]

lgtm with nits (assuming you removed the DCHECK by mistake)

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

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.cc:171:
RemoveSuggestionByID(suggestion_id);
Why did you remove the DCHECK?

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/phy...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:191:
const std::set<std::string> old_dismissed_ids = ReadDismissedIDsFromPrefs();
nit: Please add a comment like "Filter out dismissed suggestions (and also prune
dismissed suggestions that are not physically around; needed if we miss some
OnLost() event)."

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:291:
std::set<std::string> dismissed_ids = ReadDismissedIDsFromPrefs();
nit: Could you add a comment saying "Remove page_id from dismissed suggestions
if it is there."?

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/phy...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:297:
// that it can be returned.
nit: I find this comment and code misleading as it digs deep into implementation
of GetDismissedSuggestionsForDebugging. I think it suffices to say that
"Suggestion 1 should be pruned from dismissed suggestions now. To test it, we
add it back to the data source and it should be again returned in the
suggestions."

and code:

EXPECT_CALL(
    *observer(),
    OnNewSuggestions(_, provided_category(),
                     ElementsAre(HasUrl("https://resolved_url.com/1"),
                                 HasUrl("https://resolved_url.com/3"),
                                 HasUrl("https://resolved_url.com/4"))));
FireUrlFound("https://resolved_url.com/4");

(the same in the test below)

WDYT?

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:354:
}
A general comment: could you please use minimal data sets for each given test?

E.g. in ShouldPruneDismissedSuggestionsWhenFetching, pages 3 and 4 are not
needed for the test.

I think more pages then necessary make it harder to understand what exactly are
you testing.

[vitaliii, 2016-12-08 07:11:30]

Patchset #2 (id:20001) has been deleted

[vitaliii, 2016-12-08 07:13:02]

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

[commit-bot: I haz the power, 2016-12-08 07:13:16]

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

[vitaliii, 2016-12-08 07:13:42]

Hi jkrcal@,

I addressed your comments. PTAL.
DCHECK removal was intended, see the comment.

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

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.cc:171:
RemoveSuggestionByID(suggestion_id);
On 2016/12/07 14:20:01, jkrcal wrote:
> Why did you remove the DCHECK?

Because of it one could not dismiss suggestions obtained through "More" action
(i.e. through |Fetch| call). Previously, the "More" action was used only for
articles and added suggestions were propagated to new NTPs through
OnNewSuggestions, so this DCHECK did not fire. However, in Physical Web provider
the suggestions are not propagated to new NTPs and this DCHECK fires.

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/phy...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:191:
const std::set<std::string> old_dismissed_ids = ReadDismissedIDsFromPrefs();
On 2016/12/07 14:20:01, jkrcal wrote:
> nit: Please add a comment like "Filter out dismissed suggestions (and also
prune
> dismissed suggestions that are not physically around; needed if we miss some
> OnLost() event)."

Done.

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:291:
std::set<std::string> dismissed_ids = ReadDismissedIDsFromPrefs();
On 2016/12/07 14:20:01, jkrcal wrote:
> nit: Could you add a comment saying "Remove page_id from dismissed suggestions
> if it is there."?

Done.

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/phy...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:297:
// that it can be returned.
On 2016/12/07 14:20:01, jkrcal wrote:
> nit: I find this comment and code misleading as it digs deep into
implementation
> of GetDismissedSuggestionsForDebugging. I think it suffices to say that
> "Suggestion 1 should be pruned from dismissed suggestions now. To test it, we
> add it back to the data source and it should be again returned in the
> suggestions."
> 
> and code:
> 
> EXPECT_CALL(
>     *observer(),
>     OnNewSuggestions(_, provided_category(),
>                      ElementsAre(HasUrl("https://resolved_url.com/1"),
>                                  HasUrl("https://resolved_url.com/3"),
>                                  HasUrl("https://resolved_url.com/4"))));
> FireUrlFound("https://resolved_url.com/4");
> 
> (the same in the test below)
> 
> WDYT?

TBH, I am not sure. |OnNewSuggestions| is rather indirect way of getting
dismissed IDs and it seems strange to use it while having
|GetDismissedSuggestionsForDebugging|. Also in the test below |OnNewSuggestions|
is essential (behavior of fetching is tested), while here only dismissed IDs are
of interest.

The comment is to explain why page 1 is added back to the source after it has
been lost. I rewrote the comment.

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:354:
}
On 2016/12/07 14:20:01, jkrcal wrote:
> A general comment: could you please use minimal data sets for each given test?
> 
> E.g. in ShouldPruneDismissedSuggestionsWhenFetching, pages 3 and 4 are not
> needed for the test.
> 
> I think more pages then necessary make it harder to understand what exactly
are
> you testing.

Done.

3 is needed, since I want to keep data source behavior realistic.

[vitaliii, 2016-12-08 07:13:42]

Hi jkrcal@,

I addressed your comments. PTAL.
DCHECK removal was intended, see the comment.

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

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

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

Dry run: This issue passed the CQ dry run.

[jkrcal, 2016-12-08 09:55:42]

Still lgtm, thanks!

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

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.cc:171:
RemoveSuggestionByID(suggestion_id);
On 2016/12/08 07:13:41, vitaliii wrote:
> On 2016/12/07 14:20:01, jkrcal wrote:
> > Why did you remove the DCHECK?
> 
> Because of it one could not dismiss suggestions obtained through "More" action
> (i.e. through |Fetch| call). Previously, the "More" action was used only for
> articles and added suggestions were propagated to new NTPs through
> OnNewSuggestions, so this DCHECK did not fire. However, in Physical Web
provider
> the suggestions are not propagated to new NTPs and this DCHECK fires.

Ok, please expand the comment.

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/phy...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:297:
// that it can be returned.
On 2016/12/08 07:13:41, vitaliii wrote:
> On 2016/12/07 14:20:01, jkrcal wrote:
> > nit: I find this comment and code misleading as it digs deep into
> implementation
> > of GetDismissedSuggestionsForDebugging. I think it suffices to say that
> > "Suggestion 1 should be pruned from dismissed suggestions now. To test it,
we
> > add it back to the data source and it should be again returned in the
> > suggestions."
> > 
> > and code:
> > 
> > EXPECT_CALL(
> >     *observer(),
> >     OnNewSuggestions(_, provided_category(),
> >                      ElementsAre(HasUrl("https://resolved_url.com/1"),
> >                                  HasUrl("https://resolved_url.com/3"),
> >                                  HasUrl("https://resolved_url.com/4"))));
> > FireUrlFound("https://resolved_url.com/4");
> > 
> > (the same in the test below)
> > 
> > WDYT?
> 
> TBH, I am not sure. |OnNewSuggestions| is rather indirect way of getting
> dismissed IDs and it seems strange to use it while having
> |GetDismissedSuggestionsForDebugging|. Also in the test below
|OnNewSuggestions|
> is essential (behavior of fetching is tested), while here only dismissed IDs
are
> of interest.
> 
> The comment is to explain why page 1 is added back to the source after it has
> been lost. I rewrote the comment.

Ok, not important enough to argue further ;)
Thanks!

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:354:
}
On 2016/12/08 07:13:42, vitaliii wrote:
> On 2016/12/07 14:20:01, jkrcal wrote:
> > A general comment: could you please use minimal data sets for each given
test?
> > 
> > E.g. in ShouldPruneDismissedSuggestionsWhenFetching, pages 3 and 4 are not
> > needed for the test.
> > 
> > I think more pages then necessary make it harder to understand what exactly
> are
> > you testing.
> 
> Done.
> 
> 3 is needed, since I want to keep data source behavior realistic.

Acknowledged.

[vitaliii, 2016-12-08 11:05:28]

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

[vitaliii, 2016-12-08 11:05:29]

Hi bauerb@,

could you have a look at browser_prefs_android.cc change?

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

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service.cc:171:
RemoveSuggestionByID(suggestion_id);
On 2016/12/08 09:55:42, jkrcal wrote:
> On 2016/12/08 07:13:41, vitaliii wrote:
> > On 2016/12/07 14:20:01, jkrcal wrote:
> > > Why did you remove the DCHECK?
> > 
> > Because of it one could not dismiss suggestions obtained through "More"
action
> > (i.e. through |Fetch| call). Previously, the "More" action was used only for
> > articles and added suggestions were propagated to new NTPs through
> > OnNewSuggestions, so this DCHECK did not fire. However, in Physical Web
> provider
> > the suggestions are not propagated to new NTPs and this DCHECK fires.
> 
> Ok, please expand the comment.

Done.

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/phy...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2560783002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:297:
// that it can be returned.
On 2016/12/08 09:55:42, jkrcal wrote:
> On 2016/12/08 07:13:41, vitaliii wrote:
> > On 2016/12/07 14:20:01, jkrcal wrote:
> > > nit: I find this comment and code misleading as it digs deep into
> > implementation
> > > of GetDismissedSuggestionsForDebugging. I think it suffices to say that
> > > "Suggestion 1 should be pruned from dismissed suggestions now. To test it,
> we
> > > add it back to the data source and it should be again returned in the
> > > suggestions."
> > > 
> > > and code:
> > > 
> > > EXPECT_CALL(
> > >     *observer(),
> > >     OnNewSuggestions(_, provided_category(),
> > >                      ElementsAre(HasUrl("https://resolved_url.com/1"),
> > >                                  HasUrl("https://resolved_url.com/3"),
> > >                                  HasUrl("https://resolved_url.com/4"))));
> > > FireUrlFound("https://resolved_url.com/4");
> > > 
> > > (the same in the test below)
> > > 
> > > WDYT?
> > 
> > TBH, I am not sure. |OnNewSuggestions| is rather indirect way of getting
> > dismissed IDs and it seems strange to use it while having
> > |GetDismissedSuggestionsForDebugging|. Also in the test below
> |OnNewSuggestions|
> > is essential (behavior of fetching is tested), while here only dismissed IDs
> are
> > of interest.
> > 
> > The comment is to explain why page 1 is added back to the source after it
has
> > been lost. I rewrote the comment.
> 
> Ok, not important enough to argue further ;)
> Thanks!

Acknowledged.

[LeonardWalton143, 2016-12-08 11:25:03]

The ayurvedic treatments in Kerala were famous among people. Through their best
treatments people will get relief from the diseases that can't consult by other
medicines. Choose
http://chaithanyaayurvedaretreat.com/ayurveda-treatment-therapies for best
service.

[Bernhard Bauer, 2016-12-08 14:58:07]

Description was changed from

==========
[NTP::PhysicalWeb] Implement suggestion dismissal.

After this CL Physical Web suggestions on the NTP can be dismissed.
The list is stored in prefs. It is pruned on fetches and when URL is
lost.

BUG=667766
==========

to

==========
[NTP::PhysicalWeb] Implement suggestion dismissal.

After this CL Physical Web suggestions on the NTP can be dismissed.
The list is stored in prefs. It is pruned on fetches and when URL is
lost.

BUG=667766
==========

[Bernhard Bauer, 2016-12-08 14:58:07]

bauerb@chromium.org changed reviewers:
- LeonardWalton143@gmail.com

[Bernhard Bauer, 2016-12-08 15:09:39]

https://codereview.chromium.org/2560783002/diff/60001/chrome/browser/android/...
File chrome/browser/android/preferences/browser_prefs_android.cc (right):

https://codereview.chromium.org/2560783002/diff/60001/chrome/browser/android/...
chrome/browser/android/preferences/browser_prefs_android.cc:24:
ntp_snippets::RecentTabSuggestionsProvider::RegisterProfilePrefs(registry);
In retrospect, it would probably make more sense to register NTP snippets prefs
directly in browser_prefs (just inside an #ifdef) instead of here.

[vitaliii, 2016-12-09 09:20:00]

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

[commit-bot: I haz the power, 2016-12-09 09:20:11]

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

[vitaliii, 2016-12-09 09:20:31]

Hi bauerb@,

PTAL.

https://codereview.chromium.org/2560783002/diff/60001/chrome/browser/android/...
File chrome/browser/android/preferences/browser_prefs_android.cc (right):

https://codereview.chromium.org/2560783002/diff/60001/chrome/browser/android/...
chrome/browser/android/preferences/browser_prefs_android.cc:24:
ntp_snippets::RecentTabSuggestionsProvider::RegisterProfilePrefs(registry);
On 2016/12/08 15:09:39, Bernhard Bauer wrote:
> In retrospect, it would probably make more sense to register NTP snippets
prefs
> directly in browser_prefs (just inside an #ifdef) instead of here.

Done.

[commit-bot: I haz the power, 2016-12-09 10:07:03]

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

[commit-bot: I haz the power, 2016-12-09 10:07:04]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-12-09 14:33:07]

LGTM with one final nit :)

https://codereview.chromium.org/2560783002/diff/80001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2560783002/diff/80001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:554: #if defined(OS_ANDROID)
Can you move this block to line 595? I think it's unconditional registrations
first, then build flags, then OS.

[vitaliii, 2016-12-09 16:13:48]

Addressed bauerb@ nit, no need to look.

https://codereview.chromium.org/2560783002/diff/80001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2560783002/diff/80001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:554: #if defined(OS_ANDROID)
On 2016/12/09 14:33:07, Bernhard Bauer wrote:
> Can you move this block to line 595? I think it's unconditional registrations
> first, then build flags, then OS.

Done.

[vitaliii, 2016-12-09 16:13:56]

The CQ bit was checked by vitaliii@chromium.org

[vitaliii, 2016-12-09 16:13:56]

The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org,
bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2560783002/#ps100001
(title: "rebase and bauerb@ nit.")

[commit-bot: I haz the power, 2016-12-09 16:14:14]

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-12-09 17:23:42]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481300036520890,
"parent_rev": "85791b94c0d34d5e711b6a0085577934b940e1ee", "commit_rev":
"dd99b266158155e5b8a88f34576d48590378ed6f"}

[commit-bot: I haz the power, 2016-12-09 17:24:10]

Description was changed from

==========
[NTP::PhysicalWeb] Implement suggestion dismissal.

After this CL Physical Web suggestions on the NTP can be dismissed.
The list is stored in prefs. It is pruned on fetches and when URL is
lost.

BUG=667766
==========

to

==========
[NTP::PhysicalWeb] Implement suggestion dismissal.

After this CL Physical Web suggestions on the NTP can be dismissed.
The list is stored in prefs. It is pruned on fetches and when URL is
lost.

BUG=667766

Review-Url: https://codereview.chromium.org/2560783002
==========

[commit-bot: I haz the power, 2016-12-09 17:24:14]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-12-12 14:35:44]

Description was changed from

==========
[NTP::PhysicalWeb] Implement suggestion dismissal.

After this CL Physical Web suggestions on the NTP can be dismissed.
The list is stored in prefs. It is pruned on fetches and when URL is
lost.

BUG=667766

Review-Url: https://codereview.chromium.org/2560783002
==========

to

==========
[NTP::PhysicalWeb] Implement suggestion dismissal.

After this CL Physical Web suggestions on the NTP can be dismissed.
The list is stored in prefs. It is pruned on fetches and when URL is
lost.

BUG=667766

Committed: https://crrev.com/2600e8e00ef42b3399872fe8fec5ba829793e337
Cr-Commit-Position: refs/heads/master@{#437573}
==========

[commit-bot: I haz the power, 2016-12-12 14:35:45]

Patchset 5 (id:??) landed as
https://crrev.com/2600e8e00ef42b3399872fe8fec5ba829793e337
Cr-Commit-Position: refs/heads/master@{#437573}