Bug 42694

Summary: WebKitTestRunner needs layoutTestController.numberOfPages
Product: WebKit Reporter: Sam Weinig <sam>
Component: WebKit2Assignee: Chang Shu <cshu>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, cshu, darin
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
fix patch
darin: review-
fix patch 2 none

Description Sam Weinig 2010-07-20 15:16:24 PDT
WebKitTestRunner needs layoutTestController.numberOfPages
Comment 1 Sam Weinig 2010-07-20 15:31:22 PDT
<rdar://problem/8213844>
Comment 2 Chang Shu 2011-04-05 08:27:06 PDT
Created attachment 88239 [details]
fix patch
Comment 3 Darin Adler 2011-04-05 09:28:10 PDT
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.
Comment 4 Chang Shu 2011-04-05 10:48:31 PDT
> 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.
Comment 5 Chang Shu 2011-04-05 11:38:26 PDT
Created attachment 88288 [details]
fix patch 2
Comment 6 WebKit Commit Bot 2011-04-05 20:05:20 PDT
Comment on attachment 88288 [details]
fix patch 2

Clearing flags on attachment: 88288

Committed r83010: <http://trac.webkit.org/changeset/83010>
Comment 7 WebKit Commit Bot 2011-04-05 20:05:26 PDT
All reviewed patches have been landed.  Closing bug.