[Webkit-unassigned] [Bug 39557] Full screen doesn't work for video elements
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon May 24 11:38:12 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=39557
Adam Roben (aroben) <aroben at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #56856|review? |review-
Flag| |
--- Comment #22 from Adam Roben (aroben) <aroben at apple.com> 2010-05-24 11:38:12 PST ---
(From update of attachment 56856)
> + 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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list