[Webkit-unassigned] [Bug 39557] Full screen doesn't work for video elements
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue May 25 07:35:22 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=39557
--- Comment #30 from Adam Roben (aroben) <aroben at apple.com> 2010-05-25 07:35:22 PST ---
(In reply to comment #24)
> (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 [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.
--
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