[webkit-reviews] review granted: [Bug 50770] [GStreamer] PlatformVideoWindowMac implementation : [Attachment 78513] updated patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 50770: [GStreamer] PlatformVideoWindowMac implementation
https://bugs.webkit.org/show_bug.cgi?id=50770

Attachment 78513: updated patch
https://bugs.webkit.org/attachment.cgi?id=78513&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=78513&action=review

r+ if you fix the RetainPtr assignment problem.

> Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindow.h:48
> +	   PlatformWidget window() const
> +{
> +#if !PLATFORM(MAC)
> +	      return m_window;
> +#else
> +	      return m_window.get();
> +#endif
> +}

I know the style bot is happy with it, but this brace/#if indentation is
strange. It might be easier to read if this function was moved to the .mm file.



> Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowMac.mm:2
> + * Copyright (C) 2010 Igalia S.L

Probably want 2011 here.


> Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowMac.mm:31
> +    m_window = [[NSView alloc] init];

This should be m_window.adoptNS([[NSView alloc] init]);


> Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowMac.mm:44
> +	   m_videoWindow =
static_cast<PlatformWidget>(g_value_get_pointer(gst_structure_get_value(message
->structure, "nsview")));
> +	   [m_window.get() addSubview:m_videoWindow];

Might be worth ASSERT-ing m_videoWindow here.