Bug 39557 – Full screen doesn't work for video elements

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

full screen doesn't work for video elements

[Jer Noble]

Created attachment 56808 [details]
Patch

[Jer Noble]

Will attach a new patch made against an updated tree.

[Jer Noble]

Created attachment 56811 [details]
Patch

[Maciej Stachowiak]

Comment on attachment 56811 [details]
Patch

There seem to be a lot of project file changes in WebCore.vcproj relating to JSIDBDatabase files. I assume these are not intended.

Comment on attachment 56811 [details]
Patch

> +        * platform/graphics/win/MediaPlayerPrivateFullscreenWindow.cpp: Added.
> +        * platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h: Added.

Are these files new or are they being restored? If the latter, then you should use svn cp to preserve history.

[Jer Noble]

(In reply to comment #4)
> (From update of attachment 56811 [details])
> There seem to be a lot of project file changes in WebCore.vcproj relating to JSIDBDatabase files. I assume these are not intended.

Visual Studio will arbitrarily re-arrange the order of files in .vcproj files when other source files are added.  I can try to undo the damage.

[Jer Noble]

(In reply to comment #5)
> (From update of attachment 56811 [details])
> > +        * platform/graphics/win/MediaPlayerPrivateFullscreenWindow.cpp: Added.
> > +        * platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h: Added.
> 
> Are these files new or are they being restored? If the latter, then you should use svn cp to preserve history.

They are brand new.  I'm not entirely happy with the name; they could as easily been named platform/graphics/win/FullscreenWindow.cpp/h.

[Jer Noble]

Created attachment 56826 [details]
Patch

[Jer Noble]

Updated the copyright notices in MediaPlayerPrivateFullscreenWindow.h/cpp.  Removed the extraneous changes to the WebCore.vcproj.

[Jer Noble]

Created attachment 56830 [details]
Patch

[Chris Marrin]

Comment on attachment 56830 [details]
Patch

This is an unofficial review - no status change

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 60048)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,31 @@
> [phone]  Jer Noble  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        full screen doesn't work for video elements
> +        https://bugs.webkit.org/show_bug.cgi?id=39557
> +        rdar://problem/8011813
> +        
> +        Add fullscreen support for MediaPlayerPrivateVisualContext.  A new class, MediaPlayerPrivateFullscreenWindow,
> +        provides the fullscreen hwnd and layer renderer.  Any WKCACFLayer can be provided to MediaPlayerPrivateFullscreenWindow
> +        so future additional MediaPlayerPrivate implementations can use the fullscreen window.
> +        
> +        Minor additions have been made to the FloatSize and IntSize classes.

It would be nice to note the reason for the changes

> Index: WebCore/platform/graphics/FloatSize.h
> ===================================================================
> --- WebCore/platform/graphics/FloatSize.h	(revision 60048)
> +++ WebCore/platform/graphics/FloatSize.h	(working copy)
> @@ -63,6 +63,14 @@ public:
>  
>      bool isEmpty() const { return m_width <= 0 || m_height <= 0; }
>  
> +    float aspectRatio() const { return m_width / m_height; }

You should zero check m_height here to avoid divide by 0

> Index: WebCore/platform/graphics/IntSize.h
> ===================================================================
> --- WebCore/platform/graphics/IntSize.h	(revision 60048)
> +++ WebCore/platform/graphics/IntSize.h	(working copy)
> @@ -72,6 +72,8 @@ public:
>  
>      bool isEmpty() const { return m_width <= 0 || m_height <= 0; }
>      bool isZero() const { return !m_width && !m_height; }
> +
> +    float aspectRatio() const { return static_cast<float>(m_width) / static_cast<float>(m_height); }

Same as above

> Index: WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.cpp
> ===================================================================
> 
> +void MediaPlayerPrivateFullscreenWindow::createWindow()
> +{
> +    static ATOM windowAtom = 0;
> +    if (!windowAtom) {
> +        WNDCLASSEX wcex = {0};
> +        wcex.cbSize         = sizeof(WNDCLASSEX);
> +        wcex.style          = CS_HREDRAW | CS_VREDRAW | CS_OWNDC;
> +        wcex.lpfnWndProc    = &staticWndProc;
> +        wcex.hInstance      = WebCore::instanceHandle();
> +        wcex.lpszClassName  = L"MediaPlayerPrivateFullscreenWindowClass";
> +        windowAtom = ::RegisterClassEx(&wcex);

WebKit style is to not align equal signs, I believe

> +    }
> +
> +    if (m_hwnd)
> +        close();
> +
> +    MONITORINFO mi = {0};
> +    mi.cbSize = sizeof(MONITORINFO);
> +    GetMonitorInfo(MonitorFromWindow(0, MONITOR_DEFAULTTOPRIMARY), &mi);
> +    IntRect monitorRect = mi.rcMonitor;
> +
> +    CREATESTRUCTW cs = {0};
> +    cs.x = 0;
> +    cs.y = 0;
> +    cs.cx = monitorRect.width();
> +    cs.cy = monitorRect.height();
> +    cs.style = WS_POPUP | WS_VISIBLE;
> +    cs.dwExStyle = 0;
> +    cs.lpszClass = L"MediaPlayerPrivateFullscreenWindowClass";
> +    cs.lpszName = L"";
> +    cs.lpCreateParams = (LPVOID)this;
> +    
> +    m_hwnd = ::CreateWindowExW(cs.dwExStyle, 
> +                            cs.lpszClass, 
> +                            cs.lpszName, 
> +                            cs.style, 
> +                            cs.x, 
> +                            cs.y, 
> +                            cs.cx, 
> +                            cs.cy, 
> +                            cs.hwndParent, 
> +                            cs.hMenu, 
> +                            cs.hInstance, 
> +                            cs.lpCreateParams);

Webkit style is also to put params on one line, only splitting when lines are really long

> +}
> +
> +void MediaPlayerPrivateFullscreenWindow::close()
> +{
> +    ::DestroyWindow(m_hwnd);
> +    m_hwnd = 0;
> +}
> +
> +HWND MediaPlayerPrivateFullscreenWindow::hwnd() const
> +{
> +    return m_hwnd;
> +}
> +
> +WKCACFLayerRenderer* MediaPlayerPrivateFullscreenWindow::layerRenderer()
> +{
> +    return m_layerRenderer.get();
> +}

This method can be const

> +
> +WKCACFLayer* MediaPlayerPrivateFullscreenWindow::rootChildLayer()
> +{
> +    return m_rootChild.get();
> +}

As can this one.

> +
> +void MediaPlayerPrivateFullscreenWindow::setRootChildLayer(PassRefPtr<WKCACFLayer> rootChild)
> +{
> +    if (m_rootChild != rootChild) {
> +
> +        if (m_rootChild) {
> +            m_rootChild->removeFromSuperlayer();
> +            m_rootChild.clear();
> +        }
> +
> +        m_rootChild = rootChild;
> +    }
> +
> +    if (m_rootChild) {
> +        m_layerRenderer->setRootChildLayer(m_rootChild.get());
> +        WKCACFLayer* rootLayer = m_rootChild->rootLayer();
> +        CGRect rootBounds = m_rootChild->rootLayer()->bounds();
> +        m_rootChild->setFrame(rootBounds);

WKCACFLayer no longer has a setFrame() API. I removed it to simplify the API. Frame is always so confusing because it's nothing more than a composite of bounds.size and position. So you can replace this with calls to setBounds and setPosition.

> +        m_layerRenderer->setScrollFrame(IntPoint(rootBounds.origin), IntSize(rootBounds.size));
> +        m_rootChild->setBackgroundColor(CGColorGetConstantColor(kCGColorBlack));
> +        rootLayer->setBackgroundColor(CGColorGetConstantColor(kCGColorBlack));

The background of the rootChild should never be seen (always covered by the video) right? You might want to make it's background red (at least in debug mode) to make it easier to spot problems. But maybe video needs to show the background before it's started rendering?

> +    }
> +}

Since you're doing fullscreen video with a fullscreen window, it seems like you should be able to animate it like we do on Mac right? Not for now of course, but is there a bug open about this?

> +
> +LRESULT CALLBACK MediaPlayerPrivateFullscreenWindow::staticWndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
> +{
> +    LONG_PTR longPtr = GetWindowLongPtr(hWnd, GWLP_USERDATA);
> +
> +    if (!longPtr && message == WM_CREATE) {
> +        LPCREATESTRUCT lpcs = reinterpret_cast<LPCREATESTRUCT>(lParam);
> +        longPtr = (LONG_PTR)lpcs->lpCreateParams;
> +        ::SetWindowLongPtr(hWnd, GWLP_USERDATA, longPtr);
> +    }
> +
> +    if (MediaPlayerPrivateFullscreenWindow* window = reinterpret_cast<MediaPlayerPrivateFullscreenWindow*>(longPtr))
> +        return window->wndProc(hWnd, message, wParam, lParam);
> +
> +    return ::DefWindowProc(hWnd, message, wParam, lParam);    
> +}
> +
> +LRESULT MediaPlayerPrivateFullscreenWindow::wndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
> +{
> +    LRESULT lResult = 0;
> +    switch (message) {
> +    case WM_CREATE:
> +        m_hwnd = hWnd;
> +        m_layerRenderer->setHostWindow(m_hwnd);
> +        m_layerRenderer->createRenderer();
> +        m_layerRenderer->setNeedsDisplay();
> +        break;
> +    case WM_DESTROY:
> +        m_hwnd = 0;
> +        m_layerRenderer->destroyRenderer();
> +        m_layerRenderer->setHostWindow(0);
> +    case WM_WINDOWPOSCHANGED:
> +        {
> +            LPWINDOWPOS wp = (LPWINDOWPOS)lParam;
> +            if (!(wp->flags & SWP_NOMOVE)) {
> +                m_layerRenderer->resize();
> +                WKCACFLayer* rootLayer = m_rootChild->rootLayer();
> +                CGRect rootBounds = m_rootChild->rootLayer()->bounds();
> +                m_rootChild->setFrame(rootBounds);

Same as above

> +                m_rootChild->setNeedsLayout();
> +                m_layerRenderer->setScrollFrame(IntPoint(rootBounds.origin), IntSize(rootBounds.size));
> +            }
> +        }
> +        break;
> +    case WM_PAINT:
> +        m_layerRenderer->renderSoon();
> +        break;
> +    }
> +
> +    if (m_client)
> +        lResult = m_client->fullscreenClientWndProc(hWnd, message, wParam, lParam);
> +
> +    return lResult;
> +}
> +
> +}
> Index: WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h
> ===================================================================
> --- WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h	(revision 0)
> +++ WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h	(revision 0)
> @@ -0,0 +1,68 @@
> +/*
> + * Copyright (C) 2010 Apple Inc. All rights reserved.
> + * 
> + * Redistribution and use in source and binary forms, with or without 
> + * modification, are permitted provided that the following conditions 
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright 
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the 
> + *    documentation and/or other materials provided with the distribution.
> + * 
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS AS IS 
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR 
> + * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
> + * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF 
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS 
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN 
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) 
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF 
> + * THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef MediaPlayerPrivateFullscreenWindow_h
> +#define MediaPlayerPrivateFullscreenWindow_h
> +
> +#include "WKCACFLayer.h"
> +#include "WKCACFLayerRenderer.h"
> +#include <windows.h>
> +#include <wtf/OwnPtr.h>
> +
> +namespace WebCore {
> +
> +class MediaPlayerPrivateFullscreenClient {
> +public:
> +    virtual LRESULT fullscreenClientWndProc(HWND, UINT message, WPARAM, LPARAM) = 0;
> +};
> +
> +class MediaPlayerPrivateFullscreenWindow {
> +public:
> +    MediaPlayerPrivateFullscreenWindow(MediaPlayerPrivateFullscreenClient*);
> +    virtual ~MediaPlayerPrivateFullscreenWindow();
> +
> +    void createWindow();
> +    void close();
> +    
> +    HWND hwnd() const;
> +
> +    WKCACFLayerRenderer* layerRenderer();
> +
> +    WKCACFLayer* rootChildLayer();

Const for these two

> +    void setRootChildLayer(PassRefPtr<WKCACFLayer>);
> +
> +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;
> +};
> +
> +}
> +
> +#endif
> Index: WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp
> ===================================================================
> --- WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp	(revision 60048)
> +++ WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp	(working copy)
> @@ -150,7 +150,7 @@ MediaPlayerPrivateQuickTimeVisualContext
>  
>  bool MediaPlayerPrivateQuickTimeVisualContext::supportsFullscreen() const
>  {
> -    return false;
> +    return true;
>  }
>  
>  PlatformMedia MediaPlayerPrivateQuickTimeVisualContext::platformMedia() const
> @@ -765,7 +765,7 @@ void MediaPlayerPrivateQuickTimeVisualCo
>  
>                  buffer.unlockBaseAddress();
>                  layer->rootLayer()->setNeedsRender();
> -            }
> +                }

This looks odd. Why the change in indent? Hard to see whether or not it's right out of context

>      } else
>  #endif
>          m_player->repaint();
> @@ -939,6 +939,10 @@ void MediaPlayerPrivateQuickTimeVisualCo
>  #endif
>      // The layer will get hooked up via RenderLayerBacking::updateGraphicsLayerConfiguration().
>  #endif
> +
> +    // fill the newly created layer with image data
> +    if (m_visualContext)
> +        retrieveCurrentImage();
>  }
>  
>  void MediaPlayerPrivateQuickTimeVisualContext::destroyLayerForMovie()
> Index: WebCore/platform/graphics/win/WKCACFLayer.cpp
> ===================================================================
> --- WebCore/platform/graphics/win/WKCACFLayer.cpp	(revision 60048)
> +++ WebCore/platform/graphics/win/WKCACFLayer.cpp	(working copy)
> @@ -154,6 +154,7 @@ PassRefPtr<WKCACFLayer> WKCACFLayer::cre
>  
>  WKCACFLayer::WKCACFLayer(LayerType type)
>      : m_layer(AdoptCF, CACFLayerCreate(toCACFLayerType(type)))
> +    , m_layoutClient(0)
>      , m_needsDisplayOnBoundsChange(false)
>  {
>      CACFLayerSetUserData(layer(), this);
> @@ -286,11 +287,10 @@ void  WKCACFLayer::adoptSublayers(WKCACF
>  void WKCACFLayer::removeFromSuperlayer()
>  {
>      WKCACFLayer* superlayer = this->superlayer();
> -    if (!superlayer)
> -        return;
> -
>      CACFLayerRemoveFromSuperlayer(layer());
> -    superlayer->setNeedsCommit();
> +
> +    if (superlayer)
> +        superlayer->setNeedsCommit();
>  }
>  
>  WKCACFLayer* WKCACFLayer::internalSublayerAtIndex(int index) const
> @@ -343,6 +343,19 @@ void WKCACFLayer::setBounds(const CGRect
>          setNeedsDisplay();
>  }
>  
> +void WKCACFLayer::setFrame(const CGRect& rect)
> +{
> +    CGRect oldFrame = frame();
> +    if (CGRectEqualToRect(rect, oldFrame))
> +        return;
> +
> +    CACFLayerSetFrame(layer(), rect);
> +    setNeedsCommit();
> +
> +    if (m_needsDisplayOnBoundsChange)
> +        setNeedsDisplay();
> +}
> +

Ah, I see. You've added back setFrame! I'd really like to leave it out if we can.

>  void WKCACFLayer::setContentsGravity(ContentsGravityType type)
>  {
>      CACFLayerSetContentsGravity(layer(), toCACFContentsGravityType(type));
> @@ -418,6 +431,32 @@ void WKCACFLayer::internalSetNeedsDispla
>      CACFLayerSetNeedsDisplay(layer(), dirtyRect);
>  }
>  
> +void WKCACFLayer::setLayoutClient(WKCACFLayerLayoutClient* layoutClient)
> +{
> +    if (layoutClient == m_layoutClient)
> +        return;
> +
> +    m_layoutClient = layoutClient;
> +    CACFLayerSetLayoutCallback(layer(), m_layoutClient ? &layoutSublayersProc : 0);    
> +}
> +
> +void WKCACFLayer::layoutSublayersProc(CACFLayerRef caLayer) 
> +{
> +    WKCACFLayer* layer = WKCACFLayer::layer(caLayer);
> +    if (layer && layer->m_layoutClient)
> +        layer->m_layoutClient->layoutSublayersOfLayer(layer);
> +}
> +
> +WKCACFLayerLayoutClient* WKCACFLayer::layoutClient() const
> +{
> +    return m_layoutClient;
> +}
> +
> +void WKCACFLayer::setNeedsLayout()
> +{
> +    CACFLayerSetNeedsLayout(layer());
> +}
> +
>  #ifndef NDEBUG
>  static void printIndent(int indent)
>  {
> @@ -500,9 +539,9 @@ void WKCACFLayer::printLayer(int indent)
>      CGImageRef layerContents = contents();
>      if (layerContents) {
>          printIndent(indent + 1);
> -        fprintf(stderr, "(contents (image [%d %d]))\n",
> +            fprintf(stderr, "(contents (image [%d %d]))\n",
>              CGImageGetWidth(layerContents), CGImageGetHeight(layerContents));
> -    }
> +        }

More odd indenting

>  
>      // Print sublayers if needed
>      int n = sublayerCount();
> Index: WebCore/platform/graphics/win/WKCACFLayer.h
> ===================================================================
> --- WebCore/platform/graphics/win/WKCACFLayer.h	(revision 60048)
> +++ WebCore/platform/graphics/win/WKCACFLayer.h	(working copy)
> @@ -44,9 +44,15 @@
>  
>  namespace WebCore {
>  
> +class WKCACFLayer;
>  class WKCACFAnimation;
>  class WKCACFTimingFunction;

Hmmm. I see spurious class declarations here (obviously from when I first implemented this). Probably best to get rid of them while we're here.

>  
> +class WKCACFLayerLayoutClient {
> +public:
> +    virtual void layoutSublayersOfLayer(WKCACFLayer*) = 0;
> +};
> +
>  class WKCACFLayer : public RefCounted<WKCACFLayer> {
>  public:
>      enum LayerType { Layer, TransformLayer };
> @@ -63,6 +69,10 @@ public:
>  
>      virtual void drawInContext(PlatformGraphicsContext*) { }
>  
> +    virtual void setLayoutClient(WKCACFLayerLayoutClient*);
> +    virtual WKCACFLayerLayoutClient* layoutClient() const;
> +    virtual void setNeedsLayout();
> +
>      void setNeedsDisplay(const CGRect* dirtyRect = 0)
>      {
>          internalSetNeedsDisplay(dirtyRect);
> @@ -173,6 +183,9 @@ public:
>      void setFilters(CFArrayRef filters) { CACFLayerSetFilters(layer(), filters); setNeedsCommit(); }
>      CFArrayRef filters() const { return CACFLayerGetFilters(layer()); }
>  
> +    void setFrame(const CGRect&);
> +    CGRect frame() const { return CACFLayerGetFrame(layer()); }

It would be really great if we could axe this.

> +
>      void setHidden(bool hidden) { CACFLayerSetHidden(layer(), hidden); setNeedsCommit(); }
>      bool isHidden() const { return CACFLayerIsHidden(layer()); }
>  
> @@ -260,8 +273,11 @@ protected:
>      void printLayer(int indent) const;
>  #endif
>  
> +    static void layoutSublayersProc(CACFLayerRef);
> +
>  private:
>      RetainPtr<CACFLayerRef> m_layer;
> +    WKCACFLayerLayoutClient* m_layoutClient;
>      bool m_needsDisplayOnBoundsChange;
>  };
>  
> Index: WebCore/platform/graphics/win/WebTiledLayer.cpp
> ===================================================================
> --- WebCore/platform/graphics/win/WebTiledLayer.cpp	(revision 60048)
> +++ WebCore/platform/graphics/win/WebTiledLayer.cpp	(working copy)
> @@ -73,6 +73,15 @@ void WebTiledLayer::setBounds(const CGRe
>      updateTiles();
>  }
>  
> +void WebTiledLayer::setFrame(const CGRect& rect)
> +{
> +    if (CGRectEqualToRect(rect, frame()))
> +        return;
> +
> +    WebLayer::setFrame(rect);
> +    updateTiles();
> +}
> +

See above

>  void WebTiledLayer::internalSetNeedsDisplay(const CGRect* dirtyRect)
>  {
>      // FIXME: Only setNeedsDisplay for tiles that are currently visible
> Index: WebCore/platform/graphics/win/WebTiledLayer.h
> ===================================================================
> --- WebCore/platform/graphics/win/WebTiledLayer.h	(revision 60048)
> +++ WebCore/platform/graphics/win/WebTiledLayer.h	(working copy)
> @@ -39,6 +39,7 @@ public:
>      virtual ~WebTiledLayer();
>  
>      virtual void setBounds(const CGRect&);
> +    virtual void setFrame(const CGRect&);

See above

>  
>  protected:
>      WebTiledLayer(const CGSize& tileSize, GraphicsLayer* owner);
> Index: WebKit/win/ChangeLog
> ===================================================================
> --- WebKit/win/ChangeLog	(revision 60027)
> +++ WebKit/win/ChangeLog	(working copy)
> @@ -1,3 +1,24 @@
> [phone]  Jer Noble  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        full screen doesn't work for video elements

Capitalize the start of sentences

> +        https://bugs.webkit.org/show_bug.cgi?id=39557
> +        rdar://problem/8011813
> +        
> +        Modified FullscreenVideoController to work with MediaPlayerPrivateFullscreenWindow.  The FullscreenVideoController
> +        is now MediaPlayerPrivate agnostic.
> +
> +        * FullscreenVideoController.cpp:
> +        (FullscreenVideoController::LayoutClient::LayoutClient):  
> +        (FullscreenVideoController::LayoutClient::layoutSublayersOfLayer):
> +        (FullscreenVideoController::FullscreenVideoController):
> +        (FullscreenVideoController::~FullscreenVideoController):
> +        (FullscreenVideoController::enterFullscreen):
> +        (FullscreenVideoController::exitFullscreen):
> +        (FullscreenVideoController::createHUDWindow):
> +        * FullscreenVideoController.h:
> +
>  2010-05-21  Steve Block  <[e-mail]>
>  
>          Reviewed by Jeremy Orlow.
> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 60028)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,17 @@
> [phone]  Jer Noble  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Re-enabled fullscreen support on windows, and modified tests to match.

You should add the bug number here like in the other Changelog

> +
> +        * platform/win/media/controls-after-reload-expected.txt:
> +        * platform/win/media/controls-drag-timebar-expected.txt:
> +        * platform/win/media/controls-strict-expected.txt:
> +        * platform/win/media/controls-styling-expected.txt:
> +        * platform/win/media/video-controls-rendering-expected.txt:
> +        * platform/win/media/video-display-toggle-expected.txt:
> +        * platform/win/media/video-no-audio-expected.txt:
> +
>  2010-05-22  Maciej Stachowiak  <[e-mail]>
>  
>          Reviewed by Dan Bernstein.

[Jer Noble]

(In reply to comment #11)
> > +    float aspectRatio() const { return m_width / m_height; }
> 
> You should zero check m_height here to avoid divide by 0

Why? If m_height is 0, it will return either +INF or -INF, which is a true answer for the size's aspect ratio.

> > +    float aspectRatio() const { return static_cast<float>(m_width) / static_cast<float>(m_height); }
> 
> Same as above

Same question as above.

> > +        WNDCLASSEX wcex = {0};
> > +        wcex.cbSize         = sizeof(WNDCLASSEX);
> > +        wcex.style          = CS_HREDRAW | CS_VREDRAW | CS_OWNDC;
> > +        wcex.lpfnWndProc    = &staticWndProc;
> > +        wcex.hInstance      = WebCore::instanceHandle();
> > +        wcex.lpszClassName  = L"MediaPlayerPrivateFullscreenWindowClass";
> > +        windowAtom = ::RegisterClassEx(&wcex);
> 
> WebKit style is to not align equal signs, I believe

In WNDCLASS  initializers in the WebKit source code, the equals are aligned much more ofter than the opposite.  It's not called out specifically in the style guidelines, so I went with what seemed used most often.

> Webkit style is also to put params on one line, only splitting when lines are really long

Changed.

> > +WKCACFLayerRenderer* MediaPlayerPrivateFullscreenWindow::layerRenderer()
> > +{
> > +    return m_layerRenderer.get();
> > +}
> 
> This method can be const

Changed.

> > +
> > +WKCACFLayer* MediaPlayerPrivateFullscreenWindow::rootChildLayer()
> > +{
> > +    return m_rootChild.get();
> > +}
> 
> As can this one.

Changed.

> > +    if (m_rootChild) {
> > +        m_layerRenderer->setRootChildLayer(m_rootChild.get());
> > +        WKCACFLayer* rootLayer = m_rootChild->rootLayer();
> > +        CGRect rootBounds = m_rootChild->rootLayer()->bounds();
> > +        m_rootChild->setFrame(rootBounds);
> 
> WKCACFLayer no longer has a setFrame() API. I removed it to simplify the API. Frame is always so confusing because it's nothing more than a composite of bounds.size and position. So you can replace this with calls to setBounds and setPosition.

In order to reproduce the setFrame() API, the caller would need to get the anchorPoint, multiply the result by the destination bounds width and height, and add the result to the intended bottom-left corner position, and then call setBounds() and setPosition().  This turns one line of code into 7 or 8 lines of code.  Why should we remove an API that just wraps CACFLayerSetFrame()?

> > +        m_layerRenderer->setScrollFrame(IntPoint(rootBounds.origin), IntSize(rootBounds.size));
> > +        m_rootChild->setBackgroundColor(CGColorGetConstantColor(kCGColorBlack));
> > +        rootLayer->setBackgroundColor(CGColorGetConstantColor(kCGColorBlack));
> 
> The background of the rootChild should never be seen (always covered by the video) right? You might want to make it's background red (at least in debug mode) to make it easier to spot problems. But maybe video needs to show the background before it's started rendering?

m_rootChild doesn't always occupy it's parent's entire bounds.  So its parent, rootLayer, must have a background color set.  Setting that color to red will just mean the "letterbox" bars will always be red in debug builds.

> Since you're doing fullscreen video with a fullscreen window, it seems like you should be able to animate it like we do on Mac right? Not for now of course, but is there a bug open about this?

There isn't a bug yet, but we should be able to do this.

> > +    WKCACFLayerRenderer* layerRenderer();
> > +
> > +    WKCACFLayer* rootChildLayer();
> 
> Const for these two

Changed.

> >  PlatformMedia MediaPlayerPrivateQuickTimeVisualContext::platformMedia() const
> > @@ -765,7 +765,7 @@ void MediaPlayerPrivateQuickTimeVisualCo
> >  
> >                  buffer.unlockBaseAddress();
> >                  layer->rootLayer()->setNeedsRender();
> > -            }
> > +                }
> 
> This looks odd. Why the change in indent? Hard to see whether or not it's right out of context

No, it's totally wrong.  Changed.


> Ah, I see. You've added back setFrame! I'd really like to leave it out if we can.

I don't really understand why.

> > -    }
> > +        }
> 
> More odd indenting

Changed.

> > +class WKCACFLayer;
> >  class WKCACFAnimation;
> >  class WKCACFTimingFunction;
> 
> Hmmm. I see spurious class declarations here (obviously from when I first implemented this). Probably best to get rid of them while we're here.

Removed.

> > +    void setFrame(const CGRect&);
> > +    CGRect frame() const { return CACFLayerGetFrame(layer()); }
> 
> It would be really great if we could axe this.

Why?

> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        full screen doesn't work for video elements
> 
> Capitalize the start of sentences

This is the title of the bug, both in Radar and Bugzilla.  I'll change the titles of the bugs.

> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Re-enabled fullscreen support on windows, and modified tests to match.
> 
> You should add the bug number here like in the other Changelog

Whoops, looks like webkit-patch didn't do that here.  Added.

[Jer Noble]

(In reply to comment #12)
> (In reply to comment #11)
> > The background of the rootChild should never be seen (always covered by the video) right? You might want to make it's background red (at least in debug mode) to make it easier to spot problems. But maybe video needs to show the background before it's started rendering?
> 
> m_rootChild doesn't always occupy it's parent's entire bounds.  So its parent, rootLayer, must have a background color set.  Setting that color to red will just mean the "letterbox" bars will always be red in debug builds.

Whoops, disregard.  m_rootChild always occupies the entire window bounds.  We could set the background color to red in debug builds.

[Jer Noble]

Created attachment 56850 [details]
Patch

[Jer Noble]

Created attachment 56856 [details]
Patch

[Chris Marrin]

Comment on attachment 56830 [details]
Patch

This is an unofficial review - no status change

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 60048)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,31 @@
> [phone]  Jer Noble  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        full screen doesn't work for video elements
> +        https://bugs.webkit.org/show_bug.cgi?id=39557
> +        rdar://problem/8011813
> +        
> +        Add fullscreen support for MediaPlayerPrivateVisualContext.  A new class, MediaPlayerPrivateFullscreenWindow,
> +        provides the fullscreen hwnd and layer renderer.  Any WKCACFLayer can be provided to MediaPlayerPrivateFullscreenWindow
> +        so future additional MediaPlayerPrivate implementations can use the fullscreen window.
> +        
> +        Minor additions have been made to the FloatSize and IntSize classes.

It would be nice to note the reason for the changes

> Index: WebCore/platform/graphics/FloatSize.h
> ===================================================================
> --- WebCore/platform/graphics/FloatSize.h	(revision 60048)
> +++ WebCore/platform/graphics/FloatSize.h	(working copy)
> @@ -63,6 +63,14 @@ public:
>  
>      bool isEmpty() const { return m_width <= 0 || m_height <= 0; }
>  
> +    float aspectRatio() const { return m_width / m_height; }

You should zero check m_height here to avoid divide by 0

> Index: WebCore/platform/graphics/IntSize.h
> ===================================================================
> --- WebCore/platform/graphics/IntSize.h	(revision 60048)
> +++ WebCore/platform/graphics/IntSize.h	(working copy)
> @@ -72,6 +72,8 @@ public:
>  
>      bool isEmpty() const { return m_width <= 0 || m_height <= 0; }
>      bool isZero() const { return !m_width && !m_height; }
> +
> +    float aspectRatio() const { return static_cast<float>(m_width) / static_cast<float>(m_height); }

Same as above

> Index: WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.cpp
> ===================================================================
> 
> +void MediaPlayerPrivateFullscreenWindow::createWindow()
> +{
> +    static ATOM windowAtom = 0;
> +    if (!windowAtom) {
> +        WNDCLASSEX wcex = {0};
> +        wcex.cbSize         = sizeof(WNDCLASSEX);
> +        wcex.style          = CS_HREDRAW | CS_VREDRAW | CS_OWNDC;
> +        wcex.lpfnWndProc    = &staticWndProc;
> +        wcex.hInstance      = WebCore::instanceHandle();
> +        wcex.lpszClassName  = L"MediaPlayerPrivateFullscreenWindowClass";
> +        windowAtom = ::RegisterClassEx(&wcex);

WebKit style is to not align equal signs, I believe

> +    }
> +
> +    if (m_hwnd)
> +        close();
> +
> +    MONITORINFO mi = {0};
> +    mi.cbSize = sizeof(MONITORINFO);
> +    GetMonitorInfo(MonitorFromWindow(0, MONITOR_DEFAULTTOPRIMARY), &mi);
> +    IntRect monitorRect = mi.rcMonitor;
> +
> +    CREATESTRUCTW cs = {0};
> +    cs.x = 0;
> +    cs.y = 0;
> +    cs.cx = monitorRect.width();
> +    cs.cy = monitorRect.height();
> +    cs.style = WS_POPUP | WS_VISIBLE;
> +    cs.dwExStyle = 0;
> +    cs.lpszClass = L"MediaPlayerPrivateFullscreenWindowClass";
> +    cs.lpszName = L"";
> +    cs.lpCreateParams = (LPVOID)this;
> +    
> +    m_hwnd = ::CreateWindowExW(cs.dwExStyle, 
> +                            cs.lpszClass, 
> +                            cs.lpszName, 
> +                            cs.style, 
> +                            cs.x, 
> +                            cs.y, 
> +                            cs.cx, 
> +                            cs.cy, 
> +                            cs.hwndParent, 
> +                            cs.hMenu, 
> +                            cs.hInstance, 
> +                            cs.lpCreateParams);

Webkit style is also to put params on one line, only splitting when lines are really long

> +}
> +
> +void MediaPlayerPrivateFullscreenWindow::close()
> +{
> +    ::DestroyWindow(m_hwnd);
> +    m_hwnd = 0;
> +}
> +
> +HWND MediaPlayerPrivateFullscreenWindow::hwnd() const
> +{
> +    return m_hwnd;
> +}
> +
> +WKCACFLayerRenderer* MediaPlayerPrivateFullscreenWindow::layerRenderer()
> +{
> +    return m_layerRenderer.get();
> +}

This method can be const

> +
> +WKCACFLayer* MediaPlayerPrivateFullscreenWindow::rootChildLayer()
> +{
> +    return m_rootChild.get();
> +}

As can this one.

> +
> +void MediaPlayerPrivateFullscreenWindow::setRootChildLayer(PassRefPtr<WKCACFLayer> rootChild)
> +{
> +    if (m_rootChild != rootChild) {
> +
> +        if (m_rootChild) {
> +            m_rootChild->removeFromSuperlayer();
> +            m_rootChild.clear();
> +        }
> +
> +        m_rootChild = rootChild;
> +    }
> +
> +    if (m_rootChild) {
> +        m_layerRenderer->setRootChildLayer(m_rootChild.get());
> +        WKCACFLayer* rootLayer = m_rootChild->rootLayer();
> +        CGRect rootBounds = m_rootChild->rootLayer()->bounds();
> +        m_rootChild->setFrame(rootBounds);

WKCACFLayer no longer has a setFrame() API. I removed it to simplify the API. Frame is always so confusing because it's nothing more than a composite of bounds.size and position. So you can replace this with calls to setBounds and setPosition.

> +        m_layerRenderer->setScrollFrame(IntPoint(rootBounds.origin), IntSize(rootBounds.size));
> +        m_rootChild->setBackgroundColor(CGColorGetConstantColor(kCGColorBlack));
> +        rootLayer->setBackgroundColor(CGColorGetConstantColor(kCGColorBlack));

The background of the rootChild should never be seen (always covered by the video) right? You might want to make it's background red (at least in debug mode) to make it easier to spot problems. But maybe video needs to show the background before it's started rendering?

> +    }
> +}

Since you're doing fullscreen video with a fullscreen window, it seems like you should be able to animate it like we do on Mac right? Not for now of course, but is there a bug open about this?

> +
> +LRESULT CALLBACK MediaPlayerPrivateFullscreenWindow::staticWndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
> +{
> +    LONG_PTR longPtr = GetWindowLongPtr(hWnd, GWLP_USERDATA);
> +
> +    if (!longPtr && message == WM_CREATE) {
> +        LPCREATESTRUCT lpcs = reinterpret_cast<LPCREATESTRUCT>(lParam);
> +        longPtr = (LONG_PTR)lpcs->lpCreateParams;
> +        ::SetWindowLongPtr(hWnd, GWLP_USERDATA, longPtr);
> +    }
> +
> +    if (MediaPlayerPrivateFullscreenWindow* window = reinterpret_cast<MediaPlayerPrivateFullscreenWindow*>(longPtr))
> +        return window->wndProc(hWnd, message, wParam, lParam);
> +
> +    return ::DefWindowProc(hWnd, message, wParam, lParam);    
> +}
> +
> +LRESULT MediaPlayerPrivateFullscreenWindow::wndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
> +{
> +    LRESULT lResult = 0;
> +    switch (message) {
> +    case WM_CREATE:
> +        m_hwnd = hWnd;
> +        m_layerRenderer->setHostWindow(m_hwnd);
> +        m_layerRenderer->createRenderer();
> +        m_layerRenderer->setNeedsDisplay();
> +        break;
> +    case WM_DESTROY:
> +        m_hwnd = 0;
> +        m_layerRenderer->destroyRenderer();
> +        m_layerRenderer->setHostWindow(0);
> +    case WM_WINDOWPOSCHANGED:
> +        {
> +            LPWINDOWPOS wp = (LPWINDOWPOS)lParam;
> +            if (!(wp->flags & SWP_NOMOVE)) {
> +                m_layerRenderer->resize();
> +                WKCACFLayer* rootLayer = m_rootChild->rootLayer();
> +                CGRect rootBounds = m_rootChild->rootLayer()->bounds();
> +                m_rootChild->setFrame(rootBounds);

Same as above

> +                m_rootChild->setNeedsLayout();
> +                m_layerRenderer->setScrollFrame(IntPoint(rootBounds.origin), IntSize(rootBounds.size));
> +            }
> +        }
> +        break;
> +    case WM_PAINT:
> +        m_layerRenderer->renderSoon();
> +        break;
> +    }
> +
> +    if (m_client)
> +        lResult = m_client->fullscreenClientWndProc(hWnd, message, wParam, lParam);
> +
> +    return lResult;
> +}
> +
> +}
> Index: WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h
> ===================================================================
> --- WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h	(revision 0)
> +++ WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h	(revision 0)
> @@ -0,0 +1,68 @@
> +/*
> + * Copyright (C) 2010 Apple Inc. All rights reserved.
> + * 
> + * Redistribution and use in source and binary forms, with or without 
> + * modification, are permitted provided that the following conditions 
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright 
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the 
> + *    documentation and/or other materials provided with the distribution.
> + * 
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS AS IS 
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR 
> + * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
> + * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF 
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS 
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN 
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) 
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF 
> + * THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef MediaPlayerPrivateFullscreenWindow_h
> +#define MediaPlayerPrivateFullscreenWindow_h
> +
> +#include "WKCACFLayer.h"
> +#include "WKCACFLayerRenderer.h"
> +#include <windows.h>
> +#include <wtf/OwnPtr.h>
> +
> +namespace WebCore {
> +
> +class MediaPlayerPrivateFullscreenClient {
> +public:
> +    virtual LRESULT fullscreenClientWndProc(HWND, UINT message, WPARAM, LPARAM) = 0;
> +};
> +
> +class MediaPlayerPrivateFullscreenWindow {
> +public:
> +    MediaPlayerPrivateFullscreenWindow(MediaPlayerPrivateFullscreenClient*);
> +    virtual ~MediaPlayerPrivateFullscreenWindow();
> +
> +    void createWindow();
> +    void close();
> +    
> +    HWND hwnd() const;
> +
> +    WKCACFLayerRenderer* layerRenderer();
> +
> +    WKCACFLayer* rootChildLayer();

Const for these two

> +    void setRootChildLayer(PassRefPtr<WKCACFLayer>);
> +
> +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;
> +};
> +
> +}
> +
> +#endif
> Index: WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp
> ===================================================================
> --- WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp	(revision 60048)
> +++ WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp	(working copy)
> @@ -150,7 +150,7 @@ MediaPlayerPrivateQuickTimeVisualContext
>  
>  bool MediaPlayerPrivateQuickTimeVisualContext::supportsFullscreen() const
>  {
> -    return false;
> +    return true;
>  }
>  
>  PlatformMedia MediaPlayerPrivateQuickTimeVisualContext::platformMedia() const
> @@ -765,7 +765,7 @@ void MediaPlayerPrivateQuickTimeVisualCo
>  
>                  buffer.unlockBaseAddress();
>                  layer->rootLayer()->setNeedsRender();
> -            }
> +                }

This looks odd. Why the change in indent? Hard to see whether or not it's right out of context

>      } else
>  #endif
>          m_player->repaint();
> @@ -939,6 +939,10 @@ void MediaPlayerPrivateQuickTimeVisualCo
>  #endif
>      // The layer will get hooked up via RenderLayerBacking::updateGraphicsLayerConfiguration().
>  #endif
> +
> +    // fill the newly created layer with image data
> +    if (m_visualContext)
> +        retrieveCurrentImage();
>  }
>  
>  void MediaPlayerPrivateQuickTimeVisualContext::destroyLayerForMovie()
> Index: WebCore/platform/graphics/win/WKCACFLayer.cpp
> ===================================================================
> --- WebCore/platform/graphics/win/WKCACFLayer.cpp	(revision 60048)
> +++ WebCore/platform/graphics/win/WKCACFLayer.cpp	(working copy)
> @@ -154,6 +154,7 @@ PassRefPtr<WKCACFLayer> WKCACFLayer::cre
>  
>  WKCACFLayer::WKCACFLayer(LayerType type)
>      : m_layer(AdoptCF, CACFLayerCreate(toCACFLayerType(type)))
> +    , m_layoutClient(0)
>      , m_needsDisplayOnBoundsChange(false)
>  {
>      CACFLayerSetUserData(layer(), this);
> @@ -286,11 +287,10 @@ void  WKCACFLayer::adoptSublayers(WKCACF
>  void WKCACFLayer::removeFromSuperlayer()
>  {
>      WKCACFLayer* superlayer = this->superlayer();
> -    if (!superlayer)
> -        return;
> -
>      CACFLayerRemoveFromSuperlayer(layer());
> -    superlayer->setNeedsCommit();
> +
> +    if (superlayer)
> +        superlayer->setNeedsCommit();
>  }
>  
>  WKCACFLayer* WKCACFLayer::internalSublayerAtIndex(int index) const
> @@ -343,6 +343,19 @@ void WKCACFLayer::setBounds(const CGRect
>          setNeedsDisplay();
>  }
>  
> +void WKCACFLayer::setFrame(const CGRect& rect)
> +{
> +    CGRect oldFrame = frame();
> +    if (CGRectEqualToRect(rect, oldFrame))
> +        return;
> +
> +    CACFLayerSetFrame(layer(), rect);
> +    setNeedsCommit();
> +
> +    if (m_needsDisplayOnBoundsChange)
> +        setNeedsDisplay();
> +}
> +

Ah, I see. You've added back setFrame! I'd really like to leave it out if we can.

>  void WKCACFLayer::setContentsGravity(ContentsGravityType type)
>  {
>      CACFLayerSetContentsGravity(layer(), toCACFContentsGravityType(type));
> @@ -418,6 +431,32 @@ void WKCACFLayer::internalSetNeedsDispla
>      CACFLayerSetNeedsDisplay(layer(), dirtyRect);
>  }
>  
> +void WKCACFLayer::setLayoutClient(WKCACFLayerLayoutClient* layoutClient)
> +{
> +    if (layoutClient == m_layoutClient)
> +        return;
> +
> +    m_layoutClient = layoutClient;
> +    CACFLayerSetLayoutCallback(layer(), m_layoutClient ? &layoutSublayersProc : 0);    
> +}
> +
> +void WKCACFLayer::layoutSublayersProc(CACFLayerRef caLayer) 
> +{
> +    WKCACFLayer* layer = WKCACFLayer::layer(caLayer);
> +    if (layer && layer->m_layoutClient)
> +        layer->m_layoutClient->layoutSublayersOfLayer(layer);
> +}
> +
> +WKCACFLayerLayoutClient* WKCACFLayer::layoutClient() const
> +{
> +    return m_layoutClient;
> +}
> +
> +void WKCACFLayer::setNeedsLayout()
> +{
> +    CACFLayerSetNeedsLayout(layer());
> +}
> +
>  #ifndef NDEBUG
>  static void printIndent(int indent)
>  {
> @@ -500,9 +539,9 @@ void WKCACFLayer::printLayer(int indent)
>      CGImageRef layerContents = contents();
>      if (layerContents) {
>          printIndent(indent + 1);
> -        fprintf(stderr, "(contents (image [%d %d]))\n",
> +            fprintf(stderr, "(contents (image [%d %d]))\n",
>              CGImageGetWidth(layerContents), CGImageGetHeight(layerContents));
> -    }
> +        }

More odd indenting

>  
>      // Print sublayers if needed
>      int n = sublayerCount();
> Index: WebCore/platform/graphics/win/WKCACFLayer.h
> ===================================================================
> --- WebCore/platform/graphics/win/WKCACFLayer.h	(revision 60048)
> +++ WebCore/platform/graphics/win/WKCACFLayer.h	(working copy)
> @@ -44,9 +44,15 @@
>  
>  namespace WebCore {
>  
> +class WKCACFLayer;
>  class WKCACFAnimation;
>  class WKCACFTimingFunction;

Hmmm. I see spurious class declarations here (obviously from when I first implemented this). Probably best to get rid of them while we're here.

>  
> +class WKCACFLayerLayoutClient {
> +public:
> +    virtual void layoutSublayersOfLayer(WKCACFLayer*) = 0;
> +};
> +
>  class WKCACFLayer : public RefCounted<WKCACFLayer> {
>  public:
>      enum LayerType { Layer, TransformLayer };
> @@ -63,6 +69,10 @@ public:
>  
>      virtual void drawInContext(PlatformGraphicsContext*) { }
>  
> +    virtual void setLayoutClient(WKCACFLayerLayoutClient*);
> +    virtual WKCACFLayerLayoutClient* layoutClient() const;
> +    virtual void setNeedsLayout();
> +
>      void setNeedsDisplay(const CGRect* dirtyRect = 0)
>      {
>          internalSetNeedsDisplay(dirtyRect);
> @@ -173,6 +183,9 @@ public:
>      void setFilters(CFArrayRef filters) { CACFLayerSetFilters(layer(), filters); setNeedsCommit(); }
>      CFArrayRef filters() const { return CACFLayerGetFilters(layer()); }
>  
> +    void setFrame(const CGRect&);
> +    CGRect frame() const { return CACFLayerGetFrame(layer()); }

It would be really great if we could axe this.

> +
>      void setHidden(bool hidden) { CACFLayerSetHidden(layer(), hidden); setNeedsCommit(); }
>      bool isHidden() const { return CACFLayerIsHidden(layer()); }
>  
> @@ -260,8 +273,11 @@ protected:
>      void printLayer(int indent) const;
>  #endif
>  
> +    static void layoutSublayersProc(CACFLayerRef);
> +
>  private:
>      RetainPtr<CACFLayerRef> m_layer;
> +    WKCACFLayerLayoutClient* m_layoutClient;
>      bool m_needsDisplayOnBoundsChange;
>  };
>  
> Index: WebCore/platform/graphics/win/WebTiledLayer.cpp
> ===================================================================
> --- WebCore/platform/graphics/win/WebTiledLayer.cpp	(revision 60048)
> +++ WebCore/platform/graphics/win/WebTiledLayer.cpp	(working copy)
> @@ -73,6 +73,15 @@ void WebTiledLayer::setBounds(const CGRe
>      updateTiles();
>  }
>  
> +void WebTiledLayer::setFrame(const CGRect& rect)
> +{
> +    if (CGRectEqualToRect(rect, frame()))
> +        return;
> +
> +    WebLayer::setFrame(rect);
> +    updateTiles();
> +}
> +

See above

>  void WebTiledLayer::internalSetNeedsDisplay(const CGRect* dirtyRect)
>  {
>      // FIXME: Only setNeedsDisplay for tiles that are currently visible
> Index: WebCore/platform/graphics/win/WebTiledLayer.h
> ===================================================================
> --- WebCore/platform/graphics/win/WebTiledLayer.h	(revision 60048)
> +++ WebCore/platform/graphics/win/WebTiledLayer.h	(working copy)
> @@ -39,6 +39,7 @@ public:
>      virtual ~WebTiledLayer();
>  
>      virtual void setBounds(const CGRect&);
> +    virtual void setFrame(const CGRect&);

See above

>  
>  protected:
>      WebTiledLayer(const CGSize& tileSize, GraphicsLayer* owner);
> Index: WebKit/win/ChangeLog
> ===================================================================
> --- WebKit/win/ChangeLog	(revision 60027)
> +++ WebKit/win/ChangeLog	(working copy)
> @@ -1,3 +1,24 @@
> [phone]  Jer Noble  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        full screen doesn't work for video elements

Capitalize the start of sentences

> +        https://bugs.webkit.org/show_bug.cgi?id=39557
> +        rdar://problem/8011813
> +        
> +        Modified FullscreenVideoController to work with MediaPlayerPrivateFullscreenWindow.  The FullscreenVideoController
> +        is now MediaPlayerPrivate agnostic.
> +
> +        * FullscreenVideoController.cpp:
> +        (FullscreenVideoController::LayoutClient::LayoutClient):  
> +        (FullscreenVideoController::LayoutClient::layoutSublayersOfLayer):
> +        (FullscreenVideoController::FullscreenVideoController):
> +        (FullscreenVideoController::~FullscreenVideoController):
> +        (FullscreenVideoController::enterFullscreen):
> +        (FullscreenVideoController::exitFullscreen):
> +        (FullscreenVideoController::createHUDWindow):
> +        * FullscreenVideoController.h:
> +
>  2010-05-21  Steve Block  <[e-mail]>
>  
>          Reviewed by Jeremy Orlow.
> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 60028)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,17 @@
> [phone]  Jer Noble  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Re-enabled fullscreen support on windows, and modified tests to match.

You should add the bug number here like in the other Changelog

> +
> +        * platform/win/media/controls-after-reload-expected.txt:
> +        * platform/win/media/controls-drag-timebar-expected.txt:
> +        * platform/win/media/controls-strict-expected.txt:
> +        * platform/win/media/controls-styling-expected.txt:
> +        * platform/win/media/video-controls-rendering-expected.txt:
> +        * platform/win/media/video-display-toggle-expected.txt:
> +        * platform/win/media/video-no-audio-expected.txt:
> +
>  2010-05-22  Maciej Stachowiak  <[e-mail]>
>  
>          Reviewed by Dan Bernstein.

[Chris Marrin]

Ugh, sorry about reposting the same review. Please disregard!

[Chris Marrin]

(In reply to comment #12)
> (In reply to comment #11)
> > > +    float aspectRatio() const { return m_width / m_height; }
> > 
> > You should zero check m_height here to avoid divide by 0
> 
> Why? If m_height is 0, it will return either +INF or -INF, which is a true answer for the size's aspect ratio.

Are we sure no platform will throw a divide by 0 exception? If so, then it's fine. I'm just not aware of code elsewhere in WebKit that allows these sorts of shenanigans.

...
> > > +    if (m_rootChild) {
> > > +        m_layerRenderer->setRootChildLayer(m_rootChild.get());
> > > +        WKCACFLayer* rootLayer = m_rootChild->rootLayer();
> > > +        CGRect rootBounds = m_rootChild->rootLayer()->bounds();
> > > +        m_rootChild->setFrame(rootBounds);
> > 
> > WKCACFLayer no longer has a setFrame() API. I removed it to simplify the API. Frame is always so confusing because it's nothing more than a composite of bounds.size and position. So you can replace this with calls to setBounds and setPosition.
> 
> In order to reproduce the setFrame() API, the caller would need to get the anchorPoint, multiply the result by the destination bounds width and height, and add the result to the intended bottom-left corner position, and then call setBounds() and setPosition().  This turns one line of code into 7 or 8 lines of code.  Why should we remove an API that just wraps CACFLayerSetFrame()?

We never use this API in GraphicsLayer. We have to do lots of logic to flip Y and do other conversions so we always do that at the higher levels anyway. I had a couple of places where it was done in WKCACFLayer. Because my position is always(0,0), I could set the anchor point to (0,0) on creation and then just return setFrame with setBounds. If your anchor point never changes (and I assume that's the case) you can simply set the anchor point to (0,0) and then call setBounds(CGRectMake(0,0, rootBounds.size.width, rootBounds.size.height); setPosition(rootBounds.origin).

I removed setFrame to avoid needing to deal with its complexities in my tiled layer logic. It's long been the source of confusion and bugs, and since it is used so rarely, I felt (and still do) that avoiding its use is the best approach.

[Adam Roben (:aroben)]

Comment on attachment 56856 [details]
Patch

By relying on WKCACFLayer and friends, we're making full-screen not work on machines that don't support accelerated compositing. Is that OK?

[Jer Noble]

(In reply to comment #19)
> (From update of attachment 56856 [details])
> By relying on WKCACFLayer and friends, we're making full-screen not work on machines that don't support accelerated compositing. Is that OK?

I was told that we disable fullscreen for machines which do not support accelerated compositing, but I can't find anywhere in the code which ensures that.  

We could modify the fullscreen window code to fall back to CGContext rendering if acceleration isn't available.  It won't be fast, as the scaling and color correction will have to occur on the CPU.   And given the time pressure, I would argue that enhancement could wait.

[Jer Noble]

(In reply to comment #18)
> Are we sure no platform will throw a divide by 0 exception? If so, then it's fine. I'm just not aware of code elsewhere in WebKit that allows these sorts of shenanigans.

Float div by 0 will always return either +INF or -INF.

> We never use this API in GraphicsLayer. We have to do lots of logic to flip Y and do other conversions so we always do that at the higher levels anyway. I had a couple of places where it was done in WKCACFLayer. Because my position is always(0,0), I could set the anchor point to (0,0) on creation and then just return setFrame with setBounds. If your anchor point never changes (and I assume that's the case) you can simply set the anchor point to (0,0) and then call setBounds(CGRectMake(0,0, rootBounds.size.width, rootBounds.size.height); setPosition(rootBounds.origin).

Right, I could, assuming I want an anchor point of (0,0).  But how is all of that less error prone than just calling setFrame()?

> I removed setFrame to avoid needing to deal with its complexities in my tiled layer logic. It's long been the source of confusion and bugs, and since it is used so rarely, I felt (and still do) that avoiding its use is the best approach.

If anything, setFrame() is less confusing than setPosition(), setAnchorPoint(), and setBounds().  It has valid use cases, and like any API, its invalid ones.  But if it's exposed by CALayer/CACFLayer, I don't see why it should be disallowed in WKCACFLayer.

[Adam Roben (:aroben)]

Comment on attachment 56856 [details]
Patch

> +        Minor additions have been made to the FloatSize and IntSize classes: added aspectRatio() to both; added scale(float) to IntSize.

You must mean that you added scale(float) to FloatSize. But I don't think you need to mention these low-level changes here, since the exact same information is included in the list of changed files below.

> +    void scale(float scale)
> +    {
> +        m_width = m_width * scale;
> +        m_height = m_height * scale;
> +    }

Why not use *= ?

> +#include "config.h"
> +
> +#include "MediaPlayerPrivateFullscreenWindow.h"

You have an extra blank line in there.

> +MediaPlayerPrivateFullscreenWindow::MediaPlayerPrivateFullscreenWindow(MediaPlayerPrivateFullscreenClient* client)
> +    : m_client(client)
> +    , m_hwnd(0)
> +{
> +    m_layerRenderer = WKCACFLayerRenderer::create(0);
> +}

You can initialize m_layerRenderer in the initializer list.

It seems like there's nothing about this class that's specific to its use in conjunction with MediaPlayerPrivate. Maybe we should call it something like FullscreenLayerWindow?

> +MediaPlayerPrivateFullscreenWindow::~MediaPlayerPrivateFullscreenWindow()
> +{
> +    m_client = 0;
> +}

Why is this needed?

> +{
> +    static ATOM windowAtom = 0;

No need to initialize to 0; C++ does that for you.

> +    if (!windowAtom) {
> +        WNDCLASSEX wcex = {0};
> +        wcex.cbSize = sizeof(WNDCLASSEX);
> +        wcex.style = CS_HREDRAW | CS_VREDRAW | CS_OWNDC;

Why are you passing CS_OWNDC here?

> +        wcex.lpfnWndProc = &staticWndProc;

No need for the & in C++.

> +        wcex.hInstance = WebCore::instanceHandle();
> +        wcex.lpszClassName = L"MediaPlayerPrivateFullscreenWindowClass";

It would be good to put this class name in a constant.

> +    MONITORINFO mi = {0};
> +    mi.cbSize = sizeof(MONITORINFO);
> +    GetMonitorInfo(MonitorFromWindow(0, MONITOR_DEFAULTTOPRIMARY), &mi);
> +    IntRect monitorRect = mi.rcMonitor;

Seems like we should be passing the WebView's HWND to MonitorFromWindow so that the fullscreen video will appear on the same monitor as the webpage.

Should we check the return value of GetMonitorInfo?

> +    cs.x = 0;
> +    cs.y = 0;
> +    cs.cx = monitorRect.width();
> +    cs.cy = monitorRect.height();

Shouldn't we be setting cs.x and cs.y to monitorRect.x() and monitorRect.y()?

> +    cs.style = WS_POPUP | WS_VISIBLE;
> +    cs.dwExStyle = 0;
> +    cs.lpszClass = L"MediaPlayerPrivateFullscreenWindowClass";
> +    cs.lpszName = L"";
> +    cs.lpCreateParams = (LPVOID)this;
> +    
> +    m_hwnd = ::CreateWindowExW(cs.dwExStyle, cs.lpszClass, cs.lpszName, cs.style, cs.x, cs.y, cs.cx, cs.cy, cs.hwndParent, cs.hMenu, cs.hInstance, cs.lpCreateParams);
> +}

Using a CREATESTRUCTW just to pass the parameters to CreateWindowExW doesn't seem all that useful.

> +void MediaPlayerPrivateFullscreenWindow::setRootChildLayer(PassRefPtr<WKCACFLayer> rootChild)
> +{
> +    if (m_rootChild != rootChild) {
> +
> +        if (m_rootChild) {

Extra blank line here.

> +            m_rootChild->removeFromSuperlayer();
> +            m_rootChild.clear();
> +        }
> +
> +        m_rootChild = rootChild;
> +    }

No need to call .clear() right before assigning m_rootChild.

> +    if (m_rootChild) {
> +        m_layerRenderer->setRootChildLayer(m_rootChild.get());
> +        WKCACFLayer* rootLayer = m_rootChild->rootLayer();
> +        CGRect rootBounds = m_rootChild->rootLayer()->bounds();
> +        m_rootChild->setFrame(rootBounds);
> +        m_layerRenderer->setScrollFrame(IntPoint(rootBounds.origin), IntSize(rootBounds.size));
> +        m_rootChild->setBackgroundColor(CGColorGetConstantColor(kCGColorBlack));
> +        rootLayer->setBackgroundColor(CGColorGetConstantColor(kCGColorBlack));
> +    }
> +}

Do we really want to do all this work if m_rootChild hasn't changed?

An early return would make this a little easier to read.

I like Chris's idea of using a red background color in debug builds.

> +LRESULT CALLBACK MediaPlayerPrivateFullscreenWindow::staticWndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
> +{
> +    LONG_PTR longPtr = GetWindowLongPtr(hWnd, GWLP_USERDATA);
> +
> +    if (!longPtr && message == WM_CREATE) {
> +        LPCREATESTRUCT lpcs = reinterpret_cast<LPCREATESTRUCT>(lParam);
> +        longPtr = (LONG_PTR)lpcs->lpCreateParams;

reinterpret_cast would be nice instead of a C-style cast.

> +LRESULT MediaPlayerPrivateFullscreenWindow::wndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
> +{
> +    LRESULT lResult = 0;
> +    switch (message) {
> +    case WM_CREATE:
> +        m_hwnd = hWnd;

This assignment will be overwritten (with the same value) as soon as CreateWindowExW returns. Why do we assign in both places?

> +        m_layerRenderer->setHostWindow(m_hwnd);
> +        m_layerRenderer->createRenderer();
> +        m_layerRenderer->setNeedsDisplay();

Why do we call setNeedsDisplay here? We don't have a root child layer yet, right?

> +    case WM_WINDOWPOSCHANGED:
> +        {
> +            LPWINDOWPOS wp = (LPWINDOWPOS)lParam;

reinterpret_cast would be better here.

> +            if (!(wp->flags & SWP_NOMOVE)) {

You could turn this into an "early break".

If we only care about when the window has moved, we could listen for WM_MOVE instead.

> +                m_layerRenderer->resize();
> +                WKCACFLayer* rootLayer = m_rootChild->rootLayer();
> +                CGRect rootBounds = m_rootChild->rootLayer()->bounds();
> +                m_rootChild->setFrame(rootBounds);
> +                m_rootChild->setNeedsLayout();
> +                m_layerRenderer->setScrollFrame(IntPoint(rootBounds.origin), IntSize(rootBounds.size));
> +            }

I don't understand why we resize the renderer when we get a message that tells us the window has moved. (What could cause the window to move?)

> +#ifndef MediaPlayerPrivateFullscreenWindow_h
> +#define MediaPlayerPrivateFullscreenWindow_h
> +
> +#include "WKCACFLayer.h"
> +#include "WKCACFLayerRenderer.h"
> +#include <windows.h>

It would be nice to avoid pulling in windows.h to a header file like this. Can we use forward-declarations instead?

> +class MediaPlayerPrivateFullscreenClient {
> +public:
> +    virtual LRESULT fullscreenClientWndProc(HWND, UINT message, WPARAM, LPARAM) = 0;
> +};

We normally add an empty, protected, virtual destructor for client classes like this. (GCC will complain if there's no virtual destructor, but MSVC won't.)

I also personally think it's nice to put client interfaces into their own header files to reduce recompilations when changing MediaPlayerPrivateFullscreenWindow's declaration in a way that doesn't affect the declarations of classes that implement the client interface.

>  bool MediaPlayerPrivateQuickTimeVisualContext::supportsFullscreen() const
>  {
> -    return false;
> +    return true;
>  }

Don't we still want to return false when USE(ACCELERATED_COMPOSITING) is disabled or when Settings::acceleratedCompositingEnabled() is false?

> @@ -939,6 +939,10 @@ void MediaPlayerPrivateQuickTimeVisualCo
>  #endif
>      // The layer will get hooked up via RenderLayerBacking::updateGraphicsLayerConfiguration().
>  #endif
> +
> +    // fill the newly created layer with image data
> +    if (m_visualContext)
> +        retrieveCurrentImage();
>  }

Why is this now needed? (Both the comment here and the ChangeLog say *what* this does, but not *why*.)

> +void WKCACFLayer::setFrame(const CGRect& rect)
> +{
> +    CGRect oldFrame = frame();
> +    if (CGRectEqualToRect(rect, oldFrame))
> +        return;
> +
> +    CACFLayerSetFrame(layer(), rect);
> +    setNeedsCommit();
> +
> +    if (m_needsDisplayOnBoundsChange)
> +        setNeedsDisplay();
> +}

If the size of the frame hasn't changed, should we not call setNeedsDisplay()?

> +void WKCACFLayer::setLayoutClient(WKCACFLayerLayoutClient* layoutClient)
> +{
> +    if (layoutClient == m_layoutClient)
> +        return;
> +
> +    m_layoutClient = layoutClient;
> +    CACFLayerSetLayoutCallback(layer(), m_layoutClient ? &layoutSublayersProc : 0);    
> +}

Again, no need for the & in C++.

> @@ -500,7 +539,7 @@ void WKCACFLayer::printLayer(int indent)
>      CGImageRef layerContents = contents();
>      if (layerContents) {
>          printIndent(indent + 1);
> -        fprintf(stderr, "(contents (image [%d %d]))\n",
> +            fprintf(stderr, "(contents (image [%d %d]))\n",
>              CGImageGetWidth(layerContents), CGImageGetHeight(layerContents));
>      }

This seems like a step in the wrong direction.

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

Same comments here that I had about the other client interface.

>  class WKCACFLayer : public RefCounted<WKCACFLayer> {
>  public:
> @@ -63,6 +67,10 @@ public:
>  
>      virtual void drawInContext(PlatformGraphicsContext*) { }
>  
> +    virtual void setLayoutClient(WKCACFLayerLayoutClient*);
> +    virtual WKCACFLayerLayoutClient* layoutClient() const;
> +    virtual void setNeedsLayout();

I don't see why these need to be virtual.

> +    void setFrame(const CGRect&);

But this definitely *does* need to be virtual!

> +class FullscreenVideoController::LayoutClient : public WebCore::WKCACFLayerLayoutClient {

You should remove all instances of "WebCore::" from the .cpp file.

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

Should we also assert that these two layers are the same?

I think this would be easier to read if you didn't put this function inline in the class declaration.

> +        float scaleFactor = 0;
> +        if (naturalSize.aspectRatio() > layerBounds.size().aspectRatio())
> +            scaleFactor = layerBounds.width() / naturalSize.width();
> +        else
> +            scaleFactor = layerBounds.height() / naturalSize.height();

No need to initialize scaleFactor to 0.

> +        // Calculate the centered position based on the videoAnchor, videoBounds, and layerBounds:
> +        FloatPoint videoPosition;
> +        videoPosition.setX(layerCenter.x() - (0.5 - videoAnchor.x()) * videoBounds.width());
> +        videoPosition.setY(layerCenter.y() - (0.5 - videoAnchor.y()) * videoBounds.height());
> +        videoLayer->setPosition(videoPosition);
> +
> +        videoLayer->setBounds(FloatRect(FloatPoint(), naturalSize));
> +    }

Why not use setFrame instead of setPosition/setBounds?

> @@ -183,25 +229,20 @@ FullscreenVideoController::FullscreenVid
>      , m_hitWidget(0)
>      , m_movingWindow(false)
>      , m_timer(this, &FullscreenVideoController::timerFired)
> +    , m_rootChild(WebCore::WKCACFLayer::create(WebCore::WKCACFLayer::Layer))
> +    , m_layoutClient(new FullscreenVideoController::LayoutClient(this))

You can just say "new LayoutClient(this)" here.

It seems a little strange that we create m_rootChild and m_layoutClient here when we wait until enterFullscreen is called to create m_fullscreenWindow. Why not create them all in the same place?

>  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;
> +    }
> +
> +    if (m_hudWindow) {
> +        SetWindowLongPtr(m_hudWindow, 0, 0);
> +        ::DestroyWindow(m_hudWindow);
> +        m_hudWindow = 0;
> +    }

Why is it now possible for m_hudWindow to be null when it wasn't possible before? Or was it possible before? (If so, you should mention it in your ChangeLog.)

> +    // As a side effect of setting the player to invisible/visible,
> +    // the player's layer will be recreated, and will be picked up 
> +    // the next time the layer tree is synched.
> +    m_mediaElement->player()->setVisible(0);
> +    m_mediaElement->player()->setVisible(1);
>  }

Is this needed because putting the player's layer into the fullscreen window ripped it out of the page? I think this could be explained more thoroughly.

> @@ -316,6 +372,9 @@ LRESULT FullscreenVideoController::fulls
>          break;
>      case WM_LBUTTONUP:
>          onMouseUp(IntPoint(GET_X_LPARAM(lParam), GET_Y_LPARAM(lParam)));
> +    case WM_ACTIVATEAPP:
> +        if (!wParam)
> +            m_mediaElement->exitFullscreen();
>          break;
>      }

Why was this added? The ChangeLog doesn't mention it.

[Simon Fraser (smfr)]

Comment on attachment 56856 [details]
Patch

> Index: WebCore/platform/graphics/FloatSize.h
> ===================================================================

> +    void scale(float scale)
> +    {
> +        m_width = m_width * scale;
> +        m_height = m_height * scale;
> +    }

I don't like scale() as a method name. The name is ambiguous. Are you asking
about a property called "scale", or doing a scaling?

[Jer Noble]

(In reply to comment #22)
> --- Comment #22 from Adam Roben (aroben) <[e-mail]>  2010-05-24 11:38:12
PST ---
> (From update of attachment 56856 [details])
>> +        Minor additions have been made to the FloatSize and IntSize classes:
added aspectRatio() to both; added scale(float) to IntSize.
> 
> You must mean that you added scale(float) to FloatSize. But I don't think you need
to mention these low-level changes here, since the exact same information is
included in the list of changed files below.

Chris asked for them specifically, but I agree; I'll remove the specifics here.

>> +    void scale(float scale)
>> +    {
>> +        m_width = m_width * scale;
>> +        m_height = m_height * scale;
>> +    }
> 
> Why not use *= ?

No good reason. :)  Changed.

>> +#include "config.h"
>> +
>> +#include "MediaPlayerPrivateFullscreenWindow.h"
> 
> You have an extra blank line in there.

Removed.

>> +MediaPlayerPrivateFullscreenWindow::MediaPlayerPrivateFullscreenWindow(MediaPlayerPrivateFullscreenClient*
client)
>> +    : m_client(client)
>> +    , m_hwnd(0)
>> +{
>> +    m_layerRenderer = WKCACFLayerRenderer::create(0);
>> +}
> 
> You can initialize m_layerRenderer in the initializer list.

Changed.

> It seems like there's nothing about this class that's specific to its use in
conjunction with MediaPlayerPrivate. Maybe we should call it something like
FullscreenLayerWindow?

I wasn't entirely happy with the name.  I'd be happy to rename it
FullscreenLayerWindow.

>> +MediaPlayerPrivateFullscreenWindow::~MediaPlayerPrivateFullscreenWindow()
>> +{
>> +    m_client = 0;
>> +}
> 
> Why is this needed?

Extreme paranoia.  Force of habit.  I can remove it. 

>> +{
>> +    static ATOM windowAtom = 0;
> 
> No need to initialize to 0; C++ does that for you.

Removed.

>> +    if (!windowAtom) {
>> +        WNDCLASSEX wcex = {0};
>> +        wcex.cbSize = sizeof(WNDCLASSEX);
>> +        wcex.style = CS_HREDRAW | CS_VREDRAW | CS_OWNDC;
> 
> Why are you passing CS_OWNDC here?

I stole some code from a previous CoreMedia player demo, where I was using it for no
good reason.  Removed.

>> +        wcex.lpfnWndProc = &staticWndProc;
> 
> No need for the & in C++.

Habit.  Removed.

>> +        wcex.hInstance = WebCore::instanceHandle();
>> +        wcex.lpszClassName = L"MediaPlayerPrivateFullscreenWindowClass";
> 
> It would be good to put this class name in a constant.

Constanted.

>> +    MONITORINFO mi = {0};
>> +    mi.cbSize = sizeof(MONITORINFO);
>> +    GetMonitorInfo(MonitorFromWindow(0, MONITOR_DEFAULTTOPRIMARY), &mi);
>> +    IntRect monitorRect = mi.rcMonitor;
> 
> Seems like we should be passing the WebView's HWND to MonitorFromWindow so that
the fullscreen video will appear on the same monitor as the webpage.

I thought of that, but wasn't sure how to pipe that information through to the
FullscreenWindow class.  Perhaps a method added to FullscreenWindowClient?

> Should we check the return value of GetMonitorInfo?

And bail out early?  

>> +    cs.x = 0;
>> +    cs.y = 0;
>> +    cs.cx = monitorRect.width();
>> +    cs.cy = monitorRect.height();
> 
> Shouldn't we be setting cs.x and cs.y to monitorRect.x() and monitorRect.y()?

When the only monitor we ask for is the Primary one, those will always be zero.  Now
that we'll be asking for (possibly) other ones, yes, yes we should be. :)

>> +    m_hwnd = ::CreateWindowExW(cs.dwExStyle, cs.lpszClass, cs.lpszName,
cs.style, cs.x, cs.y, cs.cx, cs.cy, cs.hwndParent, cs.hMenu, cs.hInstance,
cs.lpCreateParams);
>> +}
> 
> Using a CREATESTRUCTW just to pass the parameters to CreateWindowExW doesn't seem
all that useful.
> 

True, removed.

>> +void
MediaPlayerPrivateFullscreenWindow::setRootChildLayer(PassRefPtr<WKCACFLayer>
rootChild)
>> +{
>> +    if (m_rootChild != rootChild) {
>> +
>> +        if (m_rootChild) {
> 
> Extra blank line here.

Removed.

>> +            m_rootChild->removeFromSuperlayer();
>> +            m_rootChild.clear();
>> +        }
>> +
>> +        m_rootChild = rootChild;
>> +    }
> 
> No need to call .clear() right before assigning m_rootChild.

Paranoia removed.

>> +    if (m_rootChild) {
>> +        m_layerRenderer->setRootChildLayer(m_rootChild.get());
>> +        WKCACFLayer* rootLayer = m_rootChild->rootLayer();
>> +        CGRect rootBounds = m_rootChild->rootLayer()->bounds();
>> +        m_rootChild->setFrame(rootBounds);
>> +        m_layerRenderer->setScrollFrame(IntPoint(rootBounds.origin),
IntSize(rootBounds.size));
>> +        m_rootChild->setBackgroundColor(CGColorGetConstantColor(kCGColorBlack));
>> +        rootLayer->setBackgroundColor(CGColorGetConstantColor(kCGColorBlack));
>> +    }
>> +}
> 
> Do we really want to do all this work if m_rootChild hasn't changed?
> 
> An early return would make this a little easier to read.

I had that in an earlier version, but there was a case where this all needed to be
happen even when m_rootChild hadn't changed.  I can't remember off the top of my
head why that would be.

> I like Chris's idea of using a red background color in debug builds.

Added.

>> +LRESULT CALLBACK MediaPlayerPrivateFullscreenWindow::staticWndProc(HWND hWnd,
UINT message, WPARAM wParam, LPARAM lParam)
>> +{
>> +    LONG_PTR longPtr = GetWindowLongPtr(hWnd, GWLP_USERDATA);
>> +
>> +    if (!longPtr && message == WM_CREATE) {
>> +        LPCREATESTRUCT lpcs = reinterpret_cast<LPCREATESTRUCT>(lParam);
>> +        longPtr = (LONG_PTR)lpcs->lpCreateParams;
> 
> reinterpret_cast would be nice instead of a C-style cast.

Changed.

>> +LRESULT MediaPlayerPrivateFullscreenWindow::wndProc(HWND hWnd, UINT message,
WPARAM wParam, LPARAM lParam)
>> +{
>> +    LRESULT lResult = 0;
>> +    switch (message) {
>> +    case WM_CREATE:
>> +        m_hwnd = hWnd;
> 
> This assignment will be overwritten (with the same value) as soon as
CreateWindowExW returns. Why do we assign in both places?

The assignment here is probably the necessary one, and the one which catches the
return from CreateWindowExW is the extraneous one.  If we set m_hwnd here (rather
than later) then it will be valid inside all the future wndProc calls happening as a
result of CreateWindowExW.

We could probably add an ASSERT(IsWindow(m_hwnd)); after CreateWindowExW returns.

>> +        m_layerRenderer->setHostWindow(m_hwnd);
>> +        m_layerRenderer->createRenderer();
>> +        m_layerRenderer->setNeedsDisplay();
> 
> Why do we call setNeedsDisplay here? We don't have a root child layer yet, right?

That's true in this case, but it's not guaranteed that clients will call in that
order.  Maybe we should check to see if we have a rootLayer before calling
setNeedsDisplay?

>> +    case WM_WINDOWPOSCHANGED:
>> +        {
>> +            LPWINDOWPOS wp = (LPWINDOWPOS)lParam;
> 
> reinterpret_cast would be better here.

Changed.

>> +            if (!(wp->flags & SWP_NOMOVE)) {
> 
> You could turn this into an "early break".
> 
> If we only care about when the window has moved, we could listen for WM_MOVE instead.

Gah!  No, this is totally wrong.  This should be checking against SWP_NOSIZE
instead.  And we can't handle WM_SIZE, because WKCACFLayerRenderer::resize() checks
the hwnd's size, which hasn't changed yet.

Added the "early break".

>> +                m_layerRenderer->resize();
>> +                WKCACFLayer* rootLayer = m_rootChild->rootLayer();
>> +                CGRect rootBounds = m_rootChild->rootLayer()->bounds();
>> +                m_rootChild->setFrame(rootBounds);
>> +                m_rootChild->setNeedsLayout();
>> +                m_layerRenderer->setScrollFrame(IntPoint(rootBounds.origin),
IntSize(rootBounds.size));
>> +            }
> 
> I don't understand why we resize the renderer when we get a message that tells us
the window has moved. (What could cause the window to move?)

Yeah, that's because I was doing it wrong. :-D

> 
>> +#ifndef MediaPlayerPrivateFullscreenWindow_h
>> +#define MediaPlayerPrivateFullscreenWindow_h
>> +
>> +#include "WKCACFLayer.h"
>> +#include "WKCACFLayerRenderer.h"
>> +#include <windows.h>
> 
> It would be nice to avoid pulling in windows.h to a header file like this. Can we
use forward-declarations instead?

Sure. Added.

>> +class MediaPlayerPrivateFullscreenClient {
>> +public:
>> +    virtual LRESULT fullscreenClientWndProc(HWND, UINT message, WPARAM, LPARAM)
= 0;
>> +};
> 
> We normally add an empty, protected, virtual destructor for client classes like
this. (GCC will complain if there's no virtual destructor, but MSVC won't.)

Will do.

> I also personally think it's nice to put client interfaces into their own header
files to reduce recompilations when changing MediaPlayerPrivateFullscreenWindow's
declaration in a way that doesn't affect the declarations of classes that
implement the client interface.

Sounds like a good idea.

>> bool MediaPlayerPrivateQuickTimeVisualContext::supportsFullscreen() const
>> {
>> -    return false;
>> +    return true;
>> }
> 
> Don't we still want to return false when USE(ACCELERATED_COMPOSITING) is disabled
or when Settings::acceleratedCompositingEnabled() is false?

Yes, we do.  Changed. 

>> @@ -939,6 +939,10 @@ void MediaPlayerPrivateQuickTimeVisualCo
>> #endif
>>     // The layer will get hooked up via
RenderLayerBacking::updateGraphicsLayerConfiguration().
>> #endif
>> +
>> +    // fill the newly created layer with image data
>> +    if (m_visualContext)
>> +        retrieveCurrentImage();
>> }
> 
> Why is this now needed? (Both the comment here and the ChangeLog say *what* this
does, but not *why*.)

I'll update the comment to read:

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

... or something. :)

>> +void WKCACFLayer::setFrame(const CGRect& rect)
>> +{
>> +    CGRect oldFrame = frame();
>> +    if (CGRectEqualToRect(rect, oldFrame))
>> +        return;
>> +
>> +    CACFLayerSetFrame(layer(), rect);
>> +    setNeedsCommit();
>> +
>> +    if (m_needsDisplayOnBoundsChange)
>> +        setNeedsDisplay();
>> +}
> 
> If the size of the frame hasn't changed, should we not call setNeedsDisplay()?

If the size of the frame hasn't changed, the second statement should bail us out
early.  Or did I miss something?

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

Changed.

> 
>> @@ -500,7 +539,7 @@ void WKCACFLayer::printLayer(int indent)
>>     CGImageRef layerContents = contents();
>>     if (layerContents) {
>>         printIndent(indent + 1);
>> -        fprintf(stderr, "(contents (image [%d %d]))\n",
>> +            fprintf(stderr, "(contents (image [%d %d]))\n",
>>             CGImageGetWidth(layerContents), CGImageGetHeight(layerContents));
>>     }
> 
> This seems like a step in the wrong direction.

I'm really hoping this was in reference to the indenting change. :-D  Undone.

>> +class WKCACFLayerLayoutClient {
>> +public:
>> +    virtual void layoutSublayersOfLayer(WKCACFLayer*) = 0;
>> +};
> 
> Same comments here that I had about the other client interface.

Same response.

>> class WKCACFLayer : public RefCounted<WKCACFLayer> {
>> public:
>> @@ -63,6 +67,10 @@ public:
>> 
>>     virtual void drawInContext(PlatformGraphicsContext*) { }
>> 
>> +    virtual void setLayoutClient(WKCACFLayerLayoutClient*);
>> +    virtual WKCACFLayerLayoutClient* layoutClient() const;
>> +    virtual void setNeedsLayout();
> 
> I don't see why these need to be virtual.

Hmm; if a subclass wants to do something in response to having the client set?

> 
>> +    void setFrame(const CGRect&);
> 
> But this definitely *does* need to be virtual!

When Chris removed it, it wasn't virtual then.  But it can be virtual now.

>> +class FullscreenVideoController::LayoutClient : public
WebCore::WKCACFLayerLayoutClient {
> 
> You should remove all instances of "WebCore::" from the .cpp file.

Done.

>> +    void layoutSublayersOfLayer(WKCACFLayer* layer) 
>> +    {
>> +        if (layer != m_parent->m_rootChild) 
>> +            return;
> 
> Should we also assert that these two layers are the same?

Yes, that sounds like a good idea.

> I think this would be easier to read if you didn't put this function inline in the
class declaration.

Sure.

>> +        float scaleFactor = 0;
>> +        if (naturalSize.aspectRatio() > layerBounds.size().aspectRatio())
>> +            scaleFactor = layerBounds.width() / naturalSize.width();
>> +        else
>> +            scaleFactor = layerBounds.height() / naturalSize.height();
> 
> No need to initialize scaleFactor to 0.

Changed.

>> +        // Calculate the centered position based on the videoAnchor,
videoBounds, and layerBounds:
>> +        FloatPoint videoPosition;
>> +        videoPosition.setX(layerCenter.x() - (0.5 - videoAnchor.x()) *
videoBounds.width());
>> +        videoPosition.setY(layerCenter.y() - (0.5 - videoAnchor.y()) *
videoBounds.height());
>> +        videoLayer->setPosition(videoPosition);
>> +
>> +        videoLayer->setBounds(FloatRect(FloatPoint(), naturalSize));
>> +    }
> 
> Why not use setFrame instead of setPosition/setBounds?

Hah!  I totally could.  

> 
>> @@ -183,25 +229,20 @@ FullscreenVideoController::FullscreenVid
>>     , m_hitWidget(0)
>>     , m_movingWindow(false)
>>     , m_timer(this, &FullscreenVideoController::timerFired)
>> +    , m_rootChild(WebCore::WKCACFLayer::create(WebCore::WKCACFLayer::Layer))
>> +    , m_layoutClient(new FullscreenVideoController::LayoutClient(this))
> 
> You can just say "new LayoutClient(this)" here.

Changed.

> It seems a little strange that we create m_rootChild and m_layoutClient here when
we wait until enterFullscreen is called to create m_fullscreenWindow. Why not
create them all in the same place?

No good reason.  But if we create them in the constructor, and delete them from the
destructor, they can be exempted from NULL checks.

>> 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;
>> +    }
>> +
>> +    if (m_hudWindow) {
>> +        SetWindowLongPtr(m_hudWindow, 0, 0);
>> +        ::DestroyWindow(m_hudWindow);
>> +        m_hudWindow = 0;
>> +    }
> 
> Why is it now possible for m_hudWindow to be null when it wasn't possible before?
Or was it possible before? (If so, you should mention it in your ChangeLog.)

It was always possible, if a client called "enterFullscreen()/exitFullscreen()" out
of order.  It's probably safe to call SetWindowLongPtr() and DestroyWindow() with
NULL, but its also unnecessary.

>> +    // As a side effect of setting the player to invisible/visible,
>> +    // the player's layer will be recreated, and will be picked up 
>> +    // the next time the layer tree is synched.
>> +    m_mediaElement->player()->setVisible(0);
>> +    m_mediaElement->player()->setVisible(1);
>> }
> 
> Is this needed because putting the player's layer into the fullscreen window
ripped it out of the page? I think this could be explained more thoroughly.

Yes, that's why.  I'll add more comments.

>> @@ -316,6 +372,9 @@ LRESULT FullscreenVideoController::fulls
>>         break;
>>     case WM_LBUTTONUP:
>>         onMouseUp(IntPoint(GET_X_LPARAM(lParam), GET_Y_LPARAM(lParam)));
>> +    case WM_ACTIVATEAPP:
>> +        if (!wParam)
>> +            m_mediaElement->exitFullscreen();
>>         break;
>>     }
> 
> Why was this added? The ChangeLog doesn't mention it.

I didn't want to make the Fullscreen window WS_EX_TOPMOST, as that can really screw
over developers (and users) if the app hits a breakpoint while the fullscreen window
is up.  And this matches the current behavior of the QuickTime Player fullscreen
window, when the user switches apps with Alt-Tab.  I'll make a note in the
ChangeLog.

I don't particularly agree with the HI decision to exit fullscreen mode when the app
loses focus, but we should probably try to be consistent.

[Jer Noble]

Created attachment 56937 [details]
Patch

[WebKit Review Bot]

Attachment 56937 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Last 3072 characters of output:
[3]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:116:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:120:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:121:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:121:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:122:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:122:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:123:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:124:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:124:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:125:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:125:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:129:  Extra space before ( in function call  [whitespace/parens] [4]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:134:  Extra space before ( in function call  [whitespace/parens] [4]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:135:  Extra space before ( in function call  [whitespace/parens] [4]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:136:  Extra space before ( in function call  [whitespace/parens] [4]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:137:  Extra space before ( in function call  [whitespace/parens] [4]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:138:  Extra space before ( in function call  [whitespace/parens] [4]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:139:  Extra space before ( in function call  [whitespace/parens] [4]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:140:  Extra space before ( in function call  [whitespace/parens] [4]
WebKitLibraries/win/include/WebKitSystemInterface/WebKitSystemInterface.h:141:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 37 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 56940 [details]
Patch

[Jer Noble]

Created attachment 56943 [details]
Patch

[Jer Noble]

Please ignore the extraneous check-in comment in WebCore/CommitLog.  It will be removed in the final checkin.

[Adam Roben (:aroben)]

(In reply to comment #24)
> (In reply to comment #22)
> > --- Comment #22 from Adam Roben (aroben) <[e-mail]>  2010-05-24 11:38:12
> PST ---
> > (From update of attachment 56856 [details])
> >> +        Minor additions have been made to the FloatSize and IntSize classes:
> added aspectRatio() to both; added scale(float) to IntSize.
> > 
> > You must mean that you added scale(float) to FloatSize. But I don't think you need
> to mention these low-level changes here, since the exact same information is
> included in the list of changed files below.
> 
> Chris asked for them specifically, but I agree; I'll remove the specifics here.

Just to be clear, I think it's great to have them specifically mentioned in the file-level changes.

> > It seems like there's nothing about this class that's specific to its use in
> conjunction with MediaPlayerPrivate. Maybe we should call it something like
> FullscreenLayerWindow?
> 
> I wasn't entirely happy with the name.  I'd be happy to rename it
> FullscreenLayerWindow.

I think I slightly prefer FullscreenLayerWindow, but I don't care too strongly.

> >> +    MONITORINFO mi = {0};
> >> +    mi.cbSize = sizeof(MONITORINFO);
> >> +    GetMonitorInfo(MonitorFromWindow(0, MONITOR_DEFAULTTOPRIMARY), &mi);
> >> +    IntRect monitorRect = mi.rcMonitor;
> > 
> > Seems like we should be passing the WebView's HWND to MonitorFromWindow so that
> the fullscreen video will appear on the same monitor as the webpage.
> 
> I thought of that, but wasn't sure how to pipe that information through to the
> FullscreenWindow class.  Perhaps a method added to FullscreenWindowClient?

I think passing it in the constructor would be slightly better, since we don't want to support the parent window changing during the lifetime of the FullscreenLayerWindow.

> > Should we check the return value of GetMonitorInfo?
> 
> And bail out early?  

Yeah, sorry for being vague.

> >> +    cs.x = 0;
> >> +    cs.y = 0;
> >> +    cs.cx = monitorRect.width();
> >> +    cs.cy = monitorRect.height();
> > 
> > Shouldn't we be setting cs.x and cs.y to monitorRect.x() and monitorRect.y()?
> 
> When the only monitor we ask for is the Primary one, those will always be zero.  Now
> that we'll be asking for (possibly) other ones, yes, yes we should be. :)

You can set your primary monitor to be to the right of a secondary monitor. In that case I'd imagine the primary monitor's origin would not be at 0,0 (though maybe it is still at 0,0 and all the secondary monitor's coordinates are negative?).

As you said, when using non-primary monitors we obviously have to worry about this.

> >> +    if (m_rootChild) {
> >> +        m_layerRenderer->setRootChildLayer(m_rootChild.get());
> >> +        WKCACFLayer* rootLayer = m_rootChild->rootLayer();
> >> +        CGRect rootBounds = m_rootChild->rootLayer()->bounds();
> >> +        m_rootChild->setFrame(rootBounds);
> >> +        m_layerRenderer->setScrollFrame(IntPoint(rootBounds.origin),
> IntSize(rootBounds.size));
> >> +        m_rootChild->setBackgroundColor(CGColorGetConstantColor(kCGColorBlack));
> >> +        rootLayer->setBackgroundColor(CGColorGetConstantColor(kCGColorBlack));
> >> +    }
> >> +}
> > 
> > Do we really want to do all this work if m_rootChild hasn't changed?
> > 
> > An early return would make this a little easier to read.
> 
> I had that in an earlier version, but there was a case where this all needed to be
> happen even when m_rootChild hadn't changed.  I can't remember off the top of my
> head why that would be.

I think it would be good to add a comment, even if you're not quite sure what the circumstances are. That way someone like me won't just change it without testing. :-)

> >> +LRESULT MediaPlayerPrivateFullscreenWindow::wndProc(HWND hWnd, UINT message,
> WPARAM wParam, LPARAM lParam)
> >> +{
> >> +    LRESULT lResult = 0;
> >> +    switch (message) {
> >> +    case WM_CREATE:
> >> +        m_hwnd = hWnd;
> > 
> > This assignment will be overwritten (with the same value) as soon as
> CreateWindowExW returns. Why do we assign in both places?
> 
> The assignment here is probably the necessary one, and the one which catches the
> return from CreateWindowExW is the extraneous one.  If we set m_hwnd here (rather
> than later) then it will be valid inside all the future wndProc calls happening as a
> result of CreateWindowExW.
> 
> We could probably add an ASSERT(IsWindow(m_hwnd)); after CreateWindowExW returns.

That sounds good to me.

> >> +        m_layerRenderer->setHostWindow(m_hwnd);
> >> +        m_layerRenderer->createRenderer();
> >> +        m_layerRenderer->setNeedsDisplay();
> > 
> > Why do we call setNeedsDisplay here? We don't have a root child layer yet, right?
> 
> That's true in this case, but it's not guaranteed that clients will call in that
> order.  Maybe we should check to see if we have a rootLayer before calling
> setNeedsDisplay?

Sure, that sounds good.

> >> +void WKCACFLayer::setFrame(const CGRect& rect)
> >> +{
> >> +    CGRect oldFrame = frame();
> >> +    if (CGRectEqualToRect(rect, oldFrame))
> >> +        return;
> >> +
> >> +    CACFLayerSetFrame(layer(), rect);
> >> +    setNeedsCommit();
> >> +
> >> +    if (m_needsDisplayOnBoundsChange)
> >> +        setNeedsDisplay();
> >> +}
> > 
> > If the size of the frame hasn't changed, should we not call setNeedsDisplay()?
> 
> If the size of the frame hasn't changed, the second statement should bail us out
> early.  Or did I miss something?

If only the origin of the frame changes, but not its size, you will call setNeedsDisplay(). But I don't think you need to in that case.

> >> class WKCACFLayer : public RefCounted<WKCACFLayer> {
> >> public:
> >> @@ -63,6 +67,10 @@ public:
> >> 
> >>     virtual void drawInContext(PlatformGraphicsContext*) { }
> >> 
> >> +    virtual void setLayoutClient(WKCACFLayerLayoutClient*);
> >> +    virtual WKCACFLayerLayoutClient* layoutClient() const;
> >> +    virtual void setNeedsLayout();
> > 
> > I don't see why these need to be virtual.
> 
> Hmm; if a subclass wants to do something in response to having the client set?

Yes, they would need to be virtual in that case. But there is no such subclass currently. If/when we add one, we can make them virtual. Having them virtual now just slows things down.

> >> +    void setFrame(const CGRect&);
> > 
> > But this definitely *does* need to be virtual!
> 
> When Chris removed it, it wasn't virtual then.  But it can be virtual now.

It needs to be virtual now because you added an override of it in WebTiledLayer.

> > It seems a little strange that we create m_rootChild and m_layoutClient here when
> we wait until enterFullscreen is called to create m_fullscreenWindow. Why not
> create them all in the same place?
> 
> No good reason.  But if we create them in the constructor, and delete them from the
> destructor, they can be exempted from NULL checks.

Seems like we could move the creation of m_fullscreenWindow into the constructor, too, then.

> >> 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;
> >> +    }
> >> +
> >> +    if (m_hudWindow) {
> >> +        SetWindowLongPtr(m_hudWindow, 0, 0);
> >> +        ::DestroyWindow(m_hudWindow);
> >> +        m_hudWindow = 0;
> >> +    }
> > 
> > Why is it now possible for m_hudWindow to be null when it wasn't possible before?
> Or was it possible before? (If so, you should mention it in your ChangeLog.)
> 
> It was always possible, if a client called "enterFullscreen()/exitFullscreen()" out
> of order.  It's probably safe to call SetWindowLongPtr() and DestroyWindow() with
> NULL, but its also unnecessary.

OK, I think it would be good to mention this in the ChangeLog.

> >> @@ -316,6 +372,9 @@ LRESULT FullscreenVideoController::fulls
> >>         break;
> >>     case WM_LBUTTONUP:
> >>         onMouseUp(IntPoint(GET_X_LPARAM(lParam), GET_Y_LPARAM(lParam)));
> >> +    case WM_ACTIVATEAPP:
> >> +        if (!wParam)
> >> +            m_mediaElement->exitFullscreen();
> >>         break;
> >>     }
> > 
> > Why was this added? The ChangeLog doesn't mention it.
> 
> I didn't want to make the Fullscreen window WS_EX_TOPMOST, as that can really screw
> over developers (and users) if the app hits a breakpoint while the fullscreen window
> is up.  And this matches the current behavior of the QuickTime Player fullscreen
> window, when the user switches apps with Alt-Tab.  I'll make a note in the
> ChangeLog.
> 
> I don't particularly agree with the HI decision to exit fullscreen mode when the app
> loses focus, but we should probably try to be consistent.

I think we previously decided to leave the fullscreen window up when switching to another app or window. See bug 33741. I think you should just revert this change.

[Adam Roben (:aroben)]

Comment on attachment 56943 [details]
Patch

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

[Jer Noble]

(In reply to comment #30)
> (In reply to comment #24)
> > (In reply to comment #22)
> > > --- Comment #22 from Adam Roben (aroben) <[e-mail]>  2010-05-24 11:38:12
> > I wasn't entirely happy with the name.  I'd be happy to rename it
> > FullscreenLayerWindow.
> 
> I think I slightly prefer FullscreenLayerWindow, but I don't care too strongly.

In that case, I think I'll leave it as-is and rename it once something other than the <video> element uses the fullscreen code.

> > I thought of that, but wasn't sure how to pipe that information through to the
> > FullscreenWindow class.  Perhaps a method added to FullscreenWindowClient?
> 
> I think passing it in the constructor would be slightly better, since we don't want to support the parent window changing during the lifetime of the FullscreenLayerWindow.

Got it, this with jive with the change below about creating the fullscreen HWND in FullscreenLayerWindow's constructor.

> > > Should we check the return value of GetMonitorInfo?
> > 
> > And bail out early?  
> 
> Yeah, sorry for being vague.

Changed.

> > When the only monitor we ask for is the Primary one, those will always be zero.  Now
> > that we'll be asking for (possibly) other ones, yes, yes we should be. :)
> 
> You can set your primary monitor to be to the right of a secondary monitor. In that case I'd imagine the primary monitor's origin would not be at 0,0 (though maybe it is still at 0,0 and all the secondary monitor's coordinates are negative?).

Yes, they're negative.  But regardless, changed. :)

> > I had that in an earlier version, but there was a case where this all needed to be
> > happen even when m_rootChild hadn't changed.  I can't remember off the top of my
> > head why that would be.
> 
> I think it would be good to add a comment, even if you're not quite sure what the circumstances are. That way someone like me won't just change it without testing. :-)

Added.

> > >> +        m_layerRenderer->setHostWindow(m_hwnd);
> > >> +        m_layerRenderer->createRenderer();
> > >> +        m_layerRenderer->setNeedsDisplay();
> > > 
> > > Why do we call setNeedsDisplay here? We don't have a root child layer yet, right?
> > 
> > That's true in this case, but it's not guaranteed that clients will call in that
> > order.  Maybe we should check to see if we have a rootLayer before calling
> > setNeedsDisplay?
> 
> Sure, that sounds good.

Changed.

> > >> +void WKCACFLayer::setFrame(const CGRect& rect)
> > >> +{
> > >> +    CGRect oldFrame = frame();
> > >> +    if (CGRectEqualToRect(rect, oldFrame))
> > >> +        return;
> > >> +
> > >> +    CACFLayerSetFrame(layer(), rect);
> > >> +    setNeedsCommit();
> > >> +
> > >> +    if (m_needsDisplayOnBoundsChange)
> > >> +        setNeedsDisplay();
> > >> +}
> > > 
> > > If the size of the frame hasn't changed, should we not call setNeedsDisplay()?
> > 
> > If the size of the frame hasn't changed, the second statement should bail us out
> > early.  Or did I miss something?
> 
> If only the origin of the frame changes, but not its size, you will call setNeedsDisplay(). But I don't think you need to in that case.

Changed.

> > >> class WKCACFLayer : public RefCounted<WKCACFLayer> {
> > >> public:
> > >> @@ -63,6 +67,10 @@ public:
> > >> 
> > >>     virtual void drawInContext(PlatformGraphicsContext*) { }
> > >> 
> > >> +    virtual void setLayoutClient(WKCACFLayerLayoutClient*);
> > >> +    virtual WKCACFLayerLayoutClient* layoutClient() const;
> > >> +    virtual void setNeedsLayout();
> > > 
> > > I don't see why these need to be virtual.
> > 
> > Hmm; if a subclass wants to do something in response to having the client set?
> 
> Yes, they would need to be virtual in that case. But there is no such subclass currently. If/when we add one, we can make them virtual. Having them virtual now just slows things down.

Changed.

> > No good reason.  But if we create them in the constructor, and delete them from the
> > destructor, they can be exempted from NULL checks.
> 
> Seems like we could move the creation of m_fullscreenWindow into the constructor, too, then.

Moved.

> > > Why is it now possible for m_hudWindow to be null when it wasn't possible before?
> > Or was it possible before? (If so, you should mention it in your ChangeLog.)
> > 
> > It was always possible, if a client called "enterFullscreen()/exitFullscreen()" out
> > of order.  It's probably safe to call SetWindowLongPtr() and DestroyWindow() with
> > NULL, but its also unnecessary.
> 
> OK, I think it would be good to mention this in the ChangeLog.

Oh, I finally get what you were originally asking! Yeah, we can put the assert back in.

> I think we previously decided to leave the fullscreen window up when switching to another app or window. See bug 33741. I think you should just revert this change.

Sure thing!  Pulled.

[Jer Noble]

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

[Adam Roben (:aroben)]

(In reply to comment #33)
> (In reply to comment #31)
> > (From update of attachment 56943 [details])
> > > +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.

Sorry, what I meant to say was: "I think it would be a little nicer if you reversed this if and made it into an early return, like this:

if (!m_rootChild)
    return;
"

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

Ah, interesting. I think making paint() public would be OK. You should talk to Chris, though, since he made that decision.

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

Why does using the WebView's HWND not make it possible to pass the HWND to the constructor?

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

Ah, excellent. Thanks for the clarification.

[Jer Noble]

(In reply to comment #34)
> (In reply to comment #33)
> > (In reply to comment #31)
> > 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.
> 
> Sorry, what I meant to say was: "I think it would be a little nicer if you reversed this if and made it into an early return, like this:
> 
> if (!m_rootChild)
>     return;

That makes way more sense.  Consider it done.

> > > > +    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?
> 
> Ah, interesting. I think making paint() public would be OK. You should talk to Chris, though, since he made that decision.

Will do.

> > > > +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.
> 
> Why does using the WebView's HWND not make it possible to pass the HWND to the constructor?

Because here we are getting the WebView by way of the media element, and the media element is given to the FullscreenVideoController after it's been constructed, so at construct time, we'll have no HWND to give.

[Adam Roben (:aroben)]

(In reply to comment #35)
> > Why does using the WebView's HWND not make it possible to pass the HWND to the constructor?
> 
> Because here we are getting the WebView by way of the media element, and the media element is given to the FullscreenVideoController after it's been constructed, so at construct time, we'll have no HWND to give.

I guess you're saying there's no way to get from the media element to the WebView at the time enterFullscreen() is called? That seems strange. (The WebView should certainly have an HWND by that point.)

[Jer Noble]

(In reply to comment #36)
> I guess you're saying there's no way to get from the media element to the WebView at the time enterFullscreen() is called? That seems strange. (The WebView should certainly have an HWND by that point.)

No, I'm saying there's no way to get from FullscreenVideoController to HTMLMediaElement at the time MediaPlayerPrivateFullscreenWindow is created.  (In FullscreenVideoController's constructor)

[Adam Roben (:aroben)]

(In reply to comment #37)
> (In reply to comment #36)
> > I guess you're saying there's no way to get from the media element to the WebView at the time enterFullscreen() is called? That seems strange. (The WebView should certainly have an HWND by that point.)
> 
> No, I'm saying there's no way to get from FullscreenVideoController to HTMLMediaElement at the time MediaPlayerPrivateFullscreenWindow is created.  (In FullscreenVideoController's constructor)

I see. I was confused because in your last patch, MediaPlayerPrivateFullscreenWindow isn't created until enterFullscreen() is called.

What if we passed the parent window to MediaPlayerPrivateFullscreenWindow::createWindow?

[Jer Noble]

(In reply to comment #38)
> I see. I was confused because in your last patch, MediaPlayerPrivateFullscreenWindow isn't created until enterFullscreen() is called.
> 
> What if we passed the parent window to MediaPlayerPrivateFullscreenWindow::createWindow?

I'll try that, and submit a new patch with this and all the other changes integrated.

[Jer Noble]

Created attachment 57040 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 57040 [details]
Patch

> +void MediaPlayerPrivateFullscreenWindow::createWindow(HWND parentHwnd)

It would be more accurate to call this parameter "ownerHwnd", since popup windows have owners, not parents. WebKit style says "Hwnd" should be in all-caps.

> +    MONITORINFO mi = {0};
> +    mi.cbSize = sizeof(MONITORINFO);
> +    if (!GetMonitorInfo(MonitorFromWindow(parentHwnd, MONITOR_DEFAULTTONEAREST), &mi))
> +      return;

Funny indentation here.

> +    ::CreateWindowExW(0, windowClassName, L"", WS_POPUP | WS_VISIBLE, 
> +        monitorRect.x(), monitorRect.y(), monitorRect.width(), monitorRect.height(),
> +        parentHwnd, 0, WebCore::instanceHandle(), this);

Should we be passing WS_EX_TOOLWINDOW to avoid having a taskbar item created for this window?

> +void MediaPlayerPrivateFullscreenWindow::setRootChildLayer(PassRefPtr<WKCACFLayer> rootChild)
> +{
> +    if (m_rootChild == rootChild)
> +        return;

I guess you figured out this was OK after all.

> +    void createWindow(HWND parentHwnd = 0);

I don't think there's any need for the default value for this parameter.

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

Don't forget to capitalize "fill"!

> +void WKCACFLayer::setFrame(const CGRect& rect)
> +{
> +    CGRect oldFrame = frame();
> +    if (CGRectEqualToRect(rect, oldFrame))
> +        return;
> +
> +    CACFLayerSetFrame(layer(), rect);
> +    setNeedsCommit();
> +
> +    if (m_needsDisplayOnBoundsChange && CGSizeEqualToSize(rect.size, oldFrame.size))
> +        setNeedsDisplay();
> +}

I think you meant !CGSizeEqualToSize(...), right?

> +WKCACFLayerLayoutClient* WKCACFLayer::layoutClient() const
> +{
> +    return m_layoutClient;
> +}
> +
> +void WKCACFLayer::setNeedsLayout()
> +{
> +    CACFLayerSetNeedsLayout(layer());
> +}

These could be inline in the class declaration.

> @@ -260,8 +273,11 @@ protected:
>      void printLayer(int indent) const;
>  #endif
>  
> +    static void layoutSublayersProc(CACFLayerRef);
> +

I think this can be private.

> +        Modified FullscreenVideoController to work with MediaPlayerPrivateFullscreenWindow.  The FullscreenVideoController
> +        is now MediaPlayerPrivate agnostic.  The new fullscreen window is not a topmost window, and so the behavior has
> +        been changed to exit fullscreen mode when the application becomes inactive.

I think the last sentence is no longer true.

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

You could use ASSERT_ARG here:

ASSERT_ARG(layer, layer == m_parent->m_rootChild);

> Index: LayoutTests/platform/win/media/controls-drag-timebar-expected.txt
> ===================================================================
> --- LayoutTests/platform/win/media/controls-drag-timebar-expected.txt	(revision 60167)
> +++ LayoutTests/platform/win/media/controls-drag-timebar-expected.txt	(working copy)
> @@ -4,10 +4,10 @@ RUN(video.autoplay = true)
>  RUN(video.src = 'content/test.mp4')
>  EVENT(playing)
>  EVENT(seeked)
> -Time: 2.1
> +Time: 2.3
>  EVENT(seeked)
> -Time: 2.6
> +Time: 2.8
>  EVENT(seeked)
> -Time: 3.1
> +Time: 3.4
>  END OF TEST

Are these changes really correct? Maybe the re-addition of the full-screen button makes the positions on the timebar have different values?

I don't think I need to see this again before you check it in. r=me

[Jer Noble]

(In reply to comment #41)
> (From update of attachment 57040 [details])
> > +void MediaPlayerPrivateFullscreenWindow::createWindow(HWND parentHwnd)
> 
> It would be more accurate to call this parameter "ownerHwnd", since popup windows have owners, not parents. WebKit style says "Hwnd" should be in all-caps.

Changed to ownerWindow; no need to restate the type all the time. 
 
> > +    MONITORINFO mi = {0};
> > +    mi.cbSize = sizeof(MONITORINFO);
> > +    if (!GetMonitorInfo(MonitorFromWindow(parentHwnd, MONITOR_DEFAULTTONEAREST), &mi))
> > +      return;
> 
> Funny indentation here.

Vi strikes again!  Fixed.

> > +    ::CreateWindowExW(0, windowClassName, L"", WS_POPUP | WS_VISIBLE, 
> > +        monitorRect.x(), monitorRect.y(), monitorRect.width(), monitorRect.height(),
> > +        parentHwnd, 0, WebCore::instanceHandle(), this);
> 
> Should we be passing WS_EX_TOOLWINDOW to avoid having a taskbar item created for this window?

Good idea!  Changed.

> > +void MediaPlayerPrivateFullscreenWindow::setRootChildLayer(PassRefPtr<WKCACFLayer> rootChild)
> > +{
> > +    if (m_rootChild == rootChild)
> > +        return;
> 
> I guess you figured out this was OK after all.

Indeed.

> > +    void createWindow(HWND parentHwnd = 0);
> 
> I don't think there's any need for the default value for this parameter.

Removed.

> > +    // 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();
> 
> Don't forget to capitalize "fill"!

Capital!

> > +void WKCACFLayer::setFrame(const CGRect& rect)
> > +{
> > +    CGRect oldFrame = frame();
> > +    if (CGRectEqualToRect(rect, oldFrame))
> > +        return;
> > +
> > +    CACFLayerSetFrame(layer(), rect);
> > +    setNeedsCommit();
> > +
> > +    if (m_needsDisplayOnBoundsChange && CGSizeEqualToSize(rect.size, oldFrame.size))
> > +        setNeedsDisplay();
> > +}
> 
> I think you meant !CGSizeEqualToSize(...), right?

Indeed I did.  Changed.

> > +WKCACFLayerLayoutClient* WKCACFLayer::layoutClient() const
> > +{
> > +    return m_layoutClient;
> > +}
> > +
> > +void WKCACFLayer::setNeedsLayout()
> > +{
> > +    CACFLayerSetNeedsLayout(layer());
> > +}
> 
> These could be inline in the class declaration.

Inlined!

> > @@ -260,8 +273,11 @@ protected:
> >      void printLayer(int indent) const;
> >  #endif
> >  
> > +    static void layoutSublayersProc(CACFLayerRef);
> > +
> 
> I think this can be private.

Privatized!

> > +        Modified FullscreenVideoController to work with MediaPlayerPrivateFullscreenWindow.  The FullscreenVideoController
> > +        is now MediaPlayerPrivate agnostic.  The new fullscreen window is not a topmost window, and so the behavior has
> > +        been changed to exit fullscreen mode when the application becomes inactive.
> 
> I think the last sentence is no longer true.

Removed!

> > +void FullscreenVideoController::LayoutClient::layoutSublayersOfLayer(WKCACFLayer* layer) 
> > +{
> > +    ASSERT(layer == m_parent->m_rootChild);
> 
> You could use ASSERT_ARG here:
> 
> ASSERT_ARG(layer, layer == m_parent->m_rootChild);

Asserted!

> > Index: LayoutTests/platform/win/media/controls-drag-timebar-expected.txt
> > ===================================================================
> > --- LayoutTests/platform/win/media/controls-drag-timebar-expected.txt	(revision 60167)
> > +++ LayoutTests/platform/win/media/controls-drag-timebar-expected.txt	(working copy)
> > @@ -4,10 +4,10 @@ RUN(video.autoplay = true)
> >  RUN(video.src = 'content/test.mp4')
> >  EVENT(playing)
> >  EVENT(seeked)
> > -Time: 2.1
> > +Time: 2.3
> >  EVENT(seeked)
> > -Time: 2.6
> > +Time: 2.8
> >  EVENT(seeked)
> > -Time: 3.1
> > +Time: 3.4
> >  END OF TEST
> 
> Are these changes really correct? Maybe the re-addition of the full-screen button makes the positions on the timebar have different values?

Yes, since the scrubber bar is now shorter (because of the button) each pixel on the bar maps to a slightly different time than before.

> I don't think I need to see this again before you check it in. r=me

Okay, but in addition to the above, I also added a bunch of #if USE(ACCELERATED_COMPOSITING) checks around references to WKCACFLayer/LayerRenderer, to make sure the above builds on the buildbots.

[Jer Noble]

Committed r60190: <http://trac.webkit.org/changeset/60190>