[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





--- Comment #8 from Carlos Garcia Campos <cgarcia at igalia.com>  2014-10-08 11:10:25 PST ---
(In reply to comment #7)
> (From update of attachment 239020 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239020&action=review

Thanks for the 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.

It's removed here, because this patch removes MediaPlayer::frameView().

> > 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();

I followed the pattern of all other MediaPlayer methods for consistency, that return early if the media player client is null.

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

I'm not familiar with the media player code, but all the places in MediaPlayer where the client is used, there's a null check first.

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

Agree, it also surprised me.

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

Yes, all virtual methods in MediaPlayerClient use the mediaPlayer prefix.

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

I don't really know why it's needed to know whether the media player is in a media document. It's only used by mac.

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

Yes, it was unneeded. The only one calling MediaPlayer::setFrameView is RenderVideo, and everytime it was called with a valid frame view, it also called setVisible(true), and when called with null, it also called setVisible(false), so having a valid frame view was equivalent to being visible. Since here we are already checking whether it's visible or not, the frame view check was redundant.

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

Good point, but there are other places in that file where the client is used assuming it's not null and without checking of the player has a frame view, so I'm not sure.

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