[webkit-reviews] review denied: [Bug 61728] [Qt] Implement fullscreen support on Mac with the QuickTime backend. : [Attachment 98050] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Alexis Menard
<alexis.menard at openbossa.org>'s request for review:
Bug 61728: [Qt] Implement fullscreen support on Mac with the QuickTime backend.
https://bugs.webkit.org/show_bug.cgi?id=61728

Attachment 98050: Patch
https://bugs.webkit.org/attachment.cgi?id=98050&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
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;
> + at class NSWindow;
> + at 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.