Bug 56132 – [Gtk] Add support for userscripts tests

• Component: Tools / Tests
• Status: RESOLVED INVALID

[Zan Dobersek]

This bug oversees adding support for testing the user script tests.

[Zan Dobersek]

Created attachment 85373 [details]
Add support for testing user script tests

This patch adds support for testing user script tests by adding methods for adding a user script to a page group and removing all the content from a page group to the DumpRenderTreeSupport class. These two methods are then used in DumpRenderTree tool for testing these tests.

This patch also resets defaults to consistent values right after the test has finished, before loading about:blank. Without resetting these values a failure occurs when running tests in userscripts directory. We should rethink if loading about:blank after every test is actually needed and whether there's a better way of gaining benefits that loading about:blank brings.

[Martin Robinson]

Comment on attachment 85373 [details]
Add support for testing user script tests

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().

[Alexander Butenko]

guys, why not to add public api for using userscripts from https://bugs.webkit.org/show_bug.cgi?id=32789 before adding this tests? This will provide usefull features + will simplify current patch.

[Zan Dobersek]

(In reply to comment #3)
> guys, why not to add public api for using userscripts from https://bugs.webkit.org/show_bug.cgi?id=32789 before adding this tests? This will provide usefull features + will simplify current patch.

I agree, I'll update the patch when that API lands.
Also setting the dependency on #32789.

[Martin Robinson]

Sad that we never landed this, but DumpRenderTree is gone now.