[webkit-reviews] review denied: [Bug 88271] Introduce WebViewBenchmarkSupport for performance experiments on a real WebView : [Attachment 151556] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied  review:
Bug 88271: Introduce WebViewBenchmarkSupport for performance experiments on a
real WebView
https://bugs.webkit.org/show_bug.cgi?id=88271

Attachment 151556: Patch
https://bugs.webkit.org/attachment.cgi?id=151556&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=151556&action=review


> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:43
> +    enum PaintMode {

do you need this enum?	do you plan on adding more values here in the future?

> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:44
> +	 // Paint the entire page.

nit: indentation

> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:47
> +    // Factory for creating canvases for paint calls, where multiple
canvases

you might also mention that the factory makes it convenient for the caller to
not have
to know/compute the size of the canvas up front.

> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:51
> +	   // Creates a web canvas, caller assumes ownership.

nit: this comment is self-evident.  you can leave it out.  the "create" prefix
implies the ownership transfer, and this function's name makes it clear that it
is creating a canvas.

>> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:52
>> +	    virtual WebCanvas* createWebCanvas(const WebSize&) const = 0;
> 
> I dont think this needs to be a const method. What if I wanted to count the
number of canvases created? Or track the area of paints? That requires that the
factory is non-const.

nit: we usually leave off the "Web" in method names that refer to types. 
createCanvas, returning WebCanvas*, would be a fine name for this method that
is consistent with existing code.

> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.h:65
> +    WebViewImpl* m_webViewImpl;

notice how WebViewBenchmarkSupportImpl has no real member variables other than
the backpointer to the WebViewImpl?  perhaps allocating it lazily does not
really
offer much benefit?  i suppose it trims the size of WebViewImpl by a DWORD
doing
it this way.  however, if you wanted to just have WebViewImpl have a
non-pointer
member variable of type WebViewBenchmarkSupportImpl that would be OK too.

> Source/WebKit/chromium/src/WebViewImpl.h:301
> +    virtual const WebViewBenchmarkSupport* benchmarkSupport();

nit: no need for the new line between this method declaration and
renderingStats.

the idea is that methods should be listed in the same order as they are
declared
in the interface being implemented.  the .cpp file should similarly list the
methods
in order.  (WebViewImpl seems to have some mistakes in this regard.)