Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2662393005:
[Media Router] (device discovery) Create MediaSinkService interface (Closed)
|
Created:
3 years, 10 months ago by zhaobin Modified:
3 years, 10 months ago CC:
chfremer+watch_chromium.org, chromium-reviews, feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org, media-router+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Media Router] (device discovery) Create MediaSinkService interface
Created MediaSinkService interface according to device discovery design doc.
BUG=687356
Review-Url: https://codereview.chromium.org/2662393005
Cr-Commit-Position: refs/heads/master@{#451048}
Committed: https://chromium.googlesource.com/chromium/src/+/8a4c43002d54cbf4f577a5e00b87fddc5e9b6269
Patch Set 1 #
Total comments: 10
Patch Set 2 : resolve code review comments from Derek #
Total comments: 6
Patch Set 3 : resolve code review comments from Derek #
Total comments: 17
Patch Set 4 : resolve code review comments from Mark #
Total comments: 1
Patch Set 5 : resolve code review comments from Mark #
Messages
Total messages: 23 (13 generated)
zhaobin@chromium.org changed reviewers: + imcheng@chromium.org, mfoltz@chromium.org
https://codereview.chromium.org/2662393005/diff/1/chrome/browser/media/router... File chrome/browser/media/router/media_sink_service.h (right): https://codereview.chromium.org/2662393005/diff/1/chrome/browser/media/router... chrome/browser/media/router/media_sink_service.h:10: #include "base/callback_list.h" Did you mean to include base/callback.h ? https://codereview.chromium.org/2662393005/diff/1/chrome/browser/media/router... chrome/browser/media/router/media_sink_service.h:17: // MediaSinkService defines an interface which can be implemented by DIAL, Cast Comment suggestion: A service which can be used to start device discovery and resolution in the background to generate MediaSinks. In addition, the service is capable of answering MediaSink queries using the sinks that it generated. https://codereview.chromium.org/2662393005/diff/1/chrome/browser/media/router... chrome/browser/media/router/media_sink_service.h:19: // The service is also responsible for answering MediaSink queries. Can you also document threading assumptions? e.g., This class is not thread safe. All methods must be called from the UI thread. https://codereview.chromium.org/2662393005/diff/1/chrome/browser/media/router... chrome/browser/media/router/media_sink_service.h:20: class MediaSinkService { Please also define a virtual destructor. https://codereview.chromium.org/2662393005/diff/1/chrome/browser/media/router... chrome/browser/media/router/media_sink_service.h:26: base::Callback<void(const const MediaSinkService*, Hmm, not quite sure if we still need to pass |this| anymore. Its original purpose was to have MR get the name of the service to pass to MR extension, but as discussed we don't need to do that.
https://codereview.chromium.org/2662393005/diff/1/chrome/browser/media/router... File chrome/browser/media/router/media_sink_service.h (right): https://codereview.chromium.org/2662393005/diff/1/chrome/browser/media/router... chrome/browser/media/router/media_sink_service.h:10: #include "base/callback_list.h" On 2017/02/07 00:48:14, imcheng wrote: > Did you mean to include base/callback.h ? Done. https://codereview.chromium.org/2662393005/diff/1/chrome/browser/media/router... chrome/browser/media/router/media_sink_service.h:17: // MediaSinkService defines an interface which can be implemented by DIAL, Cast On 2017/02/07 00:48:13, imcheng wrote: > Comment suggestion: > > A service which can be used to start device discovery and resolution in the > background to generate MediaSinks. In addition, the service is capable of > answering MediaSink queries using the sinks that it generated. Done. https://codereview.chromium.org/2662393005/diff/1/chrome/browser/media/router... chrome/browser/media/router/media_sink_service.h:19: // The service is also responsible for answering MediaSink queries. On 2017/02/07 00:48:13, imcheng wrote: > Can you also document threading assumptions? e.g., > > This class is not thread safe. All methods must be called from the UI thread. Done. https://codereview.chromium.org/2662393005/diff/1/chrome/browser/media/router... chrome/browser/media/router/media_sink_service.h:20: class MediaSinkService { On 2017/02/07 00:48:14, imcheng wrote: > Please also define a virtual destructor. Done. https://codereview.chromium.org/2662393005/diff/1/chrome/browser/media/router... chrome/browser/media/router/media_sink_service.h:26: base::Callback<void(const const MediaSinkService*, On 2017/02/07 00:48:13, imcheng wrote: > Hmm, not quite sure if we still need to pass |this| anymore. Its original > purpose was to have MR get the name of the service to pass to MR extension, but > as discussed we don't need to do that. Done.
lgtm. One more question. https://codereview.chromium.org/2662393005/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink_service.h (right): https://codereview.chromium.org/2662393005/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:8: #include <memory> #include <vector> https://codereview.chromium.org/2662393005/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:19: // answering MediaSink queries using the sinks that it generated. This class is nit: start the sentence about thread safety in a new line. https://codereview.chromium.org/2662393005/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:26: base::Callback<void(const std::vector<MediaSink>&)>; Since we are defining subtypes of MediaSink in another patch, would this have to become std::vector<std::unique_ptr<MediaSink>> to prevent object slicing?
https://codereview.chromium.org/2662393005/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink_service.h (right): https://codereview.chromium.org/2662393005/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:8: #include <memory> On 2017/02/07 19:25:31, imcheng wrote: > #include <vector> Done. https://codereview.chromium.org/2662393005/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:19: // answering MediaSink queries using the sinks that it generated. This class is On 2017/02/07 19:25:31, imcheng wrote: > nit: start the sentence about thread safety in a new line. Done. https://codereview.chromium.org/2662393005/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:26: base::Callback<void(const std::vector<MediaSink>&)>; On 2017/02/07 19:25:31, imcheng wrote: > Since we are defining subtypes of MediaSink in another patch, would this have to > become std::vector<std::unique_ptr<MediaSink>> to prevent object slicing? Done.
Looks good, a few questions below. https://codereview.chromium.org/2662393005/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink_service.h (right): https://codereview.chromium.org/2662393005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:18: // A service which can be used to start device discovery and resolution in the ... to start background discovery and resolution of MediaSinks. Often these are remote devices, like Chromecast. https://codereview.chromium.org/2662393005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:21: // This class is not thread safe. All methods must be called from the UI thread. I expect most of the code that implements actual sink discovery to run on the IO thread (i.e., using net::). Do you plan on copying data from the IO thread each time a sink is discovered? https://codereview.chromium.org/2662393005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:27: base::Callback<std::vector<std::unique_ptr<MediaSink>>>; Why does this need to pass ownership of the sinks it's discovered? https://codereview.chromium.org/2662393005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:31: // Starts sink discovery. No-ops if already started. Maybe the callback should be passed in the ctor if there's no reason to pass a different one? https://codereview.chromium.org/2662393005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:37: // Only one observer can be added for a given MediaSource. Where is the MediaSource passed? What happens if a duplicate observer/source is registered? https://codereview.chromium.org/2662393005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:42: // Removes a sink query and stop observing for MediaSink updates. ...stops observing MediaSink... https://codereview.chromium.org/2662393005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:43: virtual void RemoveSinkQuery(MediaSinksObserver* observer) = 0; What happens if called with an observer not registered with this class? https://codereview.chromium.org/2662393005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:44: }; Allocate a base::ThreadChecker to enforce thread safety? Should this DISALLOW_COPY_AND_ASSIGN?
https://codereview.chromium.org/2662393005/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink_service.h (right): https://codereview.chromium.org/2662393005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:18: // A service which can be used to start device discovery and resolution in the On 2017/02/09 23:47:52, mark a. foltz wrote: > ... to start background discovery and resolution of MediaSinks. Often these are > remote devices, like Chromecast. Done. https://codereview.chromium.org/2662393005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:21: // This class is not thread safe. All methods must be called from the UI thread. On 2017/02/09 23:47:51, mark a. foltz wrote: > I expect most of the code that implements actual sink discovery to run on the IO > thread (i.e., using net::). > Do you plan on copying data from the IO thread each time a sink is discovered? Done. https://codereview.chromium.org/2662393005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:27: base::Callback<std::vector<std::unique_ptr<MediaSink>>>; On 2017/02/09 23:47:52, mark a. foltz wrote: > Why does this need to pass ownership of the sinks it's discovered? Done. https://codereview.chromium.org/2662393005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:31: // Starts sink discovery. No-ops if already started. On 2017/02/09 23:47:52, mark a. foltz wrote: > Maybe the callback should be passed in the ctor if there's no reason to pass a > different one? Done. https://codereview.chromium.org/2662393005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:37: // Only one observer can be added for a given MediaSource. On 2017/02/09 23:47:52, mark a. foltz wrote: > Where is the MediaSource passed? > > What happens if a duplicate observer/source is registered? We can get MediaSource from observer->source(). We will add a DCHECK or use existing DCHECK to prevent duplicate observer. https://codereview.chromium.org/2662393005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:42: // Removes a sink query and stop observing for MediaSink updates. On 2017/02/09 23:47:52, mark a. foltz wrote: > ...stops observing MediaSink... Done. https://codereview.chromium.org/2662393005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:43: virtual void RemoveSinkQuery(MediaSinksObserver* observer) = 0; On 2017/02/09 23:47:52, mark a. foltz wrote: > What happens if called with an observer not registered with this class? no-op if observer does not exist. https://codereview.chromium.org/2662393005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:44: }; On 2017/02/09 23:47:52, mark a. foltz wrote: > Allocate a base::ThreadChecker to enforce thread safety? > > Should this DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/2662393005/diff/60001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink_service.h (right): https://codereview.chromium.org/2662393005/diff/60001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:28: base::Callback<void(const std::vector<MediaSink>&)>; Will change MediaSink to TypedMediaSink (or sth else) when we finalize design in https://codereview.chromium.org/2675033002/.
lgtm with one comment. https://codereview.chromium.org/2662393005/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/media_sink_service.h (right): https://codereview.chromium.org/2662393005/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/media_sink_service.h:37: // Only one observer can be added for a given MediaSource. On 2017/02/14 at 00:42:17, zhaobin wrote: > On 2017/02/09 23:47:52, mark a. foltz wrote: > > Where is the MediaSource passed? > > > > What happens if a duplicate observer/source is registered? > > We can get MediaSource from observer->source(). > > We will add a DCHECK or use existing DCHECK to prevent duplicate observer. That relies on the caller to keep track of a source -> observer map to ensure that no attempt is made to register a duplicate. IMO, this API would be easier to use if it: - Allowed multiple observers per source - Or, unregistered an existing observer for a source if a new one is registered.
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_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 imcheng@chromium.org, mfoltz@chromium.org Link to the patchset: https://codereview.chromium.org/2662393005/#ps80001 (title: "resolve code review comments from Mark")
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": 80001, "attempt_start_ts": 1487270127951340,
"parent_rev": "66062a4c281e2bce112816990a07a16a70a807a6", "commit_rev":
"8a4c43002d54cbf4f577a5e00b87fddc5e9b6269"}
Message was sent while issue was closed.
Description was changed from ========== [Media Router] (device discovery) Create MediaSinkService interface Created MediaSinkService interface according to device discovery design doc. BUG=687356 ========== to ========== [Media Router] (device discovery) Create MediaSinkService interface Created MediaSinkService interface according to device discovery design doc. BUG=687356 Review-Url: https://codereview.chromium.org/2662393005 Cr-Commit-Position: refs/heads/master@{#451048} Committed: https://chromium.googlesource.com/chromium/src/+/8a4c43002d54cbf4f577a5e00b87... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/8a4c43002d54cbf4f577a5e00b87... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
