Bug 157376 – [WK2] Media controls don't update if controller is created after the interface is created

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

[WK2] Media controls don't update if controller is created after the interface is created

[Jer Noble]

Created attachment 278170 [details]
Patch

[Build Bot]

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]

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 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]

Created attachment 278199 [details]
Patch for landing

[Jer Noble]

Committed r200490: <http://trac.webkit.org/changeset/200490>

[Darin Adler]

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]

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. :)