[Webkit-unassigned] [Bug 39557] Full screen doesn't work for video elements

[bugzilla-daemon at webkit.org]

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





--- Comment #33 from Jer Noble <jer.noble at apple.com>  2010-05-25 11:13:28 PST ---
(In reply to comment #31)
> (From update of attachment 56943 [details])
> I'll try not to repeat comments from comment 30.
> 
> > +MediaPlayerPrivateFullscreenWindow::~MediaPlayerPrivateFullscreenWindow()
> > +{
> > +}
> 
> Should we destroy our HWND here if that hasn't already happened?

Added! (in response to your last comment, actually)

> > +void MediaPlayerPrivateFullscreenWindow::createWindow()
> > +{
> > +    static ATOM windowAtom;
> > +    static LPCWSTR windowClassName = L"MediaPlayerPrivateFullscreenWindowClass";
> > +    if (!windowAtom) {
> > +        WNDCLASSEX wcex = {0};
> > +        wcex.cbSize = sizeof(WNDCLASSEX);
> > +        wcex.style = CS_HREDRAW | CS_VREDRAW;
> > +        wcex.lpfnWndProc = &staticWndProc;
> 
> No need for the & here.

Removed!

> > +        wcex.hInstance = WebCore::instanceHandle();
> 
> You shouldn't need the "WebCore::" here.

Removed!

> > +void MediaPlayerPrivateFullscreenWindow::close()
> > +{
> > +    ::DestroyWindow(m_hwnd);
> > +    m_hwnd = 0;
> > +}
> 
> Looks like our WM_DESTROY code will set m_hwnd to 0. We could assert that that has happened here.

Asserted!

> > +HWND MediaPlayerPrivateFullscreenWindow::hwnd() const
> > +{
> > +    return m_hwnd;
> > +}
> > +
> > +WKCACFLayerRenderer* MediaPlayerPrivateFullscreenWindow::layerRenderer() const
> > +{
> > +    return m_layerRenderer.get();
> > +}
> > +
> > +WKCACFLayer* MediaPlayerPrivateFullscreenWindow::rootChildLayer() const
> > +{
> > +    return m_rootChild.get();
> > +}
> 
> We often put trivial getters like these inline in the class declaration.

Moved!

> > +void MediaPlayerPrivateFullscreenWindow::setRootChildLayer(PassRefPtr<WKCACFLayer> rootChild)
> > +{
> > +    if (m_rootChild != rootChild) {
> > +        if (m_rootChild)
> > +            m_rootChild->removeFromSuperlayer();
> > +
> > +        m_rootChild = rootChild;
> > +    }
> > +
> > +    if (m_rootChild) {
> 
> I think it would be a little nicer if this were an early return.

This is the function that, in a previous comment, you had asked me to add a comment to saying why we don't return early so that, in your words: "someone like me won't just change it without testing." :-D

That said, I'm going to try doing and early return and discover what the corner case is and see if I can correct it.

> > +#if NDEBUG
> > +        rootLayer->setBackgroundColor(CGColorGetConstantColor(kCGColorBlack));
> > +#else
> > +        RetainPtr<CGColorRef> redColor(AdoptCF, CGColorCreateGenericRGB(1, 0, 0, 1));
> > +        rootLayer->setBackgroundColor(redColor.get());
> > +#endif
> 
> I think the normal idiom is "#ifndef NDEBUG debug code #else release code #endif"

Reversed!

> > +LRESULT CALLBACK MediaPlayerPrivateFullscreenWindow::staticWndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
> 
> No need to specify CALLBACK here. Specifying it on the declaration is all that's required.

Deleted!

> > +    case WM_PAINT:
> > +        m_layerRenderer->renderSoon();
> > +        break;
> > +    }
> 
> Why do we only render soon, rather than immediately?

Because both the render() and paint() functions are declared as private.  Is it worth moving one of those into public?

> > +class MediaPlayerPrivateFullscreenWindow {
> > +public:
> > +    MediaPlayerPrivateFullscreenWindow(MediaPlayerPrivateFullscreenClient*);
> > +    virtual ~MediaPlayerPrivateFullscreenWindow();
> 
> There's no need for this destructor to be virtual. (See <http://www.parashift.com/c++-faq-lite/virtual-functions.html#faq-20.7>.)

Unvirtualized!

> > +protected:
> > +    static LRESULT CALLBACK staticWndProc(HWND, UINT message, WPARAM, LPARAM);
> > +    LRESULT wndProc(HWND, UINT message, WPARAM, LPARAM);
> > +
> > +    MediaPlayerPrivateFullscreenClient* m_client;
> > +    OwnPtr<WKCACFLayerRenderer> m_layerRenderer;
> > +    RefPtr<WKCACFLayer> m_rootChild;
> > +    HWND m_hwnd;
> > +};
> 
> I think you should use private instead of protected.

Privatized!

> >  bool MediaPlayerPrivateQuickTimeVisualContext::supportsFullscreen() const
> >  {
> > +#if USE(ACCELERATED_COMPOSITING)
> > +    Document* document = m_player->mediaPlayerClient()->mediaPlayerOwningDocument(); 
> > +    if (document)
> > +        return document->settings()->acceleratedCompositingEnabled();
> > +    return true;
> > +#else
> >      return false;
> > +#endif
> >  }
> 
> You need to null-check the result of document->settings().

Null checked!

> It's a little surprising that we'd allow a <video> element that has no document to go into fullscreen mode.

That's true.  Or, false.  Yes, it is now false.

> (Did you test that when you disable hardware acceleration in the DirectX Control Panel, fullscreen is no longer supported?)

I tested by turning off the Debug menu "Enable Accelerated Compositing".  I'll test again with the DXCPL.

> The comment here is pretty good (though the first letter of it should be capitalized). I think you should mention in the ChangeLog that this is a pre-existing bug you're fixing.

I'll mention it.

> > +void WKCACFLayer::setLayoutClient(WKCACFLayerLayoutClient* layoutClient)
> > +{
> > +    if (layoutClient == m_layoutClient)
> > +        return;
> > +
> > +    m_layoutClient = layoutClient;
> > +    CACFLayerSetLayoutCallback(layer(), m_layoutClient ? &layoutSublayersProc : 0);    
> > +}
> 
> Still no need for the & here.

Remove!

> > +class WKCACFLayerLayoutClient {
> > +public:
> > +    virtual void layoutSublayersOfLayer(WKCACFLayer*) = 0;
> > +};
> 
> You should add a protected virtual destructor.

Added!

> > +void FullscreenVideoController::LayoutClient::layoutSublayersOfLayer(WKCACFLayer* layer) 
> > +{
> > +    if (layer != m_parent->m_rootChild) 
> > +        return;
> 
> I thought you were going to add an assertion here?

I was?  It sounds like a good idea.  I'll do that.

> > +    HTMLMediaElement* mediaElement = m_parent->m_mediaElement.get();
> > +    if (!mediaElement)
> > +        return;
> > +
> > +    WKCACFLayer* videoLayer = mediaElement->platformLayer();
> > +    if (!videoLayer || videoLayer->superlayer() != layer)
> > +        return;
> > +
> > +    FloatPoint videoAnchor = videoLayer->anchorPoint();
> 
> You don't use this variable anymore.

Removed!

> Since videoAnchor isn't used, you shouldn't mention it.

Removed!

> If you teach MediaPlayerPrivateFullscreenWindow to close itself when it is destroyed, you won't have to call close() here. I don't think you even need to set m_fullscreenWindow to 0, since it will get destroyed automatically when your destructor returns.

True.  Paranoia.  Removed!

> ...or here (and you don't need to call setRootChildLayer(0), either).

Removed here also!

> > +    // We previously ripped the mediaElement's platform layer out
> > +    // of it's orginial layer tree to display it in our fullscreen
> 
> Typo: it's -> its

Oh, I hate that typo!  Or the even worse typo, the non-posessive plural apostrophe, like "stair's".

> > +HWND FullscreenVideoController::parentHWND()
> > +{
> > +    if (m_mediaElement) {
> > +        WebView* webView = kit(m_mediaElement->document()->page());
> > +        if (webView)
> > +            return webView->topLevelParent();
> 
> Why not use the WebView's HWND? That way the fullscreen window will disappear if the user switches tabs.

An excellent idea.  Oh wait, this doesn't really jive with passing the HWND into the FullscreenLayerWindow's constructor.  Hm.  A conundrum.  I think I prefer the m_client->parentHWND() technique.

> This function could use some early-return lovin'.

Totes.

> > +void FullscreenVideoController::onKeyDown(int virtualKey)
> > +{
> > +    if (virtualKey == VK_ESCAPE) {
> > +        if (m_mediaElement)
> > +            m_mediaElement->exitFullscreen();
> > +    }
> > +}
> 
> This seems like a new behavior that's unrelated to the rest of this patch. I think it deserves its own bug report/patch. (It also isn't mentioned in the ChangeLog (not even the function name!).)

Oh, yes, at the last moment and at great expense, I noticed that ESC was no longer causing fullscreen to exit.  The keydown event is posted, but never the WM_CHAR event.  I'll add a comment in the ChangeLog.

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