Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2951523002:
Media Remoting: Add mojo interfaces between browser and extension. (Closed)
|
Created:
3 years, 6 months ago by xjz Modified:
3 years, 5 months ago CC:
Aaron Boodman, abarth-chromium, alokp+watch_chromium.org, apacible+watch_chromium.org, avayvod+watch_chromium.org, chfremer+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, darin (slow to review), erickung+watch_chromium.org, extensions-reviews_chromium.org, feature-media-reviews_chromium.org, imcheng+watch_chromium.org, isheriff+watch_chromium.org, jasonroberts+watch_google.com, mfoltz+watch_chromium.org, miu+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, xjz+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMedia Remoting: Add mojo interfaces between browser and extension.
Currently media remoting communicates with extension by text strings
through Media Router mojo API. This CL adds mojo interfaces to allow
CastRemotingConnector in browser talk directly to MediaRemoter in
extension after connected through the Media Router mojo API. These
changes also enable passing the receiver's capablities to remoting
control logic in renderer, which will be done in a follow up CL.
BUG=734672
Review-Url: https://codereview.chromium.org/2951523002
Cr-Commit-Position: refs/heads/master@{#485043}
Committed: https://chromium.googlesource.com/chromium/src/+/ef7927fa28cca59d8ca4ed06f540a38702ac0810
Patch Set 1 #Patch Set 2 : Fix compile failure on Android bots. #
Total comments: 30
Patch Set 3 : Addressed imcheng's comments. Removed OnStarted/Failed interface. #
Total comments: 44
Patch Set 4 : Addressed miu's comments. Removed fuzz tests for CastRemotingConnector. #Patch Set 5 : Add interface change for later CL passing receiver capabilities. #
Total comments: 6
Patch Set 6 : Add SinkCapabilities structure. #
Total comments: 6
Patch Set 7 : Addressed comments. #
Total comments: 2
Patch Set 8 : Fix unittests. #
Total comments: 20
Patch Set 9 : Addressed imcheng's comments. #Patch Set 10 : Rebased. #
Total comments: 6
Patch Set 11 : Addressed Robert's comments. #Patch Set 12 : Rebased again. #Messages
Total messages: 91 (67 generated)
The CQ bit was checked by xjz@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by xjz@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...
Description was changed from ========== Media Remoting: Add mojo interfaces between browser and extension. Currently media remoting communicates with extension by text strings through Media Router mojo API. This CL adds mojo interfaces to allow CastRemotingConnector in browser talk directly to MediaRemoter in extension after connected through the Media Router mojo API. These changes also enable passing the receiver's capablities to remoting control logic in renderer, which will be done in a follow up CL. This CL needs to work together with the changes in extension side to enable media remoting. BUG=734672 ========== to ========== Media Remoting: Add mojo interfaces between browser and extension. Currently media remoting communicates with extension by text strings through Media Router mojo API. This CL adds mojo interfaces to allow CastRemotingConnector in browser talk directly to MediaRemoter in extension after connected through the Media Router mojo API. These changes also enable passing the receiver's capablities to remoting control logic in renderer, which will be done in a follow up CL. This CL needs to work together with the changes in extension side to enable media remoting. https://critique.corp.google.com/#review/159454325 BUG=734672 ==========
xjz@chromium.org changed reviewers: + imcheng@chromium.org, miu@chromium.org
PTAL. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by xjz@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_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by xjz@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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by xjz@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...
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Media Router API looks good. Mostly general comments. I defer to Yuri for remoting specific logic :) https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_connector.cc (right): https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.cc:160: if (tab_id_ > 0) { So it seems this class doesn't do much if tab_id_ <= 0. Should we just return nullptr in the static Get method in that case? https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.cc:186: VLOG(3) << __func__; nit: move the VLOG either above or below all DCHECKs. https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.cc:387: notifyee->OnSinkAvailable(enabled_features_); Should we notify the other bridges with OnSinkAvailable even if |active_bridge_| is not null? https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_connector.h (right): https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.h:30: // service instance. The sink is represented by a MediaRemoter in Media Route The MR extension is desktop-specific. How about "Cast Media Route Provider"? Similarly for other comments referring to "extension" below. https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.h:46: // media_router::MediaRouterMojoImpl with the tab ID. When a mirroring route is s/MojoImpl// https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.h:48: // the extension, and it notifies the media_router::MediaRouterMojoImpl through ditto for "extension". How about something along the lines of following: ",... the Cast MRP will create a MediaRemoter and notify MediaRouter, which will notify the CastRemotingConnector registered under the tab ID being remoted." https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.h:51: // compunicate with the MediaRemoter. When CastRemotingConnector gets notified typo in "communicate" https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.h:72: class CastRemotingConnector : public base::SupportsUserData::Data, Was it a deliberate decision to not put this class under the cast namespace? https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router.h:203: // Register/UnRegister a CastRemotingConnector with the |tab_id|. For a given Registers/unregisters https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router.h:209: virtual void UnRegisterRemotingSource( s/UnRegister/Unregister https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1134: void MediaRouterMojoImpl::RegisterRemotingSource( It seems this logic could actually go into MediaRouterBase. https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1150: CastRemotingConnector* remoting_source) { I'm not sure if you need to pass in remoting_source -- it's only used for DCHECK. https://codereview.chromium.org/2951523002/diff/80001/chrome/common/media_rou... File chrome/common/media_router/mojo/media_router.mojom (right): https://codereview.chromium.org/2951523002/diff/80001/chrome/common/media_rou... chrome/common/media_router/mojo/media_router.mojom:476: // Called when a MediaRemoter is started. Could you provide a bit more info about this method, such as the meaning of the parameters? If there are additional info elsewhere that provides context, consider referring to them here. https://codereview.chromium.org/2951523002/diff/80001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/media_router_bindings.js (right): https://codereview.chromium.org/2951523002/diff/80001/chrome/renderer/resourc... chrome/renderer/resources/extensions/media_router_bindings.js:281: makeRequest: bindings.makeRequest, Put this last since it starts with lower case.. https://codereview.chromium.org/2951523002/diff/80001/media/mojo/interfaces/m... File media/mojo/interfaces/mirror_service_remoting.mojom (right): https://codereview.chromium.org/2951523002/diff/80001/media/mojo/interfaces/m... media/mojo/interfaces/mirror_service_remoting.mojom:12: // Start a remoting session. Either MirrorServiceRemotingSource.OnStarted() or Please use third person verbs when describing the method, i.e. Starts / Stops / Sends etc. similarly for below and remoting_common.mojom
The CQ bit was checked by xjz@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...
imcheng: Thanks for reviewing! Addressed all your comments. miu: Removed the OnStarted() and OnStartFaile() from the MediaRemoter as discussed offline. PTAL. Thanks! https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_connector.cc (right): https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.cc:160: if (tab_id_ > 0) { On 2017/06/22 01:13:27, imcheng wrote: > So it seems this class doesn't do much if tab_id_ <= 0. Should we just return > nullptr in the static Get method in that case? Changes the condition to (tab_id_ != -1). Currently the static Get method assumes always return a non-null ptr. The question is: Do we need to create a CastRemotingConnector if the WebContents doesn't have a valid tab_id? +miu@: Any ideas on this? https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.cc:186: VLOG(3) << __func__; On 2017/06/22 01:13:27, imcheng wrote: > nit: move the VLOG either above or below all DCHECKs. Done. https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.cc:387: notifyee->OnSinkAvailable(enabled_features_); On 2017/06/22 01:13:27, imcheng wrote: > Should we notify the other bridges with OnSinkAvailable even if |active_bridge_| > is not null? When |active_bridge_| is not null, it indicates the current remoting session isn't fully stopped yet. In that case, another remoting session should not be started, so the sources should not get the OnSinkAvailable() call. When |active_bridge_| is not null, the later StopRemoting() call will reset the |active_bridge_| and call the |remoter_| to Stop remoting session, and this OnStopped() will be called again, which will notify other sources with OnSinkAvailable. https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ca... File chrome/browser/media/cast_remoting_connector.h (right): https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.h:30: // service instance. The sink is represented by a MediaRemoter in Media Route On 2017/06/22 01:13:27, imcheng wrote: > The MR extension is desktop-specific. How about "Cast Media Route Provider"? > Similarly for other comments referring to "extension" below. Done. https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.h:46: // media_router::MediaRouterMojoImpl with the tab ID. When a mirroring route is On 2017/06/22 01:13:27, imcheng wrote: > s/MojoImpl// Done. https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.h:48: // the extension, and it notifies the media_router::MediaRouterMojoImpl through On 2017/06/22 01:13:27, imcheng wrote: > ditto for "extension". How about something along the lines of following: ",... > the Cast MRP will create a MediaRemoter and notify MediaRouter, which will > notify the CastRemotingConnector registered under the tab ID being remoted." Done. https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.h:51: // compunicate with the MediaRemoter. When CastRemotingConnector gets notified On 2017/06/22 01:13:27, imcheng wrote: > typo in "communicate" Done. https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ca... chrome/browser/media/cast_remoting_connector.h:72: class CastRemotingConnector : public base::SupportsUserData::Data, On 2017/06/22 01:13:27, imcheng wrote: > Was it a deliberate decision to not put this class under the cast namespace? It sgtm to put this class under the cast namespace. +miu@: Any concerns? https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router.h:203: // Register/UnRegister a CastRemotingConnector with the |tab_id|. For a given On 2017/06/22 01:13:27, imcheng wrote: > Registers/unregisters Done. https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/media_router.h:209: virtual void UnRegisterRemotingSource( On 2017/06/22 01:13:27, imcheng wrote: > s/UnRegister/Unregister Done. https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1134: void MediaRouterMojoImpl::RegisterRemotingSource( On 2017/06/22 01:13:27, imcheng wrote: > It seems this logic could actually go into MediaRouterBase. Done. https://codereview.chromium.org/2951523002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1150: CastRemotingConnector* remoting_source) { On 2017/06/22 01:13:27, imcheng wrote: > I'm not sure if you need to pass in remoting_source -- it's only used for > DCHECK. Removed. https://codereview.chromium.org/2951523002/diff/80001/chrome/common/media_rou... File chrome/common/media_router/mojo/media_router.mojom (right): https://codereview.chromium.org/2951523002/diff/80001/chrome/common/media_rou... chrome/common/media_router/mojo/media_router.mojom:476: // Called when a MediaRemoter is started. On 2017/06/22 01:13:27, imcheng wrote: > Could you provide a bit more info about this method, such as the meaning of the > parameters? If there are additional info elsewhere that provides context, > consider referring to them here. Done. https://codereview.chromium.org/2951523002/diff/80001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/media_router_bindings.js (right): https://codereview.chromium.org/2951523002/diff/80001/chrome/renderer/resourc... chrome/renderer/resources/extensions/media_router_bindings.js:281: makeRequest: bindings.makeRequest, On 2017/06/22 01:13:27, imcheng wrote: > Put this last since it starts with lower case.. Done. https://codereview.chromium.org/2951523002/diff/80001/media/mojo/interfaces/m... File media/mojo/interfaces/mirror_service_remoting.mojom (right): https://codereview.chromium.org/2951523002/diff/80001/media/mojo/interfaces/m... media/mojo/interfaces/mirror_service_remoting.mojom:12: // Start a remoting session. Either MirrorServiceRemotingSource.OnStarted() or On 2017/06/22 01:13:27, imcheng wrote: > Please use third person verbs when describing the method, i.e. Starts / Stops / > Sends etc. > > similarly for below and remoting_common.mojom Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Media Remoting: Add mojo interfaces between browser and extension. Currently media remoting communicates with extension by text strings through Media Router mojo API. This CL adds mojo interfaces to allow CastRemotingConnector in browser talk directly to MediaRemoter in extension after connected through the Media Router mojo API. These changes also enable passing the receiver's capablities to remoting control logic in renderer, which will be done in a follow up CL. This CL needs to work together with the changes in extension side to enable media remoting. https://critique.corp.google.com/#review/159454325 BUG=734672 ========== to ========== Media Remoting: Add mojo interfaces between browser and extension. Currently media remoting communicates with extension by text strings through Media Router mojo API. This CL adds mojo interfaces to allow CastRemotingConnector in browser talk directly to MediaRemoter in extension after connected through the Media Router mojo API. These changes also enable passing the receiver's capablities to remoting control logic in renderer, which will be done in a follow up CL. BUG=734672 ==========
Comments on PS3: https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_connector.cc (left): https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:13: #include "chrome/browser/media/cast_remoting_connector_messaging.h" You can `git rm ` this file (.h and .cc and the fuzz tests), since this isn't being used anymore. https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_connector.cc (right): https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:122: SessionTabHelper::IdForTab(contents)); What if IdForTab() returns -1? Should you not create a CastRemotingConnector, and just return null? https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:160: if (tab_id_ != -1) { Hmm...Can we prevent creating instances non-tabs? IIRC, -1 is for extension WebContentses only. Seems like the code that calls CastRemotingConnector::Get() should expect a null for extension WebContentses, or maybe not call Get() in the first place? https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:161: VLOG(3) << "Register CastRemotingConnector for tab_id = " << tab_id_; nit: Suggest VLOG(2) for most of the VLOGs added to this file. VLOG(3) usually means "expect a full screen or more of logging per second." VLOG(2) would be fine for control events. https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:205: for (RemotingBridge* notifyee : bridges_) Before this loop, let's call CastRemotingConnector::StopRemoting(active_bridge_), just in case the mojo pipes were disconnected during startup or during an active session. This will allow for more clean-up (e.g., if there are pending audio/video data pipes that need to be closed). https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:267: // Assumes the remoting session is always started successfully. Once any nit: s/Assumes/Assume/ https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:302: pending_video_sender_request_.is_pending()); See my comments in the mojom file (for the MirrorServiceRemoter interface). IMHO, you could make things much simpler (and fix a "wart" in my old code) if you bind a callback here for the return values (the stream IDs). You could also get rid of the "pending_..." member variables. Example (replacement for L294-302): const bool want_audio = audio_sender_request.is_pending(); const bool want_video = video_sender_request.is_pending(); remoter_->StartDataStreams(want_audio, want_video, base::BindOnce( &CRC::OnDataStreamsStarted, weak_factory_.GetWeakPtr(), std::move(audio_pipe), std::move(video_pipe), std::move(audio_sender_request), std::move(video_sender_request))); Notice that the pipe/request objects don't need to be stored as class members anymore, since they are moved into the bound callback. That also means, if the callback is dropped (e.g., because of a mojo error, or CRC's weak pointer is invalidated in the meantime), the pending handles will be auto-destroyed. https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:311: if (!active_bridge_) { Since this is asynch, just before this check, we should probably also sanity-check that |remoter_| is valid too: if (!remoter_) return; // Stale response to a remoter_->StartDataStreams() call. Or, maybe this should be a DCHECK(!!remoter_), and we should always ensure that weak_factory_.InvalidateWeakPtrs() is called whenever remoter_ is set to null? https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:316: if (pending_audio_sender_request_.is_pending()) { nit: "&& audio_stream_id > 0" ... and for video too. https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:372: notifyee->OnSinkAvailable(enabled_features_); Instead of this, it seems we should require that MirrorServiceRemoter call OnSinkAvailable() when it's ready again? https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:409: if (active_bridge_) I'm trying to think why OnSinkAvailable() would be called during an active session. It doesn't seem like it should. Perhaps this code should shut down what was running before? In other words, OnSinkAvailable() seems like a "ready to start next session" control signal. if (active_bridge) { LOG(WARNING) << "Unexpected OnSinkAvailable() signal: Shutting-down active bridge."; StopRemoting(active_bridge, ...something appropriate here...); } https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_connector.h (right): https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.h:14: #include "mojo/public/cpp/bindings/strong_binding.h" Looks like you meant to take this out (you're not using mojo::StrongBinding in this header file). https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.h:30: // service instance. The sink is represented by a MediaRemoter in Cast Media nit: /in Cast Media/in the Cast Media/ https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.h:46: // media_router::MediaRouter with the tab ID. When a mirroring route is nit: s/with the tab ID/with a tab ID that uniquely identifies it/ https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.h:86: // Called when a MediaRemoter is created and started in Cast MRP. This call nit: s/in Cast MRP/in the Cast MRP/ https://codereview.chromium.org/2951523002/diff/100001/media/mojo/interfaces/... File media/mojo/interfaces/mirror_service_remoting.mojom (right): https://codereview.chromium.org/2951523002/diff/100001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:13: // stared successfully. Once any failure happens, nit: s/Once/If/ Let's be optimistic, eh? :-) https://codereview.chromium.org/2951523002/diff/100001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:19: StartDataStreams(bool has_audio, bool has_video); Suggestion: This should return values so that the stream IDs aren't decoupled from the StartDataStreamsCall(). Meaning: StartDataStreams(bool audio, bool video) --> (int32 audio_stream_id, int32 video_stream_id); See comments in cast_remoting_connector.cc for more details. https://codereview.chromium.org/2951523002/diff/100001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:21: // Stops remoting media. Messages in both directions will be dropped after this 80 chars. (run `git cl format`?) https://codereview.chromium.org/2951523002/diff/100001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:39: OnDataStreamsStarted(int32 audio_stream_id, int32 video_stream_id); Per the suggestion above, you can remove this. https://codereview.chromium.org/2951523002/diff/100001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:51: // Notifies the source that fatal error occurs. Remoting session will be nit: s/that fatal error occurs/that a fatal error has occurred/ https://codereview.chromium.org/2951523002/diff/100001/media/mojo/interfaces/... File media/mojo/interfaces/remoting_common.mojom (right): https://codereview.chromium.org/2951523002/diff/100001/media/mojo/interfaces/... media/mojo/interfaces/remoting_common.mojom:1: // Copyright 2017 The Chromium Authors. All rights reserved. Can you set the similarity setting lower to show this file as a diff from remoting.mojom? Would be nice to retain code history (and to see if there are any changes from what the enums were).
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
The CQ bit was checked by xjz@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...
Addressed miu's comments. PTAL again. Thanks! https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_connector.cc (left): https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:13: #include "chrome/browser/media/cast_remoting_connector_messaging.h" On 2017/06/27 00:23:48, miu wrote: > You can `git rm ` this file (.h and .cc and the fuzz tests), since this isn't > being used anymore. Done. Removed the fuzz tests. https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_connector.cc (right): https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:122: SessionTabHelper::IdForTab(contents)); On 2017/06/27 00:23:48, miu wrote: > What if IdForTab() returns -1? Should you not create a CastRemotingConnector, > and just return null? Done. https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:160: if (tab_id_ != -1) { On 2017/06/27 00:23:48, miu wrote: > Hmm...Can we prevent creating instances non-tabs? IIRC, -1 is for extension > WebContentses only. Seems like the code that calls CastRemotingConnector::Get() > should expect a null for extension WebContentses, or maybe not call Get() in the > first place? Done. https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:161: VLOG(3) << "Register CastRemotingConnector for tab_id = " << tab_id_; On 2017/06/27 00:23:48, miu wrote: > nit: Suggest VLOG(2) for most of the VLOGs added to this file. VLOG(3) usually > means "expect a full screen or more of logging per second." VLOG(2) would be > fine for control events. Done. https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:205: for (RemotingBridge* notifyee : bridges_) On 2017/06/27 00:23:48, miu wrote: > Before this loop, let's call > CastRemotingConnector::StopRemoting(active_bridge_), just in case the mojo pipes > were disconnected during startup or during an active session. This will allow > for more clean-up (e.g., if there are pending audio/video data pipes that need > to be closed). Done. https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:267: // Assumes the remoting session is always started successfully. Once any On 2017/06/27 00:23:48, miu wrote: > nit: s/Assumes/Assume/ Done. https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:302: pending_video_sender_request_.is_pending()); On 2017/06/27 00:23:48, miu wrote: > See my comments in the mojom file (for the MirrorServiceRemoter interface). > IMHO, you could make things much simpler (and fix a "wart" in my old code) if > you bind a callback here for the return values (the stream IDs). You could also > get rid of the "pending_..." member variables. Example (replacement for > L294-302): > > const bool want_audio = audio_sender_request.is_pending(); > const bool want_video = video_sender_request.is_pending(); > remoter_->StartDataStreams(want_audio, want_video, > base::BindOnce( > &CRC::OnDataStreamsStarted, weak_factory_.GetWeakPtr(), > std::move(audio_pipe), > std::move(video_pipe), > std::move(audio_sender_request), > std::move(video_sender_request))); > > Notice that the pipe/request objects don't need to be stored as class members > anymore, since they are moved into the bound callback. That also means, if the > callback is dropped (e.g., because of a mojo error, or CRC's weak pointer is > invalidated in the meantime), the pending handles will be auto-destroyed. Done. https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:311: if (!active_bridge_) { On 2017/06/27 00:23:48, miu wrote: > Since this is asynch, just before this check, we should probably also > sanity-check that |remoter_| is valid too: > > if (!remoter_) > return; // Stale response to a remoter_->StartDataStreams() call. > > Or, maybe this should be a DCHECK(!!remoter_), and we should always ensure that > weak_factory_.InvalidateWeakPtrs() is called whenever remoter_ is set to null? Done with DCHECK. Whenever remoter_ is set to null, StopRemoting() is called, which will invalidate the weak ptr. https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:316: if (pending_audio_sender_request_.is_pending()) { On 2017/06/27 00:23:48, miu wrote: > nit: "&& audio_stream_id > 0" > > ... and for video too. Done. https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:372: notifyee->OnSinkAvailable(enabled_features_); On 2017/06/27 00:23:48, miu wrote: > Instead of this, it seems we should require that MirrorServiceRemoter call > OnSinkAvailable() when it's ready again? I think OnStopped() call already indicates that MirrorServiceRemoter is ready for next remoting session. Also, the receiver's capabilities are supposed unchanged during a cast session. So OnSinkAvailable() is only called once from the MirrorServiceRemoter when it is started. CastRemotingConnector controls telling availability/capabilities to all RemotingSources as it currently does. WDYT? https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:409: if (active_bridge_) On 2017/06/27 00:23:48, miu wrote: > I'm trying to think why OnSinkAvailable() would be called during an active > session. It doesn't seem like it should. Perhaps this code should shut down what > was running before? In other words, OnSinkAvailable() seems like a "ready to > start next session" control signal. > > if (active_bridge) { > LOG(WARNING) << "Unexpected OnSinkAvailable() signal: Shutting-down active > bridge."; > StopRemoting(active_bridge, ...something appropriate here...); > } Yes, I agree. As replied to the comment above, OnSinkAvailable() is actually only called once by MirrorServiceRemoter. How about just ignore the call during an active remoting session? https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_connector.h (right): https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.h:14: #include "mojo/public/cpp/bindings/strong_binding.h" On 2017/06/27 00:23:49, miu wrote: > Looks like you meant to take this out (you're not using mojo::StrongBinding in > this header file). Changed it to "mojo/public/cpp/bindings/binding.h". https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.h:30: // service instance. The sink is represented by a MediaRemoter in Cast Media On 2017/06/27 00:23:49, miu wrote: > nit: /in Cast Media/in the Cast Media/ Done. https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.h:46: // media_router::MediaRouter with the tab ID. When a mirroring route is On 2017/06/27 00:23:49, miu wrote: > nit: s/with the tab ID/with a tab ID that uniquely identifies it/ Done. https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.h:86: // Called when a MediaRemoter is created and started in Cast MRP. This call On 2017/06/27 00:23:49, miu wrote: > nit: s/in Cast MRP/in the Cast MRP/ Done. https://codereview.chromium.org/2951523002/diff/100001/media/mojo/interfaces/... File media/mojo/interfaces/mirror_service_remoting.mojom (right): https://codereview.chromium.org/2951523002/diff/100001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:13: // stared successfully. Once any failure happens, On 2017/06/27 00:23:49, miu wrote: > nit: s/Once/If/ Let's be optimistic, eh? :-) Done. https://codereview.chromium.org/2951523002/diff/100001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:19: StartDataStreams(bool has_audio, bool has_video); On 2017/06/27 00:23:49, miu wrote: > Suggestion: This should return values so that the stream IDs aren't decoupled > from the StartDataStreamsCall(). Meaning: > > StartDataStreams(bool audio, bool video) --> (int32 audio_stream_id, int32 > video_stream_id); > > See comments in cast_remoting_connector.cc for more details. Done. https://codereview.chromium.org/2951523002/diff/100001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:21: // Stops remoting media. Messages in both directions will be dropped after this On 2017/06/27 00:23:49, miu wrote: > 80 chars. (run `git cl format`?) Done. https://codereview.chromium.org/2951523002/diff/100001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:39: OnDataStreamsStarted(int32 audio_stream_id, int32 video_stream_id); On 2017/06/27 00:23:49, miu wrote: > Per the suggestion above, you can remove this. Done. https://codereview.chromium.org/2951523002/diff/100001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:51: // Notifies the source that fatal error occurs. Remoting session will be On 2017/06/27 00:23:49, miu wrote: > nit: s/that fatal error occurs/that a fatal error has occurred/ Done. https://codereview.chromium.org/2951523002/diff/100001/media/mojo/interfaces/... File media/mojo/interfaces/remoting_common.mojom (right): https://codereview.chromium.org/2951523002/diff/100001/media/mojo/interfaces/... media/mojo/interfaces/remoting_common.mojom:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/06/27 00:23:49, miu wrote: > Can you set the similarity setting lower to show this file as a diff from > remoting.mojom? Would be nice to retain code history (and to see if there are > any changes from what the enums were). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Comments on PS5: https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_connector.cc (right): https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:372: notifyee->OnSinkAvailable(enabled_features_); On 2017/06/28 02:09:39, xjz wrote: > On 2017/06/27 00:23:48, miu wrote: > > Instead of this, it seems we should require that MirrorServiceRemoter call > > OnSinkAvailable() when it's ready again? > > I think OnStopped() call already indicates that MirrorServiceRemoter is ready > for next remoting session. Also, the receiver's capabilities are supposed > unchanged during a cast session. So OnSinkAvailable() is only called once from > the MirrorServiceRemoter when it is started. CastRemotingConnector controls > telling availability/capabilities to all RemotingSources as it currently does. > WDYT? There is a significant period of time (1-3 seconds) between "stopped this session" and "ready to start the next session." For example, it takes time to switch the Chromecast back into mirroring mode; and we don't want to say remoting is ready until that has completed. That is why the separate OnSinkAvailable() is necessary. Also, why must Capabilities be constant? There are reasons it could be different, after learning something from executing the last remoting session. For example, network bandwidth capacity/latency may have been worse than originally anticipated; or maybe the device reports support for some codec or feature that it actually does not support (not that this would ever happen). https://codereview.chromium.org/2951523002/diff/180001/media/mojo/interfaces/... File media/mojo/interfaces/mirror_service_remoting.mojom (right): https://codereview.chromium.org/2951523002/diff/180001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:37: OnSinkAvailable(array<media.mojom.RemotingSinkCapabilities> capabilities); As discussed face-to-face, I suggest this be a struct that consists of various properties. An array<enum> doesn't, for example, allow us to specify varying data (like network bandwidth values, etc.). It's also much easier to add new capabilities fields in a struct in the future. https://codereview.chromium.org/2951523002/diff/180001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:49: // Notifies the source that a fatal error occurs. Remoting session will be s/error occurs/error has occurred/ https://codereview.chromium.org/2951523002/diff/180001/media/mojo/interfaces/... File media/mojo/interfaces/remoting_common.mojom (right): https://codereview.chromium.org/2951523002/diff/180001/media/mojo/interfaces/... media/mojo/interfaces/remoting_common.mojom:23: VIDEO_CODEC_H264, Suggest adding: AUDIO_CODEC_BASELINE_SET and VIDEO_CODEC_BASELINE_SET to represent the "baseline set" of codecs. Note that this is important for things like audio-only devices, where the lack of any VIDEO_CODEC would allow our control logic to determine that video remoting is not possible.
miu@: Thanks for reviewing. Addressed your comments. And also updated the CL on extension side. PTAL both. Thanks! https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_connector.cc (right): https://codereview.chromium.org/2951523002/diff/100001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:372: notifyee->OnSinkAvailable(enabled_features_); On 2017/06/29 20:46:50, miu wrote: > On 2017/06/28 02:09:39, xjz wrote: > > On 2017/06/27 00:23:48, miu wrote: > > > Instead of this, it seems we should require that MirrorServiceRemoter call > > > OnSinkAvailable() when it's ready again? > > > > I think OnStopped() call already indicates that MirrorServiceRemoter is ready > > for next remoting session. Also, the receiver's capabilities are supposed > > unchanged during a cast session. So OnSinkAvailable() is only called once from > > the MirrorServiceRemoter when it is started. CastRemotingConnector controls > > telling availability/capabilities to all RemotingSources as it currently does. > > WDYT? > > There is a significant period of time (1-3 seconds) between "stopped this > session" and "ready to start the next session." For example, it takes time to > switch the Chromecast back into mirroring mode; and we don't want to say > remoting is ready until that has completed. That is why the separate > OnSinkAvailable() is necessary. > > Also, why must Capabilities be constant? There are reasons it could be > different, after learning something from executing the last remoting session. > For example, network bandwidth capacity/latency may have been worse than > originally anticipated; or maybe the device reports support for some codec or > feature that it actually does not support (not that this would ever happen). Done as discussed offline. https://codereview.chromium.org/2951523002/diff/180001/media/mojo/interfaces/... File media/mojo/interfaces/mirror_service_remoting.mojom (right): https://codereview.chromium.org/2951523002/diff/180001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:37: OnSinkAvailable(array<media.mojom.RemotingSinkCapabilities> capabilities); On 2017/06/29 20:46:50, miu wrote: > As discussed face-to-face, I suggest this be a struct that consists of various > properties. An array<enum> doesn't, for example, allow us to specify varying > data (like network bandwidth values, etc.). It's also much easier to add new > capabilities fields in a struct in the future. Done. https://codereview.chromium.org/2951523002/diff/180001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:49: // Notifies the source that a fatal error occurs. Remoting session will be On 2017/06/29 20:46:50, miu wrote: > s/error occurs/error has occurred/ Done. https://codereview.chromium.org/2951523002/diff/180001/media/mojo/interfaces/... File media/mojo/interfaces/remoting_common.mojom (right): https://codereview.chromium.org/2951523002/diff/180001/media/mojo/interfaces/... media/mojo/interfaces/remoting_common.mojom:23: VIDEO_CODEC_H264, On 2017/06/29 20:46:50, miu wrote: > Suggest adding: AUDIO_CODEC_BASELINE_SET and VIDEO_CODEC_BASELINE_SET to > represent the "baseline set" of codecs. Note that this is important for things > like audio-only devices, where the lack of any VIDEO_CODEC would allow our > control logic to determine that video remoting is not possible. Done.
Comments on PS6 (just mojo interface tweaks): https://codereview.chromium.org/2951523002/diff/200001/media/mojo/interfaces/... File media/mojo/interfaces/remoting_common.mojom (right): https://codereview.chromium.org/2951523002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/remoting_common.mojom:17: enum RemotingSinkCapabilities { The values in this enum cover different "domains." Suggest splitting it up: enum RemotingSinkFeatures { RENDERING, CONTENT_DECRYPTION, }; enum RemotingSinkAudioCapabilities { CODEC_BASELINE_SET, CODEC_AAC, CODEC_OPUS, }; enum RemotingSinkVideoCapabilities { 4K, ...codecs... }; https://codereview.chromium.org/2951523002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/remoting_common.mojom:39: RemotingSinkCapabilities enable_flag; Might as well make this a set too: array<RemotingSinkFeatures> features; https://codereview.chromium.org/2951523002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/remoting_common.mojom:42: array<RemotingSinkCapabilities> key_system; Let's not add |key_system| until later.
Addressed miu's comments. Also updated the patch in extension accordingly. PTAL again. Thanks! https://codereview.chromium.org/2951523002/diff/200001/media/mojo/interfaces/... File media/mojo/interfaces/remoting_common.mojom (right): https://codereview.chromium.org/2951523002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/remoting_common.mojom:17: enum RemotingSinkCapabilities { On 2017/06/30 22:02:53, miu wrote: > The values in this enum cover different "domains." Suggest splitting it up: > > enum RemotingSinkFeatures { > RENDERING, > CONTENT_DECRYPTION, > }; > > enum RemotingSinkAudioCapabilities { > CODEC_BASELINE_SET, > CODEC_AAC, > CODEC_OPUS, > }; > > enum RemotingSinkVideoCapabilities { > 4K, > ...codecs... > }; Done. https://codereview.chromium.org/2951523002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/remoting_common.mojom:39: RemotingSinkCapabilities enable_flag; On 2017/06/30 22:02:53, miu wrote: > Might as well make this a set too: > > array<RemotingSinkFeatures> features; Done. https://codereview.chromium.org/2951523002/diff/200001/media/mojo/interfaces/... media/mojo/interfaces/remoting_common.mojom:42: array<RemotingSinkCapabilities> key_system; On 2017/06/30 22:02:53, miu wrote: > Let's not add |key_system| until later. Done.
PS7 lgtm. https://codereview.chromium.org/2951523002/diff/220001/media/mojo/interfaces/... File media/mojo/interfaces/remoting.mojom (right): https://codereview.chromium.org/2951523002/diff/220001/media/mojo/interfaces/... media/mojo/interfaces/remoting.mojom:76: // TODO(xjz): Replaced this with the SinkCapabilities struct defined in nit: s/Replaced/Replace/
xjz@chromium.org changed reviewers: + jochen@chromium.org, xhwang@chromium.org
jochen@: PTAL changes to chrome/renderer/*. xhwang@: PTAL changes to media/mojo/interfaces/*. Thanks!
The CQ bit was checked by xjz@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 xjz@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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
I'm not a good reviewer for media_router_bindings.js - is somebody else covering that file?
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Patchset #8 (id:240001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/07/03 07:54:05, jochen wrote: > I'm not a good reviewer for media_router_bindings.js - is somebody else covering > that file? Thanks jochen@. imcheng@ will cover that file and other Media Router related changes. Do other changes lgty? Still need your RS. :)
lgtm % comments https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_connector.cc (right): https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:119: const SessionID::id_type tab_id = SessionTabHelper::IdForTab(contents); FYI: MediaRouterAndroid use TabAndroid::GetAndroidId to get the tab ID on Android. Add a note / todo here to update this when we want to support remoting on Android? https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:399: // The receiver's capabilities are supposed unchanged during an active s/are supposed/should be https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_connector_unittest.cc (right): https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector_unittest.cc:59: DCHECK_EQ(-1, tab_id_); Should these DCHECKs be EXPECTs? https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector_unittest.cc:83: int32_t tab_id() { return tab_id_; } const https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector_unittest.cc:87: CastRemotingConnector* connector_; = nullptr, to be safe? https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector_unittest.cc:123: if (source_) { Will source_ ever be "nullptr"? Seems it is assigned to "non-null" during construction. https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/media_router_base.cc (right): https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/media_router_base.cc:166: remoting_sources_.emplace(tab_id, std::move(remoting_source)); The std::move isn't necessary? https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/media_router_base.h (right): https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/media_router_base.h:83: std::unordered_map<int32_t, CastRemotingConnector*> remoting_sources_; Consider using base::flat_map if the number of entries is expected to be small? (< 3-4 entries) https://cs.chromium.org/chromium/src/base/containers/README.md?type=cs https://codereview.chromium.org/2951523002/diff/260001/chrome/renderer/resour... File chrome/renderer/resources/extensions/media_router_bindings.js (right): https://codereview.chromium.org/2951523002/diff/260001/chrome/renderer/resour... chrome/renderer/resources/extensions/media_router_bindings.js:485: * @param {!remotingMojom.MirrorServiceRemotingSourcePtr} remoting_source s/remoting_source/remotingSource here and below https://codereview.chromium.org/2951523002/diff/260001/media/mojo/interfaces/... File media/mojo/interfaces/mirror_service_remoting.mojom (right): https://codereview.chromium.org/2951523002/diff/260001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:20: => (int32 audio_stream_id, int32 video_stream_id); Are the IDs returned guaranteed to be valid if the bool is true? How should CastRemotingConnector handle the case where both ids are -1?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
media/mojo/interfaces/* rs LGTM. You probably will need security owners to review .mojom files.
xjz@chromium.org changed reviewers: + rsesek@chromium.org
Addressed comments. Thanks! Robert: Need your RS on *.mojom files. PTAL. Thanks! https://codereview.chromium.org/2951523002/diff/220001/media/mojo/interfaces/... File media/mojo/interfaces/remoting.mojom (right): https://codereview.chromium.org/2951523002/diff/220001/media/mojo/interfaces/... media/mojo/interfaces/remoting.mojom:76: // TODO(xjz): Replaced this with the SinkCapabilities struct defined in On 2017/06/30 23:58:06, miu wrote: > nit: s/Replaced/Replace/ Done. https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_connector.cc (right): https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:119: const SessionID::id_type tab_id = SessionTabHelper::IdForTab(contents); On 2017/07/05 18:53:08, imcheng wrote: > FYI: MediaRouterAndroid use TabAndroid::GetAndroidId to get the tab ID on > Android. Add a note / todo here to update this when we want to support remoting > on Android? Done. https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector.cc:399: // The receiver's capabilities are supposed unchanged during an active On 2017/07/05 18:53:08, imcheng wrote: > s/are supposed/should be Done. https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/c... File chrome/browser/media/cast_remoting_connector_unittest.cc (right): https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector_unittest.cc:59: DCHECK_EQ(-1, tab_id_); On 2017/07/05 18:53:08, imcheng wrote: > Should these DCHECKs be EXPECTs? Done. https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector_unittest.cc:83: int32_t tab_id() { return tab_id_; } On 2017/07/05 18:53:08, imcheng wrote: > const Done. https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector_unittest.cc:87: CastRemotingConnector* connector_; On 2017/07/05 18:53:08, imcheng wrote: > = nullptr, to be safe? Done. https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/c... chrome/browser/media/cast_remoting_connector_unittest.cc:123: if (source_) { On 2017/07/05 18:53:08, imcheng wrote: > Will source_ ever be "nullptr"? Seems it is assigned to "non-null" during > construction. Yes, you are correct. Removed all the if condition check. https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/media_router_base.cc (right): https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/media_router_base.cc:166: remoting_sources_.emplace(tab_id, std::move(remoting_source)); On 2017/07/05 18:53:08, imcheng wrote: > The std::move isn't necessary? Done. https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/r... File chrome/browser/media/router/media_router_base.h (right): https://codereview.chromium.org/2951523002/diff/260001/chrome/browser/media/r... chrome/browser/media/router/media_router_base.h:83: std::unordered_map<int32_t, CastRemotingConnector*> remoting_sources_; On 2017/07/05 18:53:08, imcheng wrote: > Consider using base::flat_map if the number of entries is expected to be small? > (< 3-4 entries) > https://cs.chromium.org/chromium/src/base/containers/README.md?type=cs There will be one CastRemotingConnector for each webContent, so the number of entries might be large. As discussed offline, keep using std::unordered_map. https://codereview.chromium.org/2951523002/diff/260001/chrome/renderer/resour... File chrome/renderer/resources/extensions/media_router_bindings.js (right): https://codereview.chromium.org/2951523002/diff/260001/chrome/renderer/resour... chrome/renderer/resources/extensions/media_router_bindings.js:485: * @param {!remotingMojom.MirrorServiceRemotingSourcePtr} remoting_source On 2017/07/05 18:53:08, imcheng wrote: > s/remoting_source/remotingSource here and below Done. https://codereview.chromium.org/2951523002/diff/260001/media/mojo/interfaces/... File media/mojo/interfaces/mirror_service_remoting.mojom (right): https://codereview.chromium.org/2951523002/diff/260001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:20: => (int32 audio_stream_id, int32 video_stream_id); On 2017/07/05 18:53:09, imcheng wrote: > Are the IDs returned guaranteed to be valid if the bool is true? How should > CastRemotingConnector handle the case where both ids are -1? These IDs could be -1 when error occurred. CastRemotingConnector only connects the data mojo pipe with the CastRemotingSender for valid IDs.
The CQ bit was checked by xjz@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by xjz@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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
https://codereview.chromium.org/2951523002/diff/300001/media/mojo/interfaces/... File media/mojo/interfaces/mirror_service_remoting.mojom (right): https://codereview.chromium.org/2951523002/diff/300001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:20: => (int32 audio_stream_id, int32 video_stream_id); What does this return if one or both of the booleans are false? https://codereview.chromium.org/2951523002/diff/300001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:50: // be stopped immediately once this is called. Does OnStopped get delivered too? https://codereview.chromium.org/2951523002/diff/300001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:51: OnError(); Should there be an error code?
Thanks for reviewing. Addressed Robert's comments. PTAL again. https://codereview.chromium.org/2951523002/diff/300001/media/mojo/interfaces/... File media/mojo/interfaces/mirror_service_remoting.mojom (right): https://codereview.chromium.org/2951523002/diff/300001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:20: => (int32 audio_stream_id, int32 video_stream_id); On 2017/07/06 23:24:15, Robert Sesek wrote: > What does this return if one or both of the booleans are false? The stream ID will be -1. Added comments. https://codereview.chromium.org/2951523002/diff/300001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:50: // be stopped immediately once this is called. On 2017/07/06 23:24:15, Robert Sesek wrote: > Does OnStopped get delivered too? OnStopped() might be called later, but not always. OnStopped() is either in response to a MirrorServiceRemoter.Stop() call or some events (e.g. user stops casting). The current data flow is when any error occurs, OnError() is called to notify the source and the source will stop remoting immediately. If the MirrorServiceRemoter is still connected to the source, it receivers the Stop() request and will call OnStopped() later. But if the MirrorServiceRemoter is disconnected after error occurs, OnStopped() will not be called. https://codereview.chromium.org/2951523002/diff/300001/media/mojo/interfaces/... media/mojo/interfaces/mirror_service_remoting.mojom:51: OnError(); On 2017/07/06 23:24:15, Robert Sesek wrote: > Should there be an error code? There are no error codes right now. It is currently called from Media Router extension, and the error message is logged out. We might add error codes later with some refactoring. Added a TODO comment.
LGTM
The CQ bit was checked by xjz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, jochen@chromium.org, imcheng@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2951523002/#ps320001 (title: "Addressed Robert's comments.")
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #12 (id:340001) has been deleted
The CQ bit was checked by xjz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, jochen@chromium.org, imcheng@chromium.org, xhwang@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/2951523002/#ps360001 (title: "Rebased again.")
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": 360001, "attempt_start_ts": 1499452635953520,
"parent_rev": "7accad67a9abf16cb043bc4affb26332e90454ff", "commit_rev":
"ef7927fa28cca59d8ca4ed06f540a38702ac0810"}
Message was sent while issue was closed.
Description was changed from ========== Media Remoting: Add mojo interfaces between browser and extension. Currently media remoting communicates with extension by text strings through Media Router mojo API. This CL adds mojo interfaces to allow CastRemotingConnector in browser talk directly to MediaRemoter in extension after connected through the Media Router mojo API. These changes also enable passing the receiver's capablities to remoting control logic in renderer, which will be done in a follow up CL. BUG=734672 ========== to ========== Media Remoting: Add mojo interfaces between browser and extension. Currently media remoting communicates with extension by text strings through Media Router mojo API. This CL adds mojo interfaces to allow CastRemotingConnector in browser talk directly to MediaRemoter in extension after connected through the Media Router mojo API. These changes also enable passing the receiver's capablities to remoting control logic in renderer, which will be done in a follow up CL. BUG=734672 Review-Url: https://codereview.chromium.org/2951523002 Cr-Commit-Position: refs/heads/master@{#485043} Committed: https://chromium.googlesource.com/chromium/src/+/ef7927fa28cca59d8ca4ed06f540... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:360001) as https://chromium.googlesource.com/chromium/src/+/ef7927fa28cca59d8ca4ed06f540... |
