[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 #42 from Jer Noble <jer.noble at apple.com>  2010-05-25 15:10:57 PST ---
(In reply to comment #41)
> (From update of attachment 57040 [details])
> > +void MediaPlayerPrivateFullscreenWindow::createWindow(HWND parentHwnd)
> 
> It would be more accurate to call this parameter "ownerHwnd", since popup windows have owners, not parents. WebKit style says "Hwnd" should be in all-caps.

Changed to ownerWindow; no need to restate the type all the time. 

> > +    MONITORINFO mi = {0};
> > +    mi.cbSize = sizeof(MONITORINFO);
> > +    if (!GetMonitorInfo(MonitorFromWindow(parentHwnd, MONITOR_DEFAULTTONEAREST), &mi))
> > +      return;
> 
> Funny indentation here.

Vi strikes again!  Fixed.

> > +    ::CreateWindowExW(0, windowClassName, L"", WS_POPUP | WS_VISIBLE, 
> > +        monitorRect.x(), monitorRect.y(), monitorRect.width(), monitorRect.height(),
> > +        parentHwnd, 0, WebCore::instanceHandle(), this);
> 
> Should we be passing WS_EX_TOOLWINDOW to avoid having a taskbar item created for this window?

Good idea!  Changed.

> > +void MediaPlayerPrivateFullscreenWindow::setRootChildLayer(PassRefPtr<WKCACFLayer> rootChild)
> > +{
> > +    if (m_rootChild == rootChild)
> > +        return;
> 
> I guess you figured out this was OK after all.

Indeed.

> > +    void createWindow(HWND parentHwnd = 0);
> 
> I don't think there's any need for the default value for this parameter.

Removed.

> > +    // 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.
> > +    if (m_visualContext)
> > +        retrieveCurrentImage();
> 
> Don't forget to capitalize "fill"!

Capital!

> > +void WKCACFLayer::setFrame(const CGRect& rect)
> > +{
> > +    CGRect oldFrame = frame();
> > +    if (CGRectEqualToRect(rect, oldFrame))
> > +        return;
> > +
> > +    CACFLayerSetFrame(layer(), rect);
> > +    setNeedsCommit();
> > +
> > +    if (m_needsDisplayOnBoundsChange && CGSizeEqualToSize(rect.size, oldFrame.size))
> > +        setNeedsDisplay();
> > +}
> 
> I think you meant !CGSizeEqualToSize(...), right?

Indeed I did.  Changed.

> > +WKCACFLayerLayoutClient* WKCACFLayer::layoutClient() const
> > +{
> > +    return m_layoutClient;
> > +}
> > +
> > +void WKCACFLayer::setNeedsLayout()
> > +{
> > +    CACFLayerSetNeedsLayout(layer());
> > +}
> 
> These could be inline in the class declaration.

Inlined!

> > @@ -260,8 +273,11 @@ protected:
> >      void printLayer(int indent) const;
> >  #endif
> >  
> > +    static void layoutSublayersProc(CACFLayerRef);
> > +
> 
> I think this can be private.

Privatized!

> > +        Modified FullscreenVideoController to work with MediaPlayerPrivateFullscreenWindow.  The FullscreenVideoController
> > +        is now MediaPlayerPrivate agnostic.  The new fullscreen window is not a topmost window, and so the behavior has
> > +        been changed to exit fullscreen mode when the application becomes inactive.
> 
> I think the last sentence is no longer true.

Removed!

> > +void FullscreenVideoController::LayoutClient::layoutSublayersOfLayer(WKCACFLayer* layer) 
> > +{
> > +    ASSERT(layer == m_parent->m_rootChild);
> 
> You could use ASSERT_ARG here:
> 
> ASSERT_ARG(layer, layer == m_parent->m_rootChild);

Asserted!

> > Index: LayoutTests/platform/win/media/controls-drag-timebar-expected.txt
> > ===================================================================
> > --- LayoutTests/platform/win/media/controls-drag-timebar-expected.txt	(revision 60167)
> > +++ LayoutTests/platform/win/media/controls-drag-timebar-expected.txt	(working copy)
> > @@ -4,10 +4,10 @@ RUN(video.autoplay = true)
> >  RUN(video.src = 'content/test.mp4')
> >  EVENT(playing)
> >  EVENT(seeked)
> > -Time: 2.1
> > +Time: 2.3
> >  EVENT(seeked)
> > -Time: 2.6
> > +Time: 2.8
> >  EVENT(seeked)
> > -Time: 3.1
> > +Time: 3.4
> >  END OF TEST
> 
> Are these changes really correct? Maybe the re-addition of the full-screen button makes the positions on the timebar have different values?

Yes, since the scrubber bar is now shorter (because of the button) each pixel on the bar maps to a slightly different time than before.

> I don't think I need to see this again before you check it in. r=me

Okay, but in addition to the above, I also added a bunch of #if USE(ACCELERATED_COMPOSITING) checks around references to WKCACFLayer/LayerRenderer, to make sure the above builds on the buildbots.

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