Make OfflinePage ContentSuggestions dismissable

Make OfflinePage ContentSuggestions dismissable

Make MakeUniqueID and friends const methods.
Add a new pref to store the IDs of all dismissed offline pages. Add
methods in OfflinePageSuggestionsProvider to store and load these IDs.
Modify OnOfflinePagesLoaded to only forward non-dismissed items.
Implement ClearDismissedSuggestionsForDebugging and a temporary version
for GetDismissedSuggestionsForDebugging (the Snippets Internals will
need refactoring for this to work properly).

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

[Philipp Keck, 2016-08-11 10:02:36]

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

[Philipp Keck, 2016-08-11 10:02:37]

PTAL

[Marc Treib, 2016-08-11 10:28:12]

One thing still missing is cleaning up the dismissed ids list when an offline
page gets deleted.

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

https://codereview.chromium.org/2236303002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:105:
for (std::string dismissed_id : dismissed_ids_) {
const std::string&

https://codereview.chromium.org/2236303002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:174:
// TODO(pke): Make sure the URL is actually opened as an offline URL.
Huh, this kinda seems like a launch blocker TBH. Do we have a tracking bug for
that? If not, please file one, and mention it here ("crbug.com/12345")

https://codereview.chromium.org/2236303002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:182:
// TODO(pke): Sort my most recently visited and only keep the top one of
s/my/by/

https://codereview.chromium.org/2236303002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:196:
for (const auto& value : *list) {
Might as well say base::Value instead of auto, not much longer :)

https://codereview.chromium.org/2236303002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:200:
dismissed_ids_.insert(dismissed_id);
std::move(dismissed_id), to avoid a string copy?

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

https://codereview.chromium.org/2236303002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:93:
std::set<std::string> dismissed_ids_;
We could also get rid of this member, and just read from prefs on demand. Then
there's no problem with keeping the two things in sync. WDYT?

[Marc Treib, 2016-08-11 10:28:37]

On 2016/08/11 10:28:12, Marc Treib wrote:
> One thing still missing is cleaning up the dismissed ids list when an offline
> page gets deleted.

(That doesn't have to happen in this CL though)

[Philipp Keck, 2016-08-11 12:53:20]

On 2016/08/11 10:28:12, Marc Treib wrote:
> One thing still missing is cleaning up the dismissed ids list when an offline
> page gets deleted.

Yep, that's on my list. I'll implement the offline-page-deleted handling soon
(because we also need to clear it off the UI). And I'll implement the cleanup on
load (comparing with the whole list of suggestions we get) after the splitting
of categories is implemented (so in my next-next CL).

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

https://codereview.chromium.org/2236303002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:105:
for (std::string dismissed_id : dismissed_ids_) {
On 2016/08/11 10:28:12, Marc Treib wrote:
> const std::string&

Done.

https://codereview.chromium.org/2236303002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:174:
// TODO(pke): Make sure the URL is actually opened as an offline URL.
On 2016/08/11 10:28:12, Marc Treib wrote:
> Huh, this kinda seems like a launch blocker TBH. Do we have a tracking bug for
> that? If not, please file one, and mention it here ("crbug.com/12345")

This is an open question in the PRD that Patrick hasn't addressed in a while.
I'll talk to him about it.

https://codereview.chromium.org/2236303002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:182:
// TODO(pke): Sort my most recently visited and only keep the top one of
On 2016/08/11 10:28:12, Marc Treib wrote:
> s/my/by/

Done.

https://codereview.chromium.org/2236303002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:196:
for (const auto& value : *list) {
On 2016/08/11 10:28:12, Marc Treib wrote:
> Might as well say base::Value instead of auto, not much longer :)

Done.

https://codereview.chromium.org/2236303002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:200:
dismissed_ids_.insert(dismissed_id);
On 2016/08/11 10:28:12, Marc Treib wrote:
> std::move(dismissed_id), to avoid a string copy?

I'd hope that he compiler does that for me? I inserted it.

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

https://codereview.chromium.org/2236303002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:93:
std::set<std::string> dismissed_ids_;
On 2016/08/11 10:28:12, Marc Treib wrote:
> We could also get rid of this member, and just read from prefs on demand. Then
> there's no problem with keeping the two things in sync. WDYT?

I don't know -- how slow are prefs? Since we're doing this on every navigation,
it might be a relevant difference.

[Marc Treib, 2016-08-11 13:13:23]

lgtm

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

https://codereview.chromium.org/2236303002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:200:
dismissed_ids_.insert(dismissed_id);
On 2016/08/11 12:53:19, Philipp Keck wrote:
> On 2016/08/11 10:28:12, Marc Treib wrote:
> > std::move(dismissed_id), to avoid a string copy?
> 
> I'd hope that he compiler does that for me? I inserted it.

No, the compiler can't generally deduce that the variable won't be used
afterwards. There's a special case for return statements, but that's it.

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

https://codereview.chromium.org/2236303002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:93:
std::set<std::string> dismissed_ids_;
On 2016/08/11 12:53:19, Philipp Keck wrote:
> On 2016/08/11 10:28:12, Marc Treib wrote:
> > We could also get rid of this member, and just read from prefs on demand.
Then
> > there's no problem with keeping the two things in sync. WDYT?
> 
> I don't know -- how slow are prefs? Since we're doing this on every
navigation,
> it might be a relevant difference.

They're in memory at least, so it can't be too bad. Certainly not compared to
all the other stuff we're doing per navigation.
So if you agree it makes things simpler, then I'd go with removing the member.

[Philipp Keck, 2016-08-11 13:34:04]

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

[Philipp Keck, 2016-08-11 13:34:04]

bauerb@chromium.org: Please review changes in 
chrome/browser/prefs/browser_prefs.cc

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

https://codereview.chromium.org/2236303002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:200:
dismissed_ids_.insert(dismissed_id);
On 2016/08/11 13:13:23, Marc Treib wrote:
> On 2016/08/11 12:53:19, Philipp Keck wrote:
> > On 2016/08/11 10:28:12, Marc Treib wrote:
> > > std::move(dismissed_id), to avoid a string copy?
> > 
> > I'd hope that he compiler does that for me? I inserted it.
> 
> No, the compiler can't generally deduce that the variable won't be used
> afterwards. There's a special case for return statements, but that's it.

It's the case for return statements because the variable goes out of scope right
after. When return-ing a member variable, for example, it doesn't do the
optimization (of course). And it's the same here: the variable goes out of scope
right after the statement.

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

https://codereview.chromium.org/2236303002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:93:
std::set<std::string> dismissed_ids_;
On 2016/08/11 13:13:23, Marc Treib wrote:
> On 2016/08/11 12:53:19, Philipp Keck wrote:
> > On 2016/08/11 10:28:12, Marc Treib wrote:
> > > We could also get rid of this member, and just read from prefs on demand.
> Then
> > > there's no problem with keeping the two things in sync. WDYT?
> > 
> > I don't know -- how slow are prefs? Since we're doing this on every
> navigation,
> > it might be a relevant difference.
> 
> They're in memory at least, so it can't be too bad. Certainly not compared to
> all the other stuff we're doing per navigation.
> So if you agree it makes things simpler, then I'd go with removing the member.

I tried it out and it doesn't make things easier.
The operations are: "Add one" (which becomes "load all, add one, store all") and
"remove all" (which becomes "re-init empty, store all"). The first operation is
more weird code, the second is the same amount of code. And once we'll have the
selective clean-up, that will also be a bit more code.
To be precise, instead of having the member around, you need to put
dismissed_ids = Read...() in every place you want to use the member. Or you
could have specific "AddToDismissedPref" methods that directly operator on the
base::Values, but that's not simpler either.

[Marc Treib, 2016-08-11 13:41:56]

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

https://codereview.chromium.org/2236303002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:200:
dismissed_ids_.insert(dismissed_id);
On 2016/08/11 13:34:04, Philipp Keck wrote:
> On 2016/08/11 13:13:23, Marc Treib wrote:
> > On 2016/08/11 12:53:19, Philipp Keck wrote:
> > > On 2016/08/11 10:28:12, Marc Treib wrote:
> > > > std::move(dismissed_id), to avoid a string copy?
> > > 
> > > I'd hope that he compiler does that for me? I inserted it.
> > 
> > No, the compiler can't generally deduce that the variable won't be used
> > afterwards. There's a special case for return statements, but that's it.
> 
> It's the case for return statements because the variable goes out of scope
right
> after. When return-ing a member variable, for example, it doesn't do the
> optimization (of course). And it's the same here: the variable goes out of
scope
> right after the statement.

Well, try it out if you want to - I'm pretty sure the automatic-move thingie
only applies to return statements (but I'm too lazy to try it myself, or even
find the appropriate part of the standard :D).

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

https://codereview.chromium.org/2236303002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h:93:
std::set<std::string> dismissed_ids_;
On 2016/08/11 13:34:04, Philipp Keck wrote:
> On 2016/08/11 13:13:23, Marc Treib wrote:
> > On 2016/08/11 12:53:19, Philipp Keck wrote:
> > > On 2016/08/11 10:28:12, Marc Treib wrote:
> > > > We could also get rid of this member, and just read from prefs on
demand.
> > Then
> > > > there's no problem with keeping the two things in sync. WDYT?
> > > 
> > > I don't know -- how slow are prefs? Since we're doing this on every
> > navigation,
> > > it might be a relevant difference.
> > 
> > They're in memory at least, so it can't be too bad. Certainly not compared
to
> > all the other stuff we're doing per navigation.
> > So if you agree it makes things simpler, then I'd go with removing the
member.
> 
> I tried it out and it doesn't make things easier.
> The operations are: "Add one" (which becomes "load all, add one, store all")
and
> "remove all" (which becomes "re-init empty, store all"). The first operation
is
> more weird code, the second is the same amount of code. And once we'll have
the
> selective clean-up, that will also be a bit more code.
> To be precise, instead of having the member around, you need to put
> dismissed_ids = Read...() in every place you want to use the member. Or you
> could have specific "AddToDismissedPref" methods that directly operator on the
> base::Values, but that's not simpler either.

For "remove all", there's a ClearPref method.
But it's not a big deal either way.

[Bernhard Bauer, 2016-08-11 16:37:08]

lgtm

[Philipp Keck, 2016-08-12 07:36:13]

The CQ bit was checked by pke@google.com

[Philipp Keck, 2016-08-12 07:36:13]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2236303002/#ps100001
(title: "base::Value --> std::unique_ptr<base::Value>")

[commit-bot: I haz the power, 2016-08-12 07:36:30]

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-12 08:24:28]

Committed patchset #6 (id:100001)

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

Description was changed from

==========
Make OfflinePage ContentSuggestions dismissable

Make MakeUniqueID and friends const methods.
Add a new pref to store the IDs of all dismissed offline pages. Add
methods in OfflinePageSuggestionsProvider to store and load these IDs.
Modify OnOfflinePagesLoaded to only forward non-dismissed items.
Implement ClearDismissedSuggestionsForDebugging and a temporary version
for GetDismissedSuggestionsForDebugging (the Snippets Internals will
need refactoring for this to work properly).

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

to

==========
Make OfflinePage ContentSuggestions dismissable

Make MakeUniqueID and friends const methods.
Add a new pref to store the IDs of all dismissed offline pages. Add
methods in OfflinePageSuggestionsProvider to store and load these IDs.
Modify OnOfflinePagesLoaded to only forward non-dismissed items.
Implement ClearDismissedSuggestionsForDebugging and a temporary version
for GetDismissedSuggestionsForDebugging (the Snippets Internals will
need refactoring for this to work properly).

BUG=628198

Committed: https://crrev.com/c7c00479f3e57375b15c430b4057a51f8c2ce3e5
Cr-Commit-Position: refs/heads/master@{#411579}
==========

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

Patchset 6 (id:??) landed as
https://crrev.com/c7c00479f3e57375b15c430b4057a51f8c2ce3e5
Cr-Commit-Position: refs/heads/master@{#411579}