Bug 61843

Summary: Remove dependency on QTKit from wekitExitFullscreen()
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: WebKit Misc.Assignee: Jer Noble <jer.noble>
Status: RESOLVED WONTFIX    
Severity: Normal CC: eric.carlson, eric, menard, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 61848    
Attachments:
Description Flags
Patch
none
Patch
none
[PATCH 1/5] [LANDED] Move Full Screen Controllers into WebCore.
none
[PATCH 2/5] Add two new utility clasess: WebEventListener and WebMediaDelegate
none
[PATCH 3/5] WebVideoFullscreenController and WebVideoFullscreenHUDController now use a delegate
none
[PATCH 4/5] Disable calling into new full-screen API from video full-creen API. Enable video full-screen mode for AVFoundation. none

Description Jer Noble 2011-06-01 01:06:16 PDT
Remove dependency on QTKit from wekitExitFullscreen()
Comment 1 Jer Noble 2011-06-01 01:51:01 PDT
Created attachment 95567 [details]
Patch

This is the first bug of two.  The second one will build upon this one to implement the video full-screen API in WebKit2.
Comment 2 Jer Noble 2011-06-01 02:44:19 PDT
Created attachment 95570 [details]
Patch

Discovered a small problem with the volume slider; it was scaling between 0-100, and the previous patch was setting it to 0-1.
Comment 3 WebKit Review Bot 2011-06-01 02:48:27 PDT
Attachment 95570 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/mac/WebView/WebMediaDelegate.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Alexis Menard (darktears) 2011-06-01 04:24:23 PDT
Comment on attachment 95570 [details]
Patch

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

Damn this patch will badly break https://bugs.webkit.org/show_bug.cgi?id=61728

> Source/WebCore/ChangeLog:106
> +        * platform/mac/WebWindowAnimation.h: Renamed from Source/WebKit/mac/WebView/WebWindowAnimation.h.

There is a previous patch doing almost that waiting for review
https://bugs.webkit.org/show_bug.cgi?id=61522. Perhaps we should cancel it.
Comment 5 Jer Noble 2011-06-01 09:33:09 PDT
(In reply to comment #4)
> (From update of attachment 95570 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95570&action=review
> 
> Damn this patch will badly break https://bugs.webkit.org/show_bug.cgi?id=61728

Yes, it probably will.  However, it looks like QTKitFullScreenVideoHandler could just create an instance of the new WebMediaHandler class to pass along to the fullscreen controller (instead of passing an HTMLElement directly).  Apart from that, and different include paths for WebVideoFullscreenController.h, nothing here should be insurmountable.

> > Source/WebCore/ChangeLog:106
> > +        * platform/mac/WebWindowAnimation.h: Renamed from Source/WebKit/mac/WebView/WebWindowAnimation.h.
> 
> There is a previous patch doing almost that waiting for review
> https://bugs.webkit.org/show_bug.cgi?id=61522. Perhaps we should cancel it.

Lets see what happens to this bug first.  It may get roundly rejected. :)
Comment 6 Jer Noble 2011-06-01 13:17:44 PDT
Comment on attachment 95570 [details]
Patch

Clearing r?; I'll break this bug into smaller pieces and reattach.
Comment 7 Jer Noble 2011-06-01 13:31:17 PDT
Created attachment 95648 [details]
[PATCH 1/5] [LANDED] Move Full Screen Controllers into WebCore.
Comment 8 Jer Noble 2011-06-01 13:31:48 PDT
Created attachment 95649 [details]
[PATCH 2/5] Add two new utility clasess: WebEventListener and WebMediaDelegate
Comment 9 Jer Noble 2011-06-01 13:32:14 PDT
Created attachment 95650 [details]
[PATCH 3/5] WebVideoFullscreenController and  WebVideoFullscreenHUDController now use a delegate
Comment 10 Jer Noble 2011-06-01 13:32:41 PDT
Created attachment 95651 [details]
[PATCH 4/5] Disable calling into new full-screen API from video  full-creen API.  Enable video full-screen mode for  AVFoundation.
Comment 11 Jer Noble 2011-06-01 13:33:30 PDT
There is no 5/5. :-D
Comment 12 Eric Seidel (no email) 2011-06-02 08:38:36 PDT
Who should be CC'd to review these?
Comment 13 Alexis Menard (darktears) 2011-06-03 12:43:54 PDT
(In reply to comment #12)
> Who should be CC'd to review these?

Eric Carlson I believe.
Comment 14 Eric Carlson 2011-06-04 21:52:05 PDT
Comment on attachment 95648 [details]
[PATCH 1/5] [LANDED] Move Full Screen Controllers into WebCore.

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

> Source/WebCore/WebCore.exp.in:1371
>  _wkMeasureMediaUIPart
> +_wkCreateMediaUIBackgroundView
> +_wkCreateMediaUIControl
> +_wkWindowSetAlpha
> +_wkWindowSetScaledFrame
>  _wkMediaControllerThemeAvailable

It looks like exports in this file should be in sort order when not inside #if defs.
Comment 15 Alexis Menard (darktears) 2011-06-20 11:30:23 PDT
Jer could you land patch 1, so I can rebase my patch, it's quite needed for Qt? It seems that patch 2,3 and 4 are not related to us.

Thanks.
Comment 16 Jer Noble 2011-06-20 11:40:52 PDT
Patch 1/5 is blocking work on bug #61728, so I'll land that patch now.
Comment 17 Jer Noble 2011-06-20 12:29:48 PDT
Committed r89271: <http://trac.webkit.org/changeset/89271>
Comment 18 Jer Noble 2011-06-20 12:30:46 PDT
Whoops, re-opening.  Still have patches 2, 3, and 4 to review.
Comment 19 Jer Noble 2011-06-20 12:31:24 PDT
Comment on attachment 95648 [details]
[PATCH 1/5] [LANDED] Move Full Screen Controllers into WebCore.

Landed patch 1/5.  Clearing r+.
Comment 20 Jer Noble 2011-06-20 14:58:05 PDT
Committed a 32-bit build fix for patch 1/5.
 Committed r89292: <http://trac.webkit.org/changeset/89292>
Comment 21 Jer Noble 2011-06-20 15:23:23 PDT
Committed a build fix for Leopard.
Committed r89296: <http://trac.webkit.org/changeset/89296>
Comment 22 Jer Noble 2011-09-16 11:22:18 PDT
This bug is no longer being actively pursued. Clearing the r? flags from the outstanding patches and marking this bug as "not to be fixed".