Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1862913004:
[Media Router] Wire through a new MRPM call to update media sinks. (Closed)
|
Created:
4 years, 8 months ago by amp Modified:
4 years, 8 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, btolsch, chromium-apps-reviews_chromium.org, chromium-reviews, darin (slow to review), extensions-reviews_chromium.org, feature-media-reviews_chromium.org, mcasas+watch_chromium.org, media-router+watch_chromium.org, posciak+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Media Router] Wire through a new MRPM call to update media sinks.
This will allow any discovery which is based off of sink queries (such as the Cloud MRP) an opportuntity to find new sinks even if the MRP is marked as UNVAILABLE.
BUG=601065
Committed: https://crrev.com/c2f9e3a5a7bf211b14c1be7ae8bfa65a518c2b85
Cr-Commit-Position: refs/heads/master@{#387809}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fixed indent, added wake reason. #
Total comments: 8
Patch Set 3 : Addressed comments, tests still need some work. #Patch Set 4 : Rebased against head #Patch Set 5 : Fixed (and simplified) tests. #
Total comments: 6
Patch Set 6 : Address comments, fix tests on windows #Patch Set 7 : rebase #Patch Set 8 : Still trying to fix tests on windows #Patch Set 9 : Really fix tests. :) #
Total comments: 4
Patch Set 10 : add missing run loops #
Total comments: 5
Messages
Total messages: 40 (17 generated)
amp@chromium.org changed reviewers: + apacible@chromium.org, mfoltz@chromium.org
Extension side changes are still pending. I wanted to lock down the naming and proper call flow before starting the extension side.
https://codereview.chromium.org/1862913004/diff/1/chrome/browser/media/router... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1862913004/diff/1/chrome/browser/media/router... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:991: media_route_provider_->StartDiscovery(); nit: indent https://codereview.chromium.org/1862913004/diff/1/chrome/browser/media/router... File chrome/browser/media/router/mojo/media_router_mojo_metrics.h (right): https://codereview.chromium.org/1862913004/diff/1/chrome/browser/media/router... chrome/browser/media/router/mojo/media_router_mojo_metrics.h:43: USER_GESTURE_DISCOVERY = 16, This isn't being reported for the histogram. Use SetWakeReason(...) in MediaRouterMojoImpl.
https://codereview.chromium.org/1862913004/diff/1/chrome/browser/media/router... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1862913004/diff/1/chrome/browser/media/router... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:970: RunOrDefer(base::Bind(&MediaRouterMojoImpl::DoEnsureMdnsDiscoveryEnabled, Do I need this RunOrDefer logic for the discovery change as well? Is there some thread handling going on here from the user gesture to the MR process or something? https://codereview.chromium.org/1862913004/diff/1/chrome/browser/media/router... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:991: media_route_provider_->StartDiscovery(); On 2016/04/06 17:50:18, apacible wrote: > nit: indent Done. https://codereview.chromium.org/1862913004/diff/1/chrome/browser/media/router... File chrome/browser/media/router/mojo/media_router_mojo_metrics.h (right): https://codereview.chromium.org/1862913004/diff/1/chrome/browser/media/router... chrome/browser/media/router/mojo/media_router_mojo_metrics.h:43: USER_GESTURE_DISCOVERY = 16, On 2016/04/06 17:50:18, apacible wrote: > This isn't being reported for the histogram. Use SetWakeReason(...) in > MediaRouterMojoImpl. Ah, yes nice catch. That made me realize I may not be calling the extension correctly (see new comment on MediaRouterMojoImpl).
Feedback on API. Can you elaborate the patch description when you get a chance? https://codereview.chromium.org/1862913004/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/1862913004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router.mojom:292: StartDiscovery(); Nit on naming: The MRPM may already be running discovery. Perhaps UpdateMediaSinks() since the function is to potentially send a new sink list via MediaSinkObservers. I think this should take a |media_source|, like Start/StopObservingMediaSinks. Rationale: The model is that the MRPM is responsible for relaying sinks that support a specific set of media sources. This allows the MRPM to be smart about how to update, and gives the caller more control over which observers get updates. If there is a use case for "update all sources," perhaps the caller could pass an empty |media_source|. But only if necessary. https://codereview.chromium.org/1862913004/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1862913004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:991: SetWakeReason(MediaRouteProviderWakeReason::USER_GESTURE_DISCOVERY); You need to check if the MRPM is awake and post a task if it isn't awake yet. See the pattern in other methods in this API https://codereview.chromium.org/1862913004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:991: SetWakeReason(MediaRouteProviderWakeReason::USER_GESTURE_DISCOVERY); You need to check if the MRPM is awake and post a task if it isn't awake yet. See the pattern in other methods in this API https://codereview.chromium.org/1862913004/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right): https://codereview.chromium.org/1862913004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.h:341: void DoUserGestureDiscovery(); UpdateMediaSinks(media_source)/DoUpdateMediaSinks(media_source) (see below) https://codereview.chromium.org/1862913004/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_ui.cc (left): https://codereview.chromium.org/1862913004/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_ui.cc:256: #endif Thanks :)
https://codereview.chromium.org/1862913004/diff/1/chrome/browser/media/router... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1862913004/diff/1/chrome/browser/media/router... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:970: RunOrDefer(base::Bind(&MediaRouterMojoImpl::DoEnsureMdnsDiscoveryEnabled, On 2016/04/07 20:37:39, amp wrote: > Do I need this RunOrDefer logic for the discovery change as well? > > Is there some thread handling going on here from the user gesture to the MR > process or something? Good catch; see mfoltz@'s comment in patch set 2. https://codereview.chromium.org/1862913004/diff/1/chrome/browser/media/router... File chrome/browser/media/router/mojo/media_router_mojo_metrics.h (right): https://codereview.chromium.org/1862913004/diff/1/chrome/browser/media/router... chrome/browser/media/router/mojo/media_router_mojo_metrics.h:43: USER_GESTURE_DISCOVERY = 16, On 2016/04/07 20:37:39, amp wrote: > On 2016/04/06 17:50:18, apacible wrote: > > This isn't being reported for the histogram. Use SetWakeReason(...) in > > MediaRouterMojoImpl. > > Ah, yes nice catch. That made me realize I may not be calling the extension > correctly (see new comment on MediaRouterMojoImpl). One more thing -- you'll need to add this value to histograms.xml under the MediaRouteProviderWakeReason enum. Note that Brandon is also adding a wake reason to the histogram, so there will be some conflict depending on who lands first.
amp@chromium.org changed reviewers: + mpearson@chromium.org
+mpearson for histogram I updated the commit description, but I'm not sure if that is supposed to be reflected in the issue title or not... Also I'll need some help understanding the tests as I'm not sure what all the run loops are doing. cc'ing Brandon in case he has any thoughts as this is close to what he did for EnableMdnsDiscovery. https://codereview.chromium.org/1862913004/diff/1/chrome/browser/media/router... File chrome/browser/media/router/mojo/media_router_mojo_metrics.h (right): https://codereview.chromium.org/1862913004/diff/1/chrome/browser/media/router... chrome/browser/media/router/mojo/media_router_mojo_metrics.h:43: USER_GESTURE_DISCOVERY = 16, On 2016/04/08 16:46:44, apacible wrote: > On 2016/04/07 20:37:39, amp wrote: > > On 2016/04/06 17:50:18, apacible wrote: > > > This isn't being reported for the histogram. Use SetWakeReason(...) in > > > MediaRouterMojoImpl. > > > > Ah, yes nice catch. That made me realize I may not be calling the extension > > correctly (see new comment on MediaRouterMojoImpl). > > One more thing -- you'll need to add this value to histograms.xml under the > MediaRouteProviderWakeReason enum. Note that Brandon is also adding a wake > reason to the histogram, so there will be some conflict depending on who lands > first. Done, and duly noted about potential merge conflicts. https://codereview.chromium.org/1862913004/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/1862913004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router.mojom:292: StartDiscovery(); On 2016/04/07 22:45:00, mark a. foltz wrote: > Nit on naming: > > The MRPM may already be running discovery. > > Perhaps UpdateMediaSinks() since the function is to potentially send a new sink > list via MediaSinkObservers. > > I think this should take a |media_source|, like Start/StopObservingMediaSinks. > > Rationale: The model is that the MRPM is responsible for relaying sinks that > support a specific set of media sources. This allows the MRPM to be smart about > how to update, and gives the caller more control over which observers get > updates. > > If there is a use case for "update all sources," perhaps the caller could pass > an empty |media_source|. But only if necessary. > Done. I like the UpdateMediaSinks name better. For the source, I pass in the Desktop source (since that works in all mirroring cases). https://codereview.chromium.org/1862913004/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1862913004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:991: SetWakeReason(MediaRouteProviderWakeReason::USER_GESTURE_DISCOVERY); On 2016/04/07 22:45:00, mark a. foltz wrote: > You need to check if the MRPM is awake and post a task if it isn't awake yet. > See the pattern in other methods in this API Done. https://codereview.chromium.org/1862913004/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right): https://codereview.chromium.org/1862913004/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.h:341: void DoUserGestureDiscovery(); On 2016/04/07 22:45:00, mark a. foltz wrote: > UpdateMediaSinks(media_source)/DoUpdateMediaSinks(media_source) (see below) Done.
Description was changed from ========== Request discovery from MRPM on user gesture. BUG=601065 ========== to ========== [Media Router] Wire through a new MRPM call to update media sinks. This will allow any discovery which is based off of sink queries (such as the Cloud MRP) an opportuntity to find new sinks even if the MRP is marked as UNVAILABLE. BUG=601065 ==========
histograms.xml lgtm
amp@chromium.org changed reviewers: + imcheng@chromium.org
+Derek to take a look as well. Tests are now passing.
The CQ bit was checked by amp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862913004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862913004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm % minor comments https://codereview.chromium.org/1862913004/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/1862913004/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router.mojom:290: // Update media sinks capable of displaying |media_source|. s/Update/Updates https://codereview.chromium.org/1862913004/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1862913004/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:433: UpdateMediaSinks(MediaSourceForDesktop().id()); could you add a comment here why this has to be done? https://codereview.chromium.org/1862913004/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right): https://codereview.chromium.org/1862913004/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.h:340: // even if the MRP sink state is marked UNAVAILABLE. s/sink state/SinkAvailability
https://codereview.chromium.org/1862913004/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router.mojom (right): https://codereview.chromium.org/1862913004/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router.mojom:290: // Update media sinks capable of displaying |media_source|. On 2016/04/15 18:37:02, imcheng wrote: > s/Update/Updates Done. https://codereview.chromium.org/1862913004/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1862913004/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:433: UpdateMediaSinks(MediaSourceForDesktop().id()); On 2016/04/15 18:37:02, imcheng wrote: > could you add a comment here why this has to be done? Done. This was added per mfoltz comment on chrome/browser/media/router/mojo/media_router.mojom in patch set 2. I haven't seen a reply as to whether Desktop is the best source to use here. The comment I will add will be '// Allow MRPM to intelligently update sinks and observers by passing in a media source.' https://codereview.chromium.org/1862913004/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/mojo/media_router_mojo_impl.h (right): https://codereview.chromium.org/1862913004/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/mojo/media_router_mojo_impl.h:340: // even if the MRP sink state is marked UNAVAILABLE. On 2016/04/15 18:37:02, imcheng wrote: > s/sink state/SinkAvailability Done.
The CQ bit was checked by amp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862913004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862913004/120001
The CQ bit was checked by amp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862913004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862913004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by amp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, imcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1862913004/#ps160001 (title: "Really fix tests. :)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862913004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862913004/160001
btolsch@chromium.org changed reviewers: + btolsch@chromium.org
https://codereview.chromium.org/1862913004/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc (right): https://codereview.chromium.org/1862913004/diff/160001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1489: media_router_->OnUserGesture(); Add another RunLoop here to send the mojo call through to UpdateMediaSinks(). You can use RunUntilIdle() for this. https://codereview.chromium.org/1862913004/diff/160001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1529: media_router_->OnUserGesture(); Add another RunLoop here to send the mojo call through to UpdateMediaSinks(). Again, you can use RunUntilIdle() for this.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Thanks for the pointers Brandon. Hopefully this gets the tests passing. https://codereview.chromium.org/1862913004/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc (right): https://codereview.chromium.org/1862913004/diff/160001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1489: media_router_->OnUserGesture(); On 2016/04/16 00:25:55, btolsch wrote: > Add another RunLoop here to send the mojo call through to UpdateMediaSinks(). > You can use RunUntilIdle() for this. Done. https://codereview.chromium.org/1862913004/diff/160001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1529: media_router_->OnUserGesture(); On 2016/04/16 00:25:55, btolsch wrote: > Add another RunLoop here to send the mojo call through to UpdateMediaSinks(). > Again, you can use RunUntilIdle() for this. Done.
The CQ bit was checked by amp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from imcheng@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/1862913004/#ps180001 (title: "add missing run loops")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862913004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862913004/180001
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Wire through a new MRPM call to update media sinks. This will allow any discovery which is based off of sink queries (such as the Cloud MRP) an opportuntity to find new sinks even if the MRP is marked as UNVAILABLE. BUG=601065 ========== to ========== [Media Router] Wire through a new MRPM call to update media sinks. This will allow any discovery which is based off of sink queries (such as the Cloud MRP) an opportuntity to find new sinks even if the MRP is marked as UNVAILABLE. BUG=601065 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Wire through a new MRPM call to update media sinks. This will allow any discovery which is based off of sink queries (such as the Cloud MRP) an opportuntity to find new sinks even if the MRP is marked as UNVAILABLE. BUG=601065 ========== to ========== [Media Router] Wire through a new MRPM call to update media sinks. This will allow any discovery which is based off of sink queries (such as the Cloud MRP) an opportuntity to find new sinks even if the MRP is marked as UNVAILABLE. BUG=601065 Committed: https://crrev.com/c2f9e3a5a7bf211b14c1be7ae8bfa65a518c2b85 Cr-Commit-Position: refs/heads/master@{#387809} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/c2f9e3a5a7bf211b14c1be7ae8bfa65a518c2b85 Cr-Commit-Position: refs/heads/master@{#387809}
Message was sent while issue was closed.
Belated LGTM with a couple of drive by comments. Nothing urgent to address. https://codereview.chromium.org/1862913004/diff/180001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl.cc (right): https://codereview.chromium.org/1862913004/diff/180001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:434: // media source. I think some comment here about why we're discovering specifically for the desktop source would be helpful. https://codereview.chromium.org/1862913004/diff/180001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl.cc:998: SetWakeReason(MediaRouteProviderWakeReason::UPDATE_MEDIA_SINKS); We should see wakeups < the number of times the dialog is opened. https://codereview.chromium.org/1862913004/diff/180001/chrome/browser/media/r... File chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc (right): https://codereview.chromium.org/1862913004/diff/180001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1533: run_loop7.RunUntilIdle(); These run loops are getting crazy. The mock MRP should be fixed to not require this manual start/stop of the message loop. https://codereview.chromium.org/1862913004/diff/180001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1547: // Windows calls once for EnableMdnsDiscovery "...calls an additional time for..." https://codereview.chromium.org/1862913004/diff/180001/chrome/browser/media/r... chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc:1550: // All others call once for registration, and once for the user gesture. Thanks for adding comments to the expectations. |
