When we switched from Skia to BlackBerry::Platform::Graphics::GraphicsContext, we switched the winding order for culling, since the BB::P::G::GC uses clockwise geometry and requires glFrontFace(GL_CW). However, masks and filters still use counterclockwise geometry as we used to do with Skia. So they get erroneously culled out when -webkit-backface-visibility: hidden is applied.
To further complicate matters, a right-side-up transform is used when rendering layers into surfaces, so if we switch the winding order of layer geometry, instead of erroneously culling the surface when drawn to the screen, it will cull the layer when it's drawn into the surface =D
Created attachment 202800 [details] Patch
Comment on attachment 202800 [details] Patch Oops, I forgot to mention a PR number in the changelog.
Created attachment 202801 [details] Patch
I think an informal review from jpetso or someone else who knows this stuff is needed before an upstream reviewer can r+ it.
Comment on attachment 202801 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=202801&action=review > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:292 > + static float texcoords[4 * 2] = { 0, 0, 1, 0, 1, 1, 0, 1 }; We can probably make these global in the cpp, like in LayerFilterRenderer.cpp > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:165 > + FloatPoint origin() const { return FloatPoint(m_bounds.width() / 2.0f, m_bounds.height() / 2.0f); } I wonder if FloatPoint::center would make sense...
Comment on attachment 202801 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=202801&action=review The code looks good to me. Seconding Rob's comments. > Source/WebCore/platform/graphics/blackberry/LayerRenderer.cpp:336 > // If some layers should be drawed on temporary surfaces, we should do it first. > - drawLayersOnSurfaces(surfaceLayers); > + if (!surfaceLayers.isEmpty()) > + drawLayersOnSurfaces(surfaceLayers); If you're changing this code, you might want to consider also correcting the grammatical mistake in the comment. > Source/WebCore/platform/graphics/blackberry/LayerRenderer.cpp:503 > // If there are layers drawed on surfaces, we need to switch to default framebuffer. > // Otherwise, we just need to set viewport. More "drawed" that could be drawn instead. Maybe the comment should now be rephrased anyway though, because we switch back to the default framebuffer unconditionally if the function is called at all.
(In reply to comment #6) > (From update of attachment 202801 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=202801&action=review > > > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:292 > > + static float texcoords[4 * 2] = { 0, 0, 1, 0, 1, 1, 0, 1 }; > > We can probably make these global in the cpp, like in LayerFilterRenderer.cpp The texture coordinates will depend on the geometry after clipping in a future patch, so there's no point in moving this declaration around right now. There will be a LayerCompositingThread::textureCoordinates() accessor that replaces this code later in the week. > > > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:165 > > + FloatPoint origin() const { return FloatPoint(m_bounds.width() / 2.0f, m_bounds.height() / 2.0f); } > > I wonder if FloatPoint::center would make sense...
(In reply to comment #6) > (From update of attachment 202801 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=202801&action=review > > > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:292 > > + static float texcoords[4 * 2] = { 0, 0, 1, 0, 1, 1, 0, 1 }; > > We can probably make these global in the cpp, like in LayerFilterRenderer.cpp > > > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:165 > > + FloatPoint origin() const { return FloatPoint(m_bounds.width() / 2.0f, m_bounds.height() / 2.0f); } > > I wonder if FloatPoint::center would make sense... Apparently there is no FloatPoint::center method, I don't think this patch is the right time to add one.
Created attachment 203032 [details] Patch
Created attachment 203034 [details] Patch Remove one redundant instance of 1.0 in favor of 1. In other places, the .0f is required to force floating point math, and is thus allowed according to style guidelines
(In reply to comment #11) > Created an attachment (id=203034) [details] > Patch > > Remove one redundant instance of 1.0 in favor of 1. In other places, the .0f is required to force floating point math, and is thus allowed according to style guidelines Thanks to cagarcia for spotting this.
Comment on attachment 203034 [details] Patch Looks a lot cleaner!
Comment on attachment 203034 [details] Patch Clearing flags on attachment: 203034 Committed r150809: <http://trac.webkit.org/changeset/150809>
All reviewed patches have been landed. Closing bug.