Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(291)

Issue 2701633002: [Media Router] Add DialMediaSinkService and DeviceDescriptionService (Closed)

Created:
3 years, 10 months ago by zhaobin
Modified:
3 years, 7 months ago
Reviewers:
mark a. foltz, imcheng
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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1485 lines, -204 lines) Patch
M chrome/browser/media/router/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -2 lines 0 comments Download
M chrome/browser/media/router/discovery/BUILD.gn View 1 2 3 4 5 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/media/router/discovery/dial/device_description_fetcher.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
A chrome/browser/media/router/discovery/dial/device_description_service.h View 1 2 3 4 5 6 7 8 9 1 chunk +168 lines, -0 lines 0 comments Download
A chrome/browser/media/router/discovery/dial/device_description_service.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +324 lines, -0 lines 0 comments Download
A chrome/browser/media/router/discovery/dial/device_description_service_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +328 lines, -0 lines 0 comments Download
M chrome/browser/media/router/discovery/dial/dial_device_data.cc View 1 2 3 4 5 2 chunks +11 lines, -1 line 0 comments Download
M chrome/browser/media/router/discovery/dial/dial_device_data_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -2 lines 0 comments Download
A chrome/browser/media/router/discovery/dial/dial_media_sink_service.h View 1 2 3 4 5 6 7 8 9 1 chunk +89 lines, -0 lines 0 comments Download
A chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +147 lines, -0 lines 0 comments Download
A chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +232 lines, -0 lines 0 comments Download
M chrome/browser/media/router/discovery/dial/dial_registry.h View 1 2 3 4 2 chunks +4 lines, -3 lines 0 comments Download
A chrome/browser/media/router/discovery/dial/parsed_dial_device_description.h View 1 2 3 4 5 6 1 chunk +41 lines, -0 lines 0 comments Download
A chrome/browser/media/router/discovery/dial/parsed_dial_device_description.cc View 1 2 3 4 5 6 1 chunk +40 lines, -0 lines 0 comments Download
M chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.h View 1 2 3 4 5 6 7 8 2 chunks +5 lines, -14 lines 0 comments Download
M chrome/browser/media/router/discovery/dial/safe_dial_device_description_parser.cc View 1 2 3 4 5 6 1 chunk +16 lines, -37 lines 0 comments Download
M chrome/browser/media/router/media_sink_service.h View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -58 lines 0 comments Download
M chrome/browser/media/router/media_sink_service.cc View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -15 lines 0 comments Download
M chrome/common/media_router/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/common/media_router/discovery/media_sink_internal.h View 1 2 3 4 5 6 7 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/common/media_router/discovery/media_sink_internal.cc View 1 2 3 4 5 6 7 1 chunk +8 lines, -0 lines 0 comments Download
M chrome/common/media_router/discovery/media_sink_internal_unittest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +45 lines, -51 lines 0 comments Download
A + chrome/common/media_router/discovery/media_sink_service.h View 1 2 3 4 5 6 7 8 9 3 chunks +7 lines, -18 lines 0 comments Download
A + chrome/common/media_router/discovery/media_sink_service.cc View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 52 (31 generated)
zhaobin
3 years, 10 months ago (2017-02-16 18:34:47 UTC) #2
mark a. foltz
This is a *lot* of new code and it looks like a fairly straight port ...
3 years, 9 months ago (2017-03-11 00:08:16 UTC) #3
zhaobin
Resolved some code review comments from Mark. https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router/device_description_service.h File chrome/browser/media/router/device_description_service.h (right): https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router/device_description_service.h#newcode16 chrome/browser/media/router/device_description_service.h:16: #include "chrome/browser/extensions/api/dial/dial_registry.h" ...
3 years, 8 months ago (2017-03-28 13:37:00 UTC) #4
zhaobin
https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router/device_description_service.h File chrome/browser/media/router/device_description_service.h (right): https://codereview.chromium.org/2701633002/diff/1/chrome/browser/media/router/device_description_service.h#newcode33 chrome/browser/media/router/device_description_service.h:33: struct DialDeviceDescription { On 2017/03/11 00:08:15, mfoltz_ooo_until_4_10 wrote: > ...
3 years, 8 months ago (2017-04-10 18:44:42 UTC) #6
mark a. foltz
Made a pass - overall looks really good and the majority of my comments are ...
3 years, 8 months ago (2017-04-12 00:17:19 UTC) #7
imcheng
https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/router/discovery/dial/device_description_service.cc File chrome/browser/media/router/discovery/dial/device_description_service.cc (right): https://codereview.chromium.org/2701633002/diff/80001/chrome/browser/media/router/discovery/dial/device_description_service.cc#newcode21 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| ...
3 years, 8 months ago (2017-04-12 19:33:51 UTC) #8
zhaobin
- Remove DialMediaSinkCache service (ParsedDeviceDescription and MediaSinkInternal seems equivalent, no need to cache both). - ...
3 years, 8 months ago (2017-04-18 06:58:28 UTC) #11
mark a. foltz
Most of my comments are minor documentation suggestions - one question about the utility process ...
3 years, 8 months ago (2017-04-18 18:16:27 UTC) #12
zhaobin
Add unit tests for DeviceDescriptionService. Let DeviceDescriptionService control the lifetime of safe parser (utility process). ...
3 years, 8 months ago (2017-04-21 23:12:59 UTC) #14
imcheng
https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/router/discovery/dial/device_description_service.cc File chrome/browser/media/router/discovery/dial/device_description_service.cc (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/router/discovery/dial/device_description_service.cc#newcode29 chrome/browser/media/router/discovery/dial/device_description_service.cc:29: const int kDeviceDescriptionCacheTimeMins = 12 * 60; Just 12 ...
3 years, 8 months ago (2017-04-25 01:40:13 UTC) #15
mark a. foltz
LGTM with remaining comments addressed - overall this looks like a really solid piece of ...
3 years, 8 months ago (2017-04-25 21:06:26 UTC) #16
mark a. foltz
Note: Update patch description when you get a chance :)
3 years, 8 months ago (2017-04-25 21:29:49 UTC) #17
zhaobin
https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/router/discovery/dial/device_description_service.cc File chrome/browser/media/router/discovery/dial/device_description_service.cc (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/router/discovery/dial/device_description_service.cc#newcode29 chrome/browser/media/router/discovery/dial/device_description_service.cc:29: const int kDeviceDescriptionCacheTimeMins = 12 * 60; On 2017/04/25 ...
3 years, 7 months ago (2017-04-26 18:50:05 UTC) #19
imcheng
Looks good. I think it would be good to add some comments in the more ...
3 years, 7 months ago (2017-04-26 22:52:16 UTC) #20
imcheng
> Finally can you add some comments that MediaSinkInternal is for passing sinks > from ...
3 years, 7 months ago (2017-04-26 22:56:00 UTC) #21
zhaobin
On 2017/04/25 21:06:26, mark a. foltz wrote: > LGTM with remaining comments addressed - overall ...
3 years, 7 months ago (2017-04-26 23:09:20 UTC) #22
zhaobin
https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/router/discovery/dial/device_description_service.cc File chrome/browser/media/router/discovery/dial/device_description_service.cc (right): https://codereview.chromium.org/2701633002/diff/200001/chrome/browser/media/router/discovery/dial/device_description_service.cc#newcode169 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: ...
3 years, 7 months ago (2017-04-27 01:41:20 UTC) #23
imcheng
lgtm https://codereview.chromium.org/2701633002/diff/220001/chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc File chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc (right): https://codereview.chromium.org/2701633002/diff/220001/chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc#newcode26 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 ...
3 years, 7 months ago (2017-04-27 19:21:32 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2701633002/290001
3 years, 7 months ago (2017-04-30 02:12:08 UTC) #47
commit-bot: I haz the power
Committed patchset #13 (id:290001) as https://chromium.googlesource.com/chromium/src/+/a26ad25c4fd2cb7bbe286745d43e507dc658ba2c
3 years, 7 months ago (2017-04-30 03:30:21 UTC) #50
lijeffrey1
3 years, 7 months ago (2017-05-01 04:53:06 UTC) #51
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

Powered by Google App Engine
This is Rietveld 408576698