[Webkit-unassigned] [Bug 88271] initial rendering microbenchmark

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=88271





--- Comment #14 from Nat Duca <nduca at chromium.org>  2012-06-25 11:46:04 PST ---
(From update of attachment 148609)
View in context: https://bugs.webkit.org/attachment.cgi?id=148609&action=review

> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:41
> +// Interface between the rendering benchmarks and the WebViewImpl.

and the WebView.

> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:44
> +    enum PaintMode {

check webkit style guide on enums. barring anything there, grep for enums. I think we use different style.

> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:48
> +    class WebCanvasFactory {

I think you can call this CanvasFactory since its inner.

> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:54
> +    WebRenderingBenchmarkController() { }

Why have a public constructor? Seems like you dont ened a constructor at all, or if you do, it should be protected.

> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:58
> +    virtual double timePaint(WebCanvasFactory*, const PaintMode) = 0;

Lets come up with a better name. measurePaintTIme

Does this need layout called beforehand or does it do it automatically?

You need much better docs explaining what this does.

Also, I think you can pass a reference to the factory rather than a pointer. That clarifies ownership.

> Source/WebKit/chromium/public/WebView.h:459
> +    virtual WebRenderingBenchmarkController* createRenderingBenchmarkController()

Caller assumes ownership? Why?

I think WebViewImpl should ownptr<renderingBenchmarkConttroller> (lazily create it) and this can just be an accessor.

> Source/WebKit/chromium/src/WebViewImplRenderingBenchmarkController.cpp:1
> +/*

This should be called xxximpl. See for example webcompositorinputhandler and webcompositorinputhandlerimpl

Eg WebRenderingBenchmarkControllerImpl

> Source/WebKit/chromium/src/WebViewImplRenderingBenchmarkController.cpp:48
> +double WebViewImplRenderingBenchmarkController::timePaint(WebCanvasFactory* canvasFactory, const PaintMode paintMode)

The public header should document the units returned.

> Source/WebKit/chromium/src/WebViewImplRenderingBenchmarkController.cpp:50
> +    m_webViewImpl->layout();

Why expose layout if you're automatically laying out? Dont expose it if its not serving a purpose. :)

> Source/WebKit/chromium/src/WebViewImplRenderingBenchmarkController.cpp:53
> +        WebCore::GraphicsLayer* layer = m_webViewImpl->rootGraphicsLayer();

this should probably be split out to a standalone function

> Source/WebKit/chromium/src/WebViewImplRenderingBenchmarkController.cpp:61
> +            layers.prepend(layer->children());

why are you prepending? you realize that this is a vector so each prepend is super inefficient?

> Source/WebKit/chromium/src/WebViewImplRenderingBenchmarkController.cpp:67
> +            case PAINT_VIEWPORT: // TODO(dmurph): implement this

You should assert not reached in this case.

> Source/WebKit/chromium/src/WebViewImplRenderingBenchmarkController.cpp:89
> +    WebSize size = m_webViewImpl->size();

this could be another fucntion

> Source/WebKit/chromium/src/WebViewImplRenderingBenchmarkController.cpp:110
> +    WebCanvas* canvas = canvasFactory->createWebCanvas(canvasSize);

You should have
OwnPtr<WebCanvas> canvas = adoptPtr(canvasFactory->create(...));
that way you never leak

> Source/WebKit/chromium/src/WebViewImplRenderingBenchmarkController.cpp:111
> +    double beforeTime = currentTime();

you should be using monotonicallyIncreasingTime not currentTime.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.