[webkit-reviews] review granted: [Bug 48199] [GTK] Implement DumpRenderTreeSupportGtk (similarly to DumpRenderTreeSupportQt idea) : [Attachment 71977] patch v1 - initial version of DumpRenderTreeSupportGtk.

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Antonio Gomes
<tonikitoo at webkit.org>'s request for review:
Bug 48199: [GTK] Implement DumpRenderTreeSupportGtk (similarly to
DumpRenderTreeSupportQt idea)
https://bugs.webkit.org/show_bug.cgi?id=48199

Attachment 71977: patch v1 - initial version of DumpRenderTreeSupportGtk.
https://bugs.webkit.org/attachment.cgi?id=71977&action=review

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

Great patch! Thanks for doing this. Please consider fixing my nits before
landing.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:39
> +void DumpRenderTreeSupportGtk::setDumpRenderTreeModeEnabled(bool b)

I think instead of 'b' here this should be 'enabled' or something similar.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:48
> +void DumpRenderTreeSupportGtk::setLinksIncludedInFocusChain(bool b)

Same here.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:5
> +    Copyright (C) 2010 Antonio Gomes <tonikitoo at webkit.org>
> +
> +    This library is free software; you can redistribute it and/or
> +    modify it under the terms of the GNU Library General Public

This is super-nitful, but do you mind adding the asterisks along the left side
of your license blocks so they match the rest of the blocks in the directory?

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:33
> +    static void setDumpRenderTreeModeEnabled(bool b);
> +    static bool dumpRenderTreeModeEnabled();
> +
> +    static void setLinksIncludedInFocusChain(bool b);
> +    static bool linksIncludedInFocusChain();

No need for the parameter names on these declarations.