Bug 61728 – [Qt] Implement fullscreen support on Mac with the QuickTime backend.

• Component: Media
• Status: RESOLVED FIXED

[Alexis Menard (darktears)]

[Qt] Implement fullscreen support on Mac with the QuickTime backend.

[Alexis Menard (darktears)]

Created attachment 95347 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 95347 [details]
Patch

Needs to be rebased after patch 1 of https://bugs.webkit.org/show_bug.cgi?id=61843

[Alexis Menard (darktears)]

Created attachment 98050 [details]
Patch

[Eric Carlson]

Comment on attachment 98050 [details]
Patch

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

You use "FullScreen" (inner-caps) rather than "Fullscreen" everywhere, but just a year ago we went through and changed everything over to "Fullscreen" (https://bugs.webkit.org/show_bug.cgi?id=34942). I see that a fair amount of the former has crept back in, but I think it would be better to use "Fullscreen" whenever possible for file names and in code.

> Source/WebCore/platform/mac/WebVideoFullscreenHUDWindowController.mm:36
>  #import "FloatConversion.h"
>  #import "WebCoreSystemInterface.h"
> -#import <JavaScriptCore/RetainPtr.h>
> -#import <JavaScriptCore/UnusedParam.h>
> +#import <wtf/RetainPtr.h>
> +#import <wtf/UnusedParam.h>
>  #import <WebCore/HTMLMediaElement.h>
>  #import <WebKitSystemInterface.h>

The sort order is wrong now.

> Source/WebCore/platform/qt/WebCoreSystemInterface.h:46
>  @class NSURL;
> +@class NSWindow;
> +@class NSView;
>  @class QTMovie;

Ditto.

> Source/WebKit/qt/WebCoreSupport/QTKitFullScreenVideoHandler.h:37
> +private:
> +    Private* priv;
> +};

"Private" is probably not the best class name, it doesn't tell me anything about what it does.

> Source/WebKit/qt/WebCoreSupport/QTKitFullScreenVideoHandler.mm:41
> +QTKitFullScreenVideoHandler::QTKitFullScreenVideoHandler()
> +    : priv (new Private)
> +{
> +    priv->m_fullScreenController = nil;
> +}

Doesn't "priv" leak here? Is there any reason not to use an OwnPtr to avoid this?

> Source/WebKit/qt/WebCoreSupport/QTKitFullScreenVideoHandler.mm:51
> +        // First exit Fullscreen for the old mediaElement.

Nit: does "Fullscreen" need to be capitalized?

> Source/WebKit/qt/WebCoreSupport/QTKitFullScreenVideoHandler.mm:55
> +        // This previous call has to trigger exitFullScreen,
> +        // which has to clear priv->m_fullScreenController.
> +        ASSERT(!priv->m_fullScreenController);

I don't think this comment is correct - you are calling exitFullScreen directly, which clears priv->m_fullScreenController.

> Source/WebKit/qt/WebCoreSupport/QTKitFullScreenVideoHandler.mm:61
> +        NSScreen* currentScreen = [NSScreen mainScreen];
> +        [priv->m_fullScreenController enterFullscreen:currentScreen];

Why limit it to the main screen? Your users will not thank you!

> Source/WebKit/qt/WebCoreSupport/QTKitFullScreenVideoHandler.mm:65
> +    }
> +    else
> +        [priv->m_fullScreenController setMediaElement:videoElement];
> +}

How can you hit this else? You always call exitFullScreen() if m_fullScreenController is non-NULL above, and then ASSERT if it was not cleared.

[Alexis Menard (darktears)]

(In reply to comment #4)
> (From update of attachment 98050 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98050&action=review
> 
> You use "FullScreen" (inner-caps) rather than "Fullscreen" everywhere, but just a year ago we went through and changed everything over to "Fullscreen" (https://bugs.webkit.org/show_bug.cgi?id=34942). I see that a fair amount of the former has crept back in, but I think it would be better to use "Fullscreen" whenever possible for file names and in code.

Will do.

> 
> > Source/WebCore/platform/mac/WebVideoFullscreenHUDWindowController.mm:36
> >  #import "FloatConversion.h"
> >  #import "WebCoreSystemInterface.h"
> > -#import <JavaScriptCore/RetainPtr.h>
> > -#import <JavaScriptCore/UnusedParam.h>
> > +#import <wtf/RetainPtr.h>
> > +#import <wtf/UnusedParam.h>
> >  #import <WebCore/HTMLMediaElement.h>
> >  #import <WebKitSystemInterface.h>
> 
> The sort order is wrong now.

True, will fix.

> 
> > Source/WebCore/platform/qt/WebCoreSystemInterface.h:46
> >  @class NSURL;
> > +@class NSWindow;
> > +@class NSView;
> >  @class QTMovie;
> 
> Ditto.
> 
> > Source/WebKit/qt/WebCoreSupport/QTKitFullScreenVideoHandler.h:37
> > +private:
> > +    Private* priv;
> > +};
> 
> "Private" is probably not the best class name, it doesn't tell me anything about what it does.
> 

d?

> > Source/WebKit/qt/WebCoreSupport/QTKitFullScreenVideoHandler.mm:41
> > +QTKitFullScreenVideoHandler::QTKitFullScreenVideoHandler()
> > +    : priv (new Private)
> > +{
> > +    priv->m_fullScreenController = nil;
> > +}
> 
> Doesn't "priv" leak here? Is there any reason not to use an OwnPtr to avoid this?

Well I can, just didn't though about it.

> 
> > Source/WebKit/qt/WebCoreSupport/QTKitFullScreenVideoHandler.mm:51
> > +        // First exit Fullscreen for the old mediaElement.
> 
> Nit: does "Fullscreen" need to be capitalized?

No.

> 
> > Source/WebKit/qt/WebCoreSupport/QTKitFullScreenVideoHandler.mm:55
> > +        // This previous call has to trigger exitFullScreen,
> > +        // which has to clear priv->m_fullScreenController.
> > +        ASSERT(!priv->m_fullScreenController);
> 
> I don't think this comment is correct - you are calling exitFullScreen directly, which clears priv->m_fullScreenController.
> 
> > Source/WebKit/qt/WebCoreSupport/QTKitFullScreenVideoHandler.mm:61
> > +        NSScreen* currentScreen = [NSScreen mainScreen];
> > +        [priv->m_fullScreenController enterFullscreen:currentScreen];
> 
> Why limit it to the main screen? Your users will not thank you!

Is there a way to get the current screen? It seems the mainScreen behaves like it should, I have a multiple monitor setup and it always go fullscreen on the screen where the browser runs.

> 
> > Source/WebKit/qt/WebCoreSupport/QTKitFullScreenVideoHandler.mm:65
> > +    }
> > +    else
> > +        [priv->m_fullScreenController setMediaElement:videoElement];
> > +}
> 
> How can you hit this else? You always call exitFullScreen() if m_fullScreenController is non-NULL above, and then ASSERT if it was not cleared.

Ok let me remove.

[Alexis Menard (darktears)]

Created attachment 98365 [details]
Patch

[Jer Noble]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 98050 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=98050&action=review
> > 
> > You use "FullScreen" (inner-caps) rather than "Fullscreen" everywhere, but just a year ago we went through and changed everything over to "Fullscreen" (https://bugs.webkit.org/show_bug.cgi?id=34942). I see that a fair amount of the former has crept back in, but I think it would be better to use "Fullscreen" whenever possible for file names and in code.
> 
> Will do.

The proposed Mozilla Full-Screen API has uses "full-screen" and "FullScreen".  At the time when we fixed that bug above, it could have gone either way, and now that there's a proposed spec that standardizes on the capital-S, we should probably be consistent with that.

> > > Source/WebKit/qt/WebCoreSupport/QTKitFullScreenVideoHandler.h:37
> > > +private:
> > > +    Private* priv;
> > > +};
> > 
> > "Private" is probably not the best class name, it doesn't tell me anything about what it does.
> > 
> 
> d?

This is a standard Pimpl idiom, but it could probably be "PrivateData".

> > > Source/WebKit/qt/WebCoreSupport/QTKitFullScreenVideoHandler.mm:41
> > > Source/WebKit/qt/WebCoreSupport/QTKitFullScreenVideoHandler.mm:61
> > > +        NSScreen* currentScreen = [NSScreen mainScreen];
> > > +        [priv->m_fullScreenController enterFullscreen:currentScreen];
> > 
> > Why limit it to the main screen? Your users will not thank you!
> 
> Is there a way to get the current screen? It seems the mainScreen behaves like it should, I have a multiple monitor setup and it always go fullscreen on the screen where the browser runs.

-[NSWindow screen] will return the screen on which the window resides.  If you could get a handle on the WebView, this could be: 

NSScreen* currentScreen = [[webView window] screen];

[Eric Carlson]

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 98050 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=98050&action=review
> > > 
> > > You use "FullScreen" (inner-caps) rather than "Fullscreen" everywhere, but just a year ago we went through and changed everything over to "Fullscreen" (https://bugs.webkit.org/show_bug.cgi?id=34942). I see that a fair amount of the former has crept back in, but I think it would be better to use "Fullscreen" whenever possible for file names and in code.
> > 
> > Will do.
> 
> The proposed Mozilla Full-Screen API has uses "full-screen" and "FullScreen".  At the time when we fixed that bug above, it could have gone either way, and now that there's a proposed spec that standardizes on the capital-S, we should probably be consistent with that.
> 

So you will file a bug about making them consistent ;-)?

[Alexis Menard (darktears)]

Created attachment 98374 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > (From update of attachment 98050 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=98050&action=review
> > > > 
> > > > You use "FullScreen" (inner-caps) rather than "Fullscreen" everywhere, but just a year ago we went through and changed everything over to "Fullscreen" (https://bugs.webkit.org/show_bug.cgi?id=34942). I see that a fair amount of the former has crept back in, but I think it would be better to use "Fullscreen" whenever possible for file names and in code.
> > > 
> > > Will do.
> > 
> > The proposed Mozilla Full-Screen API has uses "full-screen" and "FullScreen".  At the time when we fixed that bug above, it could have gone either way, and now that there's a proposed spec that standardizes on the capital-S, we should probably be consistent with that.
> > 
> 
> So you will file a bug about making them consistent ;-)?

At least they are consistent in the Qt part :D. My last patch put back FullScreen.

[Gyuyoung Kim]

Comment on attachment 98374 [details]
Patch

Attachment 98374 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8933168

[Alexis Menard (darktears)]

Comment on attachment 98374 [details]
Patch

I'll put cq+ again the ews efl seems bogus.

[WebKit Review Bot]

Comment on attachment 98374 [details]
Patch

Clearing flags on attachment: 98374

Committed r89617: <http://trac.webkit.org/changeset/89617>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r89617 cherry-picked into qtwebkit-2.2 with commit a086be9 <http://gitorious.org/webkit/qtwebkit/commit/a086be9>