Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1827853002:
[Android, RemotePlayback] Implement HTMLMediaElement.remote.connect(). (Closed)
|
Created:
4 years, 9 months ago by whywhat Modified:
4 years, 8 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, philipj_slow, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@remote-playback-availability Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android, RemotePlayback] Implement HTMLMediaElement.remote.connect().
BUG=578833
TEST=manual
Note that the HTMLMediaElement doesn't know yet when the device picker is
closed so the promise can't ever be rejected.
Committed: https://crrev.com/425c737709699db20d84928175d0dd9d37b76a97
Cr-Commit-Position: refs/heads/master@{#384252}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Removed so far unnecessary method #Patch Set 3 : Changed m_mediaElement type to fix compile with oilpan=false #Patch Set 4 : Added connect() to the global interface list #
Total comments: 9
Patch Set 5 : Resolved the comments #Patch Set 6 : Y U HAZ OILPAN DIZABLO? >:( #
Total comments: 2
Patch Set 7 : Ownership fix for Oilpan-disabled fix #Patch Set 8 : Fixed nooilpan build #
Total comments: 2
Patch Set 9 : Fixed timing for rejecting the promises #
Dependent Patchsets: Messages
Total messages: 45 (14 generated)
avayvod@chromium.org changed reviewers: + mlamouri@chromium.org, philipj@opera.com
PTaL
https://codereview.chromium.org/1827853002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1827853002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2929: remotePlaybackClient()->connectionResultReceived(true); This is never called with false, can't the stateChanged(WebRemotePlaybackState::Connected) call be used to resolve the promise?
Removed so far unnecessary method
PTAL https://codereview.chromium.org/1827853002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1827853002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2929: remotePlaybackClient()->connectionResultReceived(true); On 2016/03/23 at 16:09:31, philipj_UTC7 wrote: > This is never called with false, can't the stateChanged(WebRemotePlaybackState::Connected) call be used to resolve the promise? Done. I'll need it later when I plumb the dialog closures back to HTMLMediaElement but I will add it then :)
Changed m_mediaElement type to fix compile with oilpan=false
Added connect() to the global interface list
The CQ bit was checked by avayvod@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/1827853002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827853002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt LGTM and I'll leave the rest to Mounir :) https://codereview.chromium.org/1827853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h (right): https://codereview.chromium.org/1827853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h:59: RawPtrWillBeWeakMember<HTMLMediaElement> m_mediaElement; Hmm, why weak? I guess that RawPtr is needed pre-Oilpan to avoid a reference cycle, but it would be nice to just have a strong reference in the Oilpan world unless that has some unintended side effect? Also, m_mediaElement is never cleared in the non-Oilpan case so I guess the pointer can become stale?
https://codereview.chromium.org/1827853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp (right): https://codereview.chromium.org/1827853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:89: // TODO(avayvod): should we have a flag or reuse the one for the PresentationRequest::start(). nit: "a flag to disable user gesture requirement". I spent a couple of seconds wondering what flag you were mentioning. https://codereview.chromium.org/1827853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:100: m_connectPromiseResolvers.append(resolver); Should you add a TODO that we are only resolving them if it succeeds? because we are not aware if it fails to connect, right? https://codereview.chromium.org/1827853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h (right): https://codereview.chromium.org/1827853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h:59: RawPtrWillBeWeakMember<HTMLMediaElement> m_mediaElement; On 2016/03/24 at 07:31:57, philipj_UTC7 wrote: > Hmm, why weak? I guess that RawPtr is needed pre-Oilpan to avoid a reference cycle, but it would be nice to just have a strong reference in the Oilpan world unless that has some unintended side effect? +1. I believe oilpan will deal with the cycle: https://docs.google.com/presentation/d/1XPu03ymz8W295mCftEC9KshH9Icxfq81YwIJQ... And that would allow someone to do this: ``` var remote = media.remote; [.. stuff happens ..] [.. media is GC'd ..] remote.connect(); ``` If `media` is a weak member, it would not be possible to connect at that point but if it's a member it wouldn't be GC'd because `remote` has a reference to it. > Also, m_mediaElement is never cleared in the non-Oilpan case so I guess the pointer can become stale? Should ~HTMLMediaElement have something to prevent this or is there a strong binding that would prevent this to happen?
https://codereview.chromium.org/1827853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h (right): https://codereview.chromium.org/1827853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h:59: RawPtrWillBeWeakMember<HTMLMediaElement> m_mediaElement; On 2016/03/24 10:47:06, Mounir Lamouri wrote: > On 2016/03/24 at 07:31:57, philipj_UTC7 wrote: > > Also, m_mediaElement is never cleared in the non-Oilpan case so I guess the > pointer can become stale? > > Should ~HTMLMediaElement have something to prevent this or is there a strong > binding that would prevent this to happen? ~HTMLMediaElement would be one way, or perhaps WeakPtrWillBeMember, in which case HTMLMediaElement::createWeakPtr is needed. So much fun given that non-Oilpan is going away, but it can't be left as a potential use-after-free :/
PTaL https://codereview.chromium.org/1827853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp (right): https://codereview.chromium.org/1827853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:89: // TODO(avayvod): should we have a flag or reuse the one for the PresentationRequest::start(). On 2016/03/24 at 10:47:06, Mounir Lamouri wrote: > nit: "a flag to disable user gesture requirement". I spent a couple of seconds wondering what flag you were mentioning. Done. https://codereview.chromium.org/1827853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:100: m_connectPromiseResolvers.append(resolver); On 2016/03/24 at 10:47:06, Mounir Lamouri wrote: > Should you add a TODO that we are only resolving them if it succeeds? because we are not aware if it fails to connect, right? We're actually getting a disconnected state change if the connection fails so I added code to reject the promise when this happens. https://codereview.chromium.org/1827853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h (right): https://codereview.chromium.org/1827853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h:59: RawPtrWillBeWeakMember<HTMLMediaElement> m_mediaElement; On 2016/03/24 at 11:06:23, philipj_UTC7 wrote: > On 2016/03/24 10:47:06, Mounir Lamouri wrote: > > On 2016/03/24 at 07:31:57, philipj_UTC7 wrote: > > > Also, m_mediaElement is never cleared in the non-Oilpan case so I guess the > > pointer can become stale? > > > > Should ~HTMLMediaElement have something to prevent this or is there a strong > > binding that would prevent this to happen? > > ~HTMLMediaElement would be one way, or perhaps WeakPtrWillBeMember, in which case HTMLMediaElement::createWeakPtr is needed. So much fun given that non-Oilpan is going away, but it can't be left as a potential use-after-free :/ Sounds like if it's a Member in Oilpan world, it should be a RefPtr in non-Oilpan world so the same trick with holding a reference to remote works?
The CQ bit was checked by avayvod@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/1827853002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827853002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Y U HAZ OILPAN DIZABLO? >:(
The CQ bit was checked by avayvod@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/1827853002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827853002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1827853002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h (right): https://codereview.chromium.org/1827853002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h:59: RawPtrWillBeWeakMember<HTMLMediaElement> m_mediaElement; On 2016/03/24 16:08:40, whywhat_OOO_March_25_28 wrote: > On 2016/03/24 at 11:06:23, philipj_UTC7 wrote: > > On 2016/03/24 10:47:06, Mounir Lamouri wrote: > > > On 2016/03/24 at 07:31:57, philipj_UTC7 wrote: > > > > Also, m_mediaElement is never cleared in the non-Oilpan case so I guess > the > > > pointer can become stale? > > > > > > Should ~HTMLMediaElement have something to prevent this or is there a strong > > > binding that would prevent this to happen? > > > > ~HTMLMediaElement would be one way, or perhaps WeakPtrWillBeMember, in which > case HTMLMediaElement::createWeakPtr is needed. So much fun given that > non-Oilpan is going away, but it can't be left as a potential use-after-free :/ > > Sounds like if it's a Member in Oilpan world, it should be a RefPtr in > non-Oilpan world so the same trick with holding a reference to remote works? The trouble is that one can't really get the same behavior with and without Oilpan in a situation like this. I think the choice is between making GC observable in both cases, and making it observable only pre-Oilpan. https://codereview.chromium.org/1827853002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h (right): https://codereview.chromium.org/1827853002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h:59: RefPtrWillBeMember<HTMLMediaElement> m_mediaElement; Is this now a reference cycle that will leak memory?
Ownership fix for Oilpan-disabled fix
The CQ bit was checked by avayvod@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/1827853002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827853002/120001
PTaL https://codereview.chromium.org/1827853002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h (right): https://codereview.chromium.org/1827853002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h:59: RefPtrWillBeMember<HTMLMediaElement> m_mediaElement; On 2016/03/29 at 05:51:45, philipj_UTC7 wrote: > Is this now a reference cycle that will leak memory? In some non-existent build I guess. I gave up and added createWeakPtr() (and now need your OWNERS approval). Hopefully this will be cleaned up soon when *WillBe* classes will be removed and the non-oilpan bot shut down.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)
Fixed nooilpan build
The CQ bit was checked by avayvod@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/1827853002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827853002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with dead code removed. https://codereview.chromium.org/1827853002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp (right): https://codereview.chromium.org/1827853002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:135: resolver->reject(DOMException::create(AbortError, "Failed to connect to the remote device.")); That sounds like dead code. I'm not sure how you can end up receiving a Disconnected "event" given that you have `m_state == state` above and you wait to be connected before changing `m_state` to Connected.
Fixed timing for rejecting the promises
https://codereview.chromium.org/1827853002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp (right): https://codereview.chromium.org/1827853002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:135: resolver->reject(DOMException::create(AbortError, "Failed to connect to the remote device.")); On 2016/03/31 at 10:15:39, Mounir Lamouri wrote: > That sounds like dead code. I'm not sure how you can end up receiving a Disconnected "event" given that you have `m_state == state` above and you wait to be connected before changing `m_state` to Connected. Good catch. I actually prefer resolving the pending promises before checking if the state changed - in the "connected" - "connected" state the list should be empty anyway. in the "disconnected" - "disconnected" case there might be promises waiting for connect to succeed. Added a TODO to cleanup this when adding "connecting" state.
The CQ bit was checked by avayvod@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1827853002/#ps160001 (title: "Fixed timing for rejecting the promises")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1827853002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827853002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [Android, RemotePlayback] Implement HTMLMediaElement.remote.connect(). BUG=578833 TEST=manual Note that the HTMLMediaElement doesn't know yet when the device picker is closed so the promise can't ever be rejected. ========== to ========== [Android, RemotePlayback] Implement HTMLMediaElement.remote.connect(). BUG=578833 TEST=manual Note that the HTMLMediaElement doesn't know yet when the device picker is closed so the promise can't ever be rejected. Committed: https://crrev.com/425c737709699db20d84928175d0dd9d37b76a97 Cr-Commit-Position: refs/heads/master@{#384252} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/425c737709699db20d84928175d0dd9d37b76a97 Cr-Commit-Position: refs/heads/master@{#384252} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
