[webkit-reviews] review requested: [Bug 186547] [WPE] Build getUserMedia support : [Attachment 342539] patch.

[bugzilla-daemon at webkit.org]

Thibault Saunier <tsaunier at gnome.org> has asked  for review:
Bug 186547: [WPE] Build getUserMedia support
https://bugs.webkit.org/show_bug.cgi?id=186547

Attachment 342539: patch.

https://bugs.webkit.org/attachment.cgi?id=342539&action=review




--- Comment #3 from Thibault Saunier <tsaunier at gnome.org> ---
Created attachment 342539

  --> https://bugs.webkit.org/attachment.cgi?id=342539&action=review

patch.

(In reply to Alejandro G. Castro from comment #2)
> Comment on attachment 342474 [details]
> [WPE] Build getUserMedia support
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=342474&action=review
> 
> LGTM, some small comments inlined.
> 
> > Source/WebCore/ChangeLog:19
> > +	     no implemented in the Cairo backend itself.
> 
> s/no/now/

Fixed.

> > Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:217
> > +	 copy = cairo_image_surface_create(CAIRO_FORMAT_ARGB32,
> 
> You can use RefPtr here. In WebKit we usually define the variales when used.

Fixed.

> > Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:221
> > +	 cr = cairo_create(copy);
> 
> Ditto here to avoid the explicit cairo_destroy later.

Fixed.

> > Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:245
> > +	 return imageData;
> 
> Is there a leak of the surface? If you return the RefPtr probably it would
> be fixed and you can avoid the destroy in the early return.

Done.

> >
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:33
4
> > +	 GRefPtr<WebKitMediaStreamSrc> self =
adoptGRef(WEBKIT_MEDIA_STREAM_SRC(gst_object_get_parent(parent)));
> 
> Does this mean we get the parent of the parent?

Yes, that is correct, this is what we want.

> >
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:35
4
> > +	 RELEASE_ASSERT(gst_element_add_pad(GST_ELEMENT(self),
GST_PAD(ghostpad)));
> 
> Do we really want to crash on release? Or we could control the situation log
> a gstreamer error and leave?

Done just that.