|
|
Created:
3 years, 10 months ago by zhaobin Modified:
3 years, 7 months ago CC:
chfremer+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org, feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org, media-router+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Media Router] Add DialMediaSinkService and DeviceDescriptionService
Discover process:
DialMediaSinkService.Start() registers itself with DialRegistry
DialMediaSinkService.OnDialDeviceEvent() gets invoked when device data comes back, and starts a 3s timer
Start description fetches for each device
DeviceDescriptionService.OnDeviceDescriptionFetchComplete() gets invoked when device description comes back.
Starts an XML parser in utility process to parse device description XML
DialMediaSinkService.OnDeviceDescriptionAvailable() gets invoked when parsing in utility process finishes
Create MediaSinkInternal and store it in DialMediaSinkService's sink map
Invoke DialMediaSinkService.FetchCompleted() when timer expires
Design doc:
https://docs.google.com/a/chromium.org/document/d/1vLpUgp5mJi6KFaCV3HEMQEZYDKtbcGdwcKNADuzuLzw/edit?usp=sharing
BUG=687375
Review-Url: https://codereview.chromium.org/2701633002
Cr-Commit-Position: refs/heads/master@{#468251}
Committed: https://chromium.googlesource.com/chromium/src/+/a26ad25c4fd2cb7bbe286745d43e507dc658ba2c
Patch Set 1 #
Total comments: 14
Patch Set 2 : Add DialMediaSinkCacheService and unit test #
Total comments: 32
Patch Set 3 : merge with master #Patch Set 4 : rebase and resolve some code review comments #Patch Set 5 : resolve code review comments from Mark cont #
Total comments: 118
Patch Set 6 : resolve code review comments from Mark and Derek #
Total comments: 46
Patch Set 7 : resolve code review comments from Mark and add device_description_service_unittest #Patch Set 8 : merge with master #
Total comments: 61
Patch Set 9 : resolve code review comments from Derek and Mark #
Total comments: 13
Patch Set 10 : resolve code review comments from Derek #Patch Set 11 : fix linux compile errors #Patch Set 12 : fix unit tests #Patch Set 13 : fix chromeos compile error #Messages
Total messages: 52 (31 generated)
zhaobin@chromium.org changed reviewers: + imcheng@chromium.org, mfoltz@chromium.org
This is a *lot* of new code and it looks like a fairly straight port from the existing script. That's a good starting point, but this could take several rounds of review to understand all of the design tradeoffs here and what makes sense from a browser-based implementation. Maybe you could set up some time to give an overview of the change and allow reviewers to give high-level feedback? I'll reply with high level comments on part of the change for now. https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router... File chrome/browser/media/router/device_description_service.h (right): https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router... chrome/browser/media/router/device_description_service.h:16: #include "chrome/browser/extensions/api/dial/dial_registry.h" I wouldn't introduce a MR -> extensions dependency; instead suggest creating a media/router/dial folder to hold dial_registry, device_description_fetcher, this class, and other pieces shared between the extensions API and the MR. Maybe discuss code location with imcheng@ and come up with a proposal? https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router... chrome/browser/media/router/device_description_service.h:33: struct DialDeviceDescription { We'll now have this struct, and DialDeviceData and DialDeviceDescriptionData that is used by extensions:: I wonder if they can be combined somehow? https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router... chrome/browser/media/router/device_description_service.h:38: std::string unique_id; Please document the meanings of these fields. https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router... chrome/browser/media/router/device_description_service.h:45: std::string device_type; Should this be an enum? https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router... File chrome/browser/media/router/dial_media_sink_service.h (right): https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router... chrome/browser/media/router/dial_media_sink_service.h:35: content::BrowserContext* browser_context); Is this a profile-keyed service? Note that we have one MR instance for both normal and incognito profiles - if this is 1:1 with the Media Router, then make sure that no behavior is dependent on the profile type. https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router... chrome/browser/media/router/dial_media_sink_service.h:71: DialRegistry* dial_registry_; The DialRegistry should become refcounted, since I can't think of a reason to have more than one per browser process; they will all see the same devices. https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router... chrome/browser/media/router/dial_media_sink_service.h:75: net::URLRequestContextGetter* request_context_; This is usually passed directly to the URLFetcher before starting a request - is there a reason to keep a pointer to it? https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/BUILD.gn (right): https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/BUILD.gn:18: "//third_party/libxml", Per our chat, I think we'll have to wait until there is a utility process available to do XML parsing. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/device_description_service.h (right): https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:34: DialDeviceDescription(); 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. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:36: DialDeviceDescription(const DialDeviceDescription& other); Should there be a default assignment operator assigned as well? https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:41: std::string ip_address; net::IPAddress https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:53: using DialRegistry = extensions::api::dial::DialRegistry; 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. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:54: using DialDeviceDescriptionData = 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. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:64: const std::string& device_id, Is this a device_label or unique_id? Can you rename to be specific? https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:81: virtual bool GetDeviceDescription( 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)? https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:87: virtual bool MayStopDeviceDescriptionFetching( It sounds like it could just be CancelFetch(device_label)? https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:95: virtual void CheckAccess(const GURL& device_description_url, Should this take DialDeviceData& like GetDeviceDescription? https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:120: bool ProcessDeviceDescription(const DialDeviceData& dial_device, ISTM that this could take just the DialDeviceDescription* pre-populated with the data in |dial_device|. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:131: bool ParseDeviceDescription(const DialDeviceData& dial_device, 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|. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:139: void OnDeviceDescriptionAvailable( OnDeviceDescriptionFetchComplete? https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:148: // Invoked when HTTP GET request finishes. 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. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:163: std::map<GURL, 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. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:171: Observer* observer_; A singleton observer would usually be called a Delegate or Callbacks, at least in code I've seen.
Resolved some code review comments from Mark. https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router... File chrome/browser/media/router/device_description_service.h (right): https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router... chrome/browser/media/router/device_description_service.h:16: #include "chrome/browser/extensions/api/dial/dial_registry.h" On 2017/03/11 00:08:15, mark a. foltz wrote: > I wouldn't introduce a MR -> extensions dependency; instead suggest creating a > media/router/dial folder to hold dial_registry, device_description_fetcher, this > class, and other pieces shared between the extensions API and the MR. > > Maybe discuss code location with imcheng@ and come up with a proposal? Done. https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router... chrome/browser/media/router/device_description_service.h:38: std::string unique_id; On 2017/03/11 00:08:15, mark a. foltz wrote: > Please document the meanings of these fields. Done. https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router... chrome/browser/media/router/device_description_service.h:45: std::string device_type; On 2017/03/11 00:08:15, mark a. foltz wrote: > Should this be an enum? Seems like a string according to spec...http://upnp.org/specs/arch/UPnP-arch-DeviceArchitecture-v2.0.pdf https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router... File chrome/browser/media/router/dial_media_sink_service.h (right): https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router... chrome/browser/media/router/dial_media_sink_service.h:71: DialRegistry* dial_registry_; On 2017/03/11 00:08:15, mark a. foltz wrote: > The DialRegistry should become refcounted, since I can't think of a reason to > have more than one per browser process; they will all see the same devices. DialRegistry is a leaky singleton now. Raw pointer ok? https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/BUILD.gn (right): https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/BUILD.gn:18: "//third_party/libxml", On 2017/03/11 00:08:15, mark a. foltz wrote: > Per our chat, I think we'll have to wait until there is a utility process > available to do XML parsing. Done. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/device_description_service.h (right): https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:34: DialDeviceDescription(); On 2017/03/11 00:08:15, mark a. foltz wrote: > 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. Done. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:36: DialDeviceDescription(const DialDeviceDescription& other); On 2017/03/11 00:08:15, mark a. foltz wrote: > Should there be a default assignment operator assigned as well? Done. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:41: std::string ip_address; On 2017/03/11 00:08:15, mark a. foltz wrote: > net::IPAddress Done. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:53: using DialRegistry = extensions::api::dial::DialRegistry; On 2017/03/11 00:08:16, mark a. foltz wrote: > 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. Done. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:64: const std::string& device_id, On 2017/03/11 00:08:15, mark a. foltz wrote: > Is this a device_label or unique_id? Can you rename to be specific? Done. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:87: virtual bool MayStopDeviceDescriptionFetching( On 2017/03/11 00:08:16, mark a. foltz wrote: > It sounds like it could just be CancelFetch(device_label)? Done. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:95: virtual void CheckAccess(const GURL& device_description_url, On 2017/03/11 00:08:15, mark a. foltz wrote: > Should this take DialDeviceData& like GetDeviceDescription? > Code removed. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:131: bool ParseDeviceDescription(const DialDeviceData& dial_device, On 2017/03/11 00:08:16, mark a. foltz wrote: > 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|. Code removed. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:139: void OnDeviceDescriptionAvailable( On 2017/03/11 00:08:16, mark a. foltz wrote: > OnDeviceDescriptionFetchComplete? Done. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:148: // Invoked when HTTP GET request finishes. On 2017/03/11 00:08:15, mark a. foltz wrote: > 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. Removed for initial impl. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:163: std::map<GURL, On 2017/03/11 00:08:15, mark a. foltz wrote: > 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. Code removed.
Description was changed from ========== [Media Router][WIP] Add DialMediaSinkService and DeviceDescriptionService Discover process: DialMediaSinkService.Start() registers itself with DialRegistry DialMediaSinkService.OnDialDeviceEvent() gets invoked when device data comes back Start description fetches for each device Invoke DialMediaSinkService.FetchCompleted() when all description fetches come back Rewriting device_description_service.js as DeviceDescriptionService TODO: Unit test Create TypedMediaSink instead of MediaSink Add timeout for FetchCompleted() BUG=687375 ========== to ========== [Media Router][WIP] Add DialMediaSinkService and DeviceDescriptionService Discover process: DialMediaSinkService.Start() registers itself with DialRegistry DialMediaSinkService.OnDialDeviceEvent() gets invoked when device data comes back Start description fetches for each device Invoke DialMediaSinkService.FetchCompleted() when all description fetches come back Rewriting device_description_service.js as DeviceDescriptionService TODO: Unit test for DeviceDescriptionService Design doc: https://docs.google.com/document/d/1lJbE4Oc9q1amkWEaD2ZL4mHEdRRlKHvwxUUcJmNC8... BUG=687375 ==========
https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router... File chrome/browser/media/router/device_description_service.h (right): https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router... chrome/browser/media/router/device_description_service.h:33: struct DialDeviceDescription { On 2017/03/11 00:08:15, mfoltz_ooo_until_4_10 wrote: > We'll now have this struct, and DialDeviceData and DialDeviceDescriptionData > that is used by extensions:: > > I wonder if they can be combined somehow? Created ParsedDialDeviceDescription struct with less fields. https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router... File chrome/browser/media/router/dial_media_sink_service.h (right): https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router... chrome/browser/media/router/dial_media_sink_service.h:35: content::BrowserContext* browser_context); On 2017/03/11 00:08:15, mfoltz_ooo_until_4_10 wrote: > Is this a profile-keyed service? > > Note that we have one MR instance for both normal and incognito profiles - if > this is 1:1 with the Media Router, then make sure that no behavior is dependent > on the profile type. We use profile to get request context for URL fetcher. https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router... chrome/browser/media/router/dial_media_sink_service.h:75: net::URLRequestContextGetter* request_context_; On 2017/03/11 00:08:15, mfoltz_ooo_until_4_10 wrote: > This is usually passed directly to the URLFetcher before starting a request - is > there a reason to keep a pointer to it? profile->GetRequestContext() should be called on UI thread. All DialMediaSinkService functions except ctor is called on IO thread. Store this to avoid posting tasks from IO thread to UI. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/device_description_service.h (right): https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:54: using DialDeviceDescriptionData = On 2017/03/11 00:08:15, mfoltz_ooo_until_4_10 wrote: > 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. Use ParsedDialDeviceDescription struct to store data from device description xml. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:81: virtual bool GetDeviceDescription( On 2017/03/11 00:08:15, mfoltz_ooo_until_4_10 wrote: > 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)? Done. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:120: bool ProcessDeviceDescription(const DialDeviceData& dial_device, On 2017/03/11 00:08:15, mfoltz_ooo_until_4_10 wrote: > ISTM that this could take just the DialDeviceDescription* pre-populated with the > data in |dial_device|. Done. https://codereview.chromium.org/2701633002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/device_description_service.h:171: Observer* observer_; On 2017/03/11 00:08:16, mfoltz_ooo_until_4_10 wrote: > A singleton observer would usually be called a Delegate or Callbacks, at least > in code I've seen. Done.
Made a pass - overall looks really good and the majority of my comments are minor documentation fixes. I did have a couple of more substantial questions/suggestions around the control flow for fetching/parsing. I will review the unit tests in the next pass. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/device_description_service.cc (right): https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:16: // TODO: Use value from chrome.dial when http://crbug.com/165289 is fixed. I'm going to close this as WontFix. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:24: size_t start_pos = pos + element_name.length() + 2; What happens if pos == std::string::npos? https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:32: // Returns false if <UDN> or <serialNumber> field does not exist in |xml_text|. This needs to be updated https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:48: if (!net::ParseURLHostnameToAddress(device_description_url.host(), &address)) This check could go earlier in DialService::ParseResponse. We should also check there that this IP address matches the address that we received the SSDP advertisement from. Feel free to add a TODO(crbug.com/679432) here. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:58: // Checks mandatory fields. Please document meaning of return value. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:77: static std::string ToString( If this is only use with D*LOG statements, use #ifndef NDEBUG around this function to remove it from release builds. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:88: << " [config_id]: "; Nit: Maybe only include this if config_id is set. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:115: // Keep fetcher if it is for |device_data.label()| and has the same Url. nit: URL https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:125: // Remove all out dated fetcheres. typo in fetchers https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:126: device_description_fetcher_map_ = std::move(existing_fetcher_map); Will this delete all fetchers whose labels are not included in |devices|? Is it safe to delete a fetcher even while the fetch is running? https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:172: // If the entry's configId does not match, or it has expired, remove it. nit: config_id https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:176: description_map_.erase(it); It looks like there will be some expired cached entries we never remove, if the devices are not passed to CheckAndUpdateCache. Do you want a garbage collection pass to run at some infrequent interval? https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:181: (*out_description) = it->second; It it necessary to make a copy here, or could CheckAndUpdateCache return a const pointer to CachedParsedDeviceDescription? I guess it depends on whether OnDeviceDescriptionFetchComplete can run before success_cb_ is finished handling the result. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:207: if (error.empty()) { Nit: I would reverse the two cases of the if() and return early, so there isn't such a long block below. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:216: DVLOG(2) << "Got device description for " << device_data.label(); Nit: These can be combined into one statement. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:222: // timeout to eventually update the cache. This would be fine with me. The only piece of data we really need from the device description is the friendly name and app URL. Nearly all devices hard code these, and they are unlikely to change over the lifetime of the device. We could set a fairly long cache timeout, say 12 hours. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:225: description_map_.insert( Can we set a maximum size on the number of cached entries? Say, 256? Most users would never hit it, but the limit would prevent this from consuming arbitrary amounts of memory even if something on the network was advertising a large number of devices. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:241: scoped_refptr<SafeDialDeviceDescriptionParser> parser( What keeps the parser alive after calling Start()? It doesn't appear to be bound in the callback. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:242: new SafeDialDeviceDescriptionParser()); Could this potentially spawn multiple utility processes? We probably want to spawn one process and feed it XML serially to avoid spawning several processes in parallel, if multiple devices are discovered at once. One thought: Since the SafeDialDescriptionParser is ref counted, you could create a scoped_refptr for each description that needs to be parsed, so that it is destroyed when the last parse completes. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:246: base::Unretained(this), device_data, What prevents the callback from being run after |this| is deleted? https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/device_description_service.h (right): https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:26: struct CachedParsedDialDeviceDescription { Creating this as a private struct below, e.g. DeviceDescriptionService::CacheEntry, might save some typing. I don't feel strongly though. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:39: base::Optional<int32_t> config_id; It might be simpler to provide a default of -1 vs. using base::Optional, as valid config_id values are non-negative. I don't feel strongly though. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:41: // Underlying device description data from xml s/xml/XML./ https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:47: // Called if parsing device description xml in utility process succeeds, and Nit: s/xml/XML/ throughout as it's an abbreviation https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:72: net::URLRequestContextGetter* request_context); Document |request_context| https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:93: // |device_description_ptr|: Parsed devcie description from utility process. typo in device https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:100: // Invoked when HTTP GET request finishes. Maybe this (and the following method) should be grouped below FetchDeviceDescription. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_cache_service.cc (right): https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_cache_service.cc:43: DCHECK(extra_data.ip_address.AssignFromIPLiteral( Does this need to run in release builds? https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_cache_service.cc:48: cached_sink.last_update_time = base::Time::Now(); Maybe it would be simpler to track a per-sink expiration time? https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_cache_service.cc:56: sink_it->second = cached_sink; Do you think it would be simpler to assign fields in sink_it->second, versus having to create a cached_sink object to replace it, since not all of the fields will change? https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:15: // The maximum time a response is expected after a M-SEARCH request. Can you say a bit about why this particular value was chosen? https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:48: NOTIMPLEMENTED(); Does this need a TODO to go back and implement this? https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:52: NOTIMPLEMENTED(); Ditto https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:55: DialRegistry* DialMediaSinkService::dial_registry() { I'm not sure this method adds much. Is it overridden in unit tests? https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:72: DialMediaSinkCacheService* DialMediaSinkService::cache_service() { If this is creating the cache service on demand, it should be named GetCacheService() and document that it does so, since it isn't a simple getter. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:94: description_service()->GetDeviceDescriptions(devices, request_context_); What happens to device descriptions fetched after finish_timer_ fires? https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:119: if (!IsDifferent(sinks, mrp_sinks_)) { Can this just be !(sinks == mrp_sinks_), if we enforce that |sinks| is sorted in a deterministic order by unique_id? https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.h (right): https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:61: // Returns true if |new_sink| is the same as |old_sinks|. s/new_sink/new_sinks/ The comment and the name of the method don't match up. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/parsed_dial_device_description.h (right): https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/parsed_dial_device_description.h:27: // Short user-friendly title. s/title/device name/ ? https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/parsed_dial_device_description.h:30: // Model name. Device model name https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/parsed_dial_device_description.h:33: // The reported device type, e.g. urn:dial-multiscreen-org:device:dial:1 Won't this always be the same for DIAL devices? I wonder if we need to include this field, or just check that it matches the constant string when the device is discovered. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/parsed_dial_device_description.h:36: // The application URL. The DIAL application URL (used to launch DIAL applications). https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/media_sink_internal.cc (right): https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/media_sink_internal.cc:68: return !operator==(other); Nice :) I did not know this trick and would have written !(*this == other) https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/media_sink_internal.cc:165: DCHECK(this->ip_address.AssignFromIPLiteral(ip_address)); This won't be called in release builds, and I feel like we should not check in unit tests that are guaranteed to fail in release builds.
https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/device_description_service.cc (right): https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:21: void Scrub(const std::string& element_name, std::string& xml_text) { nit: |xml_text| should be passed in as a pointer. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:43: static bool IsValidDeviceDescriptionUrl(const GURL& device_description_url) { Functions in anonymous namespace do not need to be static. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:59: static std::string Validate( Maybe ValidateParsedDeviceDescription to be more clear? https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:63: return "Missing uniqueId"; IMO, enum is slightly preferable to strings for indicating error reasons. It's slightly more impact and can also be used for UMA. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:77: static std::string ToString( Naming suggestion: CachedDeviceDescriptionToString https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:108: const std::vector<DialDeviceData>& devices, Is the idea that this gets called with the full device list known to DialRegistry? I think it makes sense to abandon pending fetches where the device description was updated. I think we can keep pending fetches that aren't on the list though. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:174: base::Time::Now() >= it->second.expire_time_millis) { General comment: for tests you might find it useful to place base::Time::Now() inside a virtual GetNow() method. Then in tests you can override that method to return a fake time. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:249: std::string xml_logging = Similar to above, we want to guard this with #ifndef NDEBUG. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/device_description_service.h (right): https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:33: base::Time fetch_time_millis; Omit the millis / ms part in the name / comments since the unit is encapsulated within base::Time? Here and below. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:45: class DeviceDescriptionService { Please document threading assumptions when providing documentation for this class. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:58: base::Callback<void(const std::string& device_label, Why does this only return the device label only vs. DialDeviceData itself in the success callback, or vice versa? https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:61: DeviceDescriptionService(DeviceDescriptionParseSuccessCallback success_cb, const ref for both args https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_cache_service.cc (right): https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_cache_service.cc:39: MediaSink::IconType::CAST); Should IconType be GENERIC? https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_cache_service.cc:59: bool DialMediaSinkCacheService::IsAlive(const CachedDialMediaSink& sink) { It seems we never erase from cached_sinks_; should there be a periodic mechanism to remove ones that are no longer accessible (e.g, by issuing a GET request and checking if it succeeds) based on last_update_time? https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:86: network_disconnected_ = false; This field doesn't seem to be used.
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:120001) has been deleted
- Remove DialMediaSinkCache service (ParsedDeviceDescription and MediaSinkInternal seems equivalent, no need to cache both). - Revise fetching device description logic: 1. Start a 3s oneShotTimer to invoke FetchCompleted() after start discovery; 2. Start a 3s oneShotTimer to invoke FetchCompleted() when new device descripton becomes available if previous timer has expired. The other option would be a repeating timer to poll current sink status every 30s (or other intervals). We have relative small number of devices, and they tend to be stable, it seems more efficient to trigger FetchCompleted() on device descriptions instead of on periodic timer - Start one utility process at a time and parse multiple device description XMLs in the same utility process. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/device_description_service.cc (right): https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:16: // TODO: Use value from chrome.dial when http://crbug.com/165289 is fixed. On 2017/04/12 00:17:17, mark a. foltz wrote: > I'm going to close this as WontFix. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:21: void Scrub(const std::string& element_name, std::string& xml_text) { On 2017/04/12 19:33:50, imcheng wrote: > nit: |xml_text| should be passed in as a pointer. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:24: size_t start_pos = pos + element_name.length() + 2; On 2017/04/12 00:17:17, mark a. foltz wrote: > What happens if pos == std::string::npos? Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:32: // Returns false if <UDN> or <serialNumber> field does not exist in |xml_text|. On 2017/04/12 00:17:17, mark a. foltz wrote: > This needs to be updated Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:43: static bool IsValidDeviceDescriptionUrl(const GURL& device_description_url) { On 2017/04/12 19:33:50, imcheng wrote: > Functions in anonymous namespace do not need to be static. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:48: if (!net::ParseURLHostnameToAddress(device_description_url.host(), &address)) On 2017/04/12 00:17:17, mark a. foltz wrote: > This check could go earlier in DialService::ParseResponse. We should also check > there that this IP address matches the address that we received the SSDP > advertisement from. Feel free to add a TODO(crbug.com/679432) here. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:58: // Checks mandatory fields. On 2017/04/12 00:17:17, mark a. foltz wrote: > Please document meaning of return value. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:59: static std::string Validate( On 2017/04/12 19:33:50, imcheng wrote: > Maybe ValidateParsedDeviceDescription to be more clear? Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:63: return "Missing uniqueId"; On 2017/04/12 19:33:50, imcheng wrote: > IMO, enum is slightly preferable to strings for indicating error reasons. It's > slightly more impact and can also be used for UMA. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:77: static std::string ToString( On 2017/04/12 19:33:50, imcheng wrote: > Naming suggestion: CachedDeviceDescriptionToString Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:77: static std::string ToString( On 2017/04/12 00:17:17, mark a. foltz wrote: > If this is only use with D*LOG statements, use #ifndef NDEBUG around this > function to remove it from release builds. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:88: << " [config_id]: "; On 2017/04/12 00:17:17, mark a. foltz wrote: > Nit: Maybe only include this if config_id is set. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:108: const std::vector<DialDeviceData>& devices, On 2017/04/12 19:33:50, imcheng wrote: > Is the idea that this gets called with the full device list known to > DialRegistry? > > I think it makes sense to abandon pending fetches where the device description > was updated. I think we can keep pending fetches that aren't on the list though. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:115: // Keep fetcher if it is for |device_data.label()| and has the same Url. On 2017/04/12 00:17:17, mark a. foltz wrote: > nit: URL Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:125: // Remove all out dated fetcheres. On 2017/04/12 00:17:17, mark a. foltz wrote: > typo in fetchers Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:126: device_description_fetcher_map_ = std::move(existing_fetcher_map); On 2017/04/12 00:17:17, mark a. foltz wrote: > Will this delete all fetchers whose labels are not included in |devices|? > > Is it safe to delete a fetcher even while the fetch is running? Seems to be safe. net::URLFetcher says "You may cancel the request by destroying the URLFetcher" (https://cs.chromium.org/chromium/src/net/url_request/url_fetcher.h?l=77). It also says no callbacks will occur after cancelling. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:172: // If the entry's configId does not match, or it has expired, remove it. On 2017/04/12 00:17:17, mark a. foltz wrote: > nit: config_id Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:174: base::Time::Now() >= it->second.expire_time_millis) { On 2017/04/12 19:33:50, imcheng wrote: > General comment: for tests you might find it useful to place base::Time::Now() > inside a virtual GetNow() method. Then in tests you can override that method to > return a fake time. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:176: description_map_.erase(it); On 2017/04/12 00:17:17, mark a. foltz wrote: > It looks like there will be some expired cached entries we never remove, if the > devices are not passed to CheckAndUpdateCache. Do you want a garbage collection > pass to run at some infrequent interval? https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:181: (*out_description) = it->second; On 2017/04/12 00:17:17, mark a. foltz wrote: > It it necessary to make a copy here, or could CheckAndUpdateCache return a const > pointer to CachedParsedDeviceDescription? I guess it depends on whether > OnDeviceDescriptionFetchComplete can run before success_cb_ is finished handling > the result. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:207: if (error.empty()) { On 2017/04/12 00:17:17, mark a. foltz wrote: > Nit: I would reverse the two cases of the if() and return early, so there isn't > such a long block below. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:216: DVLOG(2) << "Got device description for " << device_data.label(); On 2017/04/12 00:17:17, mark a. foltz wrote: > Nit: These can be combined into one statement. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:222: // timeout to eventually update the cache. On 2017/04/12 00:17:17, mark a. foltz wrote: > This would be fine with me. The only piece of data we really need from the > device description is the friendly name and app URL. Nearly all devices hard > code these, and they are unlikely to change over the lifetime of the device. We > could set a fairly long cache timeout, say 12 hours. Acknowledged. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:225: description_map_.insert( On 2017/04/12 00:17:17, mark a. foltz wrote: > Can we set a maximum size on the number of cached entries? Say, 256? Most > users would never hit it, but the limit would prevent this from consuming > arbitrary amounts of memory even if something on the network was advertising a > large number of devices. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:241: scoped_refptr<SafeDialDeviceDescriptionParser> parser( On 2017/04/12 00:17:17, mark a. foltz wrote: > What keeps the parser alive after calling Start()? It doesn't appear to be > bound in the callback. It is bound insdie parser->start() (https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro...) https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:242: new SafeDialDeviceDescriptionParser()); On 2017/04/12 00:17:17, mark a. foltz wrote: > Could this potentially spawn multiple utility processes? We probably want to > spawn one process and feed it XML serially to avoid spawning several processes > in parallel, if multiple devices are discovered at once. > > One thought: Since the SafeDialDescriptionParser is ref counted, you could > create a scoped_refptr for each description that needs to be parsed, so that it > is destroyed when the last parse completes. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:246: base::Unretained(this), device_data, On 2017/04/12 00:17:17, mark a. foltz wrote: > What prevents the callback from being run after |this| is deleted? (I think) |this| is owned by DialMediaSinkService, which will be owned by MediaRouter (BrowserContextKeyedService). Since browser context should outlive utility process, |this| will always be valid when callback is invoked by utility process. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.cc:249: std::string xml_logging = On 2017/04/12 19:33:50, imcheng wrote: > Similar to above, we want to guard this with #ifndef NDEBUG. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/device_description_service.h (right): https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:26: struct CachedParsedDialDeviceDescription { On 2017/04/12 00:17:17, mark a. foltz wrote: > Creating this as a private struct below, e.g. > DeviceDescriptionService::CacheEntry, might save some typing. I don't feel > strongly though. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:33: base::Time fetch_time_millis; On 2017/04/12 19:33:50, imcheng wrote: > Omit the millis / ms part in the name / comments since the unit is encapsulated > within base::Time? Here and below. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:39: base::Optional<int32_t> config_id; On 2017/04/12 00:17:18, mark a. foltz wrote: > It might be simpler to provide a default of -1 vs. using base::Optional, as > valid config_id values are non-negative. I don't feel strongly though. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:41: // Underlying device description data from xml On 2017/04/12 00:17:18, mark a. foltz wrote: > s/xml/XML./ Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:45: class DeviceDescriptionService { On 2017/04/12 19:33:50, imcheng wrote: > Please document threading assumptions when providing documentation for this > class. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:47: // Called if parsing device description xml in utility process succeeds, and On 2017/04/12 00:17:18, mark a. foltz wrote: > Nit: s/xml/XML/ throughout as it's an abbreviation Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:58: base::Callback<void(const std::string& device_label, On 2017/04/12 19:33:50, imcheng wrote: > Why does this only return the device label only vs. DialDeviceData itself in the > success callback, or vice versa? Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:61: DeviceDescriptionService(DeviceDescriptionParseSuccessCallback success_cb, On 2017/04/12 19:33:50, imcheng wrote: > const ref for both args Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:72: net::URLRequestContextGetter* request_context); On 2017/04/12 00:17:18, mark a. foltz wrote: > Document |request_context| Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:93: // |device_description_ptr|: Parsed devcie description from utility process. On 2017/04/12 00:17:17, mark a. foltz wrote: > typo in device Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/device_description_service.h:100: // Invoked when HTTP GET request finishes. On 2017/04/12 00:17:18, mark a. foltz wrote: > Maybe this (and the following method) should be grouped below > FetchDeviceDescription. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_cache_service.cc (right): https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_cache_service.cc:39: MediaSink::IconType::CAST); On 2017/04/12 19:33:51, imcheng wrote: > Should IconType be GENERIC? Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_cache_service.cc:43: DCHECK(extra_data.ip_address.AssignFromIPLiteral( On 2017/04/12 00:17:18, mark a. foltz wrote: > Does this need to run in release builds? Yes. Changed to if. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_cache_service.cc:48: cached_sink.last_update_time = base::Time::Now(); On 2017/04/12 00:17:18, mark a. foltz wrote: > Maybe it would be simpler to track a per-sink expiration time? Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_cache_service.cc:56: sink_it->second = cached_sink; On 2017/04/12 00:17:18, mark a. foltz wrote: > Do you think it would be simpler to assign fields in sink_it->second, versus > having to create a cached_sink object to replace it, since not all of the fields > will change? Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_cache_service.cc:59: bool DialMediaSinkCacheService::IsAlive(const CachedDialMediaSink& sink) { On 2017/04/12 19:33:51, imcheng wrote: > It seems we never erase from cached_sinks_; should there be a periodic mechanism > to remove ones that are no longer accessible (e.g, by issuing a GET request and > checking if it succeeds) based on last_update_time? I have removed all CheckAccess() related logic here to make this patch simple (still quite large though)...Add a TODO for check access? https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:15: // The maximum time a response is expected after a M-SEARCH request. On 2017/04/12 00:17:18, mark a. foltz wrote: > Can you say a bit about why this particular value was chosen? It is picked at random and needs to be revised. (I will close MR dialog if it keeps spinning for more than 3 seconds...) https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:48: NOTIMPLEMENTED(); On 2017/04/12 00:17:18, mark a. foltz wrote: > Does this need a TODO to go back and implement this? Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:52: NOTIMPLEMENTED(); On 2017/04/12 00:17:18, mark a. foltz wrote: > Ditto Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:55: DialRegistry* DialMediaSinkService::dial_registry() { On 2017/04/12 00:17:18, mark a. foltz wrote: > I'm not sure this method adds much. Is it overridden in unit tests? Yes, for unit tests. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:72: DialMediaSinkCacheService* DialMediaSinkService::cache_service() { On 2017/04/12 00:17:18, mark a. foltz wrote: > If this is creating the cache service on demand, it should be named > GetCacheService() and document that it does so, since it isn't a simple getter. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:86: network_disconnected_ = false; On 2017/04/12 19:33:51, imcheng wrote: > This field doesn't seem to be used. Code removed. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:94: description_service()->GetDeviceDescriptions(devices, request_context_); On 2017/04/12 00:17:18, mark a. foltz wrote: > What happens to device descriptions fetched after finish_timer_ fires? They go into CacheService, and will be returned in next FetchCompleted() if they are still alive by then. Start the timer again for such descriptions... https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:119: if (!IsDifferent(sinks, mrp_sinks_)) { On 2017/04/12 00:17:18, mark a. foltz wrote: > Can this just be !(sinks == mrp_sinks_), if we enforce that |sinks| is sorted in > a deterministic order by unique_id? Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.h (right): https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:61: // Returns true if |new_sink| is the same as |old_sinks|. On 2017/04/12 00:17:18, mark a. foltz wrote: > s/new_sink/new_sinks/ > > The comment and the name of the method don't match up. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/parsed_dial_device_description.h (right): https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/parsed_dial_device_description.h:27: // Short user-friendly title. On 2017/04/12 00:17:18, mark a. foltz wrote: > s/title/device name/ ? Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/parsed_dial_device_description.h:30: // Model name. On 2017/04/12 00:17:18, mark a. foltz wrote: > Device model name Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/parsed_dial_device_description.h:33: // The reported device type, e.g. urn:dial-multiscreen-org:device:dial:1 On 2017/04/12 00:17:18, mark a. foltz wrote: > Won't this always be the same for DIAL devices? I wonder if we need to include > this field, or just check that it matches the constant string when the device is > discovered. Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/parsed_dial_device_description.h:36: // The application URL. On 2017/04/12 00:17:18, mark a. foltz wrote: > The DIAL application URL (used to launch DIAL applications). Done. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/media_sink_internal.cc (right): https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/media_sink_internal.cc:68: return !operator==(other); On 2017/04/12 00:17:18, mark a. foltz wrote: > Nice :) I did not know this trick and would have written !(*this == other) Acknowledged. https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/media_sink_internal.cc:165: DCHECK(this->ip_address.AssignFromIPLiteral(ip_address)); On 2017/04/12 00:17:18, mark a. foltz wrote: > This won't be called in release builds, and I feel like we should not check in > unit tests that are guaranteed to fail in release builds. Code removed.
Most of my comments are minor documentation suggestions - one question about the utility process lifetime. I'll start a thread on how aggressively we should cache the descriptions. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/device_description_service.cc (right): https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:25: const int kDeviceDescriptionCacheTimeMillis = 30 * 60 * 1000; Per my earlier comment this could be much longer IMO. We can be conservative for now, but I would like to see this addressed. I can start an email thread. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:31: const int kCacheLimit = 256; kCacheMaxEntries ? https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:60: std::stringstream ss; #ifndef NDEBUG #include <sstream> #endif https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:155: // check if expired device descriptions are still alive. This is probably okay - if the device is found through discovery again, it will repopulate the cache entry. So this TODO doesn't seem necessary. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:164: DVLOG(2) << "After clean up, cache size: " << description_map_.size(); If the cache is emptied and there are no pending requests, you can cancel the repeated timer. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:167: base::Time DeviceDescriptionService::GetNow() { Is this for unit testing? https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:217: if (parser_ && parser_->GetPendingParsingRequests() == 0) For a given list of devices, is it possible for one parse to complete before the next one starts (because a HTTP request returned late)? We may want to keep the process alive until all fetches return, or move this check to the three second timer in dial_media_sink_service. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:243: base::TimeDelta::FromMilliseconds(kDeviceDescriptionCacheTimeMillis); Can you convert the cache timeout constant to use minutes (to be consistent with the timer interval constant)? https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:252: // NOTE: We could cache descriptions without a config id and rely on the Yes we should do this, to avoid repeatedly requesting descriptions from devices that don't set a config id. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:254: if (description_map_.size() < kCacheLimit && If the cache size limit has been hit, you can early return with description_data without having to set up a cache entry. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/device_description_service.h (right): https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:28: // This class fetches and parses device description xml for DIAL devices. It s/xml/XML/ https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:29: // lives on IO thread. You might mention that the parsing happens in a separate utility process via SafeDeviceDescriptionParser (instead of in this class). https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:42: // The version number. The device description version number (non-negative) https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:45: // Underlying device description data from XML. s/Underlying/Parsed/ https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:78: // Remove expired cache entry from |description_map_|. s/entry/entries/ https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:86: // is no longer valid, it is removed from the cache. Ruturns cached entry of s/is no longer valid/is expired/ Typo in Returns https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:94: // |device_data|: The device to look up. Nit: document |request_context| https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:106: // |device_data|: Device data initiating the HTTP request. Nit: document |error_message| https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:121: // Map of <device label, DeviceDescriptionFetcher> Document the meaning of the entries, e.g. "Map of current device description fetches in progress." https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:144: std::vector<MediaSinkInternal> vec_sinks(sinks.begin(), sinks.end()); Can this be inlined into Run()? https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.h (right): https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:42: // Returns instance of device description service. Create a new one if null s/null/none/ ? https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:67: void FetchCompleted(); OnFetchCompleted? https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_sink_service.h (right): https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_sink_service.h:28: base::Callback<void(const std::vector<MediaSinkInternal>&)>; So I thought that MediaSink would remain the public API the Media Router used to expose sinks to the rest of the browser, and MediaSinkInternal was for passing around the discovery code. Why was this changed here?
Patchset #7 (id:160001) has been deleted
Add unit tests for DeviceDescriptionService. Let DeviceDescriptionService control the lifetime of safe parser (utility process). Only reset when there is no pending parsing request. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/device_description_service.cc (right): https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:25: const int kDeviceDescriptionCacheTimeMillis = 30 * 60 * 1000; On 2017/04/18 18:16:26, mark a. foltz wrote: > Per my earlier comment this could be much longer IMO. We can be conservative > for now, but I would like to see this addressed. I can start an email thread. Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:31: const int kCacheLimit = 256; On 2017/04/18 18:16:26, mark a. foltz wrote: > kCacheMaxEntries ? Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:60: std::stringstream ss; On 2017/04/18 18:16:26, mark a. foltz wrote: > #ifndef NDEBUG > #include <sstream> > #endif Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:155: // check if expired device descriptions are still alive. On 2017/04/18 18:16:26, mark a. foltz wrote: > This is probably okay - if the device is found through discovery again, it will > repopulate the cache entry. So this TODO doesn't seem necessary. Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:164: DVLOG(2) << "After clean up, cache size: " << description_map_.size(); On 2017/04/18 18:16:26, mark a. foltz wrote: > If the cache is emptied and there are no pending requests, you can cancel the > repeated timer. Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:167: base::Time DeviceDescriptionService::GetNow() { On 2017/04/18 18:16:26, mark a. foltz wrote: > Is this for unit testing? Yes. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:217: if (parser_ && parser_->GetPendingParsingRequests() == 0) On 2017/04/18 18:16:26, mark a. foltz wrote: > For a given list of devices, is it possible for one parse to complete before the > next one starts (because a HTTP request returned late)? We may want to keep the > process alive until all fetches return, or move this check to the three second > timer in dial_media_sink_service. Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:243: base::TimeDelta::FromMilliseconds(kDeviceDescriptionCacheTimeMillis); On 2017/04/18 18:16:26, mark a. foltz wrote: > Can you convert the cache timeout constant to use minutes (to be consistent with > the timer interval constant)? Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:252: // NOTE: We could cache descriptions without a config id and rely on the On 2017/04/18 18:16:26, mark a. foltz wrote: > Yes we should do this, to avoid repeatedly requesting descriptions from devices > that don't set a config id. Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:254: if (description_map_.size() < kCacheLimit && On 2017/04/18 18:16:26, mark a. foltz wrote: > If the cache size limit has been hit, you can early return with description_data > without having to set up a cache entry. Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/device_description_service.h (right): https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:28: // This class fetches and parses device description xml for DIAL devices. It On 2017/04/18 18:16:26, mark a. foltz wrote: > s/xml/XML/ Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:29: // lives on IO thread. On 2017/04/18 18:16:26, mark a. foltz wrote: > You might mention that the parsing happens in a separate utility process via > SafeDeviceDescriptionParser (instead of in this class). Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:42: // The version number. On 2017/04/18 18:16:27, mark a. foltz wrote: > The device description version number (non-negative) Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:45: // Underlying device description data from XML. On 2017/04/18 18:16:26, mark a. foltz wrote: > s/Underlying/Parsed/ Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:78: // Remove expired cache entry from |description_map_|. On 2017/04/18 18:16:26, mark a. foltz wrote: > s/entry/entries/ Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:86: // is no longer valid, it is removed from the cache. Ruturns cached entry of On 2017/04/18 18:16:26, mark a. foltz wrote: > s/is no longer valid/is expired/ > > Typo in Returns Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:94: // |device_data|: The device to look up. On 2017/04/18 18:16:27, mark a. foltz wrote: > Nit: document |request_context| Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:106: // |device_data|: Device data initiating the HTTP request. On 2017/04/18 18:16:26, mark a. foltz wrote: > Nit: document |error_message| Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:121: // Map of <device label, DeviceDescriptionFetcher> On 2017/04/18 18:16:27, mark a. foltz wrote: > Document the meaning of the entries, e.g. "Map of current device description > fetches in progress." Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:144: std::vector<MediaSinkInternal> vec_sinks(sinks.begin(), sinks.end()); On 2017/04/18 18:16:27, mark a. foltz wrote: > Can this be inlined into Run()? Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.h (right): https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:42: // Returns instance of device description service. Create a new one if null On 2017/04/18 18:16:27, mark a. foltz wrote: > s/null/none/ ? Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:67: void FetchCompleted(); On 2017/04/18 18:16:27, mark a. foltz wrote: > OnFetchCompleted? Done. https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_sink_service.h (right): https://codereview.chromium.org/2701633002/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_sink_service.h:28: base::Callback<void(const std::vector<MediaSinkInternal>&)>; On 2017/04/18 18:16:27, mark a. foltz wrote: > So I thought that MediaSink would remain the public API the Media Router used to > expose sinks to the rest of the browser, and MediaSinkInternal was for passing > around the discovery code. Why was this changed here? This callback is used to pass MediaSinks from MR to MRP. Need to be MediaSinkInternal which contains DialMediaSinklExtraData.
https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/device_description_service.cc (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:29: const int kDeviceDescriptionCacheTimeMins = 12 * 60; Just 12 here? https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:29: const int kDeviceDescriptionCacheTimeMins = 12 * 60; constexpr here and below. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:140: device_description_fetcher_map_ = std::move(existing_fetcher_map); If we destroyed any old fetchers here, wouldn't be also have to remove the corresponding entry in pending_device_labels_? https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:169: return cache_pair.second.expire_time < now; It looks like the predicate is reversed? https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/device_description_service.h (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:82: void CleanUpCacheEntries(); Does this need to be public? https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:126: // Returns nullptr if parsing fails. ... process, or nullptr if parsing failed. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:154: content::BrowserThread::DeleteOnIOThread> Why do you need this? I thought this object lives on the IO thread already? https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/device_description_service_unittest.cc (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service_unittest.cc:27: void Start(const std::string& xml_text, Do you intend to mock out this method? https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:33: void DialMediaSinkService::Start() { The threading model isn't clear to me; we should clarify it in MediaSinkService's documentation. If MediaRouter is supposed to call Start() on the UI thread, then in here we need to PostTask to the IO thread. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:39: void DialMediaSinkService::Stop() { Is the plan to have MediaRouter call Stop() when shutting down? https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:117: base::TimeDelta::FromSeconds(kFetchCompleteTimeoutSecs); Seems like DelayTimer is better suited for this: https://cs.chromium.org/chromium/src/base/timer/timer.h?type=cs&q=f:base+butt... https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:143: sink_discovery_callback_.Run( We should be invoking the callback on the UI thread; use PostTask? https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.h (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:74: std::unique_ptr<DeviceDescriptionService> description_service_; Does this need DeleteOnIOThread? https://codereview.chromium.org/2701633002/diff/200001/chrome/common/media_ro... File chrome/common/media_router/discovery/media_sink_internal_unittest.h (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/common/media_ro... chrome/common/media_router/discovery/media_sink_internal_unittest.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. Can this be inlined in the .cc?
LGTM with remaining comments addressed - overall this looks like a really solid piece of work. The unit tests are really well written. It would be a good idea to double check that pending requests are cleaned up appropriately for both fetching and parsing device descriptions when a new set of devices is passed in or the service is shut down. At a latter point, can you add a README.md with a link to an updated design doc for DIAL discovery (you may need to copy it over to chromium.org to make it world readable)? Finally can you add some comments that MediaSinkInternal is for passing sinks from MR<-->MRP and not for exposure to MR clients in general. Maybe we can enforce this with DEPS - imcheng@, WDYT? Thanks again! https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/device_description_service.cc (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:29: const int kDeviceDescriptionCacheTimeMins = 12 * 60; On 2017/04/25 at 01:40:12, imcheng wrote: > Just 12 here? I assume you also meant s/Mins/Hours/? https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:140: device_description_fetcher_map_ = std::move(existing_fetcher_map); On 2017/04/25 at 01:40:12, imcheng wrote: > If we destroyed any old fetchers here, wouldn't be also have to remove the corresponding entry in pending_device_labels_? It looks like you could wait to insert the label into pending_device_labels_ until OnDeviceDescriptionFetchComplete? https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:235: error_cb_.Run(device_data, "Failed to parse device description xml"); nit: XML https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/device_description_service.h (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:144: std::map<std::string, CacheEntry> description_map_; Maybe description_cache_ ? https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:154: content::BrowserThread::DeleteOnIOThread> On 2017/04/25 at 01:40:12, imcheng wrote: > Why do you need this? I thought this object lives on the IO thread already? BrowserThread::DeleteOnThread has a shortcut that avoids posting a task to the same thread. So I don't think this is a big deal. https://cs.chromium.org/chromium/src/content/public/browser/browser_thread.h?... https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/device_description_service_unittest.cc (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service_unittest.cc:55: // Create Test Data These static methods to create test data could go into an anonymous namespace at the top level - just to keep this class focused on methods that manage the test maps. I don't feel strongly though. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service_unittest.cc:231: device_description_service()->OnDeviceDescriptionFetchError(device_data_2, Is this asserting that the original fetcher for |device_data_2| is erroring out when its URL got updated? Should the new fetcher created by GetDeviceDescriptions succeed? https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service_unittest.cc:250: EXPECT_EQ(size_t(1), description_map_.size()); Nit: Check that device 3 specifically is not removed from the cache. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:39: void DialMediaSinkService::Stop() { On 2017/04/25 at 01:40:13, imcheng wrote: > Is the plan to have MediaRouter call Stop() when shutting down? I would assume the MR would call Start/Stop depending on whether there are MediaSinkObservers for DIAL (to stay in sync with the discovery logic in the component). Does this service also need to run for media route discovery? https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:54: DialRegistry* DialMediaSinkService::dial_registry() { Does this need to be exposed through the API - what needs access to the DialRegistry? https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:6: #include "base/test/mock_callback.h" Are these #includes in the right order? https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:65: std::vector<MediaSinkInternal> CreateDialMediaSinks() { Similar comment about moving test data factory code to an anonymous namespace. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/mock_device_description_service.h (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/mock_device_description_service.h:15: class MockDeviceDescriptionService : public DeviceDescriptionService { Is this only used in DialMediaSinkServiceUnittest? Consider folding it into that file. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_sink_service.h (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_service.h:15: namespace media_router { Should this go in c/common/media/router/discovery/ ?
Note: Update patch description when you get a chance :)
Description was changed from ========== [Media Router][WIP] Add DialMediaSinkService and DeviceDescriptionService Discover process: DialMediaSinkService.Start() registers itself with DialRegistry DialMediaSinkService.OnDialDeviceEvent() gets invoked when device data comes back Start description fetches for each device Invoke DialMediaSinkService.FetchCompleted() when all description fetches come back Rewriting device_description_service.js as DeviceDescriptionService TODO: Unit test for DeviceDescriptionService Design doc: https://docs.google.com/document/d/1lJbE4Oc9q1amkWEaD2ZL4mHEdRRlKHvwxUUcJmNC8... BUG=687375 ========== to ========== [Media Router] Add DialMediaSinkService and DeviceDescriptionService Discover process: DialMediaSinkService.Start() registers itself with DialRegistry DialMediaSinkService.OnDialDeviceEvent() gets invoked when device data comes back, and starts a 3s timer Start description fetches for each device DeviceDescriptionService.OnDeviceDescriptionFetchComplete() gets invoked when device description comes back. Starts an XML parser in utility process to parse device description XML DialMediaSinkService.OnDeviceDescriptionAvailable() gets invoked when parsing in utility process finishes Create MediaSinkInternal and store it in DialMediaSinkService's sink map Invoke DialMediaSinkService.FetchCompleted() when timer expires Design doc: https://docs.google.com/document/d/1lJbE4Oc9q1amkWEaD2ZL4mHEdRRlKHvwxUUcJmNC8... BUG=687375 ==========
https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/device_description_service.cc (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:29: const int kDeviceDescriptionCacheTimeMins = 12 * 60; On 2017/04/25 01:40:12, imcheng wrote: > constexpr here and below. Done. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:29: const int kDeviceDescriptionCacheTimeMins = 12 * 60; On 2017/04/25 01:40:12, imcheng wrote: > Just 12 here? Done. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:29: const int kDeviceDescriptionCacheTimeMins = 12 * 60; On 2017/04/25 21:06:25, mark a. foltz wrote: > On 2017/04/25 at 01:40:12, imcheng wrote: > > Just 12 here? > > I assume you also meant s/Mins/Hours/? Acknowledged. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:140: device_description_fetcher_map_ = std::move(existing_fetcher_map); On 2017/04/25 01:40:12, imcheng wrote: > If we destroyed any old fetchers here, wouldn't be also have to remove the > corresponding entry in pending_device_labels_? Discussed with Derek offline. We do not need to remove entries from |pending_device_labels|. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:140: device_description_fetcher_map_ = std::move(existing_fetcher_map); On 2017/04/25 21:06:25, mark a. foltz wrote: > On 2017/04/25 at 01:40:12, imcheng wrote: > > If we destroyed any old fetchers here, wouldn't be also have to remove the > corresponding entry in pending_device_labels_? > > It looks like you could wait to insert the label into pending_device_labels_ > until OnDeviceDescriptionFetchComplete? Need to insert it here, so utility process is kept alive for current list of devices. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:169: return cache_pair.second.expire_time < now; On 2017/04/25 01:40:12, imcheng wrote: > It looks like the predicate is reversed? Seems ok? If exprie time < now(), the entry is expired and removed. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:235: error_cb_.Run(device_data, "Failed to parse device description xml"); On 2017/04/25 21:06:25, mark a. foltz wrote: > nit: XML Done. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/device_description_service.h (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:82: void CleanUpCacheEntries(); On 2017/04/25 01:40:12, imcheng wrote: > Does this need to be public? Done. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:126: // Returns nullptr if parsing fails. On 2017/04/25 01:40:12, imcheng wrote: > ... process, or nullptr if parsing failed. Done. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:144: std::map<std::string, CacheEntry> description_map_; On 2017/04/25 21:06:25, mark a. foltz wrote: > Maybe description_cache_ ? Done. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:154: content::BrowserThread::DeleteOnIOThread> On 2017/04/25 01:40:12, imcheng wrote: > Why do you need this? I thought this object lives on the IO thread already? Will mark DialMediaSinkService object as DeleteOnIOThread. (MediaRouterMojoImpl owns DialMediaSinkService. It is destroyed on Main thread, so does DialMediaSinkService object. Since timers are started on IO thread, destroying them on Main thread will cause DCHECK failures) https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.h:154: content::BrowserThread::DeleteOnIOThread> On 2017/04/25 21:06:25, mark a. foltz wrote: > On 2017/04/25 at 01:40:12, imcheng wrote: > > Why do you need this? I thought this object lives on the IO thread already? > > BrowserThread::DeleteOnThread has a shortcut that avoids posting a task to the > same thread. So I don't think this is a big deal. > > https://cs.chromium.org/chromium/src/content/public/browser/browser_thread.h?... Acknowledged. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/device_description_service_unittest.cc (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service_unittest.cc:27: void Start(const std::string& xml_text, On 2017/04/25 01:40:12, imcheng wrote: > Do you intend to mock out this method? Done. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service_unittest.cc:55: // Create Test Data On 2017/04/25 21:06:26, mark a. foltz wrote: > These static methods to create test data could go into an anonymous namespace at > the top level - just to keep this class focused on methods that manage the test > maps. I don't feel strongly though. Done. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service_unittest.cc:231: device_description_service()->OnDeviceDescriptionFetchError(device_data_2, On 2017/04/25 21:06:26, mark a. foltz wrote: > Is this asserting that the original fetcher for |device_data_2| is erroring out > when its URL got updated? > Should the new fetcher created by GetDeviceDescriptions succeed? No, |device_data_2| will fail quietly without any error...These 3 OnDeviceDescriptionFetchError() calls are simply clean up calls (or ~URLFetcher() will have DCHECK failures). https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service_unittest.cc:250: EXPECT_EQ(size_t(1), description_map_.size()); On 2017/04/25 21:06:26, mark a. foltz wrote: > Nit: Check that device 3 specifically is not removed from the cache. Done. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:33: void DialMediaSinkService::Start() { On 2017/04/25 01:40:12, imcheng wrote: > The threading model isn't clear to me; we should clarify it in > MediaSinkService's documentation. If MediaRouter is supposed to call Start() on > the UI thread, then in here we need to PostTask to the IO thread. Will do PostTask in MediaRouter. Will make DialMediaSinkService always live on IO and update documentation. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:39: void DialMediaSinkService::Stop() { On 2017/04/25 21:06:26, mark a. foltz wrote: > On 2017/04/25 at 01:40:13, imcheng wrote: > > Is the plan to have MediaRouter call Stop() when shutting down? > > I would assume the MR would call Start/Stop depending on whether there are > MediaSinkObservers for DIAL (to stay in sync with the discovery logic in the > component). Does this service also need to run for media route discovery? Figure out where to call Stop in https://codereview.chromium.org/2837363002/ ? https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:54: DialRegistry* DialMediaSinkService::dial_registry() { On 2017/04/25 21:06:26, mark a. foltz wrote: > Does this need to be exposed through the API - what needs access to the > DialRegistry? For unit tests to override this function... https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:117: base::TimeDelta::FromSeconds(kFetchCompleteTimeoutSecs); On 2017/04/25 01:40:12, imcheng wrote: > Seems like DelayTimer is better suited for this: > https://cs.chromium.org/chromium/src/base/timer/timer.h?type=cs&q=f:base+butt... I think we may want to fire the first timer event in exact 3s. Use DelayTimer it may take longer and we cannot control the exact fire time. Since our users have relative small device list, OneShotTimer() seems ok to me...Would it be a big performance concern? https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:143: sink_discovery_callback_.Run( On 2017/04/25 01:40:12, imcheng wrote: > We should be invoking the callback on the UI thread; use PostTask? Will do that in MediaRouter... https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.h (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:74: std::unique_ptr<DeviceDescriptionService> description_service_; On 2017/04/25 01:40:13, imcheng wrote: > Does this need DeleteOnIOThread? Will make DialMediaSinkService DeleteOnIOThread (https://codereview.chromium.org/2837363002/diff/1/chrome/browser/media/router...) https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:6: #include "base/test/mock_callback.h" On 2017/04/25 21:06:26, mark a. foltz wrote: > Are these #includes in the right order? Done. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc:65: std::vector<MediaSinkInternal> CreateDialMediaSinks() { On 2017/04/25 21:06:26, mark a. foltz wrote: > Similar comment about moving test data factory code to an anonymous namespace. Done. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/mock_device_description_service.h (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/mock_device_description_service.h:15: class MockDeviceDescriptionService : public DeviceDescriptionService { On 2017/04/25 21:06:26, mark a. foltz wrote: > Is this only used in DialMediaSinkServiceUnittest? Consider folding it into > that file. Done. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/media_sink_service.h (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/media_sink_service.h:15: namespace media_router { On 2017/04/25 21:06:26, mark a. foltz wrote: > Should this go in c/common/media/router/discovery/ ? Done. https://codereview.chromium.org/2701633002/diff/200001/chrome/common/media_ro... File chrome/common/media_router/discovery/media_sink_internal_unittest.h (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/common/media_ro... chrome/common/media_router/discovery/media_sink_internal_unittest.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/04/25 01:40:13, imcheng wrote: > Can this be inlined in the .cc? Seperate it so other tests can use CreateDialSinkExtraData() function...
Looks good. I think it would be good to add some comments in the more subtle parts of the implementation, such as the use of certain timers / shared parser / etc., so we don't forget why they were implemented in a certain way. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/device_description_service.cc (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:169: return cache_pair.second.expire_time < now; On 2017/04/26 18:50:04, zhaobin wrote: > On 2017/04/25 01:40:12, imcheng wrote: > > It looks like the predicate is reversed? > > Seems ok? If exprie time < now(), the entry is expired and removed. Ah of course. You are right. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:112: return; IIUC, it seems safe to reset the 3s timer even if the device description came late. This is so that the extension will be notified quickly. See also comment below. https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:117: base::TimeDelta::FromSeconds(kFetchCompleteTimeoutSecs); On 2017/04/26 18:50:05, zhaobin wrote: > On 2017/04/25 01:40:12, imcheng wrote: > > Seems like DelayTimer is better suited for this: > > > https://cs.chromium.org/chromium/src/base/timer/timer.h?type=cs&q=f:base+butt... > > I think we may want to fire the first timer event in exact 3s. Use DelayTimer it > may take longer and we cannot control the exact fire time. Since our users have > relative small device list, OneShotTimer() seems ok to me...Would it be a big > performance concern? What you are doing here is to set a 3s timer initially in OnDialDeviceEvent() and then resetting it with a new 3s timer whenever OnDeviceDescriptionAvailable in OnDeviceDescriptionAvailable(). It seems the equivalent would be to creating a DelayTimer, and calling DelayTimer::Reset() in OnDialDeviceEvent() and OnDeviceDescriptionAvilable. Performance wise, I think it would be slightly better if we call finish_timer_->Reset() instead of re-allocating a new Timer. https://codereview.chromium.org/2701633002/diff/200001/chrome/common/media_ro... File chrome/common/media_router/discovery/media_sink_internal_unittest.h (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/common/media_ro... chrome/common/media_router/discovery/media_sink_internal_unittest.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/04/26 18:50:05, zhaobin wrote: > On 2017/04/25 01:40:13, imcheng wrote: > > Can this be inlined in the .cc? > > Seperate it so other tests can use CreateDialSinkExtraData() function... It sounds like those functions belong in a test_utils.{h,cc} then? https://codereview.chromium.org/2701633002/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2701633002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:25: : MediaSinkService(callback), browser_context_(browser_context) { browser_context_ not used? https://codereview.chromium.org/2701633002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:26: DCHECK_CURRENTLY_ON(BrowserThread::IO); The constructor will be invoked by MediaRouter in the UI thread, right? https://codereview.chromium.org/2701633002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:27: request_context_ = request_context; Can this go in the initializer? https://codereview.chromium.org/2701633002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:107: MediaSink sink(description_data.unique_id, description_data.friendly_name, Note this "sink" will have a different ID when it is sent to the extension, because it derives a different sink ID using the given sink ID. Right now it is not a problem because this sink is never used within the browser. Just something to keep in mind. https://codereview.chromium.org/2701633002/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.h (right): https://codereview.chromium.org/2701633002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:25: class DialMediaSinkService : public MediaSinkService, Could you please backfill class comments here? https://codereview.chromium.org/2701633002/diff/220001/chrome/common/media_ro... File chrome/common/media_router/discovery/media_sink_service.h (right): https://codereview.chromium.org/2701633002/diff/220001/chrome/common/media_ro... chrome/common/media_router/discovery/media_sink_service.h:45: virtual void AddSinkQuery(MediaSinksObserver* observer) = 0; This will be a problem when we implement this, because MediaSinksObserver is used in the UI thread. I wonder if we should just pass in the MediaSource and let MediaRouter deal with MediaSinksObserver instead. Since we don't have plans to implement browser-side sink query for now, maybe we should just remove these methods.
> Finally can you add some comments that MediaSinkInternal is for passing sinks > from MR<-->MRP and not for exposure to MR clients in general. Maybe we can > enforce this with DEPS - imcheng@, WDYT? DEPS sounds fine. We can do that in a separate patch.
On 2017/04/25 21:06:26, mark a. foltz wrote: > LGTM with remaining comments addressed - overall this looks like a really solid > piece of work. The unit tests are really well written. > > It would be a good idea to double check that pending requests are cleaned up > appropriately for both fetching and parsing device descriptions when a new set > of devices is passed in or the service is shut down. > > At a latter point, can you add a README.md with a link to an updated design doc > for DIAL discovery (you may need to copy it over to http://chromium.org to make it > world readable)? > > Finally can you add some comments that MediaSinkInternal is for passing sinks > from MR<-->MRP and not for exposure to MR clients in general. Maybe we can > enforce this with DEPS - imcheng@, WDYT? > > Thanks again! For pending requests, it seems fine for now. When a new set of devies is passed in, we may let them finish and store parsed device description in |description_cache_| instead of aborting them. When service is shut down (in DeviceDescriptionService dtor), device fetcher objects and parser object will be destroyed, terminating URL fetching and utility process. For README.md https://codereview.chromium.org/2844103002/ MediaSinkInternal class has some class level comment: "it is not exposed to users of MediaRouter.". Will do it in a seperate patch if DEPS is needed. Description updated :)
https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/device_description_service.cc (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/device_description_service.cc:169: return cache_pair.second.expire_time < now; On 2017/04/26 22:52:15, imcheng wrote: > On 2017/04/26 18:50:04, zhaobin wrote: > > On 2017/04/25 01:40:12, imcheng wrote: > > > It looks like the predicate is reversed? > > > > Seems ok? If exprie time < now(), the entry is expired and removed. > > Ah of course. You are right. Acknowledged. https://codereview.chromium.org/2701633002/diff/200001/chrome/common/media_ro... File chrome/common/media_router/discovery/media_sink_internal_unittest.h (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/common/media_ro... chrome/common/media_router/discovery/media_sink_internal_unittest.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/04/26 22:52:16, imcheng wrote: > On 2017/04/26 18:50:05, zhaobin wrote: > > On 2017/04/25 01:40:13, imcheng wrote: > > > Can this be inlined in the .cc? > > > > Seperate it so other tests can use CreateDialSinkExtraData() function... > > It sounds like those functions belong in a test_utils.{h,cc} then? Done. https://codereview.chromium.org/2701633002/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2701633002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:25: : MediaSinkService(callback), browser_context_(browser_context) { On 2017/04/26 22:52:16, imcheng wrote: > browser_context_ not used? Done. https://codereview.chromium.org/2701633002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:26: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2017/04/26 22:52:16, imcheng wrote: > The constructor will be invoked by MediaRouter in the UI thread, right? Plan to create this in IO thread. Will update https://codereview.chromium.org/2837363002/ if (impl->enable_browser_side_discovery_) impl->StartBrowserSideDiscovery(); will become: if (impl->enable_browser_side_discovery_) { ... content::BrowserThread::PostTask( content::BrowserThread::IO, FROM_HERE, base::Bind(&MediaRouterMojoImpl::StartBrowserSideDiscovery, base::Unretained(impl), ...)); } https://codereview.chromium.org/2701633002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:27: request_context_ = request_context; On 2017/04/26 22:52:16, imcheng wrote: > Can this go in the initializer? Done. https://codereview.chromium.org/2701633002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:107: MediaSink sink(description_data.unique_id, description_data.friendly_name, On 2017/04/26 22:52:16, imcheng wrote: > Note this "sink" will have a different ID when it is sent to the extension, > because it derives a different sink ID using the given sink ID. Right now it is > not a problem because this sink is never used within the browser. Just something > to keep in mind. Added a comment. https://codereview.chromium.org/2701633002/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.h (right): https://codereview.chromium.org/2701633002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.h:25: class DialMediaSinkService : public MediaSinkService, On 2017/04/26 22:52:16, imcheng wrote: > Could you please backfill class comments here? Done. https://codereview.chromium.org/2701633002/diff/220001/chrome/common/media_ro... File chrome/common/media_router/discovery/media_sink_service.h (right): https://codereview.chromium.org/2701633002/diff/220001/chrome/common/media_ro... chrome/common/media_router/discovery/media_sink_service.h:45: virtual void AddSinkQuery(MediaSinksObserver* observer) = 0; On 2017/04/26 22:52:16, imcheng wrote: > This will be a problem when we implement this, because MediaSinksObserver is > used in the UI thread. I wonder if we should just pass in the MediaSource and > let MediaRouter deal with MediaSinksObserver instead. Since we don't have plans > to implement browser-side sink query for now, maybe we should just remove these > methods. Done.
lgtm https://codereview.chromium.org/2701633002/diff/220001/chrome/browser/media/r... File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2701633002/diff/220001/chrome/browser/media/r... chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc:26: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2017/04/27 01:41:20, zhaobin wrote: > On 2017/04/26 22:52:16, imcheng wrote: > > The constructor will be invoked by MediaRouter in the UI thread, right? > > Plan to create this in IO thread. Will update > https://codereview.chromium.org/2837363002/ > > if (impl->enable_browser_side_discovery_) > impl->StartBrowserSideDiscovery(); > > will become: > > if (impl->enable_browser_side_discovery_) { > ... > content::BrowserThread::PostTask( > content::BrowserThread::IO, FROM_HERE, > base::Bind(&MediaRouterMojoImpl::StartBrowserSideDiscovery, > base::Unretained(impl), ...)); > } Hmm, I don't think that will work. MediaRouterMojoImpl isn't thread safe, and we are passing it as a raw pointer to a different thread. We would have to make MediaRouterMojoImpl inherit RefCountedThreadSafe for that to work. I think I would prefer to keep MediaRouterMojoImpl as-is (i.e. always operating on UI thread) and make MediaSinkService inherit RefCountedThreadSafe and do the thread-hopping between IO and UI thread. This is ok for now but we might have to revise it in https://codereview.chromium.org/2837363002/. Let's chat offline so we are same page about the threading model?
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zhaobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, imcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2701633002/#ps290001 (title: "fix chromeos compile error")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 290001, "attempt_start_ts": 1493518322933160, "parent_rev": "9fb7f6bfb5a838b8e231134e5b3f3dda67c5f556", "commit_rev": "a26ad25c4fd2cb7bbe286745d43e507dc658ba2c"}
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Add DialMediaSinkService and DeviceDescriptionService Discover process: DialMediaSinkService.Start() registers itself with DialRegistry DialMediaSinkService.OnDialDeviceEvent() gets invoked when device data comes back, and starts a 3s timer Start description fetches for each device DeviceDescriptionService.OnDeviceDescriptionFetchComplete() gets invoked when device description comes back. Starts an XML parser in utility process to parse device description XML DialMediaSinkService.OnDeviceDescriptionAvailable() gets invoked when parsing in utility process finishes Create MediaSinkInternal and store it in DialMediaSinkService's sink map Invoke DialMediaSinkService.FetchCompleted() when timer expires Design doc: https://docs.google.com/document/d/1lJbE4Oc9q1amkWEaD2ZL4mHEdRRlKHvwxUUcJmNC8... BUG=687375 ========== to ========== [Media Router] Add DialMediaSinkService and DeviceDescriptionService Discover process: DialMediaSinkService.Start() registers itself with DialRegistry DialMediaSinkService.OnDialDeviceEvent() gets invoked when device data comes back, and starts a 3s timer Start description fetches for each device DeviceDescriptionService.OnDeviceDescriptionFetchComplete() gets invoked when device description comes back. Starts an XML parser in utility process to parse device description XML DialMediaSinkService.OnDeviceDescriptionAvailable() gets invoked when parsing in utility process finishes Create MediaSinkInternal and store it in DialMediaSinkService's sink map Invoke DialMediaSinkService.FetchCompleted() when timer expires Design doc: https://docs.google.com/document/d/1lJbE4Oc9q1amkWEaD2ZL4mHEdRRlKHvwxUUcJmNC8... BUG=687375 Review-Url: https://codereview.chromium.org/2701633002 Cr-Commit-Position: refs/heads/master@{#468251} Committed: https://chromium.googlesource.com/chromium/src/+/a26ad25c4fd2cb7bbe286745d43e... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:290001) as https://chromium.googlesource.com/chromium/src/+/a26ad25c4fd2cb7bbe286745d43e...
Message was sent while issue was closed.
On 2017/04/30 03:30:21, commit-bot: I haz the power wrote: > Committed patchset #13 (id:290001) as > https://chromium.googlesource.com/chromium/src/+/a26ad25c4fd2cb7bbe286745d43e... Findit suggests this CL to have introduced a flaky test according to analysis https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV..., namely DeviceDescriptionServiceTest.TestCleanUpCacheEntries Can someone please help verify? Thanks, Jeff on behalf of Findit team
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Add DialMediaSinkService and DeviceDescriptionService Discover process: DialMediaSinkService.Start() registers itself with DialRegistry DialMediaSinkService.OnDialDeviceEvent() gets invoked when device data comes back, and starts a 3s timer Start description fetches for each device DeviceDescriptionService.OnDeviceDescriptionFetchComplete() gets invoked when device description comes back. Starts an XML parser in utility process to parse device description XML DialMediaSinkService.OnDeviceDescriptionAvailable() gets invoked when parsing in utility process finishes Create MediaSinkInternal and store it in DialMediaSinkService's sink map Invoke DialMediaSinkService.FetchCompleted() when timer expires Design doc: https://docs.google.com/document/d/1lJbE4Oc9q1amkWEaD2ZL4mHEdRRlKHvwxUUcJmNC8... BUG=687375 Review-Url: https://codereview.chromium.org/2701633002 Cr-Commit-Position: refs/heads/master@{#468251} Committed: https://chromium.googlesource.com/chromium/src/+/a26ad25c4fd2cb7bbe286745d43e... ========== to ========== [Media Router] Add DialMediaSinkService and DeviceDescriptionService Discover process: DialMediaSinkService.Start() registers itself with DialRegistry DialMediaSinkService.OnDialDeviceEvent() gets invoked when device data comes back, and starts a 3s timer Start description fetches for each device DeviceDescriptionService.OnDeviceDescriptionFetchComplete() gets invoked when device description comes back. Starts an XML parser in utility process to parse device description XML DialMediaSinkService.OnDeviceDescriptionAvailable() gets invoked when parsing in utility process finishes Create MediaSinkInternal and store it in DialMediaSinkService's sink map Invoke DialMediaSinkService.FetchCompleted() when timer expires Design doc: https://docs.google.com/a/chromium.org/document/d/1vLpUgp5mJi6KFaCV3HEMQEZYDK... BUG=687375 Review-Url: https://codereview.chromium.org/2701633002 Cr-Commit-Position: refs/heads/master@{#468251} Committed: https://chromium.googlesource.com/chromium/src/+/a26ad25c4fd2cb7bbe286745d43e... ========== |