[Webkit-unassigned] [Bug 39557] Full screen doesn't work for video elements

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39557





--- Comment #32 from Jer Noble <jer.noble at apple.com>  2010-05-25 09:51:09 PST ---
(In reply to comment #30)
> (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
> > 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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.