Finalize backend for fetching more NTPSnippets.

Finalize backend for fetching more NTPSnippets.

Taken over from https://codereview.chromium.org/2446163005/#ps60001
Addressed comments in that CL.

BUG=634892

[fhorschig, 2016-11-01 18:22:09]

Patchset #3 (id:40001) has been deleted

[fhorschig, 2016-11-01 18:22:23]

Patchset #1 (id:1) has been deleted

[fhorschig, 2016-11-01 18:22:31]

Patchset #1 (id:20001) has been deleted

[fhorschig, 2016-11-01 23:50:52]

Description was changed from

==========
Finalize backend for fetching more NTPSnippets.

BUG=634892

patch from issue 2421463002 at patchset 140001
(http://crrev.com/2421463002#ps140001)
==========

to

==========
Finalize backend for fetching more NTPSnippets.

Taken over from https://codereview.chromium.org/2446163005/#ps60001
Addressed comments in that CL.

BUG=634892
==========

[fhorschig, 2016-11-02 04:24:24]

Patchset #3 (id:100001) has been deleted

[fhorschig, 2016-11-02 05:11:24]

fhorschig@chromium.org changed reviewers:
+ tschumann@chromium.org

[fhorschig, 2016-11-02 05:11:24]

Hi Tim,
could you please take a look at this CL and especially if the set dependencies
make sense?

This CL addresses comments from 
https://codereview.chromium.org/2421463002/ 
and
https://codereview.chromium.org/2446163005/

There are no OWNERS (due to various bank holidays and OOO situations) in the
reviewer list, yet. Any idea who might be willing to help?

[dgn, 2016-11-02 11:10:10]

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

[dgn, 2016-11-02 11:10:11]

Can you please merge all the changes in a single review? if you try submitting
this CL, I think it will try to submit the parent CL separately and will result
in 2 commits.

Have Marc's version as Patch Set 1, and your changes as Patch Set 2 and up.

https://codereview.chromium.org/2466863003/diff/120001/components/ntp_snippet...
File components/ntp_snippets/remote/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2466863003/diff/120001/components/ntp_snippet...
components/ntp_snippets/remote/ntp_snippets_service.cc:330:
std::move(callback).Run(std::vector<ContentSuggestion>());
why is std::move necessary here?

https://codereview.chromium.org/2466863003/diff/120001/components/ntp_snippet...
components/ntp_snippets/remote/ntp_snippets_service.cc:640: FetchingCallback
fetched_more_callback,
Can this argument be a base::Optional<FetchingCallback> ? we don't care about it
if |fetched_more| is false, right? that would also avoid giving a dummy callback
as argument.

https://codereview.chromium.org/2466863003/diff/120001/components/ntp_snippet...
File components/ntp_snippets/remote/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/2466863003/diff/120001/components/ntp_snippet...
components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:508:
std::set<std::string> known,
|known_ids| maybe? name is too unspecific as it is now.

[dgn, 2016-11-02 11:18:07]

I just realised that you made this one a fork of
https://codereview.chromium.org/2421463002/. Can you just keep all the changes
on that other CL? You don't need to keep Marc's branch building. He made his by
just taking all of your CL. So directly add all the changes from this review and
Marc's review to https://codereview.chromium.org/2421463002/. Currently I have
no idea which one I should look at and which one should be landed.

[tschumann, 2016-11-02 11:23:22]

@dgn: As Friedrich is not available, can you please have a closer look (also at
my comment)? I might just take over both CLs and submit them, so that we can get
it ready in time for FF.

https://codereview.chromium.org/2466863003/diff/120001/components/ntp_snippet...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h (right):

https://codereview.chromium.org/2466863003/diff/120001/components/ntp_snippet...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h:46:
std::set<std::string> known_suggestion_ids,
@dgn (to get quicker feedback as Friedrich is not available).

we should pass the ids per const reference. Any reason why we use per value and
rely on std::move().
The problem with per-value and std::move() is that if you forget std::move()
you'll make a copy without noticing *and* you have zombie-state data on the
caller side (moved-out variables). Their state is usually well defined with STL
data types, but accessing them rings a few alarm bells.

[dgn, 2016-11-02 12:04:33]

https://codereview.chromium.org/2466863003/diff/120001/components/ntp_snippet...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h (right):

https://codereview.chromium.org/2466863003/diff/120001/components/ntp_snippet...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h:46:
std::set<std::string> known_suggestion_ids,
On 2016/11/02 11:23:22, tschumann wrote:
> @dgn (to get quicker feedback as Friedrich is not available).
> 
> we should pass the ids per const reference. Any reason why we use per value
and
> rely on std::move().
> The problem with per-value and std::move() is that if you forget std::move()
> you'll make a copy without noticing *and* you have zombie-state data on the
> caller side (moved-out variables). Their state is usually well defined with
STL
> data types, but accessing them rings a few alarm bells.

Yes, I also would prefer having const references rather than duplicating or
moving things around manually. I see no specific reason why it is that way
currently.

https://codereview.chromium.org/2466863003/diff/120001/components/ntp_snippet...
File components/ntp_snippets/remote/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2466863003/diff/120001/components/ntp_snippet...
components/ntp_snippets/remote/ntp_snippets_service.cc:330:
std::move(callback).Run(std::vector<ContentSuggestion>());
On 2016/11/02 11:10:11, dgn wrote:
> why is std::move necessary here?

Okay, this is explained in
https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md

It still looks super dodgy to my untrained c++ eyes. Maybe add a comment to
explain why this is WAI to not move it anywhere or wrap it in a method that will
have that explanation?

[fhorschig, 2016-11-03 00:27:27]

Patchset #5 (id:160001) has been deleted

[fhorschig, 2016-11-03 00:27:35]

Patchset #4 (id:140001) has been deleted

[fhorschig, 2016-11-03 01:53:14]

Addressed all comments in the first CL:
https://codereview.chromium.org/2421463002/#ps200001

(This CL is no longer updated to avoid further confusion)

https://codereview.chromium.org/2466863003/diff/120001/components/ntp_snippet...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h (right):

https://codereview.chromium.org/2466863003/diff/120001/components/ntp_snippet...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h:46:
std::set<std::string> known_suggestion_ids,
On 2016/11/02 12:04:33, dgn wrote:
> On 2016/11/02 11:23:22, tschumann wrote:
> > @dgn (to get quicker feedback as Friedrich is not available).
> > 
> > we should pass the ids per const reference. Any reason why we use per value
> and
> > rely on std::move().
> > The problem with per-value and std::move() is that if you forget std::move()
> > you'll make a copy without noticing *and* you have zombie-state data on the
> > caller side (moved-out variables). Their state is usually well defined with
> STL
> > data types, but accessing them rings a few alarm bells.
> 
> Yes, I also would prefer having const references rather than duplicating or
> moving things around manually. I see no specific reason why it is that way
> currently.

Done.

BTW: I used the ownership of the set to std::move all elements in
NTPSnippetService::CollectIdsToExclude() into another set. Now, the inserted
strings have to be copied. What I could have used is an rvalue-reference as
parameter but copying strings is rather cheap compared to accidentally copying
the whole set, so I will go with your solution.

https://codereview.chromium.org/2466863003/diff/120001/components/ntp_snippet...
File components/ntp_snippets/remote/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2466863003/diff/120001/components/ntp_snippet...
components/ntp_snippets/remote/ntp_snippets_service.cc:330:
std::move(callback).Run(std::vector<ContentSuggestion>());
On 2016/11/02 12:04:33, dgn wrote:
> On 2016/11/02 11:10:11, dgn wrote:
> > why is std::move necessary here?
> 
> Okay, this is explained in
> https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md
> 
> It still looks super dodgy to my untrained c++ eyes. Maybe add a comment to
> explain why this is WAI to not move it anywhere or wrap it in a method that
will
> have that explanation?

Thank you very much!
You discovered (maybe unwillingly) a potential bug: this callback used to be a
OnceCallback, but it isn't one any longer. So the outer move used to be
necessary but is ABSOLUTELY WRONG currently. I move it into an explaining
function as this construct will return in the near future.

https://codereview.chromium.org/2466863003/diff/120001/components/ntp_snippet...
components/ntp_snippets/remote/ntp_snippets_service.cc:640: FetchingCallback
fetched_more_callback,
On 2016/11/02 11:10:11, dgn wrote:
> Can this argument be a base::Optional<FetchingCallback> ? we don't care about
it
> if |fetched_more| is false, right? that would also avoid giving a dummy
callback
> as argument.

Done.

Yes. This whole set of parameters is planned to change when supporting
concurrent fetches (Bug 659931).

https://codereview.chromium.org/2466863003/diff/120001/components/ntp_snippet...
File components/ntp_snippets/remote/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/2466863003/diff/120001/components/ntp_snippet...
components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:508:
std::set<std::string> known,
On 2016/11/02 11:10:11, dgn wrote:
> |known_ids| maybe? name is too unspecific as it is now.

Done.