[webkit-reviews] review granted: [Bug 37060] List item markers are not always updated after changes in the DOM : [Attachment 52553] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jakub Wieczorek
<jwieczorek at webkit.org>'s request for review:
Bug 37060: List item markers are not always updated after changes in the DOM
https://bugs.webkit.org/show_bug.cgi?id=37060

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +JSRetainPtr<JSStringRef>
LayoutTestController::markerTextForListItem(JSValueRef nodeObject) const
> +{
> +    gchar* markerTextGChar =
webkit_web_frame_marker_text_for_list_item(mainFrame, nodeObject);
> +    if (!markerTextGChar)
> +	   return 0;
> +
> +    JSRetainPtr<JSStringRef> markerText(Adopt,
JSStringCreateWithUTF8CString(markerTextGChar));
> +    return markerText;
> +}

There's a storage leak here. You need to g_free markerTextGChar.

>  #include <JavaScriptCore/JSRetainPtr.h>
>  #include <JavaScriptCore/JSStringRefBSTR.h>
>  #include <JavaScriptCore/JavaScriptCore.h>
> +#include <JavaScriptCore/JSValueRef.h>
>  #include <WebCore/COMPtr.h>
>  #include <WebKit/WebKit.h>
>  #include <WebKit/WebKitCOMAPI.h>

Alphabetical order, case sensitive, like the sort tool. That means JSValueRef.h
should be moved up one line.

But I really don't get why you need to add an include here at all.
JavaScriptCore.h includes JavaScript.h which in turn includes JSValueRef.h.