[webkit-reviews] review granted: [Bug 21562] Layering violation: MediaPlayer should not reference/use FrameView : [Attachment 239020] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 8 10:44:27 PDT 2014


Darin Adler <darin at apple.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 21562: Layering violation: MediaPlayer should not reference/use FrameView
https://bugs.webkit.org/show_bug.cgi?id=21562

Attachment 239020: Patch
https://bugs.webkit.org/attachment.cgi?id=239020&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.cp
p: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.cp
p: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?


More information about the webkit-reviews mailing list