Bug 114276

Created attachment 197268 [details] Patch

Comment on attachment 197268 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197268&action=review Looks good to me. > Source/WebCore/platform/graphics/blackberry/LayerTexture.cpp:-39 > - , m_isOpaque(false) With the disappearance of m_isOpaque, can we instead call BlackBerry::Platform::Graphics::setBufferOpaqueHint() in the appropriate place(s) so this information is not irreversibly lost? Or was there another reason that necessitates the wholesale removal of this concept? I guess we don't currently distinguish between opaque and non-opaque textures in the compositor to control the blend mode, although that's a pity.

(In reply to comment #2) > (From update of attachment 197268 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=197268&action=review > > Looks good to me. > > > Source/WebCore/platform/graphics/blackberry/LayerTexture.cpp:-39 > > - , m_isOpaque(false) > > With the disappearance of m_isOpaque, can we instead call BlackBerry::Platform::Graphics::setBufferOpaqueHint() in the appropriate place(s) so this information is not irreversibly lost? Or was there another reason that necessitates the wholesale removal of this concept? I guess we don't currently distinguish between opaque and non-opaque textures in the compositor to control the blend mode, although that's a pity. That's a great idea, I'll add that tomorrow around where we used to detect the opacity instead of dropping it all together.

(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 197268 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=197268&action=review > > > > Looks good to me. > > > > > Source/WebCore/platform/graphics/blackberry/LayerTexture.cpp:-39 > > > - , m_isOpaque(false) > > > > With the disappearance of m_isOpaque, can we instead call BlackBerry::Platform::Graphics::setBufferOpaqueHint() in the appropriate place(s) so this information is not irreversibly lost? Or was there another reason that necessitates the wholesale removal of this concept? I guess we don't currently distinguish between opaque and non-opaque textures in the compositor to control the blend mode, although that's a pity. > > That's a great idea, I'll add that tomorrow around where we used to detect the opacity instead of dropping it all together. Unfortunately, setting the opacity hint breaks the CSS Filters, probably because platform changes the blend mode or other GL state in a way webkit isn't ready for. I prefer to do that change separately.

Filed PR 325040 to investigate setting the opaque hint some time in the future.

Created attachment 197530 [details] Patch

(In reply to comment #6) > Created an attachment (id=197530) [details] > Patch Same patch as last time, only now it applies =)

Comment on attachment 197530 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197530&action=review Nice cleanup! > Source/WebCore/ChangeLog:11 > + now it's just BB::P::G::Buffer all around. Better say BlackBerry::Platform::Graphics::Buffer to make it clearer, I think. > Source/WebCore/platform/graphics/blackberry/LayerTexture.cpp:104 > + // Force creation if it's 0 Nit: finish the comment with a period. > Source/WebCore/platform/graphics/blackberry/LayerTexture.cpp:112 > + platformTexture = reinterpret_cast<GLuint>(platformBufferHandle(m_buffer)); Shouldn't it be reinterpret_cast<Platform3DObject> instead? > Source/WebCore/platform/graphics/blackberry/LayerTexture.h:61 > + // This will ensure a buffer is allocated for this texture with the requested backing and size Period here too. > Source/WebCore/platform/graphics/blackberry/TextureCacheCompositingThread.cpp:110 > + Buffer* buffer = createBuffer(size, type); > + if (!buffer) > return false; I think createBuffer is not expected to return 0, there's an assert in such case, so maybe we can skip this check, or add an assert here too. > Source/WebCore/platform/graphics/blackberry/TextureCacheCompositingThread.cpp:130 > + Buffer* buffer = createBuffer(size, type); > + if (!buffer) > return false; Ditto. > Source/WebCore/platform/graphics/blackberry/TextureCacheCompositingThread.h:54 > + PassRefPtr<LayerTexture> textureForContents(BlackBerry::Platform::Graphics::Buffer* contents); I think we can omit the parameter name in this case. > Source/WebCore/platform/graphics/blackberry/TextureCacheCompositingThread.h:58 > + PassRefPtr<LayerTexture> updateContents(const RefPtr<LayerTexture>&, BlackBerry::Platform::Graphics::Buffer* contents); Ditto.

Created attachment 197566 [details] Patch

Comment on attachment 197566 [details] Patch Adressed cgarcia's comments, straight to cq+

Comment on attachment 197566 [details] Patch Clearing flags on attachment: 197566 Committed r148192: <http://trac.webkit.org/changeset/148192>

All reviewed patches have been landed. Closing bug.