Bug 28167

Created attachment 34519 [details] the patch modified according to Eric's suggestion

Comment on attachment 34519 [details] the patch You mentioned in the other bug, that BufferedImage could be implemented by BitmapImage by implementing a BitmapImage(NativeImagePtr) constructor. Why would we not want to do that? I expect you could encounter later porting problems being the only port with a special BitmapImage-like-thing.

(In reply to comment #2) > (From update of attachment 34519 [details]) > You mentioned in the other bug, that BufferedImage could be implemented by > BitmapImage by implementing a BitmapImage(NativeImagePtr) constructor. Why > would we not want to do that? I expect you could encounter later porting > problems being the only port with a special BitmapImage-like-thing. Because I think that is ugly. BitmapImage is too powerful. It is designed to receive encoded image data, decode the data with ImageSource, display the frames even they are just partially decoded, and animate with multiple frames and correct time intervals... All of these are not necessary for ImageBuffer. ImageBuffer just holds a single-frame flat decoded buffer (or platform native image). That's why ImageBuffer::image() returns Image* but not BitmapImage*, because BitmapImage is much more than necessary. So I think other ports should also do the similiar thing: using a light class like BufferedImage. It is very simple, and doesn't required too much coding.

Furthermore, I think the best way is let ImageBuffer be a subclass of Image. It's just used for rendering purpose

ImageBuffer and Image are used for different things. ImageBuffer is a bag of bytes, designed to store image data, and used for things which need direct access to bit maps. Image is an opaque image pointer which could be backed by an ImageBuffer, or any other sort of storage. Image might also not even be a bitmap image (in the case of SVG or PDF), so it might not have a fixed size.

Created attachment 34765 [details] updated updated to implement premultiplied getting and putting image data

Comment on attachment 34765 [details] updated Why is BufferedImage specific to WINCE? That seems like a general concept that should be factored out and shared with the other ports if it's useful. Also, it violates the "one class per file" rule.

(In reply to comment #7) > (From update of attachment 34765 [details]) > Why is BufferedImage specific to WINCE? That seems like a general concept that > should be factored out and shared with the other ports if it's useful. Also, > it violates the "one class per file" rule. Is this the reason for r-? Where's the "one class per file" rule? If there is, I can find hundreds of violations. Why is contributing to webkit so difficult?

The approach we've taken is that if something isn't 100% clearly a benefit to all platforms, we kept it as WINCE and then left it possible to move it to an ENABLE(feature) or unconditional code later.

Comment on attachment 34765 [details] updated Based on the webkit-dev discussion and George's comment above, this patch is probably fine. I'm not in love with this code + *dst++ = static_cast<unsigned char>((red * alpha + 254) / 255); but I think it's ok for a first pass. Thanks for your patience.

Thanks a lot. sorry for my languages

Comment on attachment 34765 [details] updated Rejecting patch 34765 from commit-queue. This patch will require manual commit. ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

Comment on attachment 34765 [details] updated Clearing flags on attachment: 34765 Committed r47968: <http://trac.webkit.org/changeset/47968>

All reviewed patches have been landed. Closing bug.