[Webkit-unassigned] [Bug 21562] Layering violation: MediaPlayer should not reference/use FrameView

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=21562


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #239020|review?                     |review+
               Flag|                            |




--- Comment #7 from Darin Adler <darin at apple.com>  2014-10-08 10:44:25 PST ---
(From update of attachment 239020)
View in context: https://bugs.webkit.org/attachment.cgi?id=239020&action=review

Great to do this. I’m concerned about the change in MediaPlayerPrivateAVFoundation::preferredRenderingMode because it’s just removing code. Seems we could have just removed that in a separate patch if it wasn’t needed. I’d like an expert to explain why it’s not needed.

> Source/WebCore/html/HTMLMediaElement.h:586
> +    virtual bool mediaPlayerInMediaDocument() const override;

If it was me I’d probably say const override final, not that it really matters.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:593
> -    if (!m_frameView)
> +    if (!m_mediaPlayerClient)
>          return false;
> -    Document* document = m_frameView->frame().document();
> -    return document && document->isMediaDocument();
> +    return m_visible && m_mediaPlayerClient->mediaPlayerInMediaDocument();

I like writing these as a single line:

    return m_visible && m_mediaPlayerClient && m_mediaPlayerClient->mediaPlayerInMediaDocument();

But also, I wonder if it’s legal for the client to be null. Some other code seems to assume it’s not.

The name m_mediaPlayerClient is so wordy. I really wish it was just m_client given it’s a data member of the MediaPlayer class!

> Source/WebCore/platform/graphics/MediaPlayer.h:267
> +    virtual bool mediaPlayerInMediaDocument() const { return false; }

I’m not so fond of this name, because it sounds like a function that returns “a media player that is in a media document” rather than a function that answers a question about whether the media player is in a media document. I guess maybe the mediaPlayer is a prefix we use on a lot of function names. Maybe mediaPlayerIsInMediaDocument would be better.

But also, maybe there’s a more abstract way to describe this. It seems like a concept layering violation for the media player to even have a concept of “in media document”, so it would be nice to change this concept to instead express what behavior we expect from the media player rather than the situation.

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:105
> -    if (!m_player->visible() || !m_player->frameView() || assetStatus() == MediaPlayerAVAssetStatusUnknown)
> +    if (!m_player->visible() || assetStatus() == MediaPlayerAVAssetStatusUnknown)

Why is it OK to take this check out? I don’t see us doing anything new to solve the problem that this check would have been solving. Was it simply an unneeded check?

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:108
>      if (supportsAcceleratedRendering() && m_player->mediaPlayerClient()->mediaPlayerRenderingCanBeAccelerated(m_player))

This seems to assume that the client is never null. Was the m_player->frameView() check above perhaps relevant to guaranteeing that? Can the client be null?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.