Bug 42694 – WebKitTestRunner needs layoutTestController.numberOfPages

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Sam Weinig]

WebKitTestRunner needs layoutTestController.numberOfPages

[Sam Weinig]

<rdar://problem/8213844>

[Chang Shu]

Created attachment 88239 [details]
fix patch

[Darin Adler]

Comment on attachment 88239 [details]
fix patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88239&action=review

Good change, but I am not happy with how the WebKit2 changes are structured.

> Source/WebKit2/WebProcess/WebCoreSupport/WebKitTestRunnerSupport.cpp:44
> +int WebKitTestRunnerSupport::numberOfPages(WebCore::Frame* frame, float width, float height)
> +{
> +    if (!frame)
> +        return -1;
> +    if (!width)
> +        width = frame->view()->width();
> +    if (!height)
> +        height = frame->view()->height();
> +
> +    return PrintContext::numberOfPages(frame, FloatSize(width, height));
> +}

I don’t think this function needs its own file. This code can just go inside InjectedBundle::numberOfPages. While I understand that we might want to put testing functions somewhere different than functions for other purposes, I would not call that file WebKitTestRunnerSupport, nor would I make a new class.

I could imagine making a new source file InjectedBundleTestFunctions.cpp or InjectedBundleTesting.cpp and put some of the InjectedBundle function definitions in there to segregate them from the rest of the class. But this factoring seems to go too far in creating a new class just to hold some functions.

> Source/WebKit2/WebProcess/WebCoreSupport/WebKitTestRunnerSupport.h:40
> +class WebKitTestRunnerSupport {
> +
> +public:
> +
> +    WebKitTestRunnerSupport();
> +    ~WebKitTestRunnerSupport();
> +
> +    static int numberOfPages(WebCore::Frame*, float width, float height);
> +};

To me it does not make sense to have this be a class. You don’t need a class just to hold some functions.

[Chang Shu]

> I don’t think this function needs its own file. This code can just go inside InjectedBundle::numberOfPages. While I understand that we might want to put testing functions somewhere different than functions for other purposes, I would not call that file WebKitTestRunnerSupport, nor would I make a new class.
> 
> I could imagine making a new source file InjectedBundleTestFunctions.cpp or InjectedBundleTesting.cpp and put some of the InjectedBundle function definitions in there to segregate them from the rest of the class. But this factoring seems to go too far in creating a new class just to hold some functions.
> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebKitTestRunnerSupport.h:40
> > +class WebKitTestRunnerSupport {
> > +
> > +public:
> > +
> > +    WebKitTestRunnerSupport();
> > +    ~WebKitTestRunnerSupport();
> > +
> > +    static int numberOfPages(WebCore::Frame*, float width, float height);
> > +};
> 
> To me it does not make sense to have this be a class. You don’t need a class just to hold some functions.

I was mimicking the DumpRenderTreeSupport of WebKit 1. But you are right, as we have InjectedBundle.cpp, we don't need a new file/class. New patch is coming.

[Chang Shu]

Created attachment 88288 [details]
fix patch 2

[WebKit Commit Bot]

Comment on attachment 88288 [details]
fix patch 2

Clearing flags on attachment: 88288

Committed r83010: <http://trac.webkit.org/changeset/83010>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.