[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