[webkit-reviews] review denied: [Bug 42694] WebKitTestRunner needs layoutTestController.numberOfPages : [Attachment 88239] fix patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Chang Shu <cshu at webkit.org>'s request
for review:
Bug 42694: WebKitTestRunner needs layoutTestController.numberOfPages
https://bugs.webkit.org/show_bug.cgi?id=42694

Attachment 88239: fix patch
https://bugs.webkit.org/attachment.cgi?id=88239&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.