Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2347763002:
[Blink, RemotePlayback] Reject the prompt() with OperationError if there's a pending promise for th… (Closed)
|
Created:
4 years, 3 months ago by whywhat Modified:
4 years, 2 months ago Reviewers:
mlamouri (slow - plz ping) CC:
blink-reviews, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Blink, RemotePlayback] Reject the prompt() with OperationError if there's a pending promise for this element.
BUG=647441
TEST=avayvod.github.io/remote-playback/test.html + layout test
Committed: https://crrev.com/0fc8b636799720b632a58b8ad0cbe2f3bef0c7ce
Cr-Commit-Position: refs/heads/master@{#422810}
Patch Set 1 #Patch Set 2 : Added UserGesture flag for tests, removed some checks in HTMLMediaElement, added layout test #
Total comments: 11
Patch Set 3 : Addressed Mounir's comments #
Total comments: 4
Patch Set 4 : Fixed the test #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 30 (12 generated)
avayvod@chromium.org changed reviewers: + mlamouri@chromium.org
PTaL
Do you have a plan to import the tests on github to layout tests?
On 2016/09/16 at 01:30:58, haraken wrote: > Do you have a plan to import the tests on github to layout tests? Yes, definitely. I'll see if there's some test runner plumbing I can reuse for my CLs.
Added UserGesture flag for tests, removed some checks in HTMLMediaElement, added layout test
You might want to update the TEST= line too. https://codereview.chromium.org/2347763002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/remoteplayback/prompt-twice-throws.html (right): https://codereview.chromium.org/2347763002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/remoteplayback/prompt-twice-throws.html:13: internals.settings.setRemotePlaybackRequiresUserGesture(false); nstead, I think you can wrap this with a method that either simulate a click with EventSender or requires a click on a button so the test can be shared, see: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/media/med... https://codereview.chromium.org/2347763002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/Settings.in (right): https://codereview.chromium.org/2347763002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/Settings.in:424: remotePlaybackRequiresUserGesture initial=true I don't think you need this, this below. https://codereview.chromium.org/2347763002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2347763002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2205: webMediaPlayer()->requestRemotePlayback(); Why this change? https://codereview.chromium.org/2347763002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2211: webMediaPlayer()->requestRemotePlaybackControl(); ditto
https://codereview.chromium.org/2347763002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2347763002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2205: webMediaPlayer()->requestRemotePlayback(); On 2016/09/22 at 09:22:43, mlamouri (slow) wrote: > Why this change? With the gesture requirement removed, it's possible to call prompt() before the webMediaPlayer() is initialized for local playback.
Addressed Mounir's comments
https://codereview.chromium.org/2347763002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/remoteplayback/prompt-twice-throws.html (right): https://codereview.chromium.org/2347763002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/remoteplayback/prompt-twice-throws.html:13: internals.settings.setRemotePlaybackRequiresUserGesture(false); On 2016/09/22 at 09:22:43, mlamouri wrote: > nstead, I think you can wrap this with a method that either simulate a click with EventSender or requires a click on a button so the test can be shared, see: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/media/med... Done. I anticipate the flag to be needed by integration tests in the future though. https://codereview.chromium.org/2347763002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/Settings.in (right): https://codereview.chromium.org/2347763002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/Settings.in:424: remotePlaybackRequiresUserGesture initial=true On 2016/09/22 at 09:22:43, mlamouri wrote: > I don't think you need this, this below. Done. https://codereview.chromium.org/2347763002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2347763002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2205: webMediaPlayer()->requestRemotePlayback(); On 2016/09/22 at 13:18:27, whywhat wrote: > On 2016/09/22 at 09:22:43, mlamouri (slow) wrote: > > Why this change? > I think that from the test it's possible to call prompt() before the webMediaPlayer() is initialized. https://codereview.chromium.org/2347763002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2211: webMediaPlayer()->requestRemotePlaybackControl(); On 2016/09/22 at 09:22:43, mlamouri wrote: > ditto ditto.
Description was changed from ========== [Blink, RemotePlayback] Reject the prompt() with OperationError if there's a pending promise for this element. BUG=647441 TEST=avayvod.github.io/remote-playback/test.html ========== to ========== [Blink, RemotePlayback] Reject the prompt() with OperationError if there's a pending promise for this element. BUG=647441 TEST=avayvod.github.io/remote-playback/test.html + layout test ==========
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
PTAL
https://codereview.chromium.org/2347763002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2347763002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2205: webMediaPlayer()->requestRemotePlayback(); On 2016/09/30 at 19:11:33, whywhat wrote: > On 2016/09/22 at 13:18:27, whywhat wrote: > > On 2016/09/22 at 09:22:43, mlamouri (slow) wrote: > > > Why this change? > > > I think that from the test it's possible to call prompt() before the webMediaPlayer() is initialized. Does that mean that we are dropping on the floor the call if webMediaPlayer() wasn't initialised yet? https://codereview.chromium.org/2347763002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/remoteplayback/prompt-twice-throws.html (right): https://codereview.chromium.org/2347763002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/remoteplayback/prompt-twice-throws.html:20: v.remote.prompt().then( Maybe you should have another simulated user gesture? Otherwise, I think your test only passes because of the check for the popup already open happening before the utiliseUserGesture check. https://codereview.chromium.org/2347763002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/remoteplayback/prompt-twice-throws.html:30: eventSender.mouseUp(); Having a <button>click me</button> as a target would be better because it makes this test sharable with w3c.
Fixed the test
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2347763002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2347763002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2205: webMediaPlayer()->requestRemotePlayback(); On 2016/10/01 at 14:55:53, mlamouri wrote: > On 2016/09/30 at 19:11:33, whywhat wrote: > > On 2016/09/22 at 13:18:27, whywhat wrote: > > > On 2016/09/22 at 09:22:43, mlamouri (slow) wrote: > > > > Why this change? > > > > > I think that from the test it's possible to call prompt() before the webMediaPlayer() is initialized. > > Does that mean that we are dropping on the floor the call if webMediaPlayer() wasn't initialised yet? Yes, this will change once we stop forwarding this calls to webMediaPlayer() and handle them in RemotePlayback https://codereview.chromium.org/2347763002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/remoteplayback/prompt-twice-throws.html (right): https://codereview.chromium.org/2347763002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/remoteplayback/prompt-twice-throws.html:20: v.remote.prompt().then( On 2016/10/01 at 14:55:53, mlamouri wrote: > Maybe you should have another simulated user gesture? Otherwise, I think your test only passes because of the check for the popup already open happening before the utiliseUserGesture check. Hm, if the checks are reversed, the utiliseUserGesture would still work since we call prompt() twice from the same handler, no? https://codereview.chromium.org/2347763002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/remoteplayback/prompt-twice-throws.html:30: eventSender.mouseUp(); On 2016/10/01 at 14:55:53, mlamouri wrote: > Having a <button>click me</button> as a target would be better because it makes this test sharable with w3c. Alright. I'll try to remember to point noone to the play promise test example then :) (unless someone fixes it?)
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 https://codereview.chromium.org/2347763002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/remoteplayback/prompt-twice-throws.html (right): https://codereview.chromium.org/2347763002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/remoteplayback/prompt-twice-throws.html:45: </body> This file is over-indented. LayoutTests are usually not that much indented. Example: third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-visibility.html
On 2016/10/04 at 10:48:47, mlamouri wrote: > lgtm > > https://codereview.chromium.org/2347763002/diff/60001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/media/remoteplayback/prompt-twice-throws.html (right): > > https://codereview.chromium.org/2347763002/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/media/remoteplayback/prompt-twice-throws.html:45: </body> > This file is over-indented. LayoutTests are usually not that much indented. Example: third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-visibility.html Is there a style guide rule for JS or LayoutTests? So far I click on random video-controls-* tests that are not about overflow and they are all indented four spaces...
The CQ bit was checked by avayvod@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Blink, RemotePlayback] Reject the prompt() with OperationError if there's a pending promise for this element. BUG=647441 TEST=avayvod.github.io/remote-playback/test.html + layout test ========== to ========== [Blink, RemotePlayback] Reject the prompt() with OperationError if there's a pending promise for this element. BUG=647441 TEST=avayvod.github.io/remote-playback/test.html + layout test Committed: https://crrev.com/0fc8b636799720b632a58b8ad0cbe2f3bef0c7ce Cr-Commit-Position: refs/heads/master@{#422810} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0fc8b636799720b632a58b8ad0cbe2f3bef0c7ce Cr-Commit-Position: refs/heads/master@{#422810} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
