[NTP Snippets] FetchMore backend

[NTP Snippets] FetchMore backend

Taken over from https://codereview.chromium.org/2421463002/#ps120001

(Integrated in https://codereview.chromium.org/2473483006)

BUG=634892

[Marc Treib, 2016-10-27 16:13:25]

Patchset #2 (id:20001) has been deleted

[Marc Treib, 2016-10-28 14:54:25]

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

[Marc Treib, 2016-10-28 14:54:26]

+bauerb, +dgn: Not ready for submission, but this is how far I got. Mainly
adding the "known_ids" list to FetchMore is still missing for an MVP.
fhorschig will take back over now.

[Bernhard Bauer, 2016-10-28 16:26:00]

Looks pretty good; just a bit of
http://i3.kym-cdn.com/photos/images/original/001/044/247/297.png from me at the
bottom ;-)

https://codereview.chromium.org/2446163005/diff/60001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2446163005/diff/60001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_provider.h:108: virtual void
FetchMore(const Category& category,
If the plan is to migrate from the Observer to this method for regular fetches
as well, should we just from the start call this method just Fetch()?

https://codereview.chromium.org/2446163005/diff/60001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2446163005/diff/60001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:44: // TODO(treib): All
these should probably be OnceCallback.
I didn't know about that. Neat! :)

https://codereview.chromium.org/2446163005/diff/60001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right):

https://codereview.chromium.org/2446163005/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:696:
.WillOnce(WithArg<0>(MovePointeeTo(&fetched_categories)));
I realize this is just copied from existing code but... this seems like it's
past the point where using GMock actually provides value. The only advantage the
mock seems to provide is that you can verify it's been called, which is a
somewhat contentious pattern anyway (see eg.
http://martinfowler.com/articles/mocksArentStubs.html), and you can verify the
final _state_ by inspecting |fetched_categories| anyway.

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

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

[fhorschig, 2016-11-02 05:06:12]

Addressed comments in CL 2466863003.

https://codereview.chromium.org/2446163005/diff/60001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2446163005/diff/60001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_provider.h:108: virtual void
FetchMore(const Category& category,
On 2016/10/28 16:25:59, Bernhard (OOO until Nov 11) wrote:
> If the plan is to migrate from the Observer to this method for regular fetches
> as well, should we just from the start call this method just Fetch()?

Done.

https://codereview.chromium.org/2446163005/diff/60001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2446163005/diff/60001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:44: // TODO(treib): All
these should probably be OnceCallback.
On 2016/10/28 16:26:00, Bernhard (OOO until Nov 11) wrote:
> I didn't know about that. Neat! :)
Yeah, the logic was that these callbacks are responses to requests and a request
would not need more than one response.

https://codereview.chromium.org/2446163005/diff/60001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right):

https://codereview.chromium.org/2446163005/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:696:
.WillOnce(WithArg<0>(MovePointeeTo(&fetched_categories)));
On 2016/10/28 16:26:00, Bernhard (OOO until Nov 11) wrote:
> I realize this is just copied from existing code but... this seems like it's
> past the point where using GMock actually provides value. The only advantage
the
> mock seems to provide is that you can verify it's been called, which is a
> somewhat contentious pattern anyway (see eg.
> http://martinfowler.com/articles/mocksArentStubs.html), and you can verify the
> final _state_ by inspecting |fetched_categories| anyway.

On one hand, it is true that a stub would suffice in most cases. 
On the other hand, the behavior has to be tested at least once: even if there
was no new article fetched, the callback has to return. In this special case
(see test "EmptyCategoryIsOK"), we need a mock.

If it is considered better style, I can define a stub for every case that
doesn't need the explicit verification of the behavior. I would prefer to do
this change in a separate CL as it affects tests that are not explicitly tied to
these changes.

[chromium-reviews, 2016-11-02 07:14:19]

On Wednesday, November 2, 2016, <fhorschig@chromium.org> wrote:

> Addressed comments in CL 2466863003.
>
>
> https://codereview.chromium.org/2446163005/diff/60001/
> components/ntp_snippets/content_suggestions_provider.h
> File components/ntp_snippets/content_suggestions_provider.h (right):
>
> https://codereview.chromium.org/2446163005/diff/60001/
> components/ntp_snippets/content_suggestions_provider.h#newcode108
> components/ntp_snippets/content_suggestions_provider.h:108: virtual void
> FetchMore(const Category& category,
> On 2016/10/28 16:25:59, Bernhard (OOO until Nov 11) wrote:
> > If the plan is to migrate from the Observer to this method for regular
> fetches
> > as well, should we just from the start call this method just Fetch()?
>
> Done.
>
> https://codereview.chromium.org/2446163005/diff/60001/
> components/ntp_snippets/content_suggestions_service.h
> File components/ntp_snippets/content_suggestions_service.h (right):
>
> https://codereview.chromium.org/2446163005/diff/60001/
> components/ntp_snippets/content_suggestions_service.h#newcode44
> components/ntp_snippets/content_suggestions_service.h:44: //
> TODO(treib): All these should probably be OnceCallback.
> On 2016/10/28 16:26:00, Bernhard (OOO until Nov 11) wrote:
> > I didn't know about that. Neat! :)
> Yeah, the logic was that these callbacks are responses to requests and a
> request would not need more than one response.
>
> https://codereview.chromium.org/2446163005/diff/60001/
> components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
> File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
> (right):
>
> https://codereview.chromium.org/2446163005/diff/60001/
> components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc#newcode696
> components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:696:
> .WillOnce(WithArg<0>(MovePointeeTo(&fetched_categories)));
> On 2016/10/28 16:26:00, Bernhard (OOO until Nov 11) wrote:
> > I realize this is just copied from existing code but... this seems
> like it's
> > past the point where using GMock actually provides value. The only
> advantage the
> > mock seems to provide is that you can verify it's been called, which
> is a
> > somewhat contentious pattern anyway (see eg.
> > http://martinfowler.com/articles/mocksArentStubs.html), and you can
> verify the
> > final _state_ by inspecting |fetched_categories| anyway.
>
> On one hand, it is true that a stub would suffice in most cases.
> On the other hand, the behavior has to be tested at least once: even if
> there was no new article fetched, the callback has to return. In this
> special case (see test "EmptyCategoryIsOK"), we need a mock.
>
> If it is considered better style, I can define a stub for every case
> that doesn't need the explicit verification of the behavior. I would
> prefer to do this change in a separate CL as it affects tests that are
> not explicitly tied to these changes.
>

You can probably just pass in a lambda -- if I understand things right
here, there's no need for a mock callback at all in this case; as the "make
sure it gets called once" is already covered by other tests.

--Tim

>
>
> https://codereview.chromium.org/2446163005/
>


> --
> You received this message because you are subscribed to the Google Groups
> "New Tab Page development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to ntp-dev+unsubscribe@chromium.org
> <javascript:_e(%7B%7D,'cvml','ntp-dev%2Bunsubscribe@chromium.org');>.
> To post to this group, send email to ntp-dev@chromium.org
> <javascript:_e(%7B%7D,'cvml','ntp-dev@chromium.org');>.
> To view this discussion on the web visit https://groups.google.com/a/
> chromium.org/d/msgid/ntp-dev/001a114601e6598a4905404a6690%40google.com
>
<https://groups.google.com/a/chromium.org/d/msgid/ntp-dev/001a114601e6598a4905...>
> .
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[fhorschig, 2016-11-03 03:10:43]

The changes were merged back into CL 2421463002.

I could not change the mock with a lambda (context and reason below).

On 2016/11/02 07:14:19, chromium-reviews wrote:
> On Wednesday, November 2, 2016, <mailto:fhorschig@chromium.org> wrote:
> 
> > Addressed comments in CL 2466863003.
> >
> > https://codereview.chromium.org/2446163005/diff/60001/
> > components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
> > File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
> > (right):
> >
> > https://codereview.chromium.org/2446163005/diff/60001/
> > components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc#newcode696
> > components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:696:
> > .WillOnce(WithArg<0>(MovePointeeTo(&fetched_categories)));
> > On 2016/10/28 16:26:00, Bernhard (OOO until Nov 11) wrote:
> > > I realize this is just copied from existing code but... this seems
> > like it's
> > > past the point where using GMock actually provides value. The only
> > advantage the
> > > mock seems to provide is that you can verify it's been called, which
> > is a
> > > somewhat contentious pattern anyway (see eg.
> > > http://martinfowler.com/articles/mocksArentStubs.html), and you can
> > verify the
> > > final _state_ by inspecting |fetched_categories| anyway.
> >
> > On one hand, it is true that a stub would suffice in most cases.
> > On the other hand, the behavior has to be tested at least once: even if
> > there was no new article fetched, the callback has to return. In this
> > special case (see test "EmptyCategoryIsOK"), we need a mock.
> >
> > If it is considered better style, I can define a stub for every case
> > that doesn't need the explicit verification of the behavior. I would
> > prefer to do this change in a separate CL as it affects tests that are
> > not explicitly tied to these changes.
> >
> 
> You can probably just pass in a lambda -- if I understand things right
> here, there's no need for a mock callback at all in this case; as the "make
> sure it gets called once" is already covered by other tests.
> 
> --Tim

I think I get the idea. I couldn't figure out what this lambda would look like.
Can you please help me and tell me, where the following lambda is wrong?
(Even if only for future reference)

auto callback =
    base::BindOnce([&fetched_categories](
        NTPSnippetsFetcher::OptionalFetchedCategories categories) {
      fetched_categories = std::move(*categories);
    });
snippets_fetcher().FetchSnippets(params, std::move(callback));

Without the capture "[&fetched_categories]" and with empty body, it is valid.
But the fetched categories are urgently needed to verify the state.

[chromium-reviews, 2016-11-03 04:55:54]

On Thursday, November 3, 2016, <fhorschig@chromium.org> wrote:

> The changes were merged back into CL 2421463002.
>
> I could not change the mock with a lambda (context and reason below).
>
> On 2016/11/02 07:14:19, chromium-reviews wrote:
> > On Wednesday, November 2, 2016, <mailto:fhorschig@chromium.org
> <javascript:_e(%7B%7D,'cvml','fhorschig@chromium.org');>> wrote:
> >
> > > Addressed comments in CL 2466863003.
> > >
> > > https://codereview.chromium.org/2446163005/diff/60001/
> > > components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
> > > File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc
> > > (right):
> > >
> > > https://codereview.chromium.org/2446163005/diff/60001/
> > > components/ntp_snippets/remote/ntp_snippets_fetcher_
> unittest.cc#newcode696
> > > components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:696:
> > > .WillOnce(WithArg<0>(MovePointeeTo(&fetched_categories)));
> > > On 2016/10/28 16:26:00, Bernhard (OOO until Nov 11) wrote:
> > > > I realize this is just copied from existing code but... this seems
> > > like it's
> > > > past the point where using GMock actually provides value. The only
> > > advantage the
> > > > mock seems to provide is that you can verify it's been called, which
> > > is a
> > > > somewhat contentious pattern anyway (see eg.
> > > > http://martinfowler.com/articles/mocksArentStubs.html), and you can
> > > verify the
> > > > final _state_ by inspecting |fetched_categories| anyway.
> > >
> > > On one hand, it is true that a stub would suffice in most cases.
> > > On the other hand, the behavior has to be tested at least once: even if
> > > there was no new article fetched, the callback has to return. In this
> > > special case (see test "EmptyCategoryIsOK"), we need a mock.
> > >
> > > If it is considered better style, I can define a stub for every case
> > > that doesn't need the explicit verification of the behavior. I would
> > > prefer to do this change in a separate CL as it affects tests that are
> > > not explicitly tied to these changes.
> > >
> >
> > You can probably just pass in a lambda -- if I understand things right
> > here, there's no need for a mock callback at all in this case; as the
> "make
> > sure it gets called once" is already covered by other tests.
> >
> > --Tim
>
> I think I get the idea. I couldn't figure out what this lambda would look
> like.
> Can you please help me and tell me, where the following lambda is wrong?
> (Even if only for future reference)
>
> auto callback =
> base::BindOnce([&fetched_categories](
> NTPSnippetsFetcher::OptionalFetchedCategories categories) {
>  fetched_categories = std::move(*categories);
> });
> snippets_fetcher().FetchSnippets(params, std::move(callback));
>
> Without the capture "[&fetched_categories]" and with empty body, it is
> valid.
> But the fetched categories are urgently needed to verify the state.
>

Something like this should work; afaik you need to expliclty bind the
parameter. But don't worry about it in this cl:
auto callback =
base::BindOnce([](NTPSnippetsFetcher:: FetchedCategories* output,
NTPSnippetsFetcher::OptionalFetchedCategories categories) {
*output = std::move(*categories);
}, &fetched_snippets);

>
>
> https://codereview.chromium.org/2446163005/
>
-- 
> You received this message because you are subscribed to the Google Groups
> "New Tab Page development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to ntp-dev+unsubscribe@chromium.org
> <javascript:_e(%7B%7D,'cvml','ntp-dev%2Bunsubscribe@chromium.org');>.
> To post to this group, send email to ntp-dev@chromium.org
> <javascript:_e(%7B%7D,'cvml','ntp-dev@chromium.org');>.
> To view this discussion on the web visit https://groups.google.com/a/
> chromium.org/d/msgid/ntp-dev/94eb2c035cb031eccf05405ce734%40google.com
>
<https://groups.google.com/a/chromium.org/d/msgid/ntp-dev/94eb2c035cb031eccf05...>
> .
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tschumann, 2016-11-03 13:25:04]

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

[tschumann, 2016-11-03 13:25:05]

I just submitted https://codereview.chromium.org/2473483006/ which makes this CL
unnecessary. Please delete at your convenience (also, feel free to double-check
my CL didn't miss anything).

[fhorschig, 2016-11-04 17:36:02]

Description was changed from

==========
[NTP Snippets] FetchMore backend

Taken over from https://codereview.chromium.org/2421463002/#ps120001

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

to

==========
[NTP Snippets] FetchMore backend

Taken over from https://codereview.chromium.org/2421463002/#ps120001

(Integrated in https://codereview.chromium.org/2473483006)

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