Issue 2965173002: Add ContextualSuggestionsService to Omnibox

gcomanici

Patchset #1 (id:1) has been deleted

3 years, 5 months ago (2017-07-06 02:10:55 UTC) #1

gcomanici

Description was changed from ========== Create a new component `contextual_suggestions`, which provides a service to ...

3 years, 5 months ago (2017-07-06 02:23:20 UTC) #4

Description was changed from

==========
Create a new component `contextual_suggestions`, which provides a service to
fetch experimental contextual suggestions for the omnibox.

Experimental Contextual Suggestions will soon use user identity to provide
personalized suggestions. The service that is creating a request with a oauth2
token attached is becoming more complex and should live in its own component,
much like the service that is fetching suggestions for the NTP (new tab page).
This makes the logic a lot easier to test and increases code readability for the
fetcher, related experiments, and the zero suggest autocomplete provider.

Additionally, b/609084 is traking an effort to unify all the URL Fetchers that
use an oauth2 token. Creating this new component should facilitate future
changes towards this effort.

The new component `contextual_suggestions` is a simplified version of
`suggestions`. The two components are providing very similar services (i.e.
suggestions for different surfaces) but the new component is much simpler, as
currently most of the logic related to contextual suggestions is performed on
server side. Still, more functionality will soon be necessary on client side and
all the logic will be living in the new component.

BUG=692471,609084
==========

to

==========
Create a new component `contextual_suggestions`, which provides a service to
fetch experimental contextual suggestions for the omnibox.

Experimental Contextual Suggestions will soon use user identity to provide
personalized suggestions. The service that is creating a request with a oauth2
token attached is becoming more complex and should live in its own component,
much like the service that is fetching suggestions for the NTP (new tab page).
This makes the logic a lot easier to test and increases code readability for the
fetcher, related experiments, and the zero suggest autocomplete provider.

Additionally, b/609084 is traking an effort to unify all the URL Fetchers that
use an oauth2 token. Creating this new component should facilitate future
changes towards this effort.

The new component `contextual_suggestions` is a simplified version of
`suggestions`. The two components are providing very similar services (i.e.
suggestions for different surfaces) but the new component is much simpler, as
currently most of the logic related to contextual suggestions is performed on
server side. Still, more functionality will soon be necessary on client side and
all the logic will be living in the new component.

BUG=692471,609084
==========

gcomanici

gcomanici@chromium.org changed reviewers: + treib@chromium.org

3 years, 5 months ago (2017-07-06 02:23:20 UTC) #5

gcomanici

Hi Marc, Could you please take a look at the files in `components/contextual_suggestions` to make ...

3 years, 5 months ago (2017-07-06 02:25:37 UTC) #6

Marc Treib

This generally looks good; I've added a bunch of comments below. I mostly looked at ...

3 years, 5 months ago (2017-07-06 08:32:21 UTC) #7

Justin Donnelly

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

3 years, 5 months ago (2017-07-06 14:16:23 UTC) #8

Justin Donnelly

Is there a design doc for this new component? Per go/newchromefeature*, any work crossing components ...

3 years, 5 months ago (2017-07-06 14:16:24 UTC) #9

gcomanici

Thanks Marc! PTAL. https://codereview.chromium.org/2965173002/diff/60001/chrome/browser/autocomplete/contextual_suggestions_service_factory.h File chrome/browser/autocomplete/contextual_suggestions_service_factory.h (right): https://codereview.chromium.org/2965173002/diff/60001/chrome/browser/autocomplete/contextual_suggestions_service_factory.h#newcode1 chrome/browser/autocomplete/contextual_suggestions_service_factory.h:1: // Copyright (c) 2017 The Chromium ...

3 years, 5 months ago (2017-07-06 15:34:19 UTC) #10

gcomanici

On 2017/07/06 14:16:24, Justin Donnelly wrote: > Is there a design doc for this new ...

3 years, 5 months ago (2017-07-06 15:41:44 UTC) #11

gcomanici

Description was changed from ========== Create a new component `contextual_suggestions`, which provides a service to ...

3 years, 5 months ago (2017-07-06 15:43:25 UTC) #12

Description was changed from

==========
Create a new component `contextual_suggestions`, which provides a service to
fetch experimental contextual suggestions for the omnibox.

Experimental Contextual Suggestions will soon use user identity to provide
personalized suggestions. The service that is creating a request with a oauth2
token attached is becoming more complex and should live in its own component,
much like the service that is fetching suggestions for the NTP (new tab page).
This makes the logic a lot easier to test and increases code readability for the
fetcher, related experiments, and the zero suggest autocomplete provider.

Additionally, b/609084 is traking an effort to unify all the URL Fetchers that
use an oauth2 token. Creating this new component should facilitate future
changes towards this effort.

The new component `contextual_suggestions` is a simplified version of
`suggestions`. The two components are providing very similar services (i.e.
suggestions for different surfaces) but the new component is much simpler, as
currently most of the logic related to contextual suggestions is performed on
server side. Still, more functionality will soon be necessary on client side and
all the logic will be living in the new component.

BUG=692471,609084
==========

to

==========
Create a new component `contextual_suggestions`, which provides a service to
fetch experimental contextual suggestions for the omnibox.

Experimental Contextual Suggestions will soon use user identity to provide
personalized suggestions. The service that is creating a request with a oauth2
token attached is becoming more complex and should live in its own component,
much like the service that is fetching suggestions for the NTP (new tab page).
This makes the logic a lot easier to test and increases code readability for the
fetcher, related experiments, and the zero suggest autocomplete provider.

Additionally, b/609084 is traking an effort to unify all the URL Fetchers that
use an oauth2 token. Creating this new component should facilitate future
changes towards this effort.

The new component `contextual_suggestions` is a simplified version of
`suggestions`. The two components are providing very similar services (i.e.
suggestions for different surfaces) but the new component is much simpler, as
currently most of the logic related to contextual suggestions is performed on
server side. Still, more functionality will soon be necessary on client side and
all the logic will be living in the new component.


Design doc:
https://docs.google.com/a/google.com/document/d/1ATAbcfUwqmEvGJRylUYqOFUDVpvP...

BUG=692471,609084
==========

gcomanici

gcomanici@chromium.org changed reviewers: + mpearson@chromium.org

3 years, 5 months ago (2017-07-06 15:43:25 UTC) #13

Marc Treib

First description line is still too long. Mostly looks good to me now, a few ...

3 years, 5 months ago (2017-07-06 16:02:46 UTC) #14

gcomanici

Description was changed from ========== Create a new component `contextual_suggestions`, which provides a service to ...

3 years, 5 months ago (2017-07-06 16:33:47 UTC) #15

Description was changed from

==========
Create a new component `contextual_suggestions`, which provides a service to
fetch experimental contextual suggestions for the omnibox.

Experimental Contextual Suggestions will soon use user identity to provide
personalized suggestions. The service that is creating a request with a oauth2
token attached is becoming more complex and should live in its own component,
much like the service that is fetching suggestions for the NTP (new tab page).
This makes the logic a lot easier to test and increases code readability for the
fetcher, related experiments, and the zero suggest autocomplete provider.

Additionally, b/609084 is traking an effort to unify all the URL Fetchers that
use an oauth2 token. Creating this new component should facilitate future
changes towards this effort.

The new component `contextual_suggestions` is a simplified version of
`suggestions`. The two components are providing very similar services (i.e.
suggestions for different surfaces) but the new component is much simpler, as
currently most of the logic related to contextual suggestions is performed on
server side. Still, more functionality will soon be necessary on client side and
all the logic will be living in the new component.


Design doc:
https://docs.google.com/a/google.com/document/d/1ATAbcfUwqmEvGJRylUYqOFUDVpvP...

BUG=692471,609084
==========

to

==========
Create a new component `contextual_suggestions`

This component provides a service to fetch experimental contextual suggestions
for the omnibox.

Experimental Contextual Suggestions will soon use user identity to provide
personalized suggestions. The service that is creating a request with a oauth2
token attached is becoming more complex and should live in its own component,
much like the service that is fetching suggestions for the NTP (new tab page).
This makes the logic a lot easier to test and increases code readability for the
fetcher, related experiments, and the zero suggest autocomplete provider.

Additionally, b/609084 is traking an effort to unify all the URL Fetchers that
use an oauth2 token. Creating this new component should facilitate future
changes towards this effort.

The new component `contextual_suggestions` is a simplified version of
`suggestions`. The two components are providing very similar services (i.e.
suggestions for different surfaces) but the new component is much simpler, as
currently most of the logic related to contextual suggestions is performed on
server side. Still, more functionality will soon be necessary on client side and
all the logic will be living in the new component.


Design doc:
https://docs.google.com/a/google.com/document/d/1ATAbcfUwqmEvGJRylUYqOFUDVpvP...

BUG=692471,609084
==========

gcomanici

Thanks Marc for the careful review :) I changed the subject of the CL, as ...

3 years, 5 months ago (2017-07-06 16:54:17 UTC) #17

Thanks Marc for the careful review :) I changed the subject of the CL, as you
suggested.

I also forgot to reply to your comment on the name of the component. I am not
100% happy with the name. I want to get mpearson's opinion on what would be
best. I still prefer `contextual_suggestions` as we are interested in providing
contextual suggestions more than just as zero suggest suggestions. It would be
great to join efforts with the people that are working on adding contextual
suggestions to Chrome Home in this regard.

https://codereview.chromium.org/2965173002/diff/60001/components/contextual_s...
File components/contextual_suggestions/contextual_suggestions_service.cc
(right):

https://codereview.chromium.org/2965173002/diff/60001/components/contextual_s...
components/contextual_suggestions/contextual_suggestions_service.cc:133:
cookies_allowed: true
On 2017/07/06 16:02:45, Marc Treib wrote:
> On 2017/07/06 15:34:17, gcomanici wrote:
> > On 2017/07/06 08:32:20, Marc Treib wrote:
> > > Does this actually require cookies? If you're doing OAuth2 authentication,
> > then
> > > presumably cookies aren't necessary?
> > 
> > Good point. I was using the same annotation tag as before, but indeed
cookies
> > are no longer needed.
> 
> Ah, but then you also need to set net::LOAD_DO_NOT_SEND_COOKIES below.

Done.

https://codereview.chromium.org/2965173002/diff/60001/components/contextual_s...
components/contextual_suggestions/contextual_suggestions_service.cc:148: if
(!suggest_url.is_valid())
On 2017/07/06 16:02:45, Marc Treib wrote:
> On 2017/07/06 15:34:18, gcomanici wrote:
> > On 2017/07/06 08:32:20, Marc Treib wrote:
> > > If this were the case, we shouldn't get here, right? If so, maybe just
> DCHECK
> > > that the URL is valid?
> > 
> > Added a DCHECK.
> 
> It's the wrong way around though ;)

Fixed :)

https://codereview.chromium.org/2965173002/diff/60001/components/contextual_s...
File components/contextual_suggestions/contextual_suggestions_service.h (right):

https://codereview.chromium.org/2965173002/diff/60001/components/contextual_s...
components/contextual_suggestions/contextual_suggestions_service.h:54: //
executed with the result as a parameter.
On 2017/07/06 16:02:45, Marc Treib wrote:
> On 2017/07/06 15:34:18, gcomanici wrote:
> > On 2017/07/06 08:32:21, Marc Treib wrote:
> > > Sync or async?
> > 
> > sync. I changed the comments to make this clear.
> 
> No, I don't think that's correct. There are some error cases where it's sync,
> but fetching the access token is async. So if you get to the point of trying
to
> get an access token, then it'll be async.

You are right. I made the comment a bit more explicit in that regard. PTAL.

https://codereview.chromium.org/2965173002/diff/60001/components/omnibox/brow...
File components/omnibox/browser/zero_suggest_provider.cc (right):

https://codereview.chromium.org/2965173002/diff/60001/components/omnibox/brow...
components/omnibox/browser/zero_suggest_provider.cc:168:
base::Unretained(this)));
On 2017/07/06 16:02:45, Marc Treib wrote:
> On 2017/07/06 15:34:19, gcomanici wrote:
> > On 2017/07/06 08:32:21, Marc Treib wrote:
> > > Is Unretained() safe to use here? If so, please add a comment explaining
> why.
> > If
> > > not, use a WeakPtr instead.
> > 
> > I am not sure how to answer your question, so I will go with WeakPtr instead
> to
> > be safe.
> 
> Unretained is only safe if you're absolutely sure that the callback won't be
> called after "this" has been destroyed. Maybe that's the case here, I can't
> tell, but anyway weakptr doesn't hurt.

Thanks. Indeed, we should go with weakptr.

https://codereview.chromium.org/2965173002/diff/80001/chrome/browser/autocomp...
File chrome/browser/autocomplete/contextual_suggestions_service_factory.h
(right):

https://codereview.chromium.org/2965173002/diff/80001/chrome/browser/autocomp...
chrome/browser/autocomplete/contextual_suggestions_service_factory.h:17: }
On 2017/07/06 16:02:46, Marc Treib wrote:
> // namespace contextual_suggestions

Done.

https://codereview.chromium.org/2965173002/diff/80001/components/contextual_s...
File components/contextual_suggestions/contextual_suggestions_service.h (right):

https://codereview.chromium.org/2965173002/diff/80001/components/contextual_s...
components/contextual_suggestions/contextual_suggestions_service.h:53: //   *
The service is already waiting for an authenticaiton token.
On 2017/07/06 16:02:46, Marc Treib wrote:
> s/authenticaiton/authentication/

Done.

https://codereview.chromium.org/2965173002/diff/80001/components/contextual_s...
components/contextual_suggestions/contextual_suggestions_service.h:66: //
Creates an HTTTP Get request for experimental contextual suggestions. The
On 2017/07/06 16:02:46, Marc Treib wrote:
> s/HTTTP/HTTP/

Done.

https://codereview.chromium.org/2965173002/diff/80001/components/contextual_s...
File components/contextual_suggestions/features.h (right):

https://codereview.chromium.org/2965173002/diff/80001/components/contextual_s...
components/contextual_suggestions/features.h:11: 
On 2017/07/06 16:02:46, Marc Treib wrote:
> optional: Many places use a sub-namespace "features" for this.

Done.

https://codereview.chromium.org/2965173002/diff/80001/components/omnibox/brow...
File components/omnibox/browser/zero_suggest_provider.cc (right):

https://codereview.chromium.org/2965173002/diff/80001/components/omnibox/brow...
components/omnibox/browser/zero_suggest_provider.cc:199:
LogOmniboxZeroSuggestRequest(ZERO_SUGGEST_REQUEST_SENT);
On 2017/07/06 16:02:46, Marc Treib wrote:
> Is this change intended?

Yes. I forgot to add it before. It is quite important for out UMA metrics. We
were getting it for free in the previous version of zero_suggest_provider.cc,
but now I have to add it in this function.

Marc Treib

LGTM (but again, you'll have to get an owner to approve this) markusheintz@ is a ...

3 years, 5 months ago (2017-07-06 16:58:27 UTC) #18

Justin Donnelly

On 2017/07/06 15:41:44, gcomanici wrote: > On 2017/07/06 14:16:24, Justin Donnelly wrote: > > Is ...

3 years, 5 months ago (2017-07-06 17:06:37 UTC) #19

gcomanici

Description was changed from ========== Create a new component `contextual_suggestions` This component provides a service ...

3 years, 5 months ago (2017-07-06 17:41:07 UTC) #20

Description was changed from

==========
Create a new component `contextual_suggestions`

This component provides a service to fetch experimental contextual suggestions
for the omnibox.

Experimental Contextual Suggestions will soon use user identity to provide
personalized suggestions. The service that is creating a request with a oauth2
token attached is becoming more complex and should live in its own component,
much like the service that is fetching suggestions for the NTP (new tab page).
This makes the logic a lot easier to test and increases code readability for the
fetcher, related experiments, and the zero suggest autocomplete provider.

Additionally, b/609084 is traking an effort to unify all the URL Fetchers that
use an oauth2 token. Creating this new component should facilitate future
changes towards this effort.

The new component `contextual_suggestions` is a simplified version of
`suggestions`. The two components are providing very similar services (i.e.
suggestions for different surfaces) but the new component is much simpler, as
currently most of the logic related to contextual suggestions is performed on
server side. Still, more functionality will soon be necessary on client side and
all the logic will be living in the new component.


Design doc:
https://docs.google.com/a/google.com/document/d/1ATAbcfUwqmEvGJRylUYqOFUDVpvP...

BUG=692471,609084
==========

to

==========
Create a new component `contextual_suggestions`

This component provides a service to fetch experimental contextual suggestions
for the omnibox.

Experimental Contextual Suggestions will soon use user identity to provide
personalized suggestions. The service that is creating a request with a oauth2
token attached is becoming more complex and should live in its own component,
much like the service that is fetching suggestions for the NTP (new tab page).
This makes the logic a lot easier to test and increases code readability for the
fetcher, related experiments, and the zero suggest autocomplete provider.

Additionally, b/609084 is traking an effort to unify all the URL Fetchers that
use an oauth2 token. Creating this new component should facilitate future
changes towards this effort.

From a technical perspective, the new component `contextual_suggestions` is very
similar to the `suggestions` component. The two components are providing very
similar services (i.e. suggestions for different surfaces) but the new component
is much simpler, as currently most of the logic related to contextual
suggestions is performed on server side. Still, more functionality will soon be
necessary on client side and all the corresponding logic will be living in the
new component.


Design doc:
https://docs.google.com/a/google.com/document/d/1ATAbcfUwqmEvGJRylUYqOFUDVpvP...

BUG=692471,609084
==========

gcomanici

On 2017/07/06 17:06:37, Justin Donnelly wrote: > On 2017/07/06 15:41:44, gcomanici wrote: > > On ...

3 years, 5 months ago (2017-07-06 17:55:39 UTC) #21

On 2017/07/06 17:06:37, Justin Donnelly wrote:
> On 2017/07/06 15:41:44, gcomanici wrote:
> > On 2017/07/06 14:16:24, Justin Donnelly wrote:
> > > Is there a design doc for this new component? Per go/newchromefeature*,
any
> > work
> > > crossing components (which a new component does by definition) should have
a
> > > design doc sent out for review. Even if it's a simple one-pager, which is
> > > probably sufficient for your purposes.
> > > 
> > > * In the Design docs section:
> > >
> >
>
https://docs.google.com/document/d/1eqxrys_EnlCvLDXWhMZOPBDmPEuSs9xgw7VZ_CW5l...
> > 
> > Hi Justin, 
> > 
> > That's a very good point. Since the feature is not new (see
> >
>
https://docs.google.com/a/google.com/document/d/1ATAbcfUwqmEvGJRylUYqOFUDVpvP...)
> > we already have a design doc. Still, it does not include the creation of
this
> > new component. I will add a new section with more details on the new
component
> > and I will add it to the CL description.
> 
> Thanks. Can you also send to chrome-design-docs@? I'd say a new component for
> this purpose seems reasonable but I pause at your statement, "The two
components
> are providing very similar services". Why not use components/suggestions then?
> The fact that this is a simplified version of what's there doesn't seem on its
> own a compelling enough reason for a new component.

@jdonnelly

WRT the design doc: the feature already passed through all necessary reviews
(crbug.com/692471). This CL is simply moving most of the related logic outside
of components/omnibox/browser, as the previous implementations was a hack and
was intended to be a short term solution. For various reasons, it seems that we
will have to live with this fetcher for more than expected, hence the component
creation.

WRT components/suggestions: I should have been a bit more clear in that
paragraph describing the similarity to `suggestions`. The similarity I am
talking about is from a technical perspective. Ultimately, they generate
different requests to different backends. The biggest difference is that for the
new component the fetch response is processed by ZeroSuggestProvider (see
ZeroSuggestProvider::OnURLFetchComplete), whereas the component/suggestions is
processing the request directly (see
SuggestionsServiceImpl::OnURLFetchComplete). Maybe this dependence on
ZeroSuggestProvider could be eliminated in the future, but I think having a new
component is the right solution for now.

gcomanici

Description was changed from ========== Create a new component `contextual_suggestions` This component provides a service ...

3 years, 5 months ago (2017-07-06 18:01:07 UTC) #22

Description was changed from

==========
Create a new component `contextual_suggestions`

This component provides a service to fetch experimental contextual suggestions
for the omnibox.

Experimental Contextual Suggestions will soon use user identity to provide
personalized suggestions. The service that is creating a request with a oauth2
token attached is becoming more complex and should live in its own component,
much like the service that is fetching suggestions for the NTP (new tab page).
This makes the logic a lot easier to test and increases code readability for the
fetcher, related experiments, and the zero suggest autocomplete provider.

Additionally, b/609084 is traking an effort to unify all the URL Fetchers that
use an oauth2 token. Creating this new component should facilitate future
changes towards this effort.

From a technical perspective, the new component `contextual_suggestions` is very
similar to the `suggestions` component. The two components are providing very
similar services (i.e. suggestions for different surfaces) but the new component
is much simpler, as currently most of the logic related to contextual
suggestions is performed on server side. Still, more functionality will soon be
necessary on client side and all the corresponding logic will be living in the
new component.


Design doc:
https://docs.google.com/a/google.com/document/d/1ATAbcfUwqmEvGJRylUYqOFUDVpvP...

BUG=692471,609084
==========

to

==========
Create a new component `contextual_suggestions`

This component provides a service to fetch experimental contextual suggestions
for the omnibox.

Experimental Contextual Suggestions will soon use user identity to provide
personalized suggestions. The service that is creating a request with a oauth2
token attached is becoming more complex and should live in its own component,
much like the service that is fetching suggestions for the NTP (new tab page).
This makes the logic a lot easier to test and increases code readability for the
fetcher, related experiments, and the zero suggest autocomplete provider.

Additionally, b/609084 is traking an effort to unify all the URL Fetchers that
use an oauth2 token. Creating this new component should facilitate future
changes towards this effort.

From a technical perspective, the new component `contextual_suggestions` is very
similar to the `suggestions` component. The two components are providing very
similar services (i.e. suggestions for different surfaces) but the new component
is much simpler, as currently most of the logic related to contextual
suggestions is performed on server side. Still, more functionality will soon be
necessary on client side and all the corresponding logic will be living in the
new component.


Design doc:
https://docs.google.com/document/d/1FUowJo9AnSziiQfBumyicaTWDKjOcM574aOBwKIVp...

BUG=692471,609084
==========

Justin Donnelly

On 2017/07/06 17:55:39, gcomanici wrote: > WRT the design doc: the feature already passed through ...

3 years, 5 months ago (2017-07-06 18:14:01 UTC) #23

On 2017/07/06 17:55:39, gcomanici wrote:
> WRT the design doc: the feature already passed through all necessary reviews
> (crbug.com/692471). This CL is simply moving most of the related logic outside
> of components/omnibox/browser, as the previous implementations was a hack and
> was intended to be a short term solution. For various reasons, it seems that
we
> will have to live with this fetcher for more than expected, hence the
component
> creation.

Understood that the feature design went through the review process. But the
introduction of a new component obviously didn't since you just added it to your
design doc. I know it sounds like I'm trying to throw more process onto you
unnecessarily. But I'm more just trying to give you heads up that when I
introduced a new component recently I was directed to run that through
chrome-design-docs@. Given that this is pretty straightforward, I doubt you'd
encounter much friction. If you got any feedback at all other than lgtm it might
even be helpful advice for how you structure your code going forward.

But since you need a components OWNERS review of this CL anyway, I'll leave it
to them to decide whether that's strictly necessary.

> WRT components/suggestions: I should have been a bit more clear in that
> paragraph describing the similarity to `suggestions`. The similarity I am
> talking about is from a technical perspective. Ultimately, they generate
> different requests to different backends. The biggest difference is that for
the
> new component the fetch response is processed by ZeroSuggestProvider (see
> ZeroSuggestProvider::OnURLFetchComplete), whereas the component/suggestions is
> processing the request directly (see
> SuggestionsServiceImpl::OnURLFetchComplete). Maybe this dependence on
> ZeroSuggestProvider could be eliminated in the future, but I think having a
new
> component is the right solution for now.

Thanks for the explanation. And given that there are currently no DEPS between
the two components in this change then keeping them separate sounds appropriate.
But if you ever find one depending on the other because they need some similar
parsing code or data models or something, then you may want to reconsider at
that point.

gcomanici

gcomanici@chromium.org changed reviewers: + blundell@chromium.org

3 years, 5 months ago (2017-07-07 20:05:25 UTC) #24

gcomanici

Hi Colin, Can you take a look for an OWNERS's review? We discussed in a ...

3 years, 5 months ago (2017-07-07 20:05:26 UTC) #25

Mark P

A gave this a quick, haphazard review. I have two main questions / concerns about ...

3 years, 5 months ago (2017-07-07 20:13:55 UTC) #26

gcomanici

On 2017/07/07 20:13:55, Mark P wrote: > A gave this a quick, haphazard review. I ...

3 years, 5 months ago (2017-07-07 21:24:29 UTC) #27

On 2017/07/07 20:13:55, Mark P wrote:
> A gave this a quick, haphazard review.  I have two main questions / concerns
> about this design:
> 
> - it looks like a sizable chunk of code is being duplicated between
> components/omnibox/ and this new component.  Can we refactor the code so that
> all suggest requests go through the same flow so the code is not duplicated? 
> After all, the regular suggest flow also needs cookies attached to it; it just
> gets that for free right now.

I responded to a similar comment you made in the code. I am all for refactoring 
the code, but I'd leave it for a different CL, if you don't mind.

> 
> - is a new component the right place for this?  I'm nervous about new
> components, especially ones that are only used for experimental features. 

This is related to your other comment. Much like `components/suggestions`, the
logic 
of fetching contextual suggestions from a remote server should live in its own
component. This should not only include the logic used to fetch suggestions from
a backend, but also the creation of the context. Indeed, currently the URL
visited by the user and the oauth token are defining the context, but there is
no reason to stop there (e.g. the last visited URL when on chrome://newtab, the
last 5 visited urls etc.).

It will definitely make it easier to maintain and to iterate in our experiments
if all code related to this experimental feature is separate from its current
client (i.e. the omnibox).


> Relatedly, does the component align with component layering design?  Both of
> these questions are best answered by a primary component/ owner and/or a
chrome
> design doc reviewer.  I know I cannot answer them, as I periodically try to
> include a component in another and am told, no, no, no. :-)
> 
> --mark
> 
>
https://codereview.chromium.org/2965173002/diff/120001/chrome/browser/autocom...
> File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc
(right):
> 
>
https://codereview.chromium.org/2965173002/diff/120001/chrome/browser/autocom...
> chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:130:
> contextual_suggestions::ContextualSuggestionsService*
> #include the file that gives you
> contextual_suggestions::ContextualSuggestionsService explicitly
> 
>
https://codereview.chromium.org/2965173002/diff/120001/chrome/browser/autocom...
> File chrome/browser/autocomplete/chrome_autocomplete_provider_client.h
(right):
> 
>
https://codereview.chromium.org/2965173002/diff/120001/chrome/browser/autocom...
> chrome/browser/autocomplete/chrome_autocomplete_provider_client.h:11: #include
> "components/contextual_suggestions/contextual_suggestions_service.h"
> No need to add this here; you get it from the parent class.
> 
>
https://codereview.chromium.org/2965173002/diff/120001/components/contextual_...
> File components/contextual_suggestions/contextual_suggestions_service.cc
> (right):
> 
>
https://codereview.chromium.org/2965173002/diff/120001/components/contextual_...
> components/contextual_suggestions/contextual_suggestions_service.cc:119: 
> I'm a bit bothered that the whole rest of this file is (nearly) a duplicate of
> code in components/omnibox/...
> 
>
https://codereview.chromium.org/2965173002/diff/120001/components/contextual_...
> components/contextual_suggestions/contextual_suggestions_service.cc:124:
> net::DefineNetworkTrafficAnnotation("omnibox_contextual_zerosuggest", R"(
> Not sure if you should be using a new annotation here or not.
> 
>
https://codereview.chromium.org/2965173002/diff/120001/components/contextual_...
> components/contextual_suggestions/contextual_suggestions_service.cc:161:
> fetcher->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
> Are you sure you want to set this flag?
> 
>
https://codereview.chromium.org/2965173002/diff/120001/components/omnibox/bro...
> File components/omnibox/browser/autocomplete_provider_client.h (right):
> 
>
https://codereview.chromium.org/2965173002/diff/120001/components/omnibox/bro...
> components/omnibox/browser/autocomplete_provider_client.h:12: #include
> "components/contextual_suggestions/contextual_suggestions_service.h"
> forward declare; don't include
> 
>
https://codereview.chromium.org/2965173002/diff/120001/components/omnibox/bro...
> File components/omnibox/browser/zero_suggest_provider.cc (right):
> 
>
https://codereview.chromium.org/2965173002/diff/120001/components/omnibox/bro...
> components/omnibox/browser/zero_suggest_provider.cc:162: // Create a request
for
> experimental suggestions with |this| as the
> bug: probably want to set done_ = false here.

gcomanici

https://codereview.chromium.org/2965173002/diff/120001/chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right): https://codereview.chromium.org/2965173002/diff/120001/chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc#newcode130 chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:130: contextual_suggestions::ContextualSuggestionsService* On 2017/07/07 20:13:54, Mark P wrote: > #include ...

3 years, 5 months ago (2017-07-07 21:24:46 UTC) #28

https://codereview.chromium.org/2965173002/diff/120001/chrome/browser/autocom...
File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right):

https://codereview.chromium.org/2965173002/diff/120001/chrome/browser/autocom...
chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:130:
contextual_suggestions::ContextualSuggestionsService*
On 2017/07/07 20:13:54, Mark P wrote:
> #include the file that gives you
> contextual_suggestions::ContextualSuggestionsService explicitly

Done.

https://codereview.chromium.org/2965173002/diff/120001/chrome/browser/autocom...
File chrome/browser/autocomplete/chrome_autocomplete_provider_client.h (right):

https://codereview.chromium.org/2965173002/diff/120001/chrome/browser/autocom...
chrome/browser/autocomplete/chrome_autocomplete_provider_client.h:11: #include
"components/contextual_suggestions/contextual_suggestions_service.h"
On 2017/07/07 20:13:54, Mark P wrote:
> No need to add this here; you get it from the parent class.

Removed.

https://codereview.chromium.org/2965173002/diff/120001/components/contextual_...
File components/contextual_suggestions/contextual_suggestions_service.cc
(right):

https://codereview.chromium.org/2965173002/diff/120001/components/contextual_...
components/contextual_suggestions/contextual_suggestions_service.cc:119: 
On 2017/07/07 20:13:54, Mark P wrote:
> I'm a bit bothered that the whole rest of this file is (nearly) a duplicate of
> code in components/omnibox/...

I share the feeling with you and I am willing to move the whole networking logic
from components/omnibox here. I feel, though, that this CL is already large
enough. If you don't mind, I can file a bug and deal with it later. The main
purpose of this CL is to make sure that our backend gets the oauth token. I will
deal with refactoring after.

https://codereview.chromium.org/2965173002/diff/120001/components/contextual_...
components/contextual_suggestions/contextual_suggestions_service.cc:124:
net::DefineNetworkTrafficAnnotation("omnibox_contextual_zerosuggest", R"(
On 2017/07/07 20:13:54, Mark P wrote:
> Not sure if you should be using a new annotation here or not.

The annotation is different though. First, it sends "The user's OAuth2
credentials" along with the visited URL. Second, it does not allow cookies. Note
that I also removed all the info/headers that our backend is not using.

I am not familiar with the use of network traffic annotations, so I can't speak
on whether a different annotation is a must or not, but the semantics and the
policies are slightly different.

https://codereview.chromium.org/2965173002/diff/120001/components/contextual_...
components/contextual_suggestions/contextual_suggestions_service.cc:161:
fetcher->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
On 2017/07/07 20:13:54, Mark P wrote:
> Are you sure you want to set this flag?

The request sends the oauth token. As such, there is no need to send cookies.

https://codereview.chromium.org/2965173002/diff/120001/components/omnibox/bro...
File components/omnibox/browser/autocomplete_provider_client.h (right):

https://codereview.chromium.org/2965173002/diff/120001/components/omnibox/bro...
components/omnibox/browser/autocomplete_provider_client.h:12: #include
"components/contextual_suggestions/contextual_suggestions_service.h"
On 2017/07/07 20:13:54, Mark P wrote:
> forward declare; don't include

Done.

https://codereview.chromium.org/2965173002/diff/120001/components/omnibox/bro...
File components/omnibox/browser/zero_suggest_provider.cc (right):

https://codereview.chromium.org/2965173002/diff/120001/components/omnibox/bro...
components/omnibox/browser/zero_suggest_provider.cc:162: // Create a request for
experimental suggestions with |this| as the
On 2017/07/07 20:13:54, Mark P wrote:
> bug: probably want to set done_ = false here.

I am setting it in the callback function (see
ZeroSuggestProvider::OnContextualSuggestionsFetcherAvailable below). Unless I am
mistaken, done_ keeps track of whether fetching is done. I would rather set it
to false only when I start fetching. Here, I am only creating a request with
OnContextualSuggestionsFetcherAvailable as a callback.

Mark P

On 2017/07/07 21:24:29, gcomanici wrote: > On 2017/07/07 20:13:55, Mark P wrote: > > A ...

3 years, 5 months ago (2017-07-07 23:55:43 UTC) #29

blundell

On 2017/07/07 23:55:43, Mark P wrote: > On 2017/07/07 21:24:29, gcomanici wrote: > > On ...

3 years, 5 months ago (2017-07-10 11:30:37 UTC) #30

Marc Treib

On 2017/07/10 11:30:37, blundell wrote: > On 2017/07/07 23:55:43, Mark P wrote: > > On ...

3 years, 5 months ago (2017-07-10 11:39:59 UTC) #31

On 2017/07/10 11:30:37, blundell wrote:
> On 2017/07/07 23:55:43, Mark P wrote:
> > On 2017/07/07 21:24:29, gcomanici wrote:
> > > On 2017/07/07 20:13:55, Mark P wrote:
> > > > A gave this a quick, haphazard review.  I have two main questions /
> concerns
> > > > about this design:
> > > > 
> > > > - it looks like a sizable chunk of code is being duplicated between
> > > > components/omnibox/ and this new component.  Can we refactor the code so
> > that
> > > > all suggest requests go through the same flow so the code is not
> duplicated?
> > 
> > > > After all, the regular suggest flow also needs cookies attached to it;
it
> > just
> > > > gets that for free right now.
> > > 
> > > I responded to a similar comment you made in the code. I am all for
> > refactoring 
> > > the code, but I'd leave it for a different CL, if you don't mind.
> > 
> > Can you check how much the binary size increases with this new component? 
> > People are working to shave, say, things the size of 200k of memory on
> Android. 
> > If the code duplication here comes to even a fraction of that, it's worth
> fixing
> > it before submitting.
> > 
> > Client-side code has different priorities than server-side ones. :-)
> > 
> > --mark
> 
> Given the concerns on duplication between //components/{omnibox, suggestions}
> and the new component, I'm hesitant to LG the addition of the new component as
> the discussion currently stands. If there's refactoring to be done between the
> two existing components and the new one, I'd strongly prefer that we do the
> refactoring first rather than leaving it for afterwards unless there are
> compelling reasons not to. I agree with others that having this discussion on
> chrome-design-docs@ seems like the right next step -- maybe there are
compelling
> reasons to order it this way and I'm just missing them, but it will be easier
to
> tease that discussion out over an email thread/design doc than inline in code
> review. Does that make sense?

TBH, I think the similarity between the new components/contextual_suggestions
and the existing components/suggestions has been overstated. Despite the similar
names and some conceptual similarities, code-wise they are quite different. The
common part is essentially "a URL fetch with OAuth2 credentials". We have
crbug.com/609084 to extract a helper class for that, but it won't be a lot of
code, so I doubt that'd have any relevant impact on binary size.

(I know nothing about duplication between the new component and
components/omnibox, so that might still be a valid concern.)

blundell

On 2017/07/10 11:39:59, Marc Treib wrote: > On 2017/07/10 11:30:37, blundell wrote: > > On ...

3 years, 5 months ago (2017-07-10 14:34:55 UTC) #32

On 2017/07/10 11:39:59, Marc Treib wrote:
> On 2017/07/10 11:30:37, blundell wrote:
> > On 2017/07/07 23:55:43, Mark P wrote:
> > > On 2017/07/07 21:24:29, gcomanici wrote:
> > > > On 2017/07/07 20:13:55, Mark P wrote:
> > > > > A gave this a quick, haphazard review.  I have two main questions /
> > concerns
> > > > > about this design:
> > > > > 
> > > > > - it looks like a sizable chunk of code is being duplicated between
> > > > > components/omnibox/ and this new component.  Can we refactor the code
so
> > > that
> > > > > all suggest requests go through the same flow so the code is not
> > duplicated?
> > > 
> > > > > After all, the regular suggest flow also needs cookies attached to it;
> it
> > > just
> > > > > gets that for free right now.
> > > > 
> > > > I responded to a similar comment you made in the code. I am all for
> > > refactoring 
> > > > the code, but I'd leave it for a different CL, if you don't mind.
> > > 
> > > Can you check how much the binary size increases with this new component? 
> > > People are working to shave, say, things the size of 200k of memory on
> > Android. 
> > > If the code duplication here comes to even a fraction of that, it's worth
> > fixing
> > > it before submitting.
> > > 
> > > Client-side code has different priorities than server-side ones. :-)
> > > 
> > > --mark
> > 
> > Given the concerns on duplication between //components/{omnibox,
suggestions}
> > and the new component, I'm hesitant to LG the addition of the new component
as
> > the discussion currently stands. If there's refactoring to be done between
the
> > two existing components and the new one, I'd strongly prefer that we do the
> > refactoring first rather than leaving it for afterwards unless there are
> > compelling reasons not to. I agree with others that having this discussion
on
> > chrome-design-docs@ seems like the right next step -- maybe there are
> compelling
> > reasons to order it this way and I'm just missing them, but it will be
easier
> to
> > tease that discussion out over an email thread/design doc than inline in
code
> > review. Does that make sense?
> 
> TBH, I think the similarity between the new components/contextual_suggestions
> and the existing components/suggestions has been overstated. Despite the
similar
> names and some conceptual similarities, code-wise they are quite different.
The
> common part is essentially "a URL fetch with OAuth2 credentials". We have
> crbug.com/609084 to extract a helper class for that, but it won't be a lot of
> code, so I doubt that'd have any relevant impact on binary size.
> 
> (I know nothing about duplication between the new component and
> components/omnibox, so that might still be a valid concern.)

I'm totally open to being convinced; I just think it will be an easier
discussion to have as a design discussion than on a code review.

gcomanici

I will be the first to admit that I was a bit too ambitious in ...

3 years, 5 months ago (2017-07-10 16:31:07 UTC) #34

blundell

On 2017/07/10 16:31:07, gcomanici wrote: > I will be the first to admit that I ...

3 years, 5 months ago (2017-07-11 08:53:09 UTC) #35

gcomanici

Since I am going to be OOO for a few weeks starting tomorrow, I will ...

3 years, 5 months ago (2017-07-11 17:36:46 UTC) #36

gcomanici

Description was changed from ========== Create a new component `contextual_suggestions` This component provides a service ...

3 years, 5 months ago (2017-07-11 17:40:40 UTC) #37

Description was changed from

==========
Create a new component `contextual_suggestions`

This component provides a service to fetch experimental contextual suggestions
for the omnibox.

Experimental Contextual Suggestions will soon use user identity to provide
personalized suggestions. The service that is creating a request with a oauth2
token attached is becoming more complex and should live in its own component,
much like the service that is fetching suggestions for the NTP (new tab page).
This makes the logic a lot easier to test and increases code readability for the
fetcher, related experiments, and the zero suggest autocomplete provider.

Additionally, b/609084 is traking an effort to unify all the URL Fetchers that
use an oauth2 token. Creating this new component should facilitate future
changes towards this effort.

From a technical perspective, the new component `contextual_suggestions` is very
similar to the `suggestions` component. The two components are providing very
similar services (i.e. suggestions for different surfaces) but the new component
is much simpler, as currently most of the logic related to contextual
suggestions is performed on server side. Still, more functionality will soon be
necessary on client side and all the corresponding logic will be living in the
new component.


Design doc:
https://docs.google.com/document/d/1FUowJo9AnSziiQfBumyicaTWDKjOcM574aOBwKIVp...

BUG=692471,609084
==========

to

==========
Create a new class `ContextualSuggestionsService`

This class provides a service to fetch experimental contextual suggestions for
the omnibox.

Experimental Contextual Suggestions will soon use user identity to provide
personalized suggestions. The service that is creating a request with a oauth2
token attached is becoming more complex and should live in its own class, much
like the service that is fetching suggestions for the NTP (new tab page). This
makes the logic a lot easier to test and increases code readability for the
fetcher, related experiments, and the zero suggest autocomplete provider.

Additionally, b/609084 is traking an effort to unify all the URL Fetchers that
use an oauth2 token. Creating this new component should facilitate future
changes towards this effort.

From a technical perspective, the new class is similar to SuggestionsService in
componenents/suggestions. More functionality will soon be necessary on client
side and all the corresponding logic will be handled by the
ContextualSuggestionsService.

Design doc:
https://docs.google.com/document/d/1FUowJo9AnSziiQfBumyicaTWDKjOcM574aOBwKIVp...

BUG=692471,609084
==========

Mark P

I did a limited review. (Ran out of time for more.) Here are my preliminary ...

3 years, 5 months ago (2017-07-17 23:36:12 UTC) #38

blundell

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/browser/DEPS File components/omnibox/browser/DEPS (right): https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/browser/DEPS#newcode19 components/omnibox/browser/DEPS:19: "+components/signin/core/browser", On 2017/07/17 23:36:12, Mark P wrote: > I'm ...

3 years, 5 months ago (2017-07-18 15:21:16 UTC) #39

Mark P

I managed to review more of the changelist except for the bulk of components/omnibox/browser/contextual_suggestions_service.cc Below ...

3 years, 5 months ago (2017-07-18 19:11:30 UTC) #40

Toyama

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

3 years, 5 months ago (2017-07-18 20:28:46 UTC) #41

Toyama

Hi Mark, can you take another look? Also, because this is not my issue, we ...

3 years, 5 months ago (2017-07-18 20:28:48 UTC) #42

Hi Mark, can you take another look?

Also, because this is not my issue, we should probably continue the review on
https://chromium-review.googlesource.com/c/576284/ instead.

Thanks!

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
File components/omnibox/browser/contextual_suggestions_service.cc (right):

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
components/omnibox/browser/contextual_suggestions_service.cc:77:
ExperimentalZeroSuggestURL(/*visited_url=*/std::string());
On 2017/07/18 19:11:28, Mark P wrote:
> Shouldn't you provide a real URL here?  I wouldn't be surprised if
> ExperimentalZeroSuggestURL rejects constructing a suggestion URL if the
provided
> context URL is invalid.

I don't have the right context here, but everything seems to be working just
fine with or without current_url. I agree with you that it's probably better to
pass something than an empty string.

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
File components/omnibox/browser/contextual_suggestions_service.h (right):

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
components/omnibox/browser/contextual_suggestions_service.h:11: #include
"base/feature_list.h"
On 2017/07/18 19:11:28, Mark P wrote:
> nit: necessary?

Done. (removed)

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
components/omnibox/browser/contextual_suggestions_service.h:13: #include
"components/search_engines/template_url_service.h"
On 2017/07/18 19:11:28, Mark P wrote:
> nit: just forward declare; no need to include

Done.

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
components/omnibox/browser/contextual_suggestions_service.h:16: #include
"net/url_request/url_request_context_getter.h"
On 2017/07/18 19:11:29, Mark P wrote:
> nit: just forward declare; no need to include

Moved to .cc file. There are no symbols needed in this header.

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
components/omnibox/browser/contextual_suggestions_service.h:22: namespace
contextual_suggestions {
On 2017/07/18 19:11:29, Mark P wrote:
> No need for namespace, as everything is already encapsulated in a class.
> (Generally this component doesn't put its things in namespaces.)

Done.

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
components/omnibox/browser/contextual_suggestions_service.h:24: class
ContextualSuggestionsService : public KeyedService {
On 2017/07/18 19:11:29, Mark P wrote:
> nit: Please comment the purpose of this class.

Done.

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
components/omnibox/browser/contextual_suggestions_service.h:26:
ContextualSuggestionsService(SigninManagerBase* signin_manager,
On 2017/07/18 19:11:28, Mark P wrote:
> Please comment whether any of these are allowed to be null.

Done.

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
components/omnibox/browser/contextual_suggestions_service.h:38: // of the
fetcher.
On 2017/07/18 19:11:29, Mark P wrote:
> Please add/revise here to make it clearer what relationship between
> |fetcher_delegate| that processes the response and |callback| are.  I read all
> the comments below and I don't know.
> 
> Then, with that in mind, please revise the comments below.  There is also some
> formatting issues there and at least one typo, but the bigger issue is that
> they're confusing, likely due to a lack of explanation at the beginning about
> the relationship I mentioned.

I read the code and I am myself a little bit confused. I rewrote the comment
with my current understanding, but can you take a look and let me know what you
think?

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
components/omnibox/browser/contextual_suggestions_service.h:53: const
std::string& visited_url,
On 2017/07/18 19:11:28, Mark P wrote:
> nit: visited_url -> current_url or simply url
> Apparently, from looking at the code, sometimes this is called with an empty
> URL.  Normally URLs cannot be empty.  This definitely needs to be commented.

I s/visited_url/current_url/ everything in this code.

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
components/omnibox/browser/contextual_suggestions_service.h:59: // * The
|visited_url| is non-empty.
On 2017/07/18 19:11:28, Mark P wrote:
> non-empty?  Probably this should be checking whether the url is valid.

The code is currently only checking it's not empty. What kinds of checks do you
want to perform to ensure the URL is valid? Notice that the backend will check
whether the URL is valid in its on way regardless if Chrome checks here. Also,
the calling code for this service may check as well. IMHO I think it's OK to
just check for empty, but I can add more checks if you think they are necessary.

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
components/omnibox/browser/contextual_suggestions_service.h:71: // Returns a
string representing the address of the server where the zero
On 2017/07/18 19:11:29, Mark P wrote:
> Doesn't return a string...

Done. s/string/URL/

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
components/omnibox/browser/contextual_suggestions_service.h:74: static GURL
ExperimentalZeroSuggestURL(const std::string& visited_url);
On 2017/07/18 19:11:29, Mark P wrote:
> nit: I'd prefer current_url rather than visited_url.  What do you think?

Done.

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
components/omnibox/browser/contextual_suggestions_service.h:77: // suggest
request is being sent.
On 2017/07/18 19:11:29, Mark P wrote:
> This comment makes it unclear how this relates to the functions above.  Do you
> want to add (if correct)
> // Uses the experimental suggest URL as appropriate.
> And why do you need to pass in the parameter |experimental|?  Can't you just
> have this function call UseExperimentalZeroSuggestSuggestions().
> 
> Regardless, this function should probably go higher in this list of functions,
> perhaps before UseExperimentalZeroSuggestSuggestions().

I s/experimental/is_experimental/ to make the parameter a bit more clear. I
added a comment to explain |is_experimental|. PTAL.

As for calling UseExperimentalZeroSuggestions(), it is definitely possible, but
that function is not exactly free and there are a number of calls there. I think
Gheorghe's solution to memoize the call is not so bad. Do you feel strongly
about this?

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
components/omnibox/browser/contextual_suggestions_service.h:81: // does not
include a header corresponding to an authorization token.
On 2017/07/18 19:11:28, Mark P wrote:
> nit: 
> The return value does not include a header corresponding to an authorization
> token.
> ->
> The returned fetcher does not include a header with an authorization token.

Done.

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
components/omnibox/browser/contextual_suggestions_service.h:98: // Helper for
fetching OAuth2 access tokens. This is non-null iff an access
On 2017/07/18 19:11:28, Mark P wrote:
> Does
> iff
> mean
> if and only if
> 
> If so, simply say "when".

iff == if and only if

I think "iff" is stronger than "when", but we don't need to be pedantic here. I
think when reads more naturally than iff in this case.

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
components/omnibox/browser/contextual_suggestions_service.h:99: // token request
is currently in progress.
On 2017/07/18 19:11:29, Mark P wrote:
> Roughly when does that happen?  There probably should be some comment by one
of
> these functions here that says "sends a request for an OAuth2 token".

I added the comment to CreateContextualSuggestionsRequest(). PTAL.

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
components/omnibox/browser/contextual_suggestions_service.h:101: };
On 2017/07/18 19:11:29, Mark P wrote:
> nit: missing DISALLOW_COPY_AND_ASSIGN.  Is that intentional?

I can't speak for Gheorghe, but I think it was not intentional. This class holds
handles for some external objects, and copying is probably not the right thing I
think. I added the macro.

Mark P

On 2017/07/18 20:28:48, Toyama wrote: > Hi Mark, can you take another look? You replied ...

3 years, 5 months ago (2017-07-18 23:24:46 UTC) #43

Mark P

A few replies to your comments. --mark https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/browser/contextual_suggestions_service.h File components/omnibox/browser/contextual_suggestions_service.h (right): https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/browser/contextual_suggestions_service.h#newcode59 components/omnibox/browser/contextual_suggestions_service.h:59: // * ...

3 years, 5 months ago (2017-07-18 23:45:50 UTC) #44

A few replies to your comments.

--mark

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
File components/omnibox/browser/contextual_suggestions_service.h (right):

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
components/omnibox/browser/contextual_suggestions_service.h:59: // * The
|visited_url| is non-empty.
On 2017/07/18 20:28:48, Toyama wrote:
> On 2017/07/18 19:11:28, Mark P wrote:
> > non-empty?  Probably this should be checking whether the url is valid.
> 
> The code is currently only checking it's not empty. What kinds of checks do
you
> want to perform to ensure the URL is valid?
Usually we pass around GURL variables and use gurl.is_valid() to check that it's
a proper URL.

> Notice that the backend will check
> whether the URL is valid in its on way regardless if Chrome checks here. Also,
> the calling code for this service may check as well. IMHO I think it's OK to
> just check for empty, but I can add more checks if you think they are
necessary.

It's not necessary, just confusing.  You need to make the reasoning for this
clear.  It looks like this function doesn't do anything with the URL other than
check whether it's empty or not.  That makes me think you shouldn't be passing a
string to this at all.

https://codereview.chromium.org/2965173002/diff/200001/components/omnibox/bro...
components/omnibox/browser/contextual_suggestions_service.h:77: // suggest
request is being sent.
On 2017/07/18 20:28:48, Toyama wrote:
> On 2017/07/18 19:11:29, Mark P wrote:
> > This comment makes it unclear how this relates to the functions above.  Do
you
> > want to add (if correct)
> > // Uses the experimental suggest URL as appropriate.
> > And why do you need to pass in the parameter |experimental|?  Can't you just
> > have this function call UseExperimentalZeroSuggestSuggestions().
> > 
> > Regardless, this function should probably go higher in this list of
functions,
> > perhaps before UseExperimentalZeroSuggestSuggestions().
> 
> I s/experimental/is_experimental/ to make the parameter a bit more clear. I
> added a comment to explain |is_experimental|. PTAL.
> 
> As for calling UseExperimentalZeroSuggestions(), it is definitely possible,
but
> that function is not exactly free and there are a number of calls there. I
think
> Gheorghe's solution to memoize the call is not so bad. Do you feel strongly
> about this?

I don't feel strongly about memoizing as long as the comments are clear about
what each function does.

Toyama

3 years, 5 months ago (2017-07-19 00:06:45 UTC) #45

On 2017/07/18 23:24:46, Mark P wrote:
> On 2017/07/18 20:28:48, Toyama wrote:
> > Hi Mark, can you take another look?
> 
> You replied to my comments as if you fixed them, yet didn't upload a
> new patch set in this issue.
> 
> > Also, because this is not my issue, we should probably continue the review
on
> > https://chromium-review.googlesource.com/c/576284/ instead.
> 
> And the new issue that you want me to review contains only a single patchset
> that doesn't have the fixes to my comments.  Did you forget to upload a new
> patch set?
> 
> Also, I'll note that there are comments in this patchset that you didn't
> reply to at all.
> 
> I will publish additional comments on
> https://chromium-review.googlesource.com/c/576284/
> 
> --mark

I'm sorry, I forgot to do "git cl upload". It's confusing to go back and forth
between two code review systems. Let's do everything from Gerrit.

Issue 2965173002: Add ContextualSuggestionsService to Omnibox (Closed)

Description

Patch Set 1 : Initial component creation #

Patch Set 2 #

Patch Set 3 #

Patch Set 4 : Fix includes and the composition of the fetcher. #

Patch Set 5 : Remove code duplication. #

Patch Set 6 : Move contextual suggestions service to c/o/b #

Messages