[chrome.dial] Adds chrome.dial.fetchDeviceDecription API.

chrome/browser/extensions/api/dial/device_description_fetcher.h

+// Copyright (c) 2016 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 CHROME_BROWSER_EXTENSIONS_API_DIAL_DEVICE_DESCRIPTION_FETCHER_H_
+#define CHROME_BROWSER_EXTENSIONS_API_DIAL_DEVICE_DESCRIPTION_FETCHER_H_
+
+#include <memory>
+#include <string>
+
+#include "base/callback.h"
+#include "net/url_request/url_fetcher_delegate.h"
+#include "url/gurl.h"
+
+class Profile;
+
+namespace net {
+class URLFetcher;
+}
+
+namespace extensions {
+namespace api {
+namespace dial {
+
+class DialDeviceDescriptionData;
+
+// An instance of this class is used to make a single HTTP GET request with

[Wez, 2017/01/05 22:29:24]

nit: No need for "An instance of this class" - can start with "Used to..."

[mark a. foltz, 2017/01/09 21:38:07]

Done.

+// |device_description_url| to fetch a uPnP device description.  If successful,

[Wez, 2017/01/05 22:29:24]

nit: Is this description retrieval specific to DIAL, or actually uPnP-generic?
If the latter then perhaps re-locate/re-name this class accordingly?

[mark a. foltz, 2017/01/09 21:38:07]

It has DIAL-specific logic like retrieving the app URL.  Clarified comment.

[Wez, 2017/01/09 22:12:58]

Acknowledged.

+// |success_cb| is invoked with the result; otherwise, |error_cb| is invoked
+// with an error reason.  This class is not thread safe and must be run on the
+// UI thread.

[Wez, 2017/01/05 22:29:24]

nit: Re thread-safety, must this class be constructed, Start()ed AND destroyed
from the UI thread?

Is it the case that the caller can tear down the class to cancel the request if
it hasn't yet completed?

[mark a. foltz, 2017/01/09 21:38:07]

s/run/used/
Yes, destroying |fetcher_| cancels any pending request.

[Wez, 2017/01/09 22:12:58]

Acknowledged.

+class DeviceDescriptionFetcher : public net::URLFetcherDelegate {
+ public:
+  // Used to identify the net::URLFetcher instance.
+  static constexpr int kURLFetcherID = 1;

[Wez, 2017/01/05 22:29:24]

nit: This is only public for testing, and the purpose of the |id| is really to
allow tests to disambiguate multiple requests.

If we replace the ctor+Start() layout with a single Create() function that
creates & returns the fetcher then we could instead have a CreateForTest(id,
...), which would also allow tests to create multiple instances, if necessary
(though I see we don't have use for that now).

Otherwise, could we at least make this a kURLFetcherIdForTest, or a
GetUrlFetcherIdForTest() method (in the latter case the Id can still be
hard-wired internally; we'd just be decoupling the test from caring about the
constant).

[mark a. foltz, 2017/01/09 21:38:07]

I don't need multiple request IDs to test this class.  Renamed to
kURLFetcherIdForTest to clarify its use.

[Wez, 2017/01/09 22:12:58]

Acknowledged.

+
+  DeviceDescriptionFetcher(
+      const GURL& device_description_url,
+      Profile* profile,

[Wez, 2017/01/05 22:29:24]

Is the caller responsible for ensuring that the instance is torn down before
|profile|?

[mark a. foltz, 2017/01/09 21:38:07]

The DIAL API is a BrowserContext-keyed service, its extension function objects
(and thus this object) will be destroyed before the Profile.

Strictly speaking, the fetcher doesn't need to hold a Profile*, it's only used
to start the request.  If you feel strongly I can refactor to remove the
Profile* class member.

[Wez, 2017/01/09 22:12:58]

No, I think this is fine, so long as the lifetime of |profile| is well-defined;
I'd suggest simply adding a one-line comment to clarify that the caller must
ensure that DDF doesn't out-live |profile|.

[mark a. foltz, 2017/01/10 00:36:13]

Done.

+      base::OnceCallback<void(const DialDeviceDescriptionData&)> success_cb,
+      base::OnceCallback<void(const std::string&)> error_cb);
+
+  ~DeviceDescriptionFetcher() override;
+
+  void Start();

[Wez, 2017/01/05 22:29:24]

nit: Do we need a separate Start() API, or could we start the request directly
in the constructor?

[mark a. foltz, 2017/01/09 21:38:07]

Generally it's not a good idea to write constructors with heavyweight side
effects, unless it's an explicitly designed to be a ScopedFoo / RAII type
object.

[Wez, 2017/01/09 22:12:58]

Granted, but it would be reasonable to have a static Create() method that
constructed + started.  Up to you, though.

[mark a. foltz, 2017/01/10 00:36:13]

Acknowledged.

+
+ private:
+  // net::URLFetcherDelegate

[Wez, 2017/01/05 22:29:24]

nit: "|class| implementation." or "|class| interface." is more common.

[mark a. foltz, 2017/01/09 21:38:07]

Done.

+  void OnURLFetchComplete(const net::URLFetcher* source) override;
+  void OnURLFetchDownloadProgress(const net::URLFetcher* source,
+                                  int64_t current,
+                                  int64_t total,
+                                  int64_t current_network_bytes) override;
+  void OnURLFetchUploadProgress(const net::URLFetcher* source,
+                                int64_t current,
+                                int64_t total) override;
+
+  // Runs |error_cb_| with |message|.

[Wez, 2017/01/05 22:29:24]

nit: Not obvious from the comment, but it Run()s and clears |error_cb_|, right?

Do we really want to use a string error message here? Could we instead use
net::Error codes and HTTP response values to indicate errors? Since HTTP
responses are >0 and net::Error <0 you could even fold the two into a single
integer error value.

[mark a. foltz, 2017/01/09 21:38:07]

Updated comment, but as error_cb is a OnceCallback there's no need to clear.
The URLFetcher doesn't expose net::Error codes.

There are a variety of error conditions and the chrome.runtime.lastError API
only allows a string.  If there's some future code that would consume error
codes, they would be straightforward to add.

[Wez, 2017/01/09 22:12:58]

Acknowledged.

+  void ReportError(const std::string& message);
+
+  const GURL device_description_url_;
+  Profile* const profile_;
+  base::OnceCallback<void(const DialDeviceDescriptionData&)> success_cb_;
+  base::OnceCallback<void(const std::string&)> error_cb_;
+  std::unique_ptr<net::URLFetcher> fetcher_;
+};
+
+}  // namespace dial
+}  // namespace api
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_EXTENSIONS_API_DIAL_DEVICE_DESCRIPTION_FETCHER_H_