[Media Router] Add DialMediaSinkService and DeviceDescriptionService

chrome/browser/media/router/device_description_service.h

Index: chrome/browser/media/router/device_description_service.h
diff --git a/chrome/browser/media/router/device_description_service.h b/chrome/browser/media/router/device_description_service.h
new file mode 100644
index 0000000000000000000000000000000000000000..f4cf47717c17f72a32d08e7756a5320e5a813ec7
--- /dev/null
+++ b/chrome/browser/media/router/device_description_service.h
@@ -0,0 +1,176 @@
+// 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 CHROME_BROWSER_MEDIA_ROUTER_DEVICE_DESCRIPTION_SERVICE_H_
+#define CHROME_BROWSER_MEDIA_ROUTER_DEVICE_DESCRIPTION_SERVICE_H_
+
+#include <memory>
+#include <set>
+#include <string>
+#include <vector>
+
+#include "base/optional.h"
+#include "base/time/time.h"
+#include "base/timer/timer.h"
+#include "chrome/browser/extensions/api/dial/dial_registry.h"
+
+namespace extensions {
+namespace api {
+namespace dial {
+struct DialDeviceDescriptionData;
+class DeviceDescriptionFetcher;
+}
+}
+}
+
+namespace net {
+class URLRequestContextGetter;
+}
+
+namespace media_router {
+
+struct DialDeviceDescription {
+  DialDeviceDescription();

[mark a. foltz, 2017/03/11 00:08:15]

There should be a ctor that takes mandatory properties.  I believe those are
unique_id, friendly_name, ip_address, app_url, fetch_time_millis - check the
validation code in Javascript.

[zhaobin, 2017/03/28 13:37:00]

Done.

+  ~DialDeviceDescription();
+  DialDeviceDescription(const DialDeviceDescription& other);

[mark a. foltz, 2017/03/11 00:08:15]

Should there be a default assignment operator assigned as well?

[zhaobin, 2017/03/28 13:37:00]

Done.

+
+  std::string unique_id;
+  std::string device_label;
+  std::string friendly_name;
+  std::string ip_address;

[mark a. foltz, 2017/03/11 00:08:15]

net::IPAddress

[zhaobin, 2017/03/28 13:37:00]

Done.

+  GURL app_url;
+  base::Time fetch_time_millis;
+  base::Time expire_time_millis;
+  std::string device_type;
+  std::string model_name;
+  base::Optional<int32_t> config_id;
+  GURL device_description_url;
+};
+
+class DeviceDescriptionService {
+ public:
+  using DialRegistry = extensions::api::dial::DialRegistry;

[mark a. foltz, 2017/03/11 00:08:16]

I don't think chrome/browser/media should depend on extensions::.   The parts of
the extensions implementation that you need can be moved into
chrome/browser/media.

[zhaobin, 2017/03/28 13:37:00]

Done.

+  using DialDeviceDescriptionData =

[mark a. foltz, 2017/03/11 00:08:15]

ISTM that the DialDeviceDescription struct is a superset of
DialDeviceDescriptionData and DialDeviceData.  Do we need to retain all 3?

Maybe we eventually can make DialDeviceDescription the source of truth in
DialRegistry and copy data out of it to support the existing extension API.

[zhaobin, 2017/04/10 18:44:42]

Use ParsedDialDeviceDescription struct to store data from device description
xml.

+      extensions::api::dial::DialDeviceDescriptionData;
+  using DialDeviceData = extensions::api::dial::DialDeviceData;
+  using CheckAccessCallback = base::Callback<void(bool)>;
+
+  class Observer {
+   public:
+    // Methods invoked on the IO thread when device description fetch for
+    // |device_id| completed.
+    virtual void OnDeviceDescriptionAvailable(
+        const std::string& device_id,

[mark a. foltz, 2017/03/11 00:08:15]

Is this a device_label or unique_id?  Can you rename to be specific?

[zhaobin, 2017/03/28 13:37:00]

Done.

+        const DialDeviceDescription& description) = 0;
+    virtual void OnDeviceDescriptionFetchError(
+        const std::string& device_id,
+        const std::string& error_message) = 0;
+
+   protected:
+    virtual ~Observer() {}
+  };
+
+  // This class does not take ownership of |observer|.
+  explicit DeviceDescriptionService(Observer* observer);
+  virtual ~DeviceDescriptionService();
+
+  // Returns true and sets |out_description| to cached device description if
+  // there is a valid cache entry; Returns false and start device description
+  // fetch otherwise.
+  virtual bool GetDeviceDescription(

[mark a. foltz, 2017/03/11 00:08:15]

Can this take a callback argument so the caller doesn't have to care about the
status of the cache, only that it will eventually get a description (or error)?

[zhaobin, 2017/04/10 18:44:42]

Done.

+      const DialDeviceData& device,
+      net::URLRequestContextGetter* request_context,
+      DialDeviceDescription* out_description);
+
+  // Returns false if there is no pending fetcher for |device_label|.
+  virtual bool MayStopDeviceDescriptionFetching(

[mark a. foltz, 2017/03/11 00:08:16]

It sounds like it could just be CancelFetch(device_label)?

[zhaobin, 2017/03/28 13:37:00]

Done.

+      const std::string& device_label);
+
+  /**
+   * Issues a request to fetch the DIAL receiver's device description. Invoke
+   * check_access_callback with true if it was accessible, false otherwise.
+   * |device_description_url| The device description to fetch.
+   */
+  virtual void CheckAccess(const GURL& device_description_url,

[mark a. foltz, 2017/03/11 00:08:15]

Should this take DialDeviceData& like GetDeviceDescription?

[zhaobin, 2017/03/28 13:37:00]

Code removed.

+                           net::URLRequestContextGetter* request_context,
+                           const CheckAccessCallback& check_access_cb);
+
+ private:
+  // Issues a HTTP GET request for the device description. No-op if there is
+  // already a pending request.
+  void FetchDeviceDescription(const DialDeviceData& dial_device,
+                              net::URLRequestContextGetter* request_context);
+
+  // Checks the cache for a valid device description.  If the entry is found but
+  // is no longer valid, it is removed from the cache.
+  // |dial_device|: the device to look up.
+  // Returns nullptr if cache entry does not exist or is not valid.
+  bool CheckAndUpdateCache(const DialDeviceData& dial_device,
+                           DialDeviceDescription* out_description);
+
+  // Processes the result of getting device description and adds the device
+  // description to the cache.
+  // TODO(imcheng): Also cache permanent failures (b/32976125).
+  // |dial_device|: Device data from device discovery.
+  // |xml_text|: The device description.
+  // |app_url|: The application URL.
+  // Returns true and populates corresponding fields in |description| if
+  // processing succeeds; Returns false if processing fails.
+  bool ProcessDeviceDescription(const DialDeviceData& dial_device,

[mark a. foltz, 2017/03/11 00:08:15]

ISTM that this could take just the DialDeviceDescription* pre-populated with the
data in |dial_device|.

[zhaobin, 2017/04/10 18:44:42]

Done.

+                                const std::string& xmlText,
+                                const GURL& app_url,
+                                DialDeviceDescription* description);
+
+  // Parses a device description document into a DialDeviceDescription object.
+  // |dial_device|: Device data from device discovery.
+  // |xml_text|: The device description.
+  // |app_url|: The application URL.
+  // Returns true and populates corresponding fields in |description| if parsing
+  // succeeds; Returns false if parsing fails.
+  bool ParseDeviceDescription(const DialDeviceData& dial_device,

[mark a. foltz, 2017/03/11 00:08:16]

Nit: It seems like this could be written to only need |description| and
|xmlText|.  By parsing |xmlText| it can fill in missing pieces of |description|.

[zhaobin, 2017/03/28 13:37:00]

Code removed.

+                              const std::string& xml_text,
+                              const GURL& app_url,
+                              DialDeviceDescription* description);
+
+  // Invoked when HTTP GET request finishes.
+  // |dial_device|: Device data initiating the HTTP request.
+  // |description_data|: Response from HTTP request.
+  void OnDeviceDescriptionAvailable(

[mark a. foltz, 2017/03/11 00:08:16]

OnDeviceDescriptionFetchComplete?

[zhaobin, 2017/03/28 13:37:00]

Done.

+      const DialDeviceData& dial_device,
+      const DialDeviceDescriptionData& description_data);
+
+  // Invoked when HTTP GET request fails.
+  // |dial_device|: Device data initiating the HTTP request.
+  void OnDeviceDescriptionFetchError(const DialDeviceData& dial_device,
+                                     const std::string& error_message);
+
+  // Invoked when HTTP GET request finishes.

[mark a. foltz, 2017/03/11 00:08:15]

I'd like to think a bit about the design that requires this functionality.  It's
a bit fuzzy from memory, but here is what I recall:

It takes a while to detect when a DIAL device is no longer accessible (say, you
turned off your TV).  The SSDP part of discovery has to fail to find the device
twice, so there's a four minute delay.  So every time we get a list of devices
from SSDP, we spammed everything we know about with a GET to make sure they were
still there, and removed the ones that didn't respond.

There's a second use case possible, where the user has changed their proxy or
other network setting, so they could use the device before but can't now.  But
that feels like more of a corner case.

Since we are now bringing SSDP into the browser, it might not be necessary to
spam devices with GET requests just to figure out if they are still there. 
You've taken a look at this code most recently, so helping us understand the
current behavior would be great.

[zhaobin, 2017/03/28 13:37:00]

Removed for initial impl.

+  void OnCheckAccessSucceeded(
+      const CheckAccessCallback& check_access_cb,
+      const DialDeviceDescriptionData& description_data);
+
+  // Invoked when HTTP GET request fails.
+  void OnCheckAccessError(const CheckAccessCallback& check_access_cb,
+                          const std::string& error_message);
+
+  // device label, DeviceDescriptionFetcher
+  std::map<std::string,
+           std::unique_ptr<extensions::api::dial::DeviceDescriptionFetcher>>
+      device_description_fetcher_map_;
+
+  // device label, DeviceDescriptionFetcher
+  std::map<GURL,

[mark a. foltz, 2017/03/11 00:08:15]

Is this keyed by the device description URL?

I wonder if we need two maps here?  From the device_label it's possible to look
up the URL.

[zhaobin, 2017/03/28 13:37:00]

Code removed.

+           std::unique_ptr<extensions::api::dial::DeviceDescriptionFetcher>>
+      pending_access_requests_;
+
+  // device label, DialDeviceDescription
+  std::map<std::string, DialDeviceDescription> description_map_;
+
+  // This class does not take ownership of |observer_|
+  Observer* observer_;

[mark a. foltz, 2017/03/11 00:08:16]

A singleton observer would usually be called a Delegate or Callbacks, at least
in code I've seen.

[zhaobin, 2017/04/10 18:44:42]

Done.

+};
+
+}  // namespace media_router
+
+#endif  // CHROME_BROWSER_MEDIA_ROUTER_DEVICE_DESCRIPTION_SERVICE_H_