Fullscreen media controls are unusable in pagination mode

• Component: New Bugs
• Status: RESOLVED INVALID

[Jer Noble]

Fullscreen media controls are unusable in pagination mode

[Jer Noble]

Created attachment 229448 [details]
Patch

[Darin Adler]

Comment on attachment 229448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229448&action=review

> Source/WebCore/rendering/RenderFullScreen.h:44
> +    RenderBlock* ensurePlaceholder();
>      RenderBlock* placeholder() { return m_placeholder; }

We recently discussed this idiom on the WebKit mailing list. The coding style we agreed on would be like this:

    RenderBlock& placeholder();
    RenderBlock* placeholderIfExists() { return m_placeholder; }

[Alexey Proskuryakov]

EWS is currently yellow, trying to figure out why these tests are failing:

[1/2] fullscreen/full-screen-no-style-sharing.html failed unexpectedly (text diff)
[2/2] fullscreen/video-cursor-auto-hide.html failed unexpectedly (text diff)

[Build Bot]

Comment on attachment 229448 [details]
Patch

Attachment 229448 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5798796424380416

New failing tests:
fullscreen/full-screen-no-style-sharing.html
fullscreen/video-cursor-auto-hide.html

[Build Bot]

Created attachment 229459 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Jer Noble]

(In reply to comment #4)
> (From update of attachment 229448 [details])
> Attachment 229448 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/5798796424380416
> 
> New failing tests:
> fullscreen/full-screen-no-style-sharing.html

This one has some extraneous space in the dump.

> fullscreen/video-cursor-auto-hide.html

This is the one that's more concerning.

[Build Bot]

Comment on attachment 229448 [details]
Patch

Attachment 229448 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5532288821493760

New failing tests:
fullscreen/full-screen-no-style-sharing.html
fullscreen/video-cursor-auto-hide.html

[Build Bot]

Created attachment 229460 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 229448 [details]
Patch

Attachment 229448 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5976510309597184

New failing tests:
fullscreen/full-screen-no-style-sharing.html
fullscreen/video-cursor-auto-hide.html

[Build Bot]

Created attachment 229464 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Jer Noble]

(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 229448 [details] [details])
> > Attachment 229448 [details] [details] did not pass mac-wk2-ews (mac-wk2):
> > Output: http://webkit-queues.appspot.com/results/5798796424380416
> > 
> > New failing tests:
> > fullscreen/full-screen-no-style-sharing.html
> 
> This one has some extraneous space in the dump.
> 
> > fullscreen/video-cursor-auto-hide.html
> 
> This is the one that's more concerning.

Oh, that test is awful.  It's moving the mouse to the middle of the wrapper div, which in full screen mode should put it outside the area of the video.  The fact that it worked in the beginning was accidental.  I'll update the test and upload a new patch.

[Jer Noble]

Created attachment 229491 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 229491 [details]
Patch for landing.

Rejecting attachment 229491 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 229491, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/4552626645499904

[Eric Carlson]

Comment on attachment 229491 [details]
Patch for landing.

View in context: https://bugs.webkit.org/attachment.cgi?id=229491&action=review

> Source/WebCore/rendering/RenderFullScreen.h:43
> +    void clearPlaceholder();
> +    RenderBlock* ensurePlaceholder();

You forgot to make the name changes Darin suggested.

[Jer Noble]

Committed r167441: <http://trac.webkit.org/changeset/167441>

[Jon Lee]

This caused a regression where the fullscreen video might not appear at all.

[Jon Lee]

<rdar://problem/14887745>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 132152

[Jer Noble]

Closing as invalid; Hyatt fixed these pagination bugs already.