[Webkit-unassigned] [Bug 39557] Full screen doesn't work for video elements
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue May 25 14:31:39 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=39557
Adam Roben (aroben) <aroben at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #57040|review? |review+
Flag| |
--- Comment #41 from Adam Roben (aroben) <aroben at apple.com> 2010-05-25 14:31:39 PST ---
(From update of attachment 57040)
> +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
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list