[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 #12 from Jer Noble <jer.noble at apple.com>  2010-05-23 21:20:26 PST ---
(In reply to comment #11)
> > +    float aspectRatio() const { return m_width / m_height; }
> 
> You should zero check m_height here to avoid divide by 0

Why? If m_height is 0, it will return either +INF or -INF, which is a true answer for the size's aspect ratio.

> > +    float aspectRatio() const { return static_cast<float>(m_width) / static_cast<float>(m_height); }
> 
> Same as above

Same question as above.

> > +        WNDCLASSEX wcex = {0};
> > +        wcex.cbSize         = sizeof(WNDCLASSEX);
> > +        wcex.style          = CS_HREDRAW | CS_VREDRAW | CS_OWNDC;
> > +        wcex.lpfnWndProc    = &staticWndProc;
> > +        wcex.hInstance      = WebCore::instanceHandle();
> > +        wcex.lpszClassName  = L"MediaPlayerPrivateFullscreenWindowClass";
> > +        windowAtom = ::RegisterClassEx(&wcex);
> 
> WebKit style is to not align equal signs, I believe

In WNDCLASS  initializers in the WebKit source code, the equals are aligned much more ofter than the opposite.  It's not called out specifically in the style guidelines, so I went with what seemed used most often.

> Webkit style is also to put params on one line, only splitting when lines are really long

Changed.

> > +WKCACFLayerRenderer* MediaPlayerPrivateFullscreenWindow::layerRenderer()
> > +{
> > +    return m_layerRenderer.get();
> > +}
> 
> This method can be const

Changed.

> > +
> > +WKCACFLayer* MediaPlayerPrivateFullscreenWindow::rootChildLayer()
> > +{
> > +    return m_rootChild.get();
> > +}
> 
> As can this one.

Changed.

> > +    if (m_rootChild) {
> > +        m_layerRenderer->setRootChildLayer(m_rootChild.get());
> > +        WKCACFLayer* rootLayer = m_rootChild->rootLayer();
> > +        CGRect rootBounds = m_rootChild->rootLayer()->bounds();
> > +        m_rootChild->setFrame(rootBounds);
> 
> WKCACFLayer no longer has a setFrame() API. I removed it to simplify the API. Frame is always so confusing because it's nothing more than a composite of bounds.size and position. So you can replace this with calls to setBounds and setPosition.

In order to reproduce the setFrame() API, the caller would need to get the anchorPoint, multiply the result by the destination bounds width and height, and add the result to the intended bottom-left corner position, and then call setBounds() and setPosition().  This turns one line of code into 7 or 8 lines of code.  Why should we remove an API that just wraps CACFLayerSetFrame()?

> > +        m_layerRenderer->setScrollFrame(IntPoint(rootBounds.origin), IntSize(rootBounds.size));
> > +        m_rootChild->setBackgroundColor(CGColorGetConstantColor(kCGColorBlack));
> > +        rootLayer->setBackgroundColor(CGColorGetConstantColor(kCGColorBlack));
> 
> The background of the rootChild should never be seen (always covered by the video) right? You might want to make it's background red (at least in debug mode) to make it easier to spot problems. But maybe video needs to show the background before it's started rendering?

m_rootChild doesn't always occupy it's parent's entire bounds.  So its parent, rootLayer, must have a background color set.  Setting that color to red will just mean the "letterbox" bars will always be red in debug builds.

> Since you're doing fullscreen video with a fullscreen window, it seems like you should be able to animate it like we do on Mac right? Not for now of course, but is there a bug open about this?

There isn't a bug yet, but we should be able to do this.

> > +    WKCACFLayerRenderer* layerRenderer();
> > +
> > +    WKCACFLayer* rootChildLayer();
> 
> Const for these two

Changed.

> >  PlatformMedia MediaPlayerPrivateQuickTimeVisualContext::platformMedia() const
> > @@ -765,7 +765,7 @@ void MediaPlayerPrivateQuickTimeVisualCo
> >  
> >                  buffer.unlockBaseAddress();
> >                  layer->rootLayer()->setNeedsRender();
> > -            }
> > +                }
> 
> This looks odd. Why the change in indent? Hard to see whether or not it's right out of context

No, it's totally wrong.  Changed.


> Ah, I see. You've added back setFrame! I'd really like to leave it out if we can.

I don't really understand why.

> > -    }
> > +        }
> 
> More odd indenting

Changed.

> > +class WKCACFLayer;
> >  class WKCACFAnimation;
> >  class WKCACFTimingFunction;
> 
> Hmmm. I see spurious class declarations here (obviously from when I first implemented this). Probably best to get rid of them while we're here.

Removed.

> > +    void setFrame(const CGRect&);
> > +    CGRect frame() const { return CACFLayerGetFrame(layer()); }
> 
> It would be really great if we could axe this.

Why?

> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        full screen doesn't work for video elements
> 
> Capitalize the start of sentences

This is the title of the bug, both in Radar and Bugzilla.  I'll change the titles of the bugs.

> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Re-enabled fullscreen support on windows, and modified tests to match.
> 
> You should add the bug number here like in the other Changelog

Whoops, looks like webkit-patch didn't do that here.  Added.

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