Bug 88271 – Introduce WebViewBenchmarkSupport for performance experiments on a real WebView

• Component: New Bugs
• Status: RESOLVED FIXED

[Daniel Murphy]

initial rendering microbenchmark

[Daniel Murphy]

Created attachment 145662 [details]
Patch

[Nat Duca]

Comment on attachment 145662 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145662&action=review

Good first pass. Lets start gluing it all together.

> Source/WebKit/chromium/public/RenderingBenchmarkResults.h:1
> +/*

copy the style of other headers in that folder.

> Source/WebKit/chromium/public/RenderingBenchmarkResults.h:10
> +#pragma once

follow the style in the rest of the public/ folder

> Source/WebKit/chromium/public/RenderingBenchmarkResults.h:12
> +class RenderingBenchmarkResults {

We put "web" in the front of public wk apis. See other things in the folder, e.g. WebCompositorInputHandler.h

> Source/WebKit/chromium/public/RenderingBenchmarkResults.h:15
> +    virtual void addResult(const char *resultName, double result) = 0;

A const char* for the unit is probably also a good thing.

> Source/WebKit/chromium/public/WebView.h:486
> +    // Rendering benchmarks ------------------------------------------------------

Lets put this entry point with the 'GPU acceleration support' group of methods, e.g. just above visibility after transferActiveWheelBlahblah

Take a peek through this file and see if we typically pass by pointer. Pass-by-ref seems to be used only for pass-by-reference, though I might be missing something.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1614
> +void WebViewImpl::runRenderingBenchmarks(RenderingBenchmarkResults& results)

Lets get this factored into a class like we described...

RenderingBenchmark {
  const char* name() const = 0;
  void setUp(WebViewImpl* webViewImpl) {}
  void run(RenderingBenchmarkResults& results, WebViewImpl* webViewImpl) = 0;
  void tearDown(WebViewImpl* webViewImpl) { }
}

Then have another class called

AllRenderingBenchmarks : RenderingBenchmark {
   AllRenderingBenchmarks() {
      push instances benchmark subclasses of interest into m_benchmarks;
   }
   run() { run each of the instances; }
};
This runRenderingBenchamrks function should just construct a benchmark and run it. Ideally the test code will be in a bunch of classes in a cpp file --- the only thing exported by the benchmark.h would be the AllRenderingBenchmarks.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1623
> +    this->paint(&canvas, screenRect);

This works if you're in software mode. You need to handle accelerated mode as well. That is, you need to get the root graphics layer and iterate over each graphics layer painting it.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1625
> +    results.addResult("paint to bitmap time", afterPaint - beforePaint);

methinks the benchmark names should be camelcased words. E.g. paintTime

> Source/WebKit/chromium/src/WebViewImpl.cpp:1632
> +    results.addResult("paint to null canvas time", afterPaint - beforePaint);

paintTime_NullCanvas

[Daniel Murphy]

Created attachment 146072 [details]
Patch

[Daniel Murphy]

This patch separates the benchmarks into the AllRenderingBenchmarks.cpp file.  A sister chromium patch will be uploaded this afternoon with the hookup from the benchmarking extension.

Next, I'll be throwing an interface in between the WebViewImpl and the benchmarks, and expose the m_rootGraphicsLayer in that interface so we can simulate the accelerated graphics.

[Daniel Murphy]

chromium sister patch: https://chromiumcodereview.appspot.com/10537036/

[Daniel Murphy]

Created attachment 147618 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

FYI, we don't use std::auto_ptr in WebKit (or chromium) code.

[Daniel Murphy]

For reviewers who are not Nat:
Sorry, this patch wasn't meant to be reviewed by people yet, I specified '--no-review' when I uploaded but maybe it didn't take?


This patch includes the 'controller' interface for the benchmarks, and has benchmarking for the accelerated mode.

Things I was unsure of:
* I have auto_ptr's in here for exception-safe memory management.  I tried using the RefPtr, but that required deriving from a base, which was infeasible for WebCanvas (SkCanvas) and SkDevice.
* I use 0,0,width,height to draw the entire layer, and I'm not sure if there's an offset I should be using.  Shouldn't matter after next iteration.


Still To-Do:
* Find the correct clip rectangles for each layer instead of drawing them entirely.
* Perhaps move everything into 'content', because we already have the 'controller' abstraction that we can leave in WebKit

[James Robinson]

We don't use C++ exceptions either, so don't worry about exception safety :)

[Daniel Murphy]

Ok cool, I'll remove those then

[Nat Duca]

Comment on attachment 147618 [details]
Patch

Hey Daniel - can you take a crack at making the controller a public interface on webkit, and moving the actual AllBenchmarks to chrome's content/renderer folder, perthaps alongside teh benchmarking extension? I think Darin was onto something here. :)

Based on some of the discussion you heard in today's meeting, I think it will be useful to expose basic primitives like "paint the page and use this functor to create canvas objects." That would allow us to use the benchmark controller to do both microbenchmarking and this record thing.

[Daniel Murphy]

Created attachment 148609 [details]
Patch

[Nat Duca]

Comment on attachment 148609 [details]
Patch

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.

[James Robinson]

Comment on attachment 148609 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148609&action=review

>> 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.

http://trac.webkit.org/wiki/ChromiumWebKitAPI has a section on enums

[Daniel Murphy]

(In reply to comment #14)
> > 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.

That's fine as long as the controller is stateless.  Otherwise, it needs to be a factory.  Do you want to impose the stateless constraint on the controller, or the ownership constraint on the WebView?

> 
> > 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. :)

I'm using it, I have a benchmark that just calls layout.  I can remove that.

> 
> > 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?

Obviously, yes, I just couldn't find a doubly-linked list in WTF and I need to prepend for breadth-first-search (which makes applying layer transformations easier).  I just talked to Julian and he pointed me towards Deque, which looks like it doesn't copy on prepend

[Nat Duca]

I think you're overthinking the stateless thing. I think that the functions should do what they say they're going to do regardless of what was called on them in the past. So, for example, it is good that layout NOT be on the interface and that it internally do layout for you. Anyway, I'm not clear how you "impose a stateless constraint"?

Wrt deque, you could also write a recursive function. :)

[Daniel Murphy]

(In reply to comment #17)
> I think you're overthinking the stateless thing. I think that the functions should do what they say they're going to do regardless of what was called on them in the past. So, for example, it is good that layout NOT be on the interface and that it internally do layout for you. Anyway, I'm not clear how you "impose a stateless constraint"?
> 
> Wrt deque, you could also write a recursive function. :)

By stateless I meant there wouldn't be conflicts if two people were using the same controller.  I think it's better overall that the controller be stateless.  So, WebView owning it is cool.

I might use recursion if it makes the code simpler

[Daniel Murphy]

Created attachment 150035 [details]
Patch

[Daniel Murphy]

Fixes for commented issues

[Nat Duca]

This LGTM mod time->measure. JamesR for review.

[Daniel Murphy]

Created attachment 150496 [details]
Patch

[Daniel Murphy]

Fixed time->measure, cleanup up accelerated check in the benchmark impl.

[WebKit Review Bot]

Attachment 150496 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1
Source/WebKit/chromium/ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Murphy]

Created attachment 150497 [details]
Patch

[Daniel Murphy]

Fixed tab character in ChangeLog file

[James Robinson]

Comment on attachment 150496 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150496&action=review

I've left some feedback that is hopefully helpful.  Unfortunately I'm heading out now and will be gone for a few weeks, so I suggest you find another public API reviewer to work with in order to get this in landable shape.

> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:36
> +#include "platform/WebSize.h"

If the only WebSize use is a const reference then you could just use a forward declaration. Alternately, since WebSize is two ints, passing by value is probably fine.

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

I don't see what about this class is a 'controller'. When seeing a WebRenderingBenchmarkController, I expect to see a WebRenderingBenchmark type somewhere that this controls. Perhaps this could have a shorter, more direct name?

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

This enum is mis-named, see the section on enums in http://trac.webkit.org/wiki/ChromiumWebKitAPI

> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:49
> +    class CanvasFactory {

this needs some explanation of what it's doing and what the expectations are of the implementation. a factory pattern is fairly unusual in the WebKit public API

> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:51
> +        virtual ~CanvasFactory() { }

should this be public? i.e. does the WebRenderingBenchmarkController take ownership of the factory, or should that be hidden from the controller? (I suspect the answer is the latter, which means this should probably be protected).

> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:56
> +    virtual ~WebRenderingBenchmarkController() { }

Should this be public (i.e. should someone be able to delete a WebRenderingBenchmarkController via a WebRenderingBenchmarkController* if they have no knowledge of the underlying type)? the answer is usually no

> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:58
> +    // Returns the time in seconds to paint on given canvas/es.

The comment is a bit misleading since this function isn't actually given a canvas.

> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:59
> +    virtual double measurePaintTime(const CanvasFactory&, const PaintMode) const = 0;

No point in having 'const' on by-value parameters like an enum, it's copied anyway.

> Source/WebKit/chromium/public/WebView.h:466
> +    // Rending Benchmark support --------------------------------------------

typo - rendering ?

> Source/WebKit/chromium/public/WebView.h:469
> +      return 0;

4-space indentation, or just put this on one line

> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:8
> + *     * Redistributions of source code must retain the above copyright

we prefer 2-clause license for new files

> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:38
> +#include "public/WebCanvas.h"

<public/WebCanvas.h>, please

> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:48
> +using WTF::OwnPtr;
> +using WTF::Vector;

You don't need this, OwnPtr.h / Vector.h have using declarations that put OwnPtr/Vector in the global namespace

> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:52
> +#if USE(ACCELERATED_COMPOSITING)

Chromium always sets USE(ACCELERATED_COMPOSITING), there's no point in guarding stuff in /chromium/ paths with this guard

> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:56
> +    const OwnPtr<WebCanvas> canvas = WTF::adoptPtr(canvasFactory.createWebCanvas(canvasSize));

no WTF::

> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:61
> +    return monotonicallyIncreasingTime() - beforeTime;

are you trying to isolate the time spent painting from the time spent creating/destroying the canvases? This might accumulate a lot of error if you have many small layers with cheap paint times

> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:66
> +    // TODO(dmurph): go through all the layers, using the internal WebLayer

We don't use TODO(name) in WebKit, just FIXME - no name.  Or even better, file a bug and link to it.  Or even better better, don't land this code until it works.  Code in the tree is code everybody working on the project has to worry about, so there's not much use in landing code that doesn't work yet.

> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:75
> +    const FloatSize layerSize = layer.size();

we don't typically use const for locals like this. If the function is big and long and complicated having const on locals can help you figure out invariants, but a better solution to that is often to just make the function less big + long + complicated. I'd just leave the const out here.

> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:104
> +    default:

better to leave out the default: completely and have the compiler check that you have a case: for every possible value enum value

> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:110
> +    const OwnPtr<WebCanvas> canvas = WTF::adoptPtr(canvasFactory.createWebCanvas(canvasSize));

no WTF::

> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:131
> +    default:

same here re: default

> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:35
> +#include "GraphicsLayer.h"
> +#include "IntRect.h"

Forward declarations of these types would suffice

> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:42
> +    WebRenderingBenchmarkControllerImpl(WebViewImpl* webViewImpl) : m_webViewImpl(webViewImpl) { }

explicit on one-arg constructors

> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:53
> +    // returns the time in seconds

please start your comments with an uppercase letter and end them with a period. for bonus points, have comments be completely sentences.

> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:54
> +    double measureLayerPaint(const CanvasFactory&, WebCore::GraphicsLayer&, const WebCore::IntRect&) const;

why is GraphicsLayer being passed by non-const reference? is it an out parameter?

> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:55
> +    // returns the time in seconds. Currently unimplemented.

why land it if it's unimplemented? probably better to just not land it until it works

> Source/WebKit/chromium/src/WebViewImpl.cpp:749
> +    if (!m_renderingBenchmarkController.get())

all WTF smart pointers provide operator bool() overrides that do the right thing, so this check should just be written:

if (!m_renderingBenchmarkController)

> Source/WebKit/chromium/src/WebViewImpl.h:300
> +    virtual const WebRenderingBenchmarkController* renderingBenchmarkController();

we don't typically use const for this

[Nat Duca]

> > Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:55
> > +    // returns the time in seconds. Currently unimplemented.
> 
> why land it if it's unimplemented? probably better to just not land it until it works

Because we need the infrastructure for things before we need the viewport version. I think this is acceptable.

[Daniel Murphy]

Created attachment 151547 [details]
Patch

[Daniel Murphy]

Comment on attachment 150496 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150496&action=review

>> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:41
>> +class WebRenderingBenchmarkController {
> 
> I don't see what about this class is a 'controller'. When seeing a WebRenderingBenchmarkController, I expect to see a WebRenderingBenchmark type somewhere that this controls. Perhaps this could have a shorter, more direct name?

Renamed to WebViewBenchmarkSupport

>> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:43
>> +    enum PaintMode {
> 
> This enum is mis-named, see the section on enums in http://trac.webkit.org/wiki/ChromiumWebKitAPI

I think you might not be seeing the most recent patch?  This follows the guide.

>> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:49
>> +    class CanvasFactory {
> 
> this needs some explanation of what it's doing and what the expectations are of the implementation. a factory pattern is fairly unusual in the WebKit public API

Done.

>> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:51
>> +        virtual ~CanvasFactory() { }
> 
> should this be public? i.e. does the WebRenderingBenchmarkController take ownership of the factory, or should that be hidden from the controller? (I suspect the answer is the latter, which means this should probably be protected).

I haven't seen that pattern before, pretty cool.  Done.

>> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:56
>> +    virtual ~WebRenderingBenchmarkController() { }
> 
> Should this be public (i.e. should someone be able to delete a WebRenderingBenchmarkController via a WebRenderingBenchmarkController* if they have no knowledge of the underlying type)? the answer is usually no

Done.

>> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:58
>> +    // Returns the time in seconds to paint on given canvas/es.
> 
> The comment is a bit misleading since this function isn't actually given a canvas.

Done.

>> Source/WebKit/chromium/public/WebRenderingBenchmarkController.h:59
>> +    virtual double measurePaintTime(const CanvasFactory&, const PaintMode) const = 0;
> 
> No point in having 'const' on by-value parameters like an enum, it's copied anyway.

Done.

>> Source/WebKit/chromium/public/WebView.h:466
>> +    // Rending Benchmark support --------------------------------------------
> 
> typo - rendering ?

Done.

>> Source/WebKit/chromium/public/WebView.h:469
>> +      return 0;
> 
> 4-space indentation, or just put this on one line

Done.

>> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:8
>> + *     * Redistributions of source code must retain the above copyright
> 
> we prefer 2-clause license for new files

Done.

>> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:38
>> +#include "public/WebCanvas.h"
> 
> <public/WebCanvas.h>, please

Done.

>> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:48
>> +using WTF::Vector;
> 
> You don't need this, OwnPtr.h / Vector.h have using declarations that put OwnPtr/Vector in the global namespace

Done.

>> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:52
>> +#if USE(ACCELERATED_COMPOSITING)
> 
> Chromium always sets USE(ACCELERATED_COMPOSITING), there's no point in guarding stuff in /chromium/ paths with this guard

Done.

>> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:56
>> +    const OwnPtr<WebCanvas> canvas = WTF::adoptPtr(canvasFactory.createWebCanvas(canvasSize));
> 
> no WTF::

Done.

>> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:61
>> +    return monotonicallyIncreasingTime() - beforeTime;
> 
> are you trying to isolate the time spent painting from the time spent creating/destroying the canvases? This might accumulate a lot of error if you have many small layers with cheap paint times

This is intentional.  Perhaps in the future we'll measure the creation as well, but for now, it's just the paint.

>> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:66
>> +    // TODO(dmurph): go through all the layers, using the internal WebLayer
> 
> We don't use TODO(name) in WebKit, just FIXME - no name.  Or even better, file a bug and link to it.  Or even better better, don't land this code until it works.  Code in the tree is code everybody working on the project has to worry about, so there's not much use in landing code that doesn't work yet.

I agree, I think I'll remove the enumeration value then (and these methods), but keep the switch statement architecture for the next patch

>> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:75
>> +    const FloatSize layerSize = layer.size();
> 
> we don't typically use const for locals like this. If the function is big and long and complicated having const on locals can help you figure out invariants, but a better solution to that is often to just make the function less big + long + complicated. I'd just leave the const out here.

Done.

>> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:104
>> +    default:
> 
> better to leave out the default: completely and have the compiler check that you have a case: for every possible value enum value

Done.

>> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:110
>> +    const OwnPtr<WebCanvas> canvas = WTF::adoptPtr(canvasFactory.createWebCanvas(canvasSize));
> 
> no WTF::

Done.

>> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.cpp:131
>> +    default:
> 
> same here re: default

Done.

>> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:35
>> +#include "IntRect.h"
> 
> Forward declarations of these types would suffice

Done.

>> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:42
>> +    WebRenderingBenchmarkControllerImpl(WebViewImpl* webViewImpl) : m_webViewImpl(webViewImpl) { }
> 
> explicit on one-arg constructors

Done.

>> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:53
>> +    // returns the time in seconds
> 
> please start your comments with an uppercase letter and end them with a period. for bonus points, have comments be completely sentences.

Done.

>> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:54
>> +    double measureLayerPaint(const CanvasFactory&, WebCore::GraphicsLayer&, const WebCore::IntRect&) const;
> 
> why is GraphicsLayer being passed by non-const reference? is it an out parameter?

No, but the paint call is non-const

>> Source/WebKit/chromium/src/WebRenderingBenchmarkControllerImpl.h:55
>> +    // returns the time in seconds. Currently unimplemented.
> 
> why land it if it's unimplemented? probably better to just not land it until it works

I agree, removed.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:749
>> +    if (!m_renderingBenchmarkController.get())
> 
> all WTF smart pointers provide operator bool() overrides that do the right thing, so this check should just be written:
> 
> if (!m_renderingBenchmarkController)

Done.

>> Source/WebKit/chromium/src/WebViewImpl.h:300
>> +    virtual const WebRenderingBenchmarkController* renderingBenchmarkController();
> 
> we don't typically use const for this

const here enforces that WebRenderingBenchmarkController can be safely accessed by multiple people.  The controller should not be able to be changed by the caller.

[WebKit Review Bot]

Comment on attachment 151547 [details]
Patch

Attachment 151547 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13203131

[Daniel Murphy]

Created attachment 151556 [details]
Patch

[Nat Duca]

Comment on attachment 151556 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151556&action=review

Lets see what @fishd thinks.

> Source/WebKit/chromium/ChangeLog:3
> +        initial microbenchmark web view controller

Lets update this comment to be a bit more clear. E.g. "Introduce WebViewBenchmarkSupport as a way to do performance experiments on a real WebView"

> Source/WebKit/chromium/public/WebView.h:466
> +    // Benchmark support --------------------------------------------

// Benchmarking ----
^^ instead of benchmark support?

> 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.

> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:59
> +    virtual double measurePaintTime(const CanvasFactory&, PaintMode) const = 0;

Does this need to be passed by const? I think thats unnecessarily restrictive.

> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:101
> +#if USE(ACCELERATED_COMPOSITING)

Did we decide to nix this #if?

[Nat Duca]

Comment on attachment 151556 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151556&action=review

> Source/WebKit/chromium/public/WebView.h:467
> +    virtual const WebViewBenchmarkSupport* benchmarkSupport() { return 0; }

This shouldn't return a const WVBS*. It should be straight WebViewBenchmarkSupport. Sorry, I dont buy your previous comment about enforcing anything.

[Darin Fisher (:fishd, Google)]

Comment on attachment 151556 [details]
Patch

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.)

[Daniel Murphy]

Created attachment 152123 [details]
Patch

[Daniel Murphy]

Comment on attachment 151556 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151556&action=review

>> Source/WebKit/chromium/public/WebView.h:466
>> +    // Benchmark support --------------------------------------------
> 
> // Benchmarking ----
> ^^ instead of benchmark support?

Done.

>> Source/WebKit/chromium/public/WebView.h:467
>> +    virtual const WebViewBenchmarkSupport* benchmarkSupport() { return 0; }
> 
> This shouldn't return a const WVBS*. It should be straight WebViewBenchmarkSupport. Sorry, I dont buy your previous comment about enforcing anything.

Done.

>> 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?

We're adding another paint mode in the next cl

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

Done.

>> 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.

Done.  Also, renamed to CanvasController, as it will also have observer methods for when painting is about to occur and when painting is complete for a web canvas.

>> 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.

Ok

>>> 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.

Done, Done.

>> Source/WebKit/chromium/public/WebViewBenchmarkSupport.h:59
>> +    virtual double measurePaintTime(const CanvasFactory&, PaintMode) const = 0;
> 
> Does this need to be passed by const? I think thats unnecessarily restrictive.

Done.

>> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:101
>> +#if USE(ACCELERATED_COMPOSITING)
> 
> Did we decide to nix this #if?

Whoops, missed that one.  fixed.

>> 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.

Good idea, done.

>> 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.)

Done.

[Daniel Murphy]

So I forgot to call the painting report methods, I'll upload another patch tomorrow morning with those four lines added in WebViewBenchmarkSupportImpl, my bad.

[Daniel Murphy]

Created attachment 152278 [details]
Patch

[Alok Priyadarshi]

Comment on attachment 152278 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152278&action=review

> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:77
> +double WebViewBenchmarkSupportImpl::measureSoftwarePaintTime(PaintingController& paintingController, const PaintMode paintMode)

no need for "const" PaintMode.

> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:103
> +double WebViewBenchmarkSupportImpl::measurePaintTime(PaintingController& paintingController, const PaintMode paintMode)

same here

[Daniel Murphy]

Comment on attachment 152278 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152278&action=review

>> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:77
>> +double WebViewBenchmarkSupportImpl::measureSoftwarePaintTime(PaintingController& paintingController, const PaintMode paintMode)
> 
> no need for "const" PaintMode.

Done.

>> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:103
>> +double WebViewBenchmarkSupportImpl::measurePaintTime(PaintingController& paintingController, const PaintMode paintMode)
> 
> same here

Done.

[Daniel Murphy]

Created attachment 152563 [details]
Patch

[Daniel Murphy]

Created attachment 152645 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 152645 [details]
Patch

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

[Daniel Murphy]

Comment on attachment 152645 [details]
Patch

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.

Done.

>> 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.

The next enum will be PaintModeViewport.  I'd rather use enums, and they make it cleaner on the implementation side as well.

>> 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.

Done.

>> 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.

Done.

>> 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;

Done.

>> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:51
>> +    const WebSize canvasSize = WebSize(clip.width(), clip.height());
> 
> nit: same comment about gratuitous consts and WebSize construction

Done.

>> 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?

Done, I think we can assume layer.size() will always return integer values, I don't think there's another choice here either, we can only paint IntRects

>> 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(...)"

Done.

>> 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!

True, but we might want to add different time measurment modes in the future, James mentioned we might want to include the canvas creation at some point.  Nat, would we ever want that?  Or can we move the timing out of WebKit.

>> Source/WebKit/chromium/src/WebViewImpl.h:791
>> +    // Lazily created controller for microbenchmarks
> 
> nit: this comment is no longer correct

Done.

[Nat Duca]

(In reply to comment #45)
> >> 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.
> 
> The next enum will be PaintModeViewport.  I'd rather use enums, and they make it cleaner on the implementation side as well.

This has come up a few times in the reivew. We had the enum fully filled out before but then james asked us to remove it because we weren't using it. Changing function arguments in the webkit api is very time consuming, so this was the compromise I was trying to have us make.

[Daniel Murphy]

Created attachment 152900 [details]
Patch

[Daniel Murphy]

Removed timings

[Darin Fisher (:fishd, Google)]

Comment on attachment 152900 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152900&action=review

> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:43
> +using WebCore::FloatSize;

nit: I recommend a 'using namespace WebCore' here.

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

nit: IntRect clip(...)

nit: no need for WebCore:: in front of IntRect due to the 'using WebCore::IntRect' statement.

> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.h:57
> +    void paintLayer(PaintClient*, WebCore::GraphicsLayer&, const WebCore::IntRect&);

nit: insert new line between function and the next comment block.

I think it would be helpful to name the last parameter "clip".  then you could
probably delete the comment for this function as it would be fairly redundant
with the function signature.

> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.h:61
> +    // Paints the WebViewImpl using software rendering, with the given paint mode

nit: this comment is fairly redundant with the method signature.

[Daniel Murphy]

Created attachment 153054 [details]
Patch

[Daniel Murphy]

Comment on attachment 152900 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152900&action=review

>> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:43
>> +using WebCore::FloatSize;
> 
> nit: I recommend a 'using namespace WebCore' here.

Done.

>> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:63
>> +    IntRect clip = WebCore::IntRect(0, 0, layerSize.width(), layerSize.height());
> 
> nit: IntRect clip(...)
> 
> nit: no need for WebCore:: in front of IntRect due to the 'using WebCore::IntRect' statement.

Done.

>> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.h:57
>> +    void paintLayer(PaintClient*, WebCore::GraphicsLayer&, const WebCore::IntRect&);
> 
> nit: insert new line between function and the next comment block.
> 
> I think it would be helpful to name the last parameter "clip".  then you could
> probably delete the comment for this function as it would be fairly redundant
> with the function signature.

Done.

>> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.h:61
>> +    // Paints the WebViewImpl using software rendering, with the given paint mode
> 
> nit: this comment is fairly redundant with the method signature.

Done.

[Darin Fisher (:fishd, Google)]

Comment on attachment 153054 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153054&action=review

> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:76
> +    case PaintModeEverything: {

nit: no {}'s for this case.  you aren't declaring a variable.

> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:101
> +        acceleratedPaintUnclipped(paintClient, *layer);

nit: add a "break;" at the end of this case, so you don't have to remember to add one later
when you add the other cases.

[Daniel Murphy]

Created attachment 153561 [details]
Patch

[Daniel Murphy]

Created attachment 153570 [details]
Patch for commit

[WebKit Review Bot]

Comment on attachment 153570 [details]
Patch for commit

Clearing flags on attachment: 153570

Committed r123254: <http://trac.webkit.org/changeset/123254>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alok Priyadarshi]

Comment on attachment 153570 [details]
Patch for commit

View in context: https://bugs.webkit.org/attachment.cgi?id=153570&action=review

> Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:50
> +    OwnPtr<WebCanvas> canvas = adoptPtr(paintClient->createCanvas(canvasSize));

This will not work with a recording canvas. It cannot be destroyed here. PaintClient should be responsible for destroying the canvas. It is also better design to co-locate create and destroy. I will upload another patch.

[Alok Priyadarshi]

Has this been tested on acclerated-composited content? I am hitting this assert with poster-circle example:

http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderLayerBacking.cpp#L1217

poster-circle example: http://www.webkit.org/blog-files/3d-transforms/poster-circle.html

[Daniel Murphy]

Interesting... I've been running with the compositor always on, I didn't test with that specific page though.

This looks like it will require a bit of discussion with people working on the compositor

[Daniel Murphy]

Tested, and I don't hit that assertion when I call paint for that page.

[Alok Priyadarshi]

(In reply to comment #60)
> Tested, and I don't hit that assertion when I call paint for that page.

Hmm. Does not seem like the assert would depend on the type of canvas. Anyways I will ask you to test my patch once it is landed.

[Nat Duca]

(In reply to comment #57)
> (From update of attachment 153570 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153570&action=review
> 
> > Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:50
> > +    OwnPtr<WebCanvas> canvas = adoptPtr(paintClient->createCanvas(canvasSize));
> 
> This will not work with a recording canvas. It cannot be destroyed here. PaintClient should be responsible for destroying the canvas. It is also better design to co-locate create and destroy. I will upload another patch.

I think you should be precise here... it will work for one use case. Its just that it wont work if you had one-layer-per-canvas canvas to the file that you wanted to start with.

If you write the picture after every didPaint, then you'd be fine, correct?

[Alok Priyadarshi]

(In reply to comment #62)
> (In reply to comment #57)
> > (From update of attachment 153570 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=153570&action=review
> > 
> > > Source/WebKit/chromium/src/WebViewBenchmarkSupportImpl.cpp:50
> > > +    OwnPtr<WebCanvas> canvas = adoptPtr(paintClient->createCanvas(canvasSize));
> > 
> > This will not work with a recording canvas. It cannot be destroyed here. PaintClient should be responsible for destroying the canvas. It is also better design to co-locate create and destroy. I will upload another patch.
> 
> I think you should be precise here... it will work for one use case. Its just that it wont work if you had one-layer-per-canvas canvas to the file that you wanted to start with.
> 
> If you write the picture after every didPaint, then you'd be fine, correct?

No. It is not the issue of writing after every paint. Just ownership. The canvas returned by SkPicture is owned by SkPicture, and cannot be destroyed by anybody else.

I have filed https://bugs.webkit.org/show_bug.cgi?id=92008. Will upload a patch as soon as prepare-changelogs cooperates.

[Nat Duca]

Ah, that clarifies things. Thanks!