[Webkit-unassigned] [Bug 56132] [Gtk] Add support for userscripts tests

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=56132


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #85373|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #2 from Martin Robinson <mrobinson at webkit.org>  2011-03-14 08:56:53 PST ---
(From update of attachment 85373)
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().

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.