Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2728543009:
[Media Router] Custom Controls 2 - add MediaRouter::GetRouteController() (Closed)
|
Created:
3 years, 9 months ago by takumif Modified:
3 years, 8 months ago CC:
Aaron Boodman, abarth-chromium, chfremer+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, darin (slow to review), extensions-reviews_chromium.org, feature-media-reviews_chromium.org, mcasas+watch+vc_chromium.org, media-router+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Media Router] Custom Controls 2 - add MediaRouter::GetRouteController()
This CL adds GetRouteController() to the MediaRouter API. The method returns a
scoped_refptr to a MediaRouteController that is connected to the MediaController
in the component extension.
MediaRouterMojoImpl will keep a map of raw pointers to MediaRouteControllers
that are valid and are owned by their observers. MediaRouterMojoImpl is notified
when a controller is invalidated, so that it can be removed from the map.
GetRouteController() will be called in the next CL by MediaRouterUI for
instantiating an observer for MediaRouteController.
We also add two methods to the MediaRouteProvider mojo interface that are called
by GetRouteController():
- CreateMediaRouteController(): called for creating the extension-side
MediaController.
- SetMediaRouteStatusObserver(): called for setting the browser-side
MediaRouteController as a status observer.
The Chromium-side implementation of custom controls will be done in these
patches:
1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002
2. MediaRouter::GetRouteController(): this patch
3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002
4. Custom controls WebUI: http://crrev/2725503002
Custom controls design doc:
https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4G0/edit
BUG=684635
Review-Url: https://codereview.chromium.org/2728543009
Cr-Commit-Position: refs/heads/master@{#465148}
Committed: https://chromium.googlesource.com/chromium/src/+/3c8ebf96c1e2432763cac1a6fa97cf5217a8728c
Patch Set 1 : . #Patch Set 2 : More tests #
Total comments: 34
Patch Set 3 : Address Daniel's and Derek's comments #
Total comments: 24
Patch Set 4 : Address Mark's comments #
Total comments: 10
Patch Set 5 : Address Derek's comments #
Total comments: 20
Patch Set 6 : Rebase #Patch Set 7 : Address Derek's comments #
Total comments: 11
Patch Set 8 : Address Derek's comments #
Total comments: 24
Patch Set 9 : Address Mark's comments #Patch Set 10 : Don't create controller if binding is invalid #
Total comments: 11
Patch Set 11 : Address Daniel's comments #Patch Set 12 : Combine CreateMRController and SetMRStatusObserver #Messages
Total messages: 66 (37 generated)
Description was changed from ========== [Media Router] Custom Controls 2 - add MediaRouter::CreateMediaRouteController() BUG= ========== to ========== [Media Router] Custom Controls 2 - add MediaRouter::CreateMediaRouteController() BUG=684635 ==========
Description was changed from ========== [Media Router] Custom Controls 2 - add MediaRouter::CreateMediaRouteController() BUG=684635 ========== to ========== [Media Router] Custom Controls 2 - add MediaRouter::CreateMediaRouteController() The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::CreateMediaRouteController(): this patch 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... BUG=684635 ==========
Patchset #4 (id:60001) has been deleted
Description was changed from ========== [Media Router] Custom Controls 2 - add MediaRouter::CreateMediaRouteController() The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::CreateMediaRouteController(): this patch 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... BUG=684635 ========== to ========== [Media Router] Custom Controls 2 - add MediaRouter::GetRouteController() The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): this patch 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... BUG=684635 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== [Media Router] Custom Controls 2 - add MediaRouter::GetRouteController() The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): this patch 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... BUG=684635 ========== to ========== [Media Router] Custom Controls 2 - add MediaRouter::GetRouteController() This CL adds GetRouteController() to the MediaRouter API. The method returns a scoped_refptr to a MediaRouteController that is connected to the MediaController in the component extension. GetRouteController() will be called in the next CL by MediaRouterUI for instantiating an observer for MediaRouteController. MediaRouterMojoImpl will keep a map of raw pointers to MediaRouteControllers that are valid and are owned by their observers. MediaRouterMojoImpl is notified when a controller is invalidated, so that it can be removed from the map. We also add two methods to the MediaRouteProvider mojo interface that are called by GetRouteController(): - CreateMediaRouteController(): called for creating the extension-side MediaController. - SetMediaRouteStatusObserver(): called for setting the browser-side MediaRouteController as a status observer. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): this patch 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... BUG=684635 ==========
Description was changed from ========== [Media Router] Custom Controls 2 - add MediaRouter::GetRouteController() This CL adds GetRouteController() to the MediaRouter API. The method returns a scoped_refptr to a MediaRouteController that is connected to the MediaController in the component extension. GetRouteController() will be called in the next CL by MediaRouterUI for instantiating an observer for MediaRouteController. MediaRouterMojoImpl will keep a map of raw pointers to MediaRouteControllers that are valid and are owned by their observers. MediaRouterMojoImpl is notified when a controller is invalidated, so that it can be removed from the map. We also add two methods to the MediaRouteProvider mojo interface that are called by GetRouteController(): - CreateMediaRouteController(): called for creating the extension-side MediaController. - SetMediaRouteStatusObserver(): called for setting the browser-side MediaRouteController as a status observer. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): this patch 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... BUG=684635 ========== to ========== [Media Router] Custom Controls 2 - add MediaRouter::GetRouteController() This CL adds GetRouteController() to the MediaRouter API. The method returns a scoped_refptr to a MediaRouteController that is connected to the MediaController in the component extension. MediaRouterMojoImpl will keep a map of raw pointers to MediaRouteControllers that are valid and are owned by their observers. MediaRouterMojoImpl is notified when a controller is invalidated, so that it can be removed from the map. GetRouteController() will be called in the next CL by MediaRouterUI for instantiating an observer for MediaRouteController. We also add two methods to the MediaRouteProvider mojo interface that are called by GetRouteController(): - CreateMediaRouteController(): called for creating the extension-side MediaController. - SetMediaRouteStatusObserver(): called for setting the browser-side MediaRouteController as a status observer. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): this patch 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... BUG=684635 ==========
takumif@chromium.org changed reviewers: + dcheng@chromium.org, imcheng@chromium.org, mfoltz@chromium.org
Please take a look at: dcheng: media_router.mojom imcheng: overall mfoltz: overall Thanks!
https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:38: media_router_(media_router), Shall we DCHECK(media_router)? https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:378: (bool success); Out of curiosity, would it make sense to just have this return a MediaController? directly? Then I think we might not need Invalidate()
Looks good overall. I have some suggestions in how to make the API a bit safer from misuse. https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:193: // Returns a nullptr if no MediaRoute exists for the given |route_id|, if a nit: Remove the "or if on Android" part. https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:199: // Called when the MediaRouteController for |route_id| has been destroyed. nit: Describe what this method does instead of when it is called. https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:200: virtual void OnRouteControllerDestroyed(const MediaRoute::Id& route_id) = 0; It probably doesn't make sense for this method to be part of the public API. I would probably put this in private + friend class MediaRouteController. https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:200: virtual void OnRouteControllerDestroyed(const MediaRoute::Id& route_id) = 0; Naming suggestion: DetachRouteController or UnregisterRouteController. On* is usually used for observer methods. https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:42: base::Bind(&MediaRouteController::Invalidate, base::Unretained(this))); Suggest adding a OnMojoConnectionError method that would call into MR to detach itself and call Invalidate(). https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:70: void MediaRouteController::Invalidate() { To make this API a bit more safe from implementation errors (since it does require a very specific contract to be followed): - Add a boolean "is_valid" that tracks whether this controller instance is no longer valid and detached from MediaRouter. You can use the boolean to perform DCHECKs and other checks, such as in AddObserver(). - In On MojoConnectionError(), call into MediaRouter to detach the controller proactively rather than relying on dtor cleanup behavior. https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:77: return binding_.CreateInterfacePtrAndBind(); Did you mean to store it in a variable and return it last? https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:115: MediaRouter* media_router_; MediaRouter* const https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:378: (bool success); On 2017/03/27 22:15:26, dcheng wrote: > Out of curiosity, would it make sense to just have this return a > MediaController? directly? Then I think we might not need Invalidate() We still need a way to invalidate the MediaRouteController, if it encounters a Mojo connection error in the MediaController / MediaStatusObserver message pipes. Functionally, there is no big difference between this API, and one where a MediaController is returned. One advantage of this API is that the message pipe is created and bound in the browser, which means clients can begin using MediaController without waiting for the implementation to be bound in the extension side first. https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:427: const MediaRoute::Id& route_id) { Consider passing in the controller pointer into this method and use it to check against the value in the map. It can help prevent scenarios like the following (suppose controllers 1 and 2 are of the same route): Controller 1 created Controller 1 invalidated, removed from map Controller 2 created Controller 1 destroyed -> MRMojoImpl removes controller, which is not correct https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1034: if (base::ContainsKey(route_controllers_, route_id) && !success) Check !success first. Also you can combine the two calls into route_controllers_ with a find(). https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1035: route_controllers_.at(route_id)->Invalidate(); Also remove the entry from the map to be safe? We should detach the controller from MR as soon as we find out that it is invalid. https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right): https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.h:381: // Removes controllers from |route_controllers_| whose media routes do not Invalidates and removes https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc (right): https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1340: } Can you also add a test case where the route controller cannot be created?
Patchset #3 (id:120001) has been deleted
https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:193: // Returns a nullptr if no MediaRoute exists for the given |route_id|, if a On 2017/03/28 01:18:37, imcheng wrote: > nit: Remove the "or if on Android" part. Done. https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:199: // Called when the MediaRouteController for |route_id| has been destroyed. On 2017/03/28 01:18:37, imcheng wrote: > nit: Describe what this method does instead of when it is called. Done. https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:200: virtual void OnRouteControllerDestroyed(const MediaRoute::Id& route_id) = 0; On 2017/03/28 01:18:37, imcheng wrote: > Naming suggestion: DetachRouteController or UnregisterRouteController. On* is > usually used for observer methods. Changing to DetachRouteController(). https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:200: virtual void OnRouteControllerDestroyed(const MediaRoute::Id& route_id) = 0; On 2017/03/28 01:18:37, imcheng wrote: > It probably doesn't make sense for this method to be part of the public API. I > would probably put this in private + friend class MediaRouteController. Done. https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:25: controller_->RemoveObserver(this); No longer necessary because the controller won't use the observer list after |is_valid_| becomes false. https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:38: media_router_(media_router), On 2017/03/27 22:15:26, dcheng wrote: > Shall we DCHECK(media_router)? Done. https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:42: base::Bind(&MediaRouteController::Invalidate, base::Unretained(this))); On 2017/03/28 01:18:37, imcheng wrote: > Suggest adding a OnMojoConnectionError method that would call into MR to detach > itself and call Invalidate(). I'm not sure if we need OnMojoConnectionError() and Invalidate() separately, if the point is to keep DetachRouteController() out of Invalidate(). We can just call DetachRouteController() from Invalidate(), since Invalidate() can call the dtor and the dtor has to call DetachRouteController() anyways (to cover the case in which the controller is deleted by deleting all the scoped_refptrs to it). https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:70: void MediaRouteController::Invalidate() { On 2017/03/28 01:18:37, imcheng wrote: > To make this API a bit more safe from implementation errors (since it does > require a very specific contract to be followed): > > - Add a boolean "is_valid" that tracks whether this controller instance is no > longer valid and detached from MediaRouter. You can use the boolean to perform > DCHECKs and other checks, such as in AddObserver(). > - In On MojoConnectionError(), call into MediaRouter to detach the controller > proactively rather than relying on dtor cleanup behavior. Added is_valid_ that gets set to false in Invalidate(). Since Invalidate() notifies the observers, the observers shouldn't/can't be using the controller after the controller becomes invalid. https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:77: return binding_.CreateInterfacePtrAndBind(); On 2017/03/28 01:18:37, imcheng wrote: > Did you mean to store it in a variable and return it last? Yes. I'm surprised that this even compiled. https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:115: MediaRouter* media_router_; On 2017/03/28 01:18:38, imcheng wrote: > MediaRouter* const Done. https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:427: const MediaRoute::Id& route_id) { On 2017/03/28 01:18:38, imcheng wrote: > Consider passing in the controller pointer into this method and use it to check > against the value in the map. It can help prevent scenarios like the following > (suppose controllers 1 and 2 are of the same route): > > Controller 1 created > Controller 1 invalidated, removed from map > Controller 2 created > Controller 1 destroyed -> MRMojoImpl removes controller, which is not correct Done. https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1034: if (base::ContainsKey(route_controllers_, route_id) && !success) On 2017/03/28 01:18:38, imcheng wrote: > Check !success first. Also you can combine the two calls into route_controllers_ > with a find(). Done. https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1035: route_controllers_.at(route_id)->Invalidate(); On 2017/03/28 01:18:38, imcheng wrote: > Also remove the entry from the map to be safe? We should detach the controller > from MR as soon as we find out that it is invalid. Calling DetachRouteController() from Invalidate(). https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right): https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.h:381: // Removes controllers from |route_controllers_| whose media routes do not On 2017/03/28 01:18:38, imcheng wrote: > Invalidates and removes Done. https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc (right): https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1340: } On 2017/03/28 01:18:38, imcheng wrote: > Can you also add a test case where the route controller cannot be created? Done.
https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:195: // or if on Android. or just "if the controller could not be instantiated" https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:199: // Called when the MediaRouteController for |route_id| has been destroyed. On 2017/03/28 at 01:18:37, imcheng wrote: > nit: Describe what this method does instead of when it is called. It's virtual so it doesn't do anything. If there is a specific operation it *must* do it should be named to reflect that. https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:194: // connection with the extension-side controller could not be established. How does this behave on Android? Suggest dropping "extension-side," it's an implementation detail that could belong as a comment on MediaRouterMojoImpl. https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:79: is_valid_ = false; Is the concern that the OnMediaStatusUpdated will get called after Invalidate()? Or that DetachMediaRouteController/observer.InvalidateController() will somehow call back onto |this|? https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:97: if (is_valid_) DCHECK(!is_valid_)? https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:96: // already set, then the previous binding gets invalidated. When would this binding need to be reset? My understanding was that the controller object <--> ObserverPtr would be 1:1. https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:371: // for it, or there was an error while creating a controller. I think an earlier draft returned an existing controller if it already exists. Is it possible to do that here? Otherwise this is a little fragile - only the code that owns the previously-created controller can call it safely. Since I'm pretty sure only the MR is ever going to call this code (and it owns the map of routes to controllers) not a big deal. More of a general API design bikeshed. https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:373: // created controller is destroyed when |media_controller| becomes invalid. If a returned controller is invalidated/destroyed, can this method be called again on the same route id to create a new one? Please clarify comment above. https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:410: mojom::MediaControllerPtr media_controller; Can you prefix Mojo variables with mojo_ for readability? https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:416: media_route_provider_->CreateMediaRouteController( What guarantees that the event page is alive? Most calls through to the MRPM will queues requests if necessary until the event page is ready to receive them. https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:423: return route_controller; What state is this in before OnMediaControllerCreated is invoked? https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1027: controller->Invalidate(); Doesn't this call back into DetachRouteController() above (which will be a no-op since you just erase()d controller)? Can you add a specific method like controller->OnRouteInvalid() that will drop observers without re-entrancy here? https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1034: void MediaRouterMojoImpl::OnMediaControllerCreated( Shouldn't this return a boolean per the mojom?
https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2728543009/diff/100001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:42: base::Bind(&MediaRouteController::Invalidate, base::Unretained(this))); On 2017/03/29 02:34:49, takumif wrote: > On 2017/03/28 01:18:37, imcheng wrote: > > Suggest adding a OnMojoConnectionError method that would call into MR to > detach > > itself and call Invalidate(). > > I'm not sure if we need OnMojoConnectionError() and Invalidate() separately, if > the point is to keep DetachRouteController() out of Invalidate(). We can just > call DetachRouteController() from Invalidate(), since Invalidate() can call the > dtor and the dtor has to call DetachRouteController() anyways (to cover the case > in which the controller is deleted by deleting all the scoped_refptrs to it). Right now we have MR::RemoveInvalidRouteControllers -> MRC::Invalidate -> MR::DetachRouteController, which is redundant. By separating the Mojo error case from the route terminated case, it's clearer who is supposed to perform the map entry cleanup. https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:96: // already set, then the previous binding gets invalidated. On 2017/03/30 23:03:28, mark a. foltz wrote: > When would this binding need to be reset? My understanding was that the > controller object <--> ObserverPtr would be 1:1. I think we should add a DCHECK(!binding_.is_bound()) inside this method so we don't accidentally call this twice. https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:371: // for it, or there was an error while creating a controller. On 2017/03/30 23:03:28, mark a. foltz wrote: > I think an earlier draft returned an existing controller if it already exists. > Is it possible to do that here? Otherwise this is a little fragile - only the > code that owns the previously-created controller can call it safely. > > Since I'm pretty sure only the MR is ever going to call this code (and it owns > the map of routes to controllers) not a big deal. More of a general API design > bikeshed. We can bind multiple interface requests to the same controller instance in the component, if we need to do this in the future. For now, it is slight simpler if we can assume if there can only be one binding per controller.
Patchset #4 (id:160001) has been deleted
https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:194: // connection with the extension-side controller could not be established. On 2017/03/30 23:03:28, mfoltz_ooo_until_4_10 wrote: > How does this behave on Android? Suggest dropping "extension-side," it's an > implementation detail that could belong as a comment on MediaRouterMojoImpl. It'd return a nullptr for now. Dropped the last part since invalidation is done asynchronously now. https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:79: is_valid_ = false; On 2017/03/30 23:03:28, mfoltz_ooo_until_4_10 wrote: > Is the concern that the OnMediaStatusUpdated will get called after Invalidate()? Yes, and addition of new observers during or after Invalidate(). > Or that DetachMediaRouteController/observer.InvalidateController() will somehow > call back onto |this|? https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:97: if (is_valid_) On 2017/03/30 23:03:28, mark a. foltz wrote: > DCHECK(!is_valid_)? is_valid_ would be true here if the controller was destroyed by closing all route details views (disposing all scoped_refptrs), in which case Invalidate() wouldn't get called. https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:96: // already set, then the previous binding gets invalidated. On 2017/03/31 01:30:24, imcheng wrote: > On 2017/03/30 23:03:28, mark a. foltz wrote: > > When would this binding need to be reset? My understanding was that the > > controller object <--> ObserverPtr would be 1:1. > > I think we should add a DCHECK(!binding_.is_bound()) inside this method so we > don't accidentally call this twice. Done. https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:96: // already set, then the previous binding gets invalidated. On 2017/03/30 23:03:28, mark a. foltz wrote: > When would this binding need to be reset? My understanding was that the > controller object <--> ObserverPtr would be 1:1. This shouldn't be reset. Changing the comment. https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router.mojom:373: // created controller is destroyed when |media_controller| becomes invalid. On 2017/03/30 23:03:28, mark a. foltz wrote: > If a returned controller is invalidated/destroyed, can this method be called > again on the same route id to create a new one? Please clarify comment above. Done. https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:410: mojom::MediaControllerPtr media_controller; On 2017/03/30 23:03:28, mark a. foltz wrote: > Can you prefix Mojo variables with mojo_ for readability? Done. https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:416: media_route_provider_->CreateMediaRouteController( On 2017/03/30 23:03:28, mark a. foltz wrote: > What guarantees that the event page is alive? Most calls through to the MRPM > will queues requests if necessary until the event page is ready to receive them. These requests should be queued as well. I can't std::move arguments into base::Bind() *, so I'm giving MediaRouteController a method for getting MediaControllerRequest, and calling that in DoCreateMediaRouteController(). * I tried using base::BindOnce() instead, but was having issues with queuing requests with std::deque::push_back(). It was calling the copy ctor of base::OnceCallback rather than the move ctor, even with the use of std::move(). https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:423: return route_controller; On 2017/03/30 23:03:28, mark a. foltz wrote: > What state is this in before OnMediaControllerCreated is invoked? Before DoCreateMediaRouteController() is called, route_controller won't be accepting any media commands, which is fine because the media controls in WebUI will only be shown after the first media status update. https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1027: controller->Invalidate(); On 2017/03/30 23:03:28, mark a. foltz wrote: > Doesn't this call back into DetachRouteController() above (which will be a no-op > since you just erase()d controller)? > > Can you add a specific method like controller->OnRouteInvalid() that will drop > observers without re-entrancy here? Yes. Added OnRouteInvalid(). https://codereview.chromium.org/2728543009/diff/140001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1034: void MediaRouterMojoImpl::OnMediaControllerCreated( On 2017/03/30 23:03:28, mark a. foltz wrote: > Shouldn't this return a boolean per the mojom? No, this is the callback that gets called, so it doesn't need to return anything.
https://codereview.chromium.org/2728543009/diff/180001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2728543009/diff/180001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:76: is_valid_ = false; You could just call OnRouteInvalid here. Actually I would suggest you name this OnMojoConnectionError() and make it private, and rename OnRouteInvalid() to Invalidate(). Then it is clear that a cleanup sequence is: 1) OnMojoConnectionError() / MediaRouter detects route is terminated and removes controller from mapping 2) Invalidate() 3) Observers drop their controller refrerences 4) Controller gets deleted. https://codereview.chromium.org/2728543009/diff/180001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2728543009/diff/180001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:417: base::Bind(&MediaRouterMojoImpl::DoCreateMediaRouteController, As discussed, a flaw of doing this is that the Mojo MediaController message pipe isn't bound yet when this call returns; if the client issues a command at this point, it will DCHECK or the call will not go through. You can't add a OnceCallback to |pending_requests_|; I think a reasonable approach here is to define a separate request queue. Then you should be able to create the interface request in GetRouteController, bind it to the OnceCallback and push the callback into the new queue. This should be fine since it does not depend on the relative order of other types of requests. https://codereview.chromium.org/2728543009/diff/180001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1060: it->second->Invalidate(); Erase the controller from the mapping here; this can avoid a back and forth call between the controller and the MR.
Patchset #5 (id:200001) has been deleted
Patchset #5 (id:220001) has been deleted
https://codereview.chromium.org/2728543009/diff/180001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2728543009/diff/180001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:76: is_valid_ = false; On 2017/04/05 07:18:02, imcheng wrote: > You could just call OnRouteInvalid here. Actually I would suggest you name this > OnMojoConnectionError() and make it private, and rename OnRouteInvalid() to > Invalidate(). Then it is clear that a cleanup sequence is: > > 1) OnMojoConnectionError() / MediaRouter detects route is terminated and removes > controller from mapping > 2) Invalidate() > 3) Observers drop their controller refrerences > 4) Controller gets deleted. Done. https://codereview.chromium.org/2728543009/diff/180001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2728543009/diff/180001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:417: base::Bind(&MediaRouterMojoImpl::DoCreateMediaRouteController, On 2017/04/05 07:18:02, imcheng wrote: > As discussed, a flaw of doing this is that the Mojo MediaController message pipe > isn't bound yet when this call returns; if the client issues a command at this > point, it will DCHECK or the call will not go through. You can't add a > OnceCallback to |pending_requests_|; I think a reasonable approach here is to > define a separate request queue. Then you should be able to create the interface > request in GetRouteController, bind it to the OnceCallback and push the callback > into the new queue. This should be fine since it does not depend on the relative > order of other types of requests. I tried using OnceClosures again and somehow got it to work. So I'll be using that instead. https://codereview.chromium.org/2728543009/diff/180001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1060: it->second->Invalidate(); On 2017/04/05 07:18:02, imcheng wrote: > Erase the controller from the mapping here; this can avoid a back and forth call > between the controller and the MR. Done.
Looking much better. Thanks for converting to OnceClosure in MRMojoImpl. https://codereview.chromium.org/2728543009/diff/180001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2728543009/diff/180001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:92: DCHECK(!binding_.is_bound()); Did you mean to revert this? https://codereview.chromium.org/2728543009/diff/180001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2728543009/diff/180001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:98: // called at most once in the lifetime of |this|. Did you mean to revert the "must only be called once" part? https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:71: if (!is_valid_) Maybe this should be a DCHECK? This should only be called as long as binding_ is still valid. https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:80: for (Observer& observer : observers_) Consider cleaning up the Mojo objects here for completeness since |this| is no longer in a usable state. IIUC, for Binding we can call Close() and for InterfacePtr we can call reset(). https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:31: // controller is terminated or has an error, OnControllerInvalidated() will be s/OnControllerInvalidated/Invalidate https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:32: // called by the MediaRouter or a Mojo error handler to make observers dispose s/a Mojo error handler/OnMojoConnectionError() https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:110: // and |observers_| to dispose their references to |this|. |this| gets I don't think you need the last sentence. It's already been mentioned above in class level comments. https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:420: RunOrDefer( Please add a SetWakeReason() call here for UMA purposes. https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:421: base::BindOnce(&MediaRouterMojoImpl::DoCreateMediaRouteController, Is there a reason why we still need separate methods DoCreateMediaRouteController and DoSetMediaRouteStatusObserver? Or can they be combined? https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:424: base::Bind(&MediaRouterMojoImpl::OnMediaControllerCreated, You don't have to Bind OnMediaControllerCreated beforehand; just Bind it when you're about to call media_route_provider_->CreateMediaRouteController(). https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1060: Could you add a DVLOG_WITH_INSTANCE here for dev purposes? https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1064: it = route_controllers_.erase(it); No need to assign |it| with returned value.
https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:71: if (!is_valid_) On 2017/04/06 21:14:04, imcheng wrote: > Maybe this should be a DCHECK? This should only be called as long as binding_ is > still valid. Okay, we'll close the mojo connection in Invalidate() so it should be safe to DCHECK here. https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:80: for (Observer& observer : observers_) On 2017/04/06 21:14:04, imcheng wrote: > Consider cleaning up the Mojo objects here for completeness since |this| is no > longer in a usable state. IIUC, for Binding we can call Close() and for > InterfacePtr we can call reset(). Done. https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:31: // controller is terminated or has an error, OnControllerInvalidated() will be On 2017/04/06 21:14:04, imcheng wrote: > s/OnControllerInvalidated/Invalidate Done. https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:32: // called by the MediaRouter or a Mojo error handler to make observers dispose On 2017/04/06 21:14:04, imcheng wrote: > s/a Mojo error handler/OnMojoConnectionError() Done. https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:110: // and |observers_| to dispose their references to |this|. |this| gets On 2017/04/06 21:14:04, imcheng wrote: > I don't think you need the last sentence. It's already been mentioned above in > class level comments. Removed. https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:420: RunOrDefer( On 2017/04/06 21:14:05, imcheng wrote: > Please add a SetWakeReason() call here for UMA purposes. Done. https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:421: base::BindOnce(&MediaRouterMojoImpl::DoCreateMediaRouteController, On 2017/04/06 21:14:05, imcheng wrote: > Is there a reason why we still need separate methods > DoCreateMediaRouteController and DoSetMediaRouteStatusObserver? Or can they be > combined? None of the other Do.. methods call multiple MRP methods, but I think it's fine to. Combined. https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:424: base::Bind(&MediaRouterMojoImpl::OnMediaControllerCreated, On 2017/04/06 21:14:05, imcheng wrote: > You don't have to Bind OnMediaControllerCreated beforehand; just Bind it when > you're about to call media_route_provider_->CreateMediaRouteController(). Done. https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1060: On 2017/04/06 21:14:05, imcheng wrote: > Could you add a DVLOG_WITH_INSTANCE here for dev purposes? Done. https://codereview.chromium.org/2728543009/diff/240001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1064: it = route_controllers_.erase(it); On 2017/04/06 21:14:05, imcheng wrote: > No need to assign |it| with returned value. Oops, done.
A couple more thoughts. Looks good otherwise. https://codereview.chromium.org/2728543009/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2728543009/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:421: CREATE_MEDIA_ROUTE_CONTROLLER_AND_SET_OBSERVER); Maybe just CREATE_MEDIA_ROUTE_CONTROLLER? https://codereview.chromium.org/2728543009/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:425: std::move(mojo_media_controller_request), Thinking a bit more about this, the InterfaceRequest and InterfacePtr being passed into the callback here might are tied to the MediaRouteController object, which could be destroyed by the time the Do...() method is invoked. It seems you would have to check the validity of the Mojo objects inside the Do...() method before calling to the MRP. dcheng@, would that work or do you have other suggestions? https://codereview.chromium.org/2728543009/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1072: DVLOG_WITH_INSTANCE(1) << "OnMediaControllerCreated: " << route_id; Move this to the top and log |success| as well. https://codereview.chromium.org/2728543009/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1073: auto it = route_controllers_.find(route_id); This could be problematic for the reason that we don't know which controller we are erasing here. We probably don't need to erase as long as the MediaRouteProvider closes the pipes on failure, to trigger OnMojoConnectionError() in the controller object. https://codereview.chromium.org/2728543009/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc (right): https://codereview.chromium.org/2728543009/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:18: #include "base/synchronization/waitable_event.h" #include not needed? https://codereview.chromium.org/2728543009/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_metrics.h (right): https://codereview.chromium.org/2728543009/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_metrics.h:47: CREATE_MEDIA_ROUTE_CONTROLLER_AND_SET_OBSERVER = 19, You will also need to update tools/metrics/histograms/histograms.xml
Patchset #8 (id:300001) has been deleted
https://codereview.chromium.org/2728543009/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2728543009/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:421: CREATE_MEDIA_ROUTE_CONTROLLER_AND_SET_OBSERVER); On 2017/04/07 22:45:47, imcheng wrote: > Maybe just CREATE_MEDIA_ROUTE_CONTROLLER? Done. https://codereview.chromium.org/2728543009/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1072: DVLOG_WITH_INSTANCE(1) << "OnMediaControllerCreated: " << route_id; On 2017/04/07 22:45:48, imcheng wrote: > Move this to the top and log |success| as well. Done. https://codereview.chromium.org/2728543009/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:1073: auto it = route_controllers_.find(route_id); On 2017/04/07 22:45:48, imcheng wrote: > This could be problematic for the reason that we don't know which controller we > are erasing here. We probably don't need to erase as long as the > MediaRouteProvider closes the pipes on failure, to trigger > OnMojoConnectionError() in the controller object. Removing the invalidation code here. Changing the comment for mojo CreateMediaRouteController() to require closing of the interface request in the case of a failure. https://codereview.chromium.org/2728543009/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc (right): https://codereview.chromium.org/2728543009/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:18: #include "base/synchronization/waitable_event.h" On 2017/04/07 22:45:48, imcheng wrote: > #include not needed? Removed. https://codereview.chromium.org/2728543009/diff/280001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_metrics.h (right): https://codereview.chromium.org/2728543009/diff/280001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_metrics.h:47: CREATE_MEDIA_ROUTE_CONTROLLER_AND_SET_OBSERVER = 19, On 2017/04/07 22:45:48, imcheng wrote: > You will also need to update tools/metrics/histograms/histograms.xml Done.
LGTM % some wordsmithing suggestions you can take or leave. Overall this looks nice and clean. Two minor comments on the design (not blocking this change): 1. It seems like it's the responsibility of code that lives in MediaRouterMojoImpl to set up the MediaRouteController correctly. That probably makes sense in the current architecture, where the controller is always provided by the MRPM, but may be limiting in the future and makes the MediaRouteController tricky to use in other contexts. 2. This is adding some optional functionality to the base MR API that will not be implemented by Android in the near term. We may find a use for it with the upcoming work to refactor Media Remoting, but we may want to consider a moving to a decorator/extensions based pattern for MR to keep the API focused on common functionality. https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:265: // Removes the MediaRouteController for |route_id| from the list of Please add a comment explaining who calls this. I assume the MediaRouteController calls it with itself? https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... File chrome/browser/media/router/media_router_base.cc (right): https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/media_router_base.cc:126: return std::find_if(routes.begin(), routes.end(), Can this be implemented in internal_routes_observer_, e.g. as HasRoute(route_id)? https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... File chrome/browser/media/router/media_router_base.h (right): https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/media_router_base.h:66: bool IsRouteValid(const std::string& route_id) const; This might be better named "IsRouteKnown". It's checking whether the last route update from the MRPM has the listed route. It's not clear exactly what "valid" means in this context. https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:71: // |mojo_media_controller|. |mojo_media_controller| must be bound to a message When does this binding actually happen? It looks like it's done in the callback in MediaRouterMojoImpl, after the ctor is called. https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:73: // destroyed. Can you be specific: "notified via DetachRouteController" https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:90: // Notifies |observers_| to dispose their references to |this|. |this| gets I don't mind calling out |this| when it's ambiguous which object is being referred to. However, maybe you can drop some of these references by clarifying the surrounding text or just saying "the controller". https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:94: // Returns a mojo pointer bound to |this| by |binding_|. If |binding_| is binding_ is an implementation detail of this class - this can be written not to mention it: Returns a MediaStatusObserverPtr bound to |this|. If a previous observer was bound, its binding is invalidated. https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:109: // the mojo MediaStatus provider is no longer valid. Notifies |media_router_| s/mojo MediaController/MediaControllerPtr/ s/mojo Media Status provider/MediaStatusObserver binding/ https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:125: // Observers that |this| notifies of status updates. The observers share the s/|this| notifies/are notified/ https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller_unittest.cc (right): https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:23: const char kRouteId[] = "routeId"; constexpr https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:90: mojom::MediaStatusObserverPtr mojo_observer = It might be a good idea to test the binding invalidation behavior - calling BindObserverPtr() twice on the same controller.
lgtm after Mark's comments are addressed
+holte@ for histograms.xml Please take a look, thanks! https://codereview.chromium.org/2728543009/diff/180001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.cc (right): https://codereview.chromium.org/2728543009/diff/180001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.cc:92: DCHECK(!binding_.is_bound()); On 2017/04/06 21:14:04, imcheng wrote: > Did you mean to revert this? No, putting back. https://codereview.chromium.org/2728543009/diff/180001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2728543009/diff/180001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:98: // called at most once in the lifetime of |this|. On 2017/04/06 21:14:04, imcheng wrote: > Did you mean to revert the "must only be called once" part? No, putting it back. https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... File chrome/browser/media/router/media_router.h (right): https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/media_router.h:265: // Removes the MediaRouteController for |route_id| from the list of On 2017/04/11 19:43:19, mark a. foltz wrote: > Please add a comment explaining who calls this. I assume the > MediaRouteController calls it with itself? Done. Right, it's called just by MediaRouteController. https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... File chrome/browser/media/router/media_router_base.cc (right): https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/media_router_base.cc:126: return std::find_if(routes.begin(), routes.end(), On 2017/04/11 19:43:19, mark a. foltz wrote: > Can this be implemented in internal_routes_observer_, e.g. as > HasRoute(route_id)? We could do that, but MRBase would still need a method that just calls that, in order to expose it to MRMojoImpl. Would that be better? https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... File chrome/browser/media/router/media_router_base.h (right): https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/media_router_base.h:66: bool IsRouteValid(const std::string& route_id) const; On 2017/04/11 19:43:19, mark a. foltz wrote: > This might be better named "IsRouteKnown". It's checking whether the last route > update from the MRPM has the listed route. It's not clear exactly what "valid" > means in this context. Done. https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller.h (right): https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:71: // |mojo_media_controller|. |mojo_media_controller| must be bound to a message On 2017/04/11 19:43:19, mark a. foltz wrote: > When does this binding actually happen? It looks like it's done in the callback > in MediaRouterMojoImpl, after the ctor is called. > Right, I forgot to update the comment. Updated. https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:73: // destroyed. On 2017/04/11 19:43:19, mark a. foltz wrote: > Can you be specific: "notified via DetachRouteController" Done. https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:90: // Notifies |observers_| to dispose their references to |this|. |this| gets On 2017/04/11 19:43:19, mark a. foltz wrote: > I don't mind calling out |this| when it's ambiguous which object is being > referred to. However, maybe you can drop some of these references by clarifying > the surrounding text or just saying "the controller". Done. https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:94: // Returns a mojo pointer bound to |this| by |binding_|. If |binding_| is On 2017/04/11 19:43:19, mark a. foltz wrote: > binding_ is an implementation detail of this class - this can be written not to > mention it: > > Returns a MediaStatusObserverPtr bound to |this|. If a previous observer was > bound, its binding is invalidated. Done. https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:109: // the mojo MediaStatus provider is no longer valid. Notifies |media_router_| On 2017/04/11 19:43:19, mark a. foltz wrote: > s/mojo MediaController/MediaControllerPtr/ > s/mojo Media Status provider/MediaStatusObserver binding/ Done. https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller.h:125: // Observers that |this| notifies of status updates. The observers share the On 2017/04/11 19:43:19, mark a. foltz wrote: > s/|this| notifies/are notified/ Done. https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller_unittest.cc (right): https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:23: const char kRouteId[] = "routeId"; On 2017/04/11 19:43:20, mark a. foltz wrote: > constexpr Done. https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:90: mojom::MediaStatusObserverPtr mojo_observer = On 2017/04/11 19:43:19, mark a. foltz wrote: > It might be a good idea to test the binding invalidation behavior - calling > BindObserverPtr() twice on the same controller. BindObserverPtr() should only be called once in the controller's lifetime. Updated method comment and added a DCHECK to enforce that.
takumif@chromium.org changed reviewers: + holte@chromium.org
(forgot to add to reviewers last time) +holte@ for histograms.xml Please take a look, thanks!
The CQ bit was checked by takumif@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.
histograms lgtm
The CQ bit was checked by takumif@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.
Sorry for the slow review. https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller_unittest.cc (right): https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:39: new MediaRouteController(kRouteId, std::move(media_controller_ptr), Nit: from a readability perspective, this is a little confusing. Consider using base::MakeShared. https://codereview.chromium.org/2728543009/diff/360001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller_unittest.cc (right): https://codereview.chromium.org/2728543009/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:102: base::RunLoop run_loop2; This should be able to just use the original RunLoop on line 93. https://codereview.chromium.org/2728543009/diff/360001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2728543009/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:426: route_controllers_.insert({route_id, route_controller.get()}); Or just: emplace(route_id, route_controller.get()) which will avoid any potential implicit conversions. https://codereview.chromium.org/2728543009/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:773: media_route_provider_->SetMediaRouteStatusObserver(route_id, Should we just do this as part of CreateMediaRouteController? https://codereview.chromium.org/2728543009/diff/360001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc (right): https://codereview.chromium.org/2728543009/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1282: base::RunLoop run_loop2; Can this just use the original RunLoop from line 1241?
The CQ bit was checked by takumif@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...
https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller_unittest.cc (right): https://codereview.chromium.org/2728543009/diff/320001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:39: new MediaRouteController(kRouteId, std::move(media_controller_ptr), On 2017/04/14 05:30:07, dcheng wrote: > Nit: from a readability perspective, this is a little confusing. Consider using > base::MakeShared. Done. https://codereview.chromium.org/2728543009/diff/360001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller_unittest.cc (right): https://codereview.chromium.org/2728543009/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:102: base::RunLoop run_loop2; On 2017/04/14 05:30:07, dcheng wrote: > This should be able to just use the original RunLoop on line 93. How do I do that? I thought Run()/RunUntilIdle() can only called once. Calling RunUntilIdle() twice in a row causes a CHECK failure. https://codereview.chromium.org/2728543009/diff/360001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2728543009/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:426: route_controllers_.insert({route_id, route_controller.get()}); On 2017/04/14 05:30:07, dcheng wrote: > Or just: > > emplace(route_id, route_controller.get()) > > which will avoid any potential implicit conversions. Done. https://codereview.chromium.org/2728543009/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:773: media_route_provider_->SetMediaRouteStatusObserver(route_id, On 2017/04/14 05:30:07, dcheng wrote: > Should we just do this as part of CreateMediaRouteController? We went back and forth between keeping the methods together or separate. Some of the arguments for keeping them separate are that the controller and the media status provider might not be the same object, and it'd be easier to allow multiple observers per route in the future if we wanted to. https://codereview.chromium.org/2728543009/diff/360001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc (right): https://codereview.chromium.org/2728543009/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1282: base::RunLoop run_loop2; On 2017/04/14 05:30:08, dcheng wrote: > Can this just use the original RunLoop from line 1241? How do I do that? I thought Run()/RunUntilIdle() can only called once.
https://codereview.chromium.org/2728543009/diff/360001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller_unittest.cc (right): https://codereview.chromium.org/2728543009/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:102: base::RunLoop run_loop2; On 2017/04/14 18:42:58, takumif wrote: > On 2017/04/14 05:30:07, dcheng wrote: > > This should be able to just use the original RunLoop on line 93. > > How do I do that? I thought Run()/RunUntilIdle() can only called once. Calling > RunUntilIdle() twice in a row causes a CHECK failure. It can be called more than once. https://codereview.chromium.org/2728543009/diff/360001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/2728543009/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:773: media_route_provider_->SetMediaRouteStatusObserver(route_id, On 2017/04/14 18:42:59, takumif wrote: > On 2017/04/14 05:30:07, dcheng wrote: > > Should we just do this as part of CreateMediaRouteController? > > We went back and forth between keeping the methods together or separate. Some of > the arguments for keeping them separate are that the controller and the media > status provider might not be the same object, and it'd be easier to allow > multiple observers per route in the future if we wanted to. I would suggest keeping it together until there's a clear need to change the API. Right now, the additional complexity doesn't seem needed.
https://codereview.chromium.org/2728543009/diff/360001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_route_controller_unittest.cc (right): https://codereview.chromium.org/2728543009/diff/360001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_route_controller_unittest.cc:102: base::RunLoop run_loop2; On 2017/04/14 19:25:25, dcheng wrote: > On 2017/04/14 18:42:58, takumif wrote: > > On 2017/04/14 05:30:07, dcheng wrote: > > > This should be able to just use the original RunLoop on line 93. > > > > How do I do that? I thought Run()/RunUntilIdle() can only called once. Calling > > RunUntilIdle() twice in a row causes a CHECK failure. > > It can be called more than once. Actually you're right. I'm confused =) You can eliminate the variable and just write base::RunLoop().RunUntilIdle()
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dcheng@ - we have already discussed this API in a previous patch and also in person. In the end we have have settled on keeping them separate as it is more flexible than keeping them together. I can assure you we put a lot of thought into this. =) In the future if we decide that it's better to combine them, then the work to do so is IMO just as easy (or difficult) as going the other way. For now, it's important for us to decide on one and move forward. Let me know what you think.
On 2017/04/17 18:27:18, imcheng wrote: > dcheng@ - we have already discussed this API in a previous patch and also in > person. In the end we have have settled on keeping them separate as it is more > flexible than keeping them together. I can assure you we put a lot of thought > into this. =) > > In the future if we decide that it's better to combine them, then the work to do > so is IMO just as easy (or difficult) as going the other way. For now, it's > important for us to decide on one and move forward. > > Let me know what you think. From an IPC perspective, it's undesirable to expose capabilities that aren't actually needed. In this case, we can assume that the browser process is managing all this correctly, but it's much simpler to reason about if pipes are just bound correctly from the beginning of an object's lifetime. This is fundamentally impossible when split into two methods.
Patchset #12 (id:400001) has been deleted
I've combined CreateMediaRouteController() and SetMediaRouteStatusObserver(). Please take a look, thanks!
The CQ bit was checked by takumif@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.
LGTM
The CQ bit was checked by takumif@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, imcheng@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2728543009/#ps420001 (title: "Combine CreateMRController and SetMRStatusObserver")
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": 420001, "attempt_start_ts": 1492493088373790,
"parent_rev": "da7cc8ca4c326895886b10df62d513fac256d74f", "commit_rev":
"3c8ebf96c1e2432763cac1a6fa97cf5217a8728c"}
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Custom Controls 2 - add MediaRouter::GetRouteController() This CL adds GetRouteController() to the MediaRouter API. The method returns a scoped_refptr to a MediaRouteController that is connected to the MediaController in the component extension. MediaRouterMojoImpl will keep a map of raw pointers to MediaRouteControllers that are valid and are owned by their observers. MediaRouterMojoImpl is notified when a controller is invalidated, so that it can be removed from the map. GetRouteController() will be called in the next CL by MediaRouterUI for instantiating an observer for MediaRouteController. We also add two methods to the MediaRouteProvider mojo interface that are called by GetRouteController(): - CreateMediaRouteController(): called for creating the extension-side MediaController. - SetMediaRouteStatusObserver(): called for setting the browser-side MediaRouteController as a status observer. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): this patch 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... BUG=684635 ========== to ========== [Media Router] Custom Controls 2 - add MediaRouter::GetRouteController() This CL adds GetRouteController() to the MediaRouter API. The method returns a scoped_refptr to a MediaRouteController that is connected to the MediaController in the component extension. MediaRouterMojoImpl will keep a map of raw pointers to MediaRouteControllers that are valid and are owned by their observers. MediaRouterMojoImpl is notified when a controller is invalidated, so that it can be removed from the map. GetRouteController() will be called in the next CL by MediaRouterUI for instantiating an observer for MediaRouteController. We also add two methods to the MediaRouteProvider mojo interface that are called by GetRouteController(): - CreateMediaRouteController(): called for creating the extension-side MediaController. - SetMediaRouteStatusObserver(): called for setting the browser-side MediaRouteController as a status observer. The Chromium-side implementation of custom controls will be done in these patches: 1. Mojo/MediaStatus/MediaRouteController: http://crrev/2727123002 2. MediaRouter::GetRouteController(): this patch 3. MRUI/MRWebUIMessageHandler: http://crrev/2731033002 4. Custom controls WebUI: http://crrev/2725503002 Custom controls design doc: https://docs.google.com/document/d/1_8QxdFIiiJX39jR1Wi1Zn9FW-Y66EMvX1GmQZvjN4... BUG=684635 Review-Url: https://codereview.chromium.org/2728543009 Cr-Commit-Position: refs/heads/master@{#465148} Committed: https://chromium.googlesource.com/chromium/src/+/3c8ebf96c1e2432763cac1a6fa97... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:420001) as https://chromium.googlesource.com/chromium/src/+/3c8ebf96c1e2432763cac1a6fa97...
Message was sent while issue was closed.
LGTM with new API. |
