[Qt] Implement fullscreen support on Mac with the QuickTime backend.
Created attachment 95347 [details] Patch
Comment on attachment 95347 [details] Patch Needs to be rebased after patch 1 of https://bugs.webkit.org/show_bug.cgi?id=61843
Created attachment 98050 [details] Patch
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.
(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.
Created attachment 98365 [details] Patch
(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];
(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 ;-)?
Created attachment 98374 [details] Patch
(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.
Comment on attachment 98374 [details] Patch Attachment 98374 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/8933168
Comment on attachment 98374 [details] Patch I'll put cq+ again the ews efl seems bogus.
Comment on attachment 98374 [details] Patch Clearing flags on attachment: 98374 Committed r89617: <http://trac.webkit.org/changeset/89617>
All reviewed patches have been landed. Closing bug.
Revision r89617 cherry-picked into qtwebkit-2.2 with commit a086be9 <http://gitorious.org/webkit/qtwebkit/commit/a086be9>