[webkit-reviews] review denied: [Bug 48429] [Gtk] Populate DumpRenderTreeSupportGtk : [Attachment 73245] part I - moved webkit_web_frame* method in webkitprivat.h to DRTSupportGtk.

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Antonio Gomes
<tonikitoo at webkit.org>'s request for review:
Bug 48429: [Gtk] Populate DumpRenderTreeSupportGtk
https://bugs.webkit.org/show_bug.cgi?id=48429

Attachment 73245: part I - moved webkit_web_frame* method in webkitprivat.h to
DRTSupportGtk.
https://bugs.webkit.org/attachment.cgi?id=73245&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=73245&action=review

Great patch. One general comment is that I would like to avoid using gchar,
gint, etc in DumpRenderTreeSupport API. Our guidelines indicate that we want to
avoid using GLib types, unless we are dealing with GLib-style APIs.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:106
> +gchar* DumpRenderTreeSupportGtk::getInnerText(WebKitWebFrame* frame)

I think that since this is a private API we can return a CString here and
relieve the burden of memory management from DumpRenderTree.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:108
> +    g_return_val_if_fail(WEBKIT_IS_WEB_FRAME(frame), NULL);

NULL should be 0 in new code.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:133
> +gchar* DumpRenderTreeSupportGtk::dumpRenderTree(WebKitWebFrame* frame)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_WEB_FRAME(frame), NULL);
> +

Same comments as above.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:156
> +gchar* DumpRenderTreeSupportGtk::counterValueForElementById(WebKitWebFrame*
frame, const gchar* id)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_WEB_FRAME(frame), NULL);

Ditto.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:180
> +    g_return_val_if_fail(WEBKIT_IS_WEB_FRAME(frame), NULL);

NULL -> 0.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:202
> +    g_return_val_if_fail(WEBKIT_IS_WEB_FRAME(frame), NULL);

Ditto.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:242
> +bool DumpRenderTreeSupportGtk::pauseSvgAnimation(WebKitWebFrame* frame,
const gchar* animationId, double time, const gchar* elementId)

Svg should be SVG as per the style rules.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:258
> +gchar* DumpRenderTreeSupportGtk::markerTextForListItem(WebKitWebFrame*
frame, JSContextRef context, JSValueRef nodeObject)

CString here as well.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:304
> +    g_return_val_if_fail(WEBKIT_IS_WEB_FRAME(frame), NULL);

NULL -> 0 throughout this method.