[webkit-reviews] review granted: [Bug 61279] [Qt] Enable usage of QuickTime mediaplayer for the Qt port on Mac. : [Attachment 94679] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Alexis Menard
<alexis.menard at openbossa.org>'s request for review:
Bug 61279: [Qt] Enable usage of QuickTime mediaplayer for the Qt port on Mac.
https://bugs.webkit.org/show_bug.cgi?id=61279

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

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
r+, but please consider making the changes I suggest before committing.


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

> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:712
> +#if PLATFORM(QT) && USE(QTKIT)
> +    return 0;
> +#else
>      return m_qtVideoLayer.get();
> +#endif

You could get rid of the #if here by returning 0 if there is no layer:

    if (!m_qtVideoLayer)
	return 0;
    return m_qtVideoLayer.get();

> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:1348
> +#if PLATFORM(QT) && USE(QTKIT)
> +    CGContextRef cgContext = static_cast<CGContextRef>([[NSGraphicsContext
currentContext] graphicsPort]);
> +    CGContextSaveGState(cgContext);
> +    CGContextSetInterpolationQuality(cgContext, kCGInterpolationLow);
> +    CGContextTranslateCTM(cgContext, r.x(), r.y() + r.height());
> +    CGContextScaleCTM(cgContext, scaleFactor.width(), scaleFactor.height());

> +
> +    newContext = [NSGraphicsContext currentContext];

A comment here about why you can't use the passed context will help people not
familiar with the Qt architecture understand and not break this in the future.

> Source/WebCore/platform/mac/WebCoreObjCExtras.mm:40
> +#include <utility>
>  #include <objc/objc-auto.h>
>  #include <objc/objc-runtime.h>
>  #include <wtf/Assertions.h>

The new #include should be inserted in sort order.