[webkit-reviews] review denied: [Bug 56132] [Gtk] Add support for userscripts tests : [Attachment 85373] Add support for testing user script tests

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 56132: [Gtk] Add support for userscripts tests
https://bugs.webkit.org/show_bug.cgi?id=56132

Attachment 85373: Add support for testing user script tests
https://bugs.webkit.org/attachment.cgi?id=85373&action=review

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

Looks good, though I think these need to be clumped with the WebView-type
methods in DRTSupport and receive the WebView as the first argument.

> LayoutTests/platform/gtk/Skipped:-260
> -# We lack userscripts support.
> -userscripts

There are a few more tests that also rely on userscripts that are skipped here.
Please unskip them as well when you land this patch.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:330
>  /**
> + * addUserScript
> + * @frame: a #WebKitWebFrame
> + * @sourceCode: code of a user script
> + * @runAtStart: whether to run the script at start
> + * @allFrames: whether to inject the script in all frames
> + *
> + */

I don't think you need this documentation. It doesn't add anything beyond the
variable names.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:331
> +void DumpRenderTreeSupportGtk::addUserScript(WebKitWebFrame* frame, const
char* sourceCode, bool runAtStart, bool allFrames)

Please call runAtStart, injectAtDocumentStart and allFrames, injectInAllFrames.
I think it makes sense to keep the names close to their actual meanings.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:337
> +    Frame* coreFrame = core(frame);
> +    if (!coreFrame)
> +	   return;

You don't use coreFrame. Why calculate it?

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:339
> +    fprintf(stderr, "DumpRenderTreeSupportGtk::addUserScript\n");

This seems like it should be removed.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:342
> +    WebKitWebView* webView = getViewFromFrame(frame);
> +    Page* page = core(webView);
> +    page->group().addUserScriptToWorld(mainThreadNormalWorld(), sourceCode,
KURL(), 0, 0, runAtStart ? InjectAtDocumentStart : InjectAtDocumentEnd,
allFrames ? InjectInAllFrames : InjectInTopFrameOnly);

You should just pass the WebView into this method, since DRT already has a
handle on it. Please make this one statement, but split over multiple lines:

core(webView)->group().addUserScriptToWorld(mainThreadNormalWorld(),
sourceCode, KURL(), 0, 0
					    injectAtDocumentStart ?
InjectAtDocumentStart : InjectAtDocumentEnd,
					    injectInAllFrames ?
InjectInAllFrames : InjectInTopFrameOnly);

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:368
> + * removeAllUserContent
> + * @frame: a #WebKitWebFrame
> + *
> + */

Ditto on the removal.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:376
> +    Frame* coreFrame = core(frame);
> +    if (!coreFrame)
> +	   return;
> +

You don't use coreFrame here. No need to cache it.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:380
> +    WebKitWebView* webView = getViewFromFrame(frame);
> +    Page* page = core(webView);
> +    page->group().removeAllUserContent();
> +}

Again, you should pass the WebView here and just do this in one statement:
core(webView)->group().removeAllUserContent().