Summary: | WebKitTestRunner needs layoutTestController.numberOfPages | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||
Component: | WebKit2 | Assignee: | 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
Sam Weinig
2010-07-20 15:16:24 PDT
Created attachment 88239 [details]
fix patch
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. > 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.
Created attachment 88288 [details]
fix patch 2
Comment on attachment 88288 [details] fix patch 2 Clearing flags on attachment: 88288 Committed r83010: <http://trac.webkit.org/changeset/83010> All reviewed patches have been landed. Closing bug. |