[Media Router] Replace ListenForRouteMessages mojo API |
|||||||||
Issue descriptionThere are a couple of shortcomings with the current API: 1) The fact that it has to return a value means it doesn't work if the event page becomes suspended before the value is returned -- in the extension the promise will not get resolved, and messaging from the extension to the page will be completely broken. Currently, we prevent this by keeping the extension alive (i.e, prevent suspension) when there is a local route, but this blocks the work to improve the extension's performance. 2) The logic on the extension side, in particular the cleanup logic, has to be written in a weird way to accomodate this API. The suggestion is to replace the API with one that just starts listening for route messages, and for the messages to be delivered back to MR via a MediaRouter Mojo API invoked by the extension.
,
Jul 7 2016
,
Jul 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bf455be1850399b8b374809cfc2584e0611e1c77 commit bf455be1850399b8b374809cfc2584e0611e1c77 Author: imcheng <imcheng@chromium.org> Date: Mon Jul 18 20:34:47 2016 [Media Router] Replace route messaging API. Remove old callback-based ListenForRouteMessages API and replace it with StartListeningForRouteMessages and OnRouteMessagesReceived. The rationale is return value based API doesn't work well for event pages; when the event page is suspended, the callback supplied by the old API is lost and cannot be restored when the extension wakes up. Switching to the listener model unblocks the work to reduce the nessesity of keeping the event page alive and should result in considerably simpler logic on the extension side. To maintain backwards compatibility with older extensions, a smaller adapter logic is introduced in media_router_bindings.js. Add missing WakeReason for StartListeningForRouteMessages. Also added OWNERs for the mojom file to comply with presubmit. BUG= 625007 Review-Url: https://codereview.chromium.org/2111303003 Cr-Commit-Position: refs/heads/master@{#406083} [add] https://crrev.com/bf455be1850399b8b374809cfc2584e0611e1c77/chrome/browser/media/router/mojo/OWNERS [modify] https://crrev.com/bf455be1850399b8b374809cfc2584e0611e1c77/chrome/browser/media/router/mojo/media_router.mojom [modify] https://crrev.com/bf455be1850399b8b374809cfc2584e0611e1c77/chrome/browser/media/router/mojo/media_router_mojo_impl.cc [modify] https://crrev.com/bf455be1850399b8b374809cfc2584e0611e1c77/chrome/browser/media/router/mojo/media_router_mojo_impl.h [modify] https://crrev.com/bf455be1850399b8b374809cfc2584e0611e1c77/chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc [modify] https://crrev.com/bf455be1850399b8b374809cfc2584e0611e1c77/chrome/browser/media/router/mojo/media_router_mojo_metrics.h [modify] https://crrev.com/bf455be1850399b8b374809cfc2584e0611e1c77/chrome/browser/media/router/mojo/media_router_mojo_test.h [modify] https://crrev.com/bf455be1850399b8b374809cfc2584e0611e1c77/extensions/renderer/resources/media_router_bindings.js [modify] https://crrev.com/bf455be1850399b8b374809cfc2584e0611e1c77/tools/metrics/histograms/histograms.xml
,
Jul 18 2016
,
Jul 18 2016
,
Jul 19 2016
,
Jul 19 2016
Before we approve merge to M52, Could you please confirm whether this change is baked/verified in Canary and safe to merge? Also is this change applicable to all OS or any specific OS?
,
Jul 19 2016
Hi govind@, this is for M53. The change is in today's canary and I have manually verified it. This is for all desktop OS.
,
Jul 19 2016
Approving merge to M53 branch 2785 based on comment #8. Please try to merge this by EOD today if possible. Thank you.
,
Jul 19 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c872eeb60ada88d0ef235a464e147c4384fe266a commit c872eeb60ada88d0ef235a464e147c4384fe266a Author: Derek Cheng <imcheng@chromium.org> Date: Tue Jul 19 21:23:15 2016 [Media Router] Replace route messaging API. Remove old callback-based ListenForRouteMessages API and replace it with StartListeningForRouteMessages and OnRouteMessagesReceived. The rationale is return value based API doesn't work well for event pages; when the event page is suspended, the callback supplied by the old API is lost and cannot be restored when the extension wakes up. Switching to the listener model unblocks the work to reduce the nessesity of keeping the event page alive and should result in considerably simpler logic on the extension side. To maintain backwards compatibility with older extensions, a smaller adapter logic is introduced in media_router_bindings.js. Add missing WakeReason for StartListeningForRouteMessages. Also added OWNERs for the mojom file to comply with presubmit. BUG= 625007 Review-Url: https://codereview.chromium.org/2111303003 Cr-Commit-Position: refs/heads/master@{#406083} (cherry picked from commit bf455be1850399b8b374809cfc2584e0611e1c77) Review URL: https://codereview.chromium.org/2161073003 . Cr-Commit-Position: refs/branch-heads/2785@{#226} Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382} [add] https://crrev.com/c872eeb60ada88d0ef235a464e147c4384fe266a/chrome/browser/media/router/mojo/OWNERS [modify] https://crrev.com/c872eeb60ada88d0ef235a464e147c4384fe266a/chrome/browser/media/router/mojo/media_router.mojom [modify] https://crrev.com/c872eeb60ada88d0ef235a464e147c4384fe266a/chrome/browser/media/router/mojo/media_router_mojo_impl.cc [modify] https://crrev.com/c872eeb60ada88d0ef235a464e147c4384fe266a/chrome/browser/media/router/mojo/media_router_mojo_impl.h [modify] https://crrev.com/c872eeb60ada88d0ef235a464e147c4384fe266a/chrome/browser/media/router/mojo/media_router_mojo_impl_unittest.cc [modify] https://crrev.com/c872eeb60ada88d0ef235a464e147c4384fe266a/chrome/browser/media/router/mojo/media_router_mojo_metrics.h [modify] https://crrev.com/c872eeb60ada88d0ef235a464e147c4384fe266a/chrome/browser/media/router/mojo/media_router_mojo_test.h [modify] https://crrev.com/c872eeb60ada88d0ef235a464e147c4384fe266a/extensions/renderer/resources/media_router_bindings.js [modify] https://crrev.com/c872eeb60ada88d0ef235a464e147c4384fe266a/tools/metrics/histograms/histograms.xml
,
Jul 20 2016
,
Jul 23 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/31b15d17748cedc65bdfb2d611a12bb66cedf234 commit 31b15d17748cedc65bdfb2d611a12bb66cedf234 Author: imcheng <imcheng@chromium.org> Date: Sat Jul 23 20:56:50 2016 [Media Router] Fix typo in media_router_bindings.js The typo was introduced in crrev.com/2111303003. There is no real impact since the handler function is set and used with the correct name everywhere else. BUG= 625007 Review-Url: https://codereview.chromium.org/2174183002 Cr-Commit-Position: refs/heads/master@{#407373} [modify] https://crrev.com/31b15d17748cedc65bdfb2d611a12bb66cedf234/extensions/renderer/resources/media_router_bindings.js |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by sheriffbot@chromium.org
, Jul 1 2016