| Summary: | [WK2] Media controls don't update if controller is created after the interface is created | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||
| Component: | New Bugs | Assignee: | Jer Noble <jer.noble> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | buildbot, rniwa | ||||||||
| Priority: | P2 | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Jer Noble
2016-05-05 09:54:28 PDT
Created attachment 278170 [details]
Patch
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. 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
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 Created attachment 278199 [details]
Patch for landing
Committed r200490: <http://trac.webkit.org/changeset/200490> 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? 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. :) |