WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
157376
[WK2] Media controls don't update if controller is created after the interface is created
https://bugs.webkit.org/show_bug.cgi?id=157376
Summary
[WK2] Media controls don't update if controller is created after the interfac...
Jer Noble
Reported
2016-05-05 09:54:28 PDT
[WK2] Media controls don't update if controller is created after the interface is created
Attachments
Patch
(50.97 KB, patch)
2016-05-05 09:56 PDT
,
Jer Noble
bdakin
: review+
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from ews103 for mac-yosemite
(1018.97 KB, application/zip)
2016-05-05 10:31 PDT
,
Build Bot
no flags
Details
Patch for landing
(60.47 KB, patch)
2016-05-05 15:34 PDT
,
Jer Noble
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Jer Noble
Comment 1
2016-05-05 09:56:02 PDT
Created
attachment 278170
[details]
Patch
Build Bot
Comment 2
2016-05-05 10:31:15 PDT
Comment on
attachment 278170
[details]
Patch
Attachment 278170
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.webkit.org/results/1272026
Number of test failures exceeded the failure limit.
Build Bot
Comment 3
2016-05-05 10:31:18 PDT
Created
attachment 278173
[details]
Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Beth Dakin
Comment 4
2016-05-05 14:50:11 PDT
Comment on
attachment 278170
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=278170&action=review
R=me but definitely take a look at those broken builds.
> Source/WebCore/ChangeLog:10 > + called. This necessatates a bunch of changes in HTMLMediaElement and related classes to
*necessitates
Jer Noble
Comment 5
2016-05-05 15:34:43 PDT
Created
attachment 278199
[details]
Patch for landing
Jer Noble
Comment 6
2016-05-05 16:17:45 PDT
Committed
r200490
: <
http://trac.webkit.org/changeset/200490
>
Darin Adler
Comment 7
2016-05-06 12:19:34 PDT
Comment on
attachment 278199
[details]
Patch for landing View in context:
https://bugs.webkit.org/attachment.cgi?id=278199&action=review
> Source/WebCore/html/MediaController.cpp:96 > + Ref<TimeRanges> bufferedRanges = m_mediaElements.first()->buffered();
Can this use auto?
> Source/WebCore/html/MediaController.cpp:110 > + Ref<TimeRanges> seekableRanges = m_mediaElements.first()->seekable();
Ditto.
> Source/WebCore/html/MediaController.cpp:124 > + Ref<TimeRanges> playedRanges = m_mediaElements.first()->played();
Ditto.
> Source/WebCore/platform/cocoa/WebPlaybackSessionModel.h:62 > + virtual Vector<WTF::String> audioMediaSelectionOptions() const = 0;
Should just be String, not WTF::String.
> Source/WebCore/platform/cocoa/WebPlaybackSessionModel.h:64 > + virtual Vector<WTF::String> legibleMediaSelectionOptions() const = 0;
Ditto.
> Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.h:73 > + double duration() const final; > + double currentTime() const final; > + double bufferedTime() const final;
Should any of these overrides have been private or protected instead of public?
> Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.h:78 > + Vector<WTF::String> audioMediaSelectionOptions() const final;
Ditto.
> Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.h:80 > + Vector<WTF::String> legibleMediaSelectionOptions() const final;
Ditto.
> Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:279 > +const Vector<AtomicString>& WebPlaybackSessionModelMediaElement::observedEventNames()
There’s a stray space here.
> Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:293 > + static NeverDestroyed<Vector<AtomicString>> sEventNames; > > - if (track->enabled()) > - selectedAudioIndex = index; > + if (!sEventNames.get().size()) { > + sEventNames.get().append(eventNames().durationchangeEvent); > + sEventNames.get().append(eventNames().pauseEvent); > + sEventNames.get().append(eventNames().playEvent); > + sEventNames.get().append(eventNames().ratechangeEvent); > + sEventNames.get().append(eventNames().timeupdateEvent); > + sEventNames.get().append(eventNames().addtrackEvent); > + sEventNames.get().append(eventNames().removetrackEvent); > + sEventNames.get().append(eventNames().webkitcurrentplaybacktargetiswirelesschangedEvent); > } > + return sEventNames.get();
Not new code, but: This is not the best pattern for a static initialization like this. It’s better to have a function that builds the vector and just write something more like this: static NeverDestroyed<Vector<AtomicString>> names = createObservedEventNamesVector(); return names.get(); It’s also better to not unroll loops that do append over and over again. You can use an array or an initializer_list and a loop to avoid that. It’s also best to use reserveInitialCapacity and uncheckedAppend. Or call a function that does that (the Vector constructor that takes two iterators, perhaps?).
> Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:299 > + static NeverDestroyed<AtomicString> sEventNameAll = "allEvents"; > + return sEventNameAll;
Not new code, but: The "s" prefix here is not needed and not our normal coding style. Should possibly use the ConstructAtomicString idiom here.
> Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:337 > +Vector<WTF::String> WebPlaybackSessionModelMediaElement::audioMediaSelectionOptions() const
Just String, not WTF::String.
> Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:347 > + for (auto& audioTrack : m_audioTracksForMenu) > + audioTrackDisplayNames.append(captionPreferences.displayNameForTrack(audioTrack.get()));
More efficient to use reserveInitialCapacity and uncheckedAppend.
> Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:358 > + return 0;
Really correct to use 0 in this case?
> Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:361 > +Vector<WTF::String> WebPlaybackSessionModelMediaElement::legibleMediaSelectionOptions() const
Same comment apply here as in audioMediaSelectionOptions. But also, can we take these two almost identical functions and make them share code?
> Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:387 > + if (!m_mediaElement || !m_mediaElement->mediaControlsHost()) > + return selectedIndex; > + > + AtomicString displayMode = m_mediaElement->mediaControlsHost()->captionDisplayMode(); > + TextTrack* offItem = m_mediaElement->mediaControlsHost()->captionMenuOffItem(); > + TextTrack* automaticItem = m_mediaElement->mediaControlsHost()->captionMenuAutomaticItem();
Seems repetitive. On more local variable for mediaControlsHost maybe? Could call it “controls”.
> Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:413 > + return m_mediaElement ? m_mediaElement->webkitCurrentPlaybackTargetIsWireless() : false;
I often prefer to use && in cases like these (happens in a couple other places).
> Source/WebCore/platform/mac/WebPlaybackSessionInterfaceMac.mm:114 > + WebPlaybackControlsManager* controlsManager = playBackControlsManager(); > + [controlsManager setSeekableTimeRanges:timeRangesToArray(timeRanges).get()];
Why the local variable?
Jer Noble
Comment 8
2016-05-06 18:07:17 PDT
I've got further refactoring work to do, so I'll take care of all these in a follow up bug. (In reply to
comment #7
)
> Comment on
attachment 278199
[details]
> Patch for landing > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=278199&action=review
> > > Source/WebCore/html/MediaController.cpp:96 > > + Ref<TimeRanges> bufferedRanges = m_mediaElements.first()->buffered(); > > Can this use auto?
Yes.
> > Source/WebCore/html/MediaController.cpp:110 > > + Ref<TimeRanges> seekableRanges = m_mediaElements.first()->seekable(); > > Ditto.
Yes.
> > Source/WebCore/html/MediaController.cpp:124 > > + Ref<TimeRanges> playedRanges = m_mediaElements.first()->played(); > > Ditto.
Yes.
> > Source/WebCore/platform/cocoa/WebPlaybackSessionModel.h:62 > > + virtual Vector<WTF::String> audioMediaSelectionOptions() const = 0; > > Should just be String, not WTF::String.
Ok.
> > Source/WebCore/platform/cocoa/WebPlaybackSessionModel.h:64 > > + virtual Vector<WTF::String> legibleMediaSelectionOptions() const = 0; > > Ditto.
Ok.
> > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.h:73 > > + double duration() const final; > > + double currentTime() const final; > > + double bufferedTime() const final; > > Should any of these overrides have been private or protected instead of > public?
> > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.h:78 > > + Vector<WTF::String> audioMediaSelectionOptions() const final; > > Ditto. > > > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.h:80 > > + Vector<WTF::String> legibleMediaSelectionOptions() const final; > > Ditto. > > > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:279 > > +const Vector<AtomicString>& WebPlaybackSessionModelMediaElement::observedEventNames() > > There’s a stray space here.
Removed.
> > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:293 > > + static NeverDestroyed<Vector<AtomicString>> sEventNames; > > > > - if (track->enabled()) > > - selectedAudioIndex = index; > > + if (!sEventNames.get().size()) { > > + sEventNames.get().append(eventNames().durationchangeEvent); > > + sEventNames.get().append(eventNames().pauseEvent); > > + sEventNames.get().append(eventNames().playEvent); > > + sEventNames.get().append(eventNames().ratechangeEvent); > > + sEventNames.get().append(eventNames().timeupdateEvent); > > + sEventNames.get().append(eventNames().addtrackEvent); > > + sEventNames.get().append(eventNames().removetrackEvent); > > + sEventNames.get().append(eventNames().webkitcurrentplaybacktargetiswirelesschangedEvent); > > } > > + return sEventNames.get(); > > Not new code, but: This is not the best pattern for a static initialization > like this. It’s better to have a function that builds the vector and just > write something more like this: > > static NeverDestroyed<Vector<AtomicString>> names = > createObservedEventNamesVector(); > return names.get(); > > It’s also better to not unroll loops that do append over and over again. You > can use an array or an initializer_list and a loop to avoid that.
Sure thing; will update.
> It’s also best to use reserveInitialCapacity and uncheckedAppend. Or call a > function that does that (the Vector constructor that takes two iterators, > perhaps?). > > > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:299 > > + static NeverDestroyed<AtomicString> sEventNameAll = "allEvents"; > > + return sEventNameAll; > > Not new code, but: The "s" prefix here is not needed and not our normal > coding style. Should possibly use the ConstructAtomicString idiom here.
Okay.
> > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:337 > > +Vector<WTF::String> WebPlaybackSessionModelMediaElement::audioMediaSelectionOptions() const > > Just String, not WTF::String. > > > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:347 > > + for (auto& audioTrack : m_audioTracksForMenu) > > + audioTrackDisplayNames.append(captionPreferences.displayNameForTrack(audioTrack.get())); > > More efficient to use reserveInitialCapacity and uncheckedAppend.
Ok.
> > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:358 > > + return 0; > > Really correct to use 0 in this case?
It's not a regression, but it definitely needs to be fixed.
> > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:361 > > +Vector<WTF::String> WebPlaybackSessionModelMediaElement::legibleMediaSelectionOptions() const > > Same comment apply here as in audioMediaSelectionOptions. But also, can we > take these two almost identical functions and make them share code?
Unfortunately, captionPreferences.displayNameForTrack() is overloaded with the two different track types. We could get fancy with a templated static helper, I guess.
> > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:387 > > + if (!m_mediaElement || !m_mediaElement->mediaControlsHost()) > > + return selectedIndex; > > + > > + AtomicString displayMode = m_mediaElement->mediaControlsHost()->captionDisplayMode(); > > + TextTrack* offItem = m_mediaElement->mediaControlsHost()->captionMenuOffItem(); > > + TextTrack* automaticItem = m_mediaElement->mediaControlsHost()->captionMenuAutomaticItem(); > > Seems repetitive. On more local variable for mediaControlsHost maybe? Could > call it “controls”.
"host".
> > Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:413 > > + return m_mediaElement ? m_mediaElement->webkitCurrentPlaybackTargetIsWireless() : false; > > I often prefer to use && in cases like these (happens in a couple other > places).
Indeed!
> > Source/WebCore/platform/mac/WebPlaybackSessionInterfaceMac.mm:114 > > + WebPlaybackControlsManager* controlsManager = playBackControlsManager(); > > + [controlsManager setSeekableTimeRanges:timeRangesToArray(timeRanges).get()]; > > Why the local variable?
No good reason. :)
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