[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 14:46:49 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=39557
--- Comment #24 from Jer Noble <jer.noble at apple.com> 2010-05-24 14:46:49 PST ---
(In reply to comment #22)
> --- 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.
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.
--
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