Bug 131497

Summary: [iOS][WK2] Videos should animate into and out of fullscreen.
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: New BugsAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, eric.carlson, glenn, jonlee, philipj, rniwa, sergio, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Patch none

Description Jer Noble 2014-04-10 10:08:36 PDT
[iOS][WK2] Videos should animate into and out of fullscreen.
Comment 1 Jer Noble 2014-04-10 10:18:47 PDT
Created attachment 229058 [details]
Patch
Comment 2 Jer Noble 2014-04-10 20:52:48 PDT
Created attachment 229103 [details]
Patch
Comment 3 Jer Noble 2014-04-10 21:17:28 PDT
Created attachment 229105 [details]
Patch

Now with 100% more changes to WebCore.exp.in.
Comment 4 Build Bot 2014-04-10 23:43:33 PDT
Comment on attachment 229105 [details]
Patch

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

New failing tests:
transitions/cancel-transition.html
Comment 5 Build Bot 2014-04-10 23:43:38 PDT
Created attachment 229112 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 6 Eric Carlson 2014-04-11 09:25:45 PDT
Comment on attachment 229105 [details]
Patch

r=me but Jeremy should have a close look as well, and *then* you will need to find a WK2 reviewer to give it an official r+
Comment 7 Jeremy Jones 2014-04-11 11:58:51 PDT
Comment on attachment 229105 [details]
Patch

unofficial r=me.  Once nit is that obj-c coordinates are points as doubles. IntRect won't be able to represent every pixel location.
Comment 8 Simon Fraser (smfr) 2014-04-11 12:10:53 PDT
Comment on attachment 229105 [details]
Patch

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

> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:54
> +static IntRect screenRectOfContents(Node* node)
> +{
> +    if (!node || !node->isElementNode())

Seems like the fullscreen code should be changed to use Element everywhere, right. Is it even possible to take a non-element node fullscreen?

Better name for the function would be screenRectForNode().
Comment 9 Jer Noble 2014-04-11 13:42:28 PDT
(In reply to comment #7)
> (From update of attachment 229105 [details])
> unofficial r=me.  Once nit is that obj-c coordinates are points as doubles. IntRect won't be able to represent every pixel location.

Unfortunately, we only have IntRects from our WebCore screen coordinate functions.
Comment 10 Jer Noble 2014-04-11 13:43:06 PDT
(In reply to comment #8)
> (From update of attachment 229105 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229105&action=review
> 
> > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:54
> > +static IntRect screenRectOfContents(Node* node)
> > +{
> > +    if (!node || !node->isElementNode())
> 
> Seems like the fullscreen code should be changed to use Element everywhere, right. Is it even possible to take a non-element node fullscreen?

Sure, but that involves a lot of changes to our full screen ChromeClient APIs.  We should clean out some of that chaff though.

> Better name for the function would be screenRectForNode().

Sure thing.
Comment 11 Jer Noble 2014-04-11 14:51:42 PDT
Committed r167153: <http://trac.webkit.org/changeset/167153>
Comment 12 Jer Noble 2014-04-15 16:36:15 PDT
Reopening to attach new patch.
Comment 13 Jer Noble 2014-04-15 16:36:18 PDT
Created attachment 229417 [details]
Patch
Comment 14 Jer Noble 2014-04-15 17:56:02 PDT
Whoops, wrong bug.