Bug 161503 – Refactor the heuristic for showing media controls to take all media sessions into account

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Wenson Hsieh]

Refactor the heuristic for showing media controls to take all media sessions into account

[Wenson Hsieh]

Created attachment 287672 [details]
Patch

[Wenson Hsieh]

<rdar://problem/28033783>

[Wenson Hsieh]

Created attachment 287776 [details]
Patch

[Wenson Hsieh]

Created attachment 287785 [details]
Add missing ifdef guards to fix other platforms.

[Wenson Hsieh]

Created attachment 287805 [details]
Move main content sizing helpers back into MediaElementSession.cpp

[Darin Adler]

Comment on attachment 287805 [details]
Move main content sizing helpers back into MediaElementSession.cpp

View in context: https://bugs.webkit.org/attachment.cgi?id=287805&action=review

The patch generally looks good, but the mediaSessionInfo() and currentSessionInfos() functions have design problems. See my comments below.

> Source/WebCore/html/HTMLMediaElement.cpp:359
> +static bool mediaSessionMayBeConfusedForMainContent(const MediaSessionInfo& session)

"confused with" is probably good grammar, but I don’t think "confused for" is

> Source/WebCore/html/HTMLMediaElement.cpp:372
> +static const PlatformMediaSession* bestMediaSessionForShowingPlaybackControlsManager()

This function should return a MediaElementSession*; it will never return any other kind of PlatformMediaSession, and the single caller downcasts to MediaElementSession.

> Source/WebCore/html/HTMLMediaElement.cpp:376
> +    for (MediaSessionInfo info : PlatformMediaSessionManager::sharedManager().currentSessionInfos()) {

This unnecessarily makes a copy of each MediaSessionInfo as we iterate the vector. Instead the type should be MediaSessionInfo& or, better, just auto&, to use the objects in place within the vector.

> Source/WebCore/html/HTMLMediaElement.cpp:7233
> +    const PlatformMediaSession* bestMediaSession = bestMediaSessionForShowingPlaybackControlsManager();

auto* would be better than writing out the type here

> Source/WebCore/html/MediaElementSession.cpp:221
> +    RenderElement* renderer = m_element.renderer();

It’s generally incorrect to access the renderer for an element without having a guarantee that the code is only called when style is updated and layout is up to date. Putting calls to renderer behind abstractions like the session increases the risk that we will do that incorrectly. We should keep calls from the session to the renderer to the absolute minimum. Ideally the element calls its own renderer, not through an intermediary object, and it should do so only in functions called when renderer is known to be up to date.

There’s also a type problem here. Putting a renderer into a local variable that is specifically RenderElement* throws away type information. The renderer of an HTMLMediaElement is guaranteed to be a RenderMedia*, which both has more operations than what RenderElement* has, and might even have more efficient less polymorphic versions of some functions that RenderElement has. That’s why best style is to use auto* rather than RenderElement* in a case like this.

> Source/WebCore/platform/audio/PlatformMediaSession.h:170
> +    virtual void resetPlaybackSessionState() { };

No semicolon after a function body like this (see other functions above and below).

> Source/WebCore/platform/audio/PlatformMediaSession.h:192
> +struct MediaSessionInfo {

"info" is typically not a great name for any class; it’s mostly vacuous since all objects contain information. And this class is more than just some information about a media session; it contains a pointer to a specific media session object. It’s sort of “a media session with some precomputed attributes of that session”, which is only good because we happen to know the exact set that one particular algorithm needs. This is kind of a peculiar API. It has all the things that one particular caller happens to need, so it’s quite specific to its one use, but it masquerades as a general purpose collection of information.

It’s a poor design pattern to vend objects with raw pointers; keeping one around too long and using the pointer can lead to a serious bug, and really there is no reason to do that here.

There is a kind of false abstraction here, because MediaSession objects that are not MediaElementSession return an info structure but these never are used for anything; just immediately ruled out. The code in HTMLMediaElement already knows it’s specifically only interested in MediaElementSession.

A better design would be to add member functions to MediaElementSession that answer any of these questions as needed to make that code clear, then any structure needed to collect these and sort them would be something that the file defining bestMediaSessionForShowingPlaybackControlsManager would define locally for its purposes as needed.

> Source/WebCore/platform/audio/PlatformMediaSession.h:201
> +    MediaSessionInfo(const PlatformMediaSession* session, bool canShowControlsManager, bool isVisibleInViewport, bool isPlayingWithAudio, bool isLargeEnoughForMainContent, double timeOfLastUserInteraction)
> +        : session(session)
> +        , canShowControlsManager(canShowControlsManager)
> +        , isVisibleInViewport(isVisibleInViewport)
> +        , isPlayingWithAudio(isPlayingWithAudio)
> +        , isLargeEnoughForMainContent(isLargeEnoughForMainContent)
> +        , timeOfLastUserInteraction(timeOfLastUserInteraction)
> +    {
> +    }

A struct does not need a constructor like this; callers can simply use aggregate initialization even without this.

[Wenson Hsieh]

Comment on attachment 287805 [details]
Move main content sizing helpers back into MediaElementSession.cpp

View in context: https://bugs.webkit.org/attachment.cgi?id=287805&action=review

>> Source/WebCore/html/HTMLMediaElement.cpp:359
>> +static bool mediaSessionMayBeConfusedForMainContent(const MediaSessionInfo& session)
> 
> "confused with" is probably good grammar, but I don’t think "confused for" is

True -- changed to "mediaSessionMayBeConfusedWithMainContent"

>> Source/WebCore/html/HTMLMediaElement.cpp:372
>> +static const PlatformMediaSession* bestMediaSessionForShowingPlaybackControlsManager()
> 
> This function should return a MediaElementSession*; it will never return any other kind of PlatformMediaSession, and the single caller downcasts to MediaElementSession.

Changed to return a MediaElementSession*.

>> Source/WebCore/html/HTMLMediaElement.cpp:376
>> +    for (MediaSessionInfo info : PlatformMediaSessionManager::sharedManager().currentSessionInfos()) {
> 
> This unnecessarily makes a copy of each MediaSessionInfo as we iterate the vector. Instead the type should be MediaSessionInfo& or, better, just auto&, to use the objects in place within the vector.

Sounds good -- changed to use auto&.

> Source/WebCore/html/HTMLMediaElement.cpp:387
> +    MediaSessionInfo strongestSessionCandidate = candidateSessionInfos.first();

MediaSessionInfo -> auto

>> Source/WebCore/html/HTMLMediaElement.cpp:7233
>> +    const PlatformMediaSession* bestMediaSession = bestMediaSessionForShowingPlaybackControlsManager();
> 
> auto* would be better than writing out the type here

Changed to use auto.

>> Source/WebCore/html/MediaElementSession.cpp:221
>> +    RenderElement* renderer = m_element.renderer();
> 
> It’s generally incorrect to access the renderer for an element without having a guarantee that the code is only called when style is updated and layout is up to date. Putting calls to renderer behind abstractions like the session increases the risk that we will do that incorrectly. We should keep calls from the session to the renderer to the absolute minimum. Ideally the element calls its own renderer, not through an intermediary object, and it should do so only in functions called when renderer is known to be up to date.
> 
> There’s also a type problem here. Putting a renderer into a local variable that is specifically RenderElement* throws away type information. The renderer of an HTMLMediaElement is guaranteed to be a RenderMedia*, which both has more operations than what RenderElement* has, and might even have more efficient less polymorphic versions of some functions that RenderElement has. That’s why best style is to use auto* rather than RenderElement* in a case like this.

I see -- this is concerning! Our current heuristic for determining whether autoplaying videos should present media controls goes through a main content check that uses the element renderer (asking for width, height, etc.) so the renderer being out of date when updating the controls manager is a problem we may have today; perhaps what we need to do when scheduling an update is to check if the renderer needs to be updated, and defer the update until after the renderer updates. I'll add a comment describing this.

To the second point, changed RenderElement* to auto.

>> Source/WebCore/platform/audio/PlatformMediaSession.h:170
>> +    virtual void resetPlaybackSessionState() { };
> 
> No semicolon after a function body like this (see other functions above and below).

Whoops, good catch. Fixed!

>> Source/WebCore/platform/audio/PlatformMediaSession.h:192
>> +struct MediaSessionInfo {
> 
> "info" is typically not a great name for any class; it’s mostly vacuous since all objects contain information. And this class is more than just some information about a media session; it contains a pointer to a specific media session object. It’s sort of “a media session with some precomputed attributes of that session”, which is only good because we happen to know the exact set that one particular algorithm needs. This is kind of a peculiar API. It has all the things that one particular caller happens to need, so it’s quite specific to its one use, but it masquerades as a general purpose collection of information.
> 
> It’s a poor design pattern to vend objects with raw pointers; keeping one around too long and using the pointer can lead to a serious bug, and really there is no reason to do that here.
> 
> There is a kind of false abstraction here, because MediaSession objects that are not MediaElementSession return an info structure but these never are used for anything; just immediately ruled out. The code in HTMLMediaElement already knows it’s specifically only interested in MediaElementSession.
> 
> A better design would be to add member functions to MediaElementSession that answer any of these questions as needed to make that code clear, then any structure needed to collect these and sort them would be something that the file defining bestMediaSessionForShowingPlaybackControlsManager would define locally for its purposes as needed.

This makes sense. I've changed MediaElementSession to have member functions that answer questions asked when determining which session is the best candidate. To ensure that we don't keep asking the session for the same thing, I've moved session info MediaElementSessionInfo to be defined in the implementation, so it's only used when computing the best candidate session.

>> Source/WebCore/platform/audio/PlatformMediaSession.h:201
>> +    }
> 
> A struct does not need a constructor like this; callers can simply use aggregate initialization even without this.

Removed, since I'm not relying on a copy constructor here anymore.

> Source/WebKit2/WebProcess/cocoa/WebPlaybackSessionManager.mm:422
> +    HTMLMediaElement *element = ensureModel(contextId).mediaElement();

HTMLMediaElement * -> auto

[Wenson Hsieh]

Created attachment 287872 [details]
Addresses review feedback

[Darin Adler]

Comment on attachment 287872 [details]
Addresses review feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=287872&action=review

> Source/WebCore/html/MediaElementSession.cpp:294
> +bool MediaElementSession::isVisibleInViewportOrFullscreen() const
> +{
> +    if (m_element.isFullscreen())
> +        return true;
> +
> +    auto renderer = m_element.renderer();
> +    return renderer && renderer->visibleInViewportState() == RenderElement::VisibleInViewport;
> +}

Not sure we need this function to be a member of session. It’s a question about the media element, not something specific to the session, and there is not any logic shared with other functions in the session class. I think it could be done inside HTMLMediaElement.cpp. This would have the benefit of moving the renderer access into the element class rather than hiding it under a layer of abstraction.

But I am not sure about this suggestion.

> Source/WebCore/html/MediaElementSession.cpp:299
> +bool MediaElementSession::isPlayingAudio() const
> +{
> +    return m_element.isPlaying() && m_element.hasAudio() && !m_element.muted();
> +}

Ditto.

[Wenson Hsieh]

Comment on attachment 287872 [details]
Addresses review feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=287872&action=review

>> Source/WebCore/html/MediaElementSession.cpp:294
>> +}
> 
> Not sure we need this function to be a member of session. It’s a question about the media element, not something specific to the session, and there is not any logic shared with other functions in the session class. I think it could be done inside HTMLMediaElement.cpp. This would have the benefit of moving the renderer access into the element class rather than hiding it under a layer of abstraction.
> 
> But I am not sure about this suggestion.

I think this makes sense, especially since the helper that uses this is in HTMLMediaElement in the first place. Moved to HTMLMediaElement.

>> Source/WebCore/html/MediaElementSession.cpp:299
>> +}
> 
> Ditto.

Moved to HTMLMediaElement.

[Wenson Hsieh]

Created attachment 287874 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 287874 [details]
Patch for landing

Clearing flags on attachment: 287874

Committed r205412: <http://trac.webkit.org/changeset/205412>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.