Add ContextualSuggestionsService to Omnibox

components/omnibox/browser/contextual_suggestions_service.h

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_OMNIBOX_BROWSER_CONTEXTUAL_SUGGESTIONS_SERVICE_H_
+#define COMPONENTS_OMNIBOX_BROWSER_CONTEXTUAL_SUGGESTIONS_SERVICE_H_
+
+#include <memory>
+#include <string>
+
+#include "base/feature_list.h"

[Mark P, 2017/07/18 19:11:28]

nit: necessary?

[Toyama, 2017/07/18 20:28:47]

Done. (removed)

+#include "components/keyed_service/core/keyed_service.h"
+#include "components/search_engines/template_url_service.h"

[Mark P, 2017/07/18 19:11:28]

nit: just forward declare; no need to include

[Toyama, 2017/07/18 20:28:47]

Done.

+#include "components/signin/core/browser/access_token_fetcher.h"
+#include "net/url_request/url_fetcher_delegate.h"
+#include "net/url_request/url_request_context_getter.h"

[Mark P, 2017/07/18 19:11:29]

nit: just forward declare; no need to include

[Toyama, 2017/07/18 20:28:47]

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

+#include "url/gurl.h"
+
+class OAuth2TokenService;
+class SigninManagerBase;
+
+namespace contextual_suggestions {

[Mark P, 2017/07/18 19:11:29]

No need for namespace, as everything is already encapsulated in a class.
(Generally this component doesn't put its things in namespaces.)

[Toyama, 2017/07/18 20:28:47]

Done.

+
+class ContextualSuggestionsService : public KeyedService {

[Mark P, 2017/07/18 19:11:29]

nit: Please comment the purpose of this class.

[Toyama, 2017/07/18 20:28:47]

Done.

+ public:
+  ContextualSuggestionsService(SigninManagerBase* signin_manager,

[Mark P, 2017/07/18 19:11:28]

Please comment whether any of these are allowed to be null.

[Toyama, 2017/07/18 20:28:47]

Done.

+                               OAuth2TokenService* token_service,
+                               TemplateURLService* template_url_service,
+                               net::URLRequestContextGetter* request_context);
+
+  ~ContextualSuggestionsService() override;
+
+  using ContextualSuggestionsCallback =
+      base::OnceCallback<void(std::unique_ptr<net::URLFetcher> fetcher)>;
+
+  // Creates a fetch request for contextual suggestions for |visited_url|, with
+  // |fetcher_delegate| as the URLFetcherDelegate that will process the response
+  // of the fetcher.

[Mark P, 2017/07/18 19:11:29]

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.

[Toyama, 2017/07/18 20:28:47]

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?

+  //
+  // * If the URL to send the request to, which is constructed using field trial
+  //   parameters, is invalid, |callback| is executed synchronously witha
+  //   nullptr for |fetcher|.
+  // * If the service is not using experimental suggestions, |callback| is
+  //   executed synchronously with a valid |fetcher| as a parameter.
+  // * If the service is supposed to use experimental suggestions and it is
+  // waiting for an authentication token,  |callback| is executed synchronously
+  // with a nullptr for |fetcher|. * If the service is supposed to use
+  // experimental suggestions and it is not waiting for an authentication token,
+  // a request for an auth token is made asynchronously. After the token is
+  // obtained and attached to the |fetcher|, |callback| is called with a valid
+  // |fetcher| as a parameter.
+  void CreateContextualSuggestionsRequest(
+      const std::string& visited_url,

[Mark P, 2017/07/18 19:11:28]

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.

[Toyama, 2017/07/18 20:28:48]

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

+      net::URLFetcherDelegate* fetcher_delegate,
+      ContextualSuggestionsCallback callback);
+
+ private:
+  // Returns true if the folowing conditions are valid:
+  // * The |visited_url| is non-empty.

[Mark P, 2017/07/18 19:11:28]

non-empty?  Probably this should be checking whether the url is valid.

[Toyama, 2017/07/18 20:28:48]

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.

[Mark P, 2017/07/18 23:45:50]

Usually we pass around GURL variables and use gurl.is_valid() to check that it's
a proper URL.
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.

+  // * The |default_provider| is Google.
+  // * The user is in the ZeroSuggestRedirectToChrome field trial.
+  // * The field trial provides a valid server address where the suggest request
+  //   is redirected.
+  // * The suggest request is over HTTPS. This avoids leaking the current page
+  //   URL or personal data in unencrypted network traffic.
+  // Note: these checks are in addition to CanSendUrl() on the default
+  // contextual suggestion URL.
+  bool UseExperimentalZeroSuggestSuggestions(
+      const std::string& visited_url) const;
+
+  // Returns a string representing the address of the server where the zero

[Mark P, 2017/07/18 19:11:29]

Doesn't return a string...

[Toyama, 2017/07/18 20:28:47]

Done. s/string/URL/

+  // suggest requests are being redirected when serving experimental
+  // suggestions.
+  static GURL ExperimentalZeroSuggestURL(const std::string& visited_url);

[Mark P, 2017/07/18 19:11:29]

nit: I'd prefer current_url rather than visited_url.  What do you think?

[Toyama, 2017/07/18 20:28:47]

Done.

+
+  // Returns a string representing the address of the server where the zero
+  // suggest request is being sent.

[Mark P, 2017/07/18 19:11:29]

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().

[Toyama, 2017/07/18 20:28:48]

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?

[Mark P, 2017/07/18 23:45:50]

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

+  GURL ZeroSuggestURL(const std::string& visited_url, bool experimental) const;
+
+  // Creates an HTTP Get request for contextual suggestions. The return value
+  // does not include a header corresponding to an authorization token.

[Mark P, 2017/07/18 19:11:28]

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.

[Toyama, 2017/07/18 20:28:48]

Done.

+  std::unique_ptr<net::URLFetcher> CreateRequest(
+      const std::string& visited_url,
+      bool experimental,
+      net::URLFetcherDelegate* fetcher_delegate) const;
+
+  // Called when an access token request completes (successfully or not).
+  void AccessTokenAvailable(std::unique_ptr<net::URLFetcher> fetcher,
+                            ContextualSuggestionsCallback callback,
+                            const GoogleServiceAuthError& error,
+                            const std::string& access_token);
+
+  net::URLRequestContextGetter* request_context_;
+  SigninManagerBase* signin_manager_;
+  TemplateURLService* template_url_service_;
+  OAuth2TokenService* token_service_;
+
+  // Helper for fetching OAuth2 access tokens. This is non-null iff an access

[Mark P, 2017/07/18 19:11:28]

Does
iff
mean
if and only if

If so, simply say "when".

[Toyama, 2017/07/18 20:28:48]

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.

+  // token request is currently in progress.

[Mark P, 2017/07/18 19:11:29]

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".

[Toyama, 2017/07/18 20:28:47]

I added the comment to CreateContextualSuggestionsRequest(). PTAL.

+  std::unique_ptr<AccessTokenFetcher> token_fetcher_;
+};

[Mark P, 2017/07/18 19:11:29]

nit: missing DISALLOW_COPY_AND_ASSIGN.  Is that intentional?

[Toyama, 2017/07/18 20:28:47]

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.

+
+}  // namespace contextual_suggestions
+
+#endif  // COMPONENTS_OMNIBOX_BROWSER_CONTEXTUAL_SUGGESTIONS_SERVICE_H_