[webkit-reviews] review denied: [Bug 39557] Full screen doesn't work for video elements : [Attachment 56943] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 39557: Full screen doesn't work for video elements
https://bugs.webkit.org/show_bug.cgi?id=39557

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

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
I'll try not to repeat comments from comment 30.

> +MediaPlayerPrivateFullscreenWindow::~MediaPlayerPrivateFullscreenWindow()
> +{
> +}

Should we destroy our HWND here if that hasn't already happened?

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

> +	   wcex.hInstance = WebCore::instanceHandle();

You shouldn't need the "WebCore::" here.

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

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

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

> +#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"

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

> +    case WM_PAINT:
> +	   m_layerRenderer->renderSoon();
> +	   break;
> +    }

Why do we only render soon, rather than immediately?

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

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

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

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

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

> @@ -939,6 +947,12 @@ void MediaPlayerPrivateQuickTimeVisualCo
>  #endif
>      // The layer will get hooked up via
RenderLayerBacking::updateGraphicsLayerConfiguration().
>  #endif
> +
> +    // fill the newly created layer with image data, so we're not looking at

> +    // an empty layer until the next time a new image is available, which
could
> +    // be a long time if we're paused.
> +    if (m_visualContext)
> +	   retrieveCurrentImage();
>  }

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.

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

> +class WKCACFLayerLayoutClient {
> +public:
> +    virtual void layoutSublayersOfLayer(WKCACFLayer*) = 0;
> +};

You should add a protected virtual destructor.

> +void
FullscreenVideoController::LayoutClient::layoutSublayersOfLayer(WKCACFLayer*
layer) 
> +{
> +    if (layer != m_parent->m_rootChild) 
> +	   return;

I thought you were going to add an assertion here?

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

> +    FloatRect layerBounds = layer->bounds();
> +
> +    FloatSize videoSize = mediaElement->player()->naturalSize();
> +    float scaleFactor;
> +    if (videoSize.aspectRatio() > layerBounds.size().aspectRatio())
> +	   scaleFactor = layerBounds.width() / videoSize.width();
> +    else
> +	   scaleFactor = layerBounds.height() / videoSize.height();
> +    videoSize.scale(scaleFactor);
> +
> +    // Calculate the centered position based on the videoAnchor,
videoBounds, and layerBounds:

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

>  FullscreenVideoController::~FullscreenVideoController()
>  {
> -    if (movie())
> -	   movie()->exitFullscreen();
> -}
> +    m_rootChild->setLayoutClient(0);
>  
> -QTMovieGWorld* FullscreenVideoController::movie() const
> -{
> -    if (!m_mediaElement)
> -	   return 0;
> -
> -    PlatformMedia platformMedia = m_mediaElement->platformMedia();
> -    if (platformMedia.type != PlatformMedia::QTMovieGWorldType)
> -	   return 0;
> -
> -    return platformMedia.media.qtMovieGWorld;
> +    if (m_fullscreenWindow) {
> +	   m_fullscreenWindow->close();
> +	   m_fullscreenWindow = 0;
> +    }
>  }

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.

>  void FullscreenVideoController::exitFullscreen()
>  {
> -    SetWindowLongPtr(m_hudWindow, 0, 0);
> -    if (movie())
> -	   movie()->exitFullscreen();
> -
> -    ASSERT(!IsWindow(m_hudWindow));
> -    m_videoWindow = 0;
> -    m_hudWindow = 0;
> +    if (m_fullscreenWindow) {
> +	   m_fullscreenWindow->close();
> +	   m_fullscreenWindow->setRootChildLayer(0);
> +	   m_fullscreenWindow = 0;
> +    }

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

> +    // 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

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

This function could use some early-return lovin'.

> +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!).)