[Webkit-unassigned] [Bug 186547] [WPE] Build getUserMedia support

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=186547

Thibault Saunier <tsaunier at gnome.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #342474|0                           |1
        is obsolete|                            |
 Attachment #342539|                            |review?, commit-queue?
              Flags|                            |

--- 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:334
> > +    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:354
> > +    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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180612/9bb728c2/attachment.html>