Bug 92008 – [chromium] WebViewBenchmarkSupport::paint API does not work with recording canvas

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Alok Priyadarshi]

WebViewBenchmarkSupport owns the canvas created by PaintClient. This does not work in the case of a recording canvas, which is owned by SkPicture.

[Alok Priyadarshi]

Created attachment 153866 [details]
proposed 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.

[Nat Duca]

Comment on attachment 153866 [details]
proposed patch

Dumb quesiton --- why not just not doing the ownptr thing and clarifying on the comment that the client must ensure the created canvases get deleted? E.g. no destroyCanvas call

[Alok Priyadarshi]

(In reply to comment #3)
> (From update of attachment 153866 [details])
> Dumb quesiton --- why not just not doing the ownptr thing and clarifying on the comment that the client must ensure the created canvases get deleted? E.g. no destroyCanvas call

Yeah - if we clarify that the canvas can be disposed of after didPaint call. I thought being explicit would be better.

Darin: What do you think?

[Daniel Murphy]

So the issue is that it wasn't destructing?  I'm doing the following:

SkDevice* device = new SkDevice(SkBitmap::kARGB_8888_Config,
                                size.width,
                                size.height,
                                false);
WebCanvas* canvas = new WebCanvas(device);
device->unref();
return canvas;

for creating a canvas, which bascically matches the SkCanvas constructor for creating a canvas using a bitmap

[Daniel Murphy]

Looking at SkPicture, this makes sense.  Alok suggested having willPaint return a canvas, which sounds sounds good.

[Darin Fisher (:fishd, Google)]

(In reply to comment #6)
> Looking at SkPicture, this makes sense.  Alok suggested having willPaint return a canvas, which sounds sounds good.

I like the idea of having willPaint allocate the WebCanvas and didPaint destroy the canvas.

  WebCanvas* willPaint(const WebSize&) = 0;
  void didPaint(WebCanvas*) = 0;

Then, we can just drop the create/destroyCanvas methods.  Fewer methods == more better.

[Alok Priyadarshi]

Created attachment 154096 [details]
proposed patch

Got rid of create/destroyCanvas. Modified will/didPaint interface and added comments.

[Nat Duca]

Comment on attachment 154096 [details]
proposed patch

I like this. @fishd?

[WebKit Review Bot]

Comment on attachment 154096 [details]
proposed patch

Clearing flags on attachment: 154096

Committed r123645: <http://trac.webkit.org/changeset/123645>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.