Summary: | [skia] Optimize ImageBuffer constructor when accelerated | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ben Wells <benwells> | ||||||
Component: | Canvas | Assignee: | Stephen White <senorblanco> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | alokp, bsalomon, jamesr, kbr, mdelaney7, reed, senorblanco, vangelis | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Ben Wells
2011-09-20 21:13:46 PDT
Created attachment 108153 [details]
Patch
Comment on attachment 108153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108153&action=review R=me, left some comments > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:69 > + GraphicsContext3D* context3D = SharedGraphicsContext3D::create(0); we really should remove the parameter from SharedGraphicsContext3D::create(), it's not useful > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:76 > + SkCanvas* canvas = new SkCanvas(); nit: if you move this down below line 83 I think you can avoid having to manually delete this if the texture thingy fails also, if you make this function return a PassOwnPtr<> then you can make this an OwnPtr<SkCanvas>, have the return value be canvas.release() and not have to worry about saying 'delete' at all Comment on attachment 108153 [details]
Patch
lgtm. I like James' suggestion about moving the creation of canvas after creating the texture.
Created attachment 108198 [details]
Updated per comments
Thanks for your review. (In reply to comment #2) > (From update of attachment 108153 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=108153&action=review > > R=me, left some comments > > > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:69 > > + GraphicsContext3D* context3D = SharedGraphicsContext3D::create(0); > > we really should remove the parameter from SharedGraphicsContext3D::create(), it's not useful Done. Also renamed SharedGraphicsContext3D::create() to get(), since it's only a create the first time. > > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:76 > > + SkCanvas* canvas = new SkCanvas(); > > nit: if you move this down below line 83 I think you can avoid having to manually delete this if the texture thingy fails Done. > also, if you make this function return a PassOwnPtr<> then you can make this an OwnPtr<SkCanvas>, have the return value be canvas.release() and not have to worry about saying 'delete' at all I tried that, but it meant all the early-returns needed to return a PassRefPtr<SkCanvas*>(0), which looked ugly. Comment on attachment 108198 [details] Updated per comments View in context: https://bugs.webkit.org/attachment.cgi?id=108198&action=review > Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.h:37 > + static GraphicsContext3D* get(); this header file is kind of hilariously tiny now. I actually think you can remove the three #include lines and just leave a forward declaration of GraphicsContext3D. > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:85 > + SkCanvas* canvas = new SkCanvas(); I think I'd still mildly prefer having this function be: PassOwnPtr<SkCanvas> createAcceleratedCanvas(...) and this line being OwnPtr<SkCanvas> canvas = adoptPtr(new SkCanvas()) just to try to keep the raw pointers down. it's not a very practical concern (In reply to comment #5) > > also, if you make this function return a PassOwnPtr<> then you can make this an OwnPtr<SkCanvas>, have the return value be canvas.release() and not have to worry about saying 'delete' at all > > I tried that, but it meant all the early-returns needed to return a PassRefPtr<SkCanvas*>(0), which looked ugly. Aha! You want nullptr Committed r95658: <http://trac.webkit.org/changeset/95658> |