| Summary: | Skia image decoder does not need to ref its internal buffer | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Peter Kasting <pkasting> | ||||
| Component: | WebCore Misc. | Assignee: | Peter Kasting <pkasting> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | brettw | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | PC | ||||||
| OS: | Windows XP | ||||||
| Attachments: |
|
||||||
|
Description
Peter Kasting
2009-04-20 16:38:24 PDT
Created attachment 29629 [details] patch v1 Though he's not a WebKit reviewer, brettw@chromium.org wrote this code originally and should probably give an opinion on it. The original reason this code exists is that these are put into a Vector as described in the big comment above RGBA32Buffer. This vector still exists. Can you describe what changed that makes it no longer necessary? The reason this code existed is not just because these are put into a vector, but because they are put into a vector _and we used to return raw pointers to the underlying Skia objects_, which could become invalid when the vector copied on resize. After landing the patch for http://code.google.com/p/chromium/issues/detail?id=4298 , we no longer deal with these raw objects, so it's safe to put things in vectors and have them copied, as other consumers of the image data won't suddenly find their pointers invalidated. Comment on attachment 29629 [details] patch v1 > - m_bitmapRef = other.m_bitmapRef; > + m_bitmap = other.m_bitmap; > + m_bitmap.lockPixels(); // Without this, getAddr32() returns NULL. This comment reflects the symptom rather than the cause. I would say instead "Always keep the pixels locked since we will be writing directly into it." or something like that. > - m_bitmapRef = RefCountedNativeImageSkia::create(); > - SkBitmap& bmp = bitmap(); > - const SkBitmap& otherBmp = other.bitmap(); > - bmp.setConfig(SkBitmap::kARGB_8888_Config, other.width(), > - other.height(), otherBmp.rowBytes()); > - bmp.allocPixels(); > + m_bitmap.reset(); > + const NativeImageSkia& otherBitmap = other.bitmap(); > + m_bitmap.setConfig(SkBitmap::kARGB_8888_Config, otherBitmap.width(), > + otherBitmap.height(), otherBitmap.rowBytes()); > + m_bitmap.allocPixels(); > if (width() > 0 && height() > 0) { > - memcpy(bmp.getAddr32(0, 0), > - otherBmp.getAddr32(0, 0), > - otherBmp.rowBytes() * height()); > + memcpy(m_bitmap.getAddr32(0, 0), > + otherBitmap.getAddr32(0, 0), > + otherBitmap.rowBytes() * height()); > } > } I think you can replace this function with SkBitmap::copyTo, which didn't exist when we first wrote this, it was added in response to our complaints of having to do this kind of thing :) Otherwise, this Looks Good To Me. Comment on attachment 29629 [details]
patch v1
You have commit bit. This looks fine from a WebKit perspective. Assuming you address Brett's comments when landing, this is r=me.
|