WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
163659
[Modern Media Controls] Media Controller: click-to-start support
https://bugs.webkit.org/show_bug.cgi?id=163659
Summary
[Modern Media Controls] Media Controller: click-to-start support
Antoine Quint
Reported
2016-10-19 05:30:05 PDT
The media controller should correctly show and remove the start button based on the right media criteria.
Attachments
Patch
(64.21 KB, patch)
2016-10-19 05:37 PDT
,
Antoine Quint
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2
(
deleted
)
2016-10-19 06:52 PDT
,
Build Bot
no flags
Details
Patch
(63.90 KB, patch)
2016-10-19 10:21 PDT
,
Antoine Quint
no flags
Details
Formatted Diff
Diff
Patch for landing
(63.72 KB, patch)
2016-10-19 11:32 PDT
,
Antoine Quint
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2016-10-19 05:31:08 PDT
<
rdar://problem/28845656
>
Antoine Quint
Comment 2
2016-10-19 05:37:08 PDT
Created
attachment 292060
[details]
Patch
Build Bot
Comment 3
2016-10-19 06:52:01 PDT
Comment on
attachment 292060
[details]
Patch
Attachment 292060
[details]
did not pass ios-sim-ews (ios-simulator-wk2): Output:
http://webkit-queues.webkit.org/results/2324130
New failing tests: media/modern-media-controls/start-support/start-support-fullscreen.html
Build Bot
Comment 4
2016-10-19 06:52:04 PDT
Created
attachment 292065
[details]
Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Antoine Quint
Comment 5
2016-10-19 10:18:18 PDT
Raised
https://bugs.webkit.org/show_bug.cgi?id=163669
to track the iOS failure.
Antoine Quint
Comment 6
2016-10-19 10:21:43 PDT
Created
attachment 292079
[details]
Patch
Dean Jackson
Comment 7
2016-10-19 10:58:41 PDT
Comment on
attachment 292079
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=292079&action=review
> Source/WebCore/ChangeLog:30 > + * Modules/modern-media-controls/media/media-controller-support.js: Copied from Source/WebCore/Modules/modern-media-controls/media/media-controller.js.
I wonder what you should do with this line?
> Source/WebCore/ChangeLog:40 > + * Modules/modern-media-controls/media/start-support.js: Copied from Source/WebCore/Modules/modern-media-controls/media/media-controller.js.
And this one.
> Source/WebCore/Modules/modern-media-controls/media/media-controller-support.js:35 > + const media = mediaController.media; > + for (let eventType of this.mediaEvents) > + media.addEventListener(eventType, this);
Don't bother with the local variable? Just use mediaController.media.addEventListener.
> Source/WebCore/Modules/modern-media-controls/media/media-controller-support.js:49 > + // Implemented by subclasses.
Must these be implemented by subclasses? Is this parent version never called? If so, maybe put a console error here to make it clear (e.g. the closest we can get to an ASSERT)
> Source/WebCore/Modules/modern-media-controls/media/start-support.js:57 > + syncControl() > + { > + this.mediaController.controls.showsStartButton = this._shouldShowStartButton(); > + }
When is this called?
> LayoutTests/media/modern-media-controls/start-support/start-support-manual-play.html:59 > + media.addEventListener("pause", function() { > + debug(""); > + debug("Media is paused"); > + shouldBeFalse("mediaController.controls.showsStartButton"); > + debug(""); > + shadowRoot.host.remove(); > + media.remove(); > + finishJSTest(); > + });
What guarantees that this event listener will run after the media controls have reacted to the same event?
Antoine Quint
Comment 8
2016-10-19 11:05:29 PDT
(In reply to
comment #7
)
> Comment on
attachment 292079
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=292079&action=review
> > > Source/WebCore/ChangeLog:30 > > + * Modules/modern-media-controls/media/media-controller-support.js: Copied from Source/WebCore/Modules/modern-media-controls/media/media-controller.js. > > I wonder what you should do with this line?
I think I know what you mean.
> > Source/WebCore/ChangeLog:40 > > + * Modules/modern-media-controls/media/start-support.js: Copied from Source/WebCore/Modules/modern-media-controls/media/media-controller.js. > > And this one.
Sure.
> > Source/WebCore/Modules/modern-media-controls/media/media-controller-support.js:35 > > + const media = mediaController.media; > > + for (let eventType of this.mediaEvents) > > + media.addEventListener(eventType, this); > > Don't bother with the local variable? Just use > mediaController.media.addEventListener.
OK.
> > Source/WebCore/Modules/modern-media-controls/media/media-controller-support.js:49 > > + // Implemented by subclasses. > > Must these be implemented by subclasses? Is this parent version never > called? If so, maybe put a console error here to make it clear (e.g. the > closest we can get to an ASSERT)
They need not be implemented by subclasses, but are designed to be. If there is no control provided, neither buttonWasClicked() not syncControl() would ever be called.
> > Source/WebCore/Modules/modern-media-controls/media/start-support.js:57 > > + syncControl() > > + { > > + this.mediaController.controls.showsStartButton = this._shouldShowStartButton(); > > + } > > When is this called?
At the end of construction and as a result of media events being triggered.
> > LayoutTests/media/modern-media-controls/start-support/start-support-manual-play.html:59 > > + media.addEventListener("pause", function() { > > + debug(""); > > + debug("Media is paused"); > > + shouldBeFalse("mediaController.controls.showsStartButton"); > > + debug(""); > > + shadowRoot.host.remove(); > > + media.remove(); > > + finishJSTest(); > > + }); > > What guarantees that this event listener will run after the media controls > have reacted to the same event?
Nothing as it stands, but it works! I supposed I could evaluate all of these in a setTimeout().
Antoine Quint
Comment 9
2016-10-19 11:32:20 PDT
Created
attachment 292086
[details]
Patch for landing
Antoine Quint
Comment 10
2016-10-19 11:36:11 PDT
(In reply to
comment #8
)
> (In reply to
comment #7
) > > Comment on
attachment 292079
[details]
> > > LayoutTests/media/modern-media-controls/start-support/start-support-manual-play.html:59 > > > + media.addEventListener("pause", function() { > > > + debug(""); > > > + debug("Media is paused"); > > > + shouldBeFalse("mediaController.controls.showsStartButton"); > > > + debug(""); > > > + shadowRoot.host.remove(); > > > + media.remove(); > > > + finishJSTest(); > > > + }); > > > > What guarantees that this event listener will run after the media controls > > have reacted to the same event? > > Nothing as it stands, but it works! I supposed I could evaluate all of these > in a setTimeout().
Actually, they're registered first in the MediaControllerSupport constructor, and only after in the tests, so they're fired first in MediaControllerSupport.
WebKit Commit Bot
Comment 11
2016-10-19 12:07:18 PDT
Comment on
attachment 292086
[details]
Patch for landing Clearing flags on attachment: 292086 Committed
r207554
: <
http://trac.webkit.org/changeset/207554
>
WebKit Commit Bot
Comment 12
2016-10-19 12:07:22 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug