[webkit-reviews] review granted: [Bug 39557] Full screen doesn't work for video elements : [Attachment 57040] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 39557: Full screen doesn't work for video elements
https://bugs.webkit.org/show_bug.cgi?id=39557

Attachment 57040: Patch
https://bugs.webkit.org/attachment.cgi?id=57040&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +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.

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

Funny indentation here.

> +    ::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?

> +void
MediaPlayerPrivateFullscreenWindow::setRootChildLayer(PassRefPtr<WKCACFLayer>
rootChild)
> +{
> +    if (m_rootChild == rootChild)
> +	   return;

I guess you figured out this was OK after all.

> +    void createWindow(HWND parentHwnd = 0);

I don't think there's any need for the default value for this parameter.

> +    // 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"!

> +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?

> +WKCACFLayerLayoutClient* WKCACFLayer::layoutClient() const
> +{
> +    return m_layoutClient;
> +}
> +
> +void WKCACFLayer::setNeedsLayout()
> +{
> +    CACFLayerSetNeedsLayout(layer());
> +}

These could be inline in the class declaration.

> @@ -260,8 +273,11 @@ protected:
>      void printLayer(int indent) const;
>  #endif
>  
> +    static void layoutSublayersProc(CACFLayerRef);
> +

I think this can be private.

> +	   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.

> +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);

> 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?

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