Bug 67054 - Allow canvas backing store to be lazily allocated
Summary: Allow canvas backing store to be lazily allocated
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Matthew Delaney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-08-26 13:02 PDT by Matthew Delaney
Modified: 2011-10-04 11:40 PDT (History)
2 users (show)

See Also:


Attachments
Patch (2.35 KB, patch)
2011-10-03 15:25 PDT, Matthew Delaney
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matthew Delaney 2011-08-26 13:02:59 PDT
I noticed that the lazy construction of a canvas (i.e. lazy allocation of backing store, etc.) is thwarted by setting the lineWidth in the CRC2D ctor. This was added in https://bugs.webkit.org/show_bug.cgi?id=26187 to rectify the issue that skia and cairo graphics contexts start with linewidth equal to 0 and 2 respectively where canvas needs them to be 1.

I imagine there's a better solution here that allows us to continue to have lazy construction/allocation for canvas. In particular, the case that sucks is when a page creates a bunch of canvases (e.g. pages that use cufonts), then get their 2d contexts, then resize the canvases, then use them. This has each canvas getting a 300x150 backing store created then throw right away once the width/height are changed.
Comment 1 Radar WebKit Bug Importer 2011-08-26 13:03:54 PDT
<rdar://problem/10031538>
Comment 2 Matthew Delaney 2011-10-03 15:25:54 PDT
Created attachment 109540 [details]
Patch
Comment 3 Matthew Delaney 2011-10-03 15:30:07 PDT
For the record, setting lineWidth() ends up grabbing the graphics context of the ImageBuffer to set the line width on. Grabbing the graphics context requires creation of the ImageBuffer.

Instead, I added the line width setter call to createImageBuffer alongside the other setter calls for other default settings. This ensures the platform graphics context gets the correct default value (for all platforms) and removes the dependency between HTMLCanvasElement creation and ImageBuffer creation which was time consuming and often a waste of memory (when the width and height were called shortly thereafter in most cases).
Comment 4 Darin Adler 2011-10-03 15:33:06 PDT
Comment on attachment 109540 [details]
Patch

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

r=me on this; it’s OK to land this as is, but I think the fix isn’t quite right

> Source/WebCore/ChangeLog:3
> +        Investigate removing lineWidth() from CanvasRenderingContext2D.cpp's ctor

This is a really confusing name for the patch!

The patch itself seems clearly like a good idea.

> Source/WebCore/html/HTMLCanvasElement.cpp:461
> +    m_imageBuffer->context()->setStrokeThickness(1);

It seems to me that it’s a bug that the a GraphicsContext starts with a stroke thickness that is platform-dependent. I understand that the different graphics libraries have different defaults, but the point of the platform layer is to abstract away such differences. The code that sets the thickness to 1 should be in the platform directory instead of here in the canvas code.
Comment 5 Matthew Delaney 2011-10-04 11:40:38 PDT
Committed r96624: <http://trac.webkit.org/changeset/96624>