This flag was only needed when calling BitmapPixels::setSize(). Instead we can pass the ImageDecoder::m_premultiplyAlpha to ImageFrame::setSize() which will pass it to BitmapPixels::setSize().
Created attachment 283537 [details] Patch
Created attachment 283557 [details] Patch
Created attachment 283565 [details] Patch
Created attachment 283569 [details] Patch
Created attachment 283571 [details] Patch
Created attachment 283577 [details] Patch
Created attachment 283578 [details] Patch
Created attachment 283580 [details] Patch
Created attachment 283589 [details] Patch for review
Created attachment 288644 [details] Patch
Comment on attachment 288644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288644&action=review > Source/WebCore/ChangeLog:10 > + This flag was only needed when calling ImageBackingStore::setSize(). Instead > + we can pass the ImageDecoder::m_premultiplyAlpha to ImageFrame::setSize() > + which will pass it to ImageBackingStore::setSize(). Why does setting the size care about premultiplied alpha?
(In reply to comment #11) > Comment on attachment 288644 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=288644&action=review > > > Source/WebCore/ChangeLog:10 > > + This flag was only needed when calling ImageBackingStore::setSize(). Instead > > + we can pass the ImageDecoder::m_premultiplyAlpha to ImageFrame::setSize() > > + which will pass it to ImageBackingStore::setSize(). > > Why does setting the size care about premultiplied alpha? It doesn't, I think it's because we only want the m_premultiplyAlpha to be passed to the ImageBackingStore constructor and ImageBackingStore is created in setSize().
I agree that passing premultiplyAlpha to setSize() is weird from the API point of view. But also because setSize() is weird as it is implemented. It can't be called if the ImageFrame already has a backing store, and it always creates a new ImageBackingStore. So, maybe if we rename the method as initializeBackingStore or simply initialize() it makes more sense to pass a size and premultiplyalpha as initialization parameters. We could probably rename copyBitmapData() also as initialize(). Another possibility (in addition to the rename that I think we should do anyway, because setSize and copyBitmapData are confusing), would be to let the decoder pass premultiplyAlpha to setPixel and blendPixel methods, that would just pass it to the backing store, since those are the only methods needing it.
Created attachment 288695 [details] Patch
Created attachment 288696 [details] Patch
Created attachment 288697 [details] Patch
Created attachment 288698 [details] Patch
(In reply to comment #13) > I agree that passing premultiplyAlpha to setSize() is weird from the API > point of view. But also because setSize() is weird as it is implemented. It > can't be called if the ImageFrame already has a backing store, and it always > creates a new ImageBackingStore. So, maybe if we rename the method as > initializeBackingStore or simply initialize() it makes more sense to pass a > size and premultiplyalpha as initialization parameters. We could probably > rename copyBitmapData() also as initialize(). I like this suggestion. I renamed setSize() and copyBitmapData() to initializeBackingStore(). I preferred it over initialize() because ImageFrame will be shared between CG and non-CG code and initialize() is very generic name. Calling initializeBackingStore() makes it clearer that we want to initialize the backing store only which is only needed by non CG ports. > Another possibility (in > addition to the rename that I think we should do anyway, because setSize and > copyBitmapData are confusing), would be to let the decoder pass > premultiplyAlpha to setPixel and blendPixel methods, that would just pass it > to the backing store, since those are the only methods needing it. I am not sure about this suggestion. It can actually make creating the backing store simpler and can make setPixel() and blendPixel() static or even global functions. But having the backing store not aware whether its pixels are premultiplied or not, seems weird to me. Anyway this seems a big change and I can do it in a separate patch if we need it.
Comment on attachment 288698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288698&action=review > Source/WebCore/ChangeLog:10 > + to ImageFrame::setSize(), which should be renamed ImageFrame::initializeBackingStore(). s/should be/is/ > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:179 > + if (!backingStore) > + return false; Should backingStore just be a reference instead? > Source/WebCore/platform/image-decoders/ImageDecoder.h:174 > + bool premultiplyAlpha() const { return m_premultiplyAlpha; } premultipiesAlpha()?
Created attachment 288717 [details] Patch
Created attachment 288719 [details] Patch
Comment on attachment 288719 [details] Patch Clearing flags on attachment: 288719 Committed r205877: <http://trac.webkit.org/changeset/205877>
All reviewed patches have been landed. Closing bug.