[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 #34 from Adam Roben (aroben) <aroben at apple.com>  2010-05-25 11:30:00 PST ---
(In reply to comment #33)
> (In reply to comment #31)
> > (From update of attachment 56943)
> > > +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.

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