Bug 61843 – Remove dependency on QTKit from wekitExitFullscreen()

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit Misc.

[Jer Noble]

Remove dependency on QTKit from wekitExitFullscreen()

[Jer Noble]

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.

[Jer Noble]

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.

[WebKit Review Bot]

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.

[Alexis Menard (darktears)]

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.

[Jer Noble]

(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. :)

[Jer Noble]

Comment on attachment 95570 [details]
Patch

Clearing r?; I'll break this bug into smaller pieces and reattach.

[Jer Noble]

Created attachment 95648 [details]
[PATCH 1/5] [LANDED] Move Full Screen Controllers into WebCore.

[Jer Noble]

Created attachment 95649 [details]
[PATCH 2/5] Add two new utility clasess: WebEventListener and WebMediaDelegate

[Jer Noble]

Created attachment 95650 [details]
[PATCH 3/5] WebVideoFullscreenController and  WebVideoFullscreenHUDController now use a delegate

[Jer Noble]

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.

[Jer Noble]

There is no 5/5. :-D

[Eric Seidel (no email)]

Who should be CC'd to review these?

[Alexis Menard (darktears)]

(In reply to comment #12)
> Who should be CC'd to review these?

Eric Carlson I believe.

[Eric Carlson]

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.

[Alexis Menard (darktears)]

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.

[Jer Noble]

Patch 1/5 is blocking work on bug #61728, so I'll land that patch now.

[Jer Noble]

Committed r89271: <http://trac.webkit.org/changeset/89271>

[Jer Noble]

Whoops, re-opening.  Still have patches 2, 3, and 4 to review.

[Jer Noble]

Comment on attachment 95648 [details]
[PATCH 1/5] [LANDED] Move Full Screen Controllers into WebCore.

Landed patch 1/5.  Clearing r+.

[Jer Noble]

Committed a 32-bit build fix for patch 1/5.
 Committed r89292: <http://trac.webkit.org/changeset/89292>

[Jer Noble]

Committed a build fix for Leopard.
Committed r89296: <http://trac.webkit.org/changeset/89296>

[Jer Noble]

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