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

[bugzilla-daemon at webkit.org]

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

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

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


> Source/WebKit/chromium/public/WebView.h:454
> +    // Benchmarking support --------------------------------------------

nit: add a new line below the section comment to be consistent with the rest
of this file.

> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:45
> +	   PaintModeEverything

it is hard to review the name of this enum without considering what the other
enum values will be.  PaintModeEverything reads a bit awkwardly.  another
option besides enums is to just have multiple entry points.  for example,
paint could be named paintEverything or paintAll.

> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:52
> +    class PaintingController {

This seems like more of a Client interface than a Controller interface.
Since the method this goes with is named "paint", I'd probably call this
interface PaintClient.

> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:56
> +	   // Called by the WebViewBenchmakrSupport when painting is about to

typo: WebViewBenchmakrSupport

I recommend using the more conventional names for these functions:
willPaint and didPaint

We use these prefixes to denote before and after events.

> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:68
> +    virtual double paint(PaintingController&, PaintMode) = 0;

nit: it is more conventional to pass callback interfaces by pointer.

  virtual double paint(PaintClient*, PaintMode) = 0;

> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:51
> +    const WebSize canvasSize = WebSize(clip.width(), clip.height());

nit: same comment about gratuitous consts and WebSize construction

> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:66
> +    const IntRect clip = WebCore::IntRect(0, 0, layerSize.width(),
layerSize.height());

nit: should be "IntRect clip(...)"

nit: isn't there a potential lossy conversion here from float to int?
Or, can we assume that layer.size() always returns integer values?

> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:92
> +    const WebSize canvasSize = WebSize(paintSize.width, paintSize.height);

nit: no need for 'const' on these local variables.  it just adds noise.

nit: instead of "WebSize foo = WebSize(...)," just do "WebSize foo(...)"

> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:95
> +    double beforeTime = monotonicallyIncreasingTime();

it occurs to me that the timing code could be implemented on the client
side.  in willPaint record the start time.  in didPaint record the stop
time.  this means less code in webkit!

> Source/WebKit/chromium/src/WebViewImpl.h:791
> +    // Lazily created controller for microbenchmarks

nit: this comment is no longer correct