Remove "get" prefix from getters and remove dead code.
Created attachment 202516 [details] Patch
Comment on attachment 202516 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=202516&action=review Nice cleanup! > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:150 > +static FloatQuad transformLayerRect(const LayerCompositingThread* layer, const TransformationMatrix& transform, const IntRect& rect) transformLayerRect sounds more like the parameters is modified instead of that a transformed rect is returned. Maybe transformedLayerRect would be clearer and consistent with other methods like transformedHolePunchRect() > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:156 > float x = -bounds.width() / 2.0 + rect.x(); > float y = -bounds.height() / 2.0 + rect.y(); > float w = rect.width(); > float h = rect.height(); You could probably fix this too. This could be FloatRect(-bounds.width() / 2.0 + rect.x(), -bounds.height() / 2.0 + rect.y(), rect.size()); > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:158 > + result.setP1(transform.mapPoint(FloatPoint(x, y))); Using a FloatRect this could just be minXMinYCorner() > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:159 > + result.setP2(transform.mapPoint(FloatPoint(x, y + h))); minXMaxYCorner() > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:160 > + result.setP3(transform.mapPoint(FloatPoint(x + w, y + h))); maxXMaxYCorner() > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:161 > + result.setP4(transform.mapPoint(FloatPoint(x + w, y))); maxXMinYCorner()
Created attachment 202529 [details] Patch
(In reply to comment #3) > Created an attachment (id=202529) [details] > Patch I avoided the naming problem here by getting rid of the "getTransformedRect" method altogether.
Comment on attachment 202529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=202529&action=review > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:245 > + FloatQuad surfaceQuad = drawTransform.mapQuad(FloatRect(-origin(), bounds())); Oops, FloatQuad::FloatQuad(const FloatRect&) puts the rect corners in the wrong positions, so the surface get "rotated" 90 degrees since the texture coords used no longer matches the corners...
(In reply to comment #5) > (From update of attachment 202529 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=202529&action=review > > > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:245 > > + FloatQuad surfaceQuad = drawTransform.mapQuad(FloatRect(-origin(), bounds())); > > Oops, FloatQuad::FloatQuad(const FloatRect&) puts the rect corners in the wrong positions, so the surface get "rotated" 90 degrees since the texture coords used no longer matches the corners... Using counterclockwise geometry like it used to do doesn't play well with the new culling code used with the BB::P::G::GraphicsContext. We switched to glFrontFace(GL_CW) to work with layers whose content is a display list, but kept some of the old CCW geometry for layers whose content isn't, like WebGL, masks and filters...
Alright, I'll need to go and fix bug #116616 first =)
Created attachment 202803 [details] Patch
Comment on attachment 202803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=202803&action=review > Source/WebCore/ChangeLog:18 > + Also remove dead code related to LayerData::m_holePunchClipRect, which > + was part of an eventually disabled fix for video clipping in iframes, > + PR 99638. Since we now use AC layers for iframes, the bug is no longer > + reproducible, and the dead code will never need to be enabled again. Maybe you can split the patch since this dead code doesn't depend on changes on bug #116616, right?
(In reply to comment #9) > (From update of attachment 202803 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=202803&action=review > > > Source/WebCore/ChangeLog:18 > > + Also remove dead code related to LayerData::m_holePunchClipRect, which > > + was part of an eventually disabled fix for video clipping in iframes, > > + PR 99638. Since we now use AC layers for iframes, the bug is no longer > > + reproducible, and the dead code will never need to be enabled again. > > Maybe you can split the patch since this dead code doesn't depend on changes on bug #116616, right? I think it makes sense to keep them together - dead code is really also a style issue I think.
Comment on attachment 202803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=202803&action=review Looks safe and reasonable. > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:254 > // Vertex data for the bounds of this layer > FloatQuad m_transformedBounds; > // The bounding rectangle of the transformed layer > - FloatRect m_drawRect; > + FloatRect m_boundingBox; If m_boundingBox is the .boundingBox() of m_transformedBounds, would it make sense to name the two of them in a very similar way? Such as making it m_transformedBoundingBox, for instance. > Source/WebCore/platform/graphics/blackberry/LayerRendererSurface.h:57 > - FloatRect drawRect() const; > + // These use normalized device coordinates > + FloatRect boundingBox() const; > FloatQuad transformedBounds() const; Same question as for the comment above.
(In reply to comment #11) > (From update of attachment 202803 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=202803&action=review > > Looks safe and reasonable. > > > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:254 > > // Vertex data for the bounds of this layer > > FloatQuad m_transformedBounds; > > // The bounding rectangle of the transformed layer > > - FloatRect m_drawRect; > > + FloatRect m_boundingBox; > > If m_boundingBox is the .boundingBox() of m_transformedBounds, would it make sense to name the two of them in a very similar way? Such as making it m_transformedBoundingBox, for instance. > > > Source/WebCore/platform/graphics/blackberry/LayerRendererSurface.h:57 > > - FloatRect drawRect() const; > > + // These use normalized device coordinates > > + FloatRect boundingBox() const; > > FloatQuad transformedBounds() const; > > Same question as for the comment above. There were two reasons I didn't name it "m_transformedBoundingBox" - I think the name is too long, and it's unclear whether it's the transformed bounding box, or the bounding box of the transformed bounding rect (when it is in fact the latter).
(In reply to comment #12) > (In reply to comment #11) > > (From update of attachment 202803 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=202803&action=review > > > > Looks safe and reasonable. > > > > > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.h:254 > > > // Vertex data for the bounds of this layer > > > FloatQuad m_transformedBounds; > > > // The bounding rectangle of the transformed layer > > > - FloatRect m_drawRect; > > > + FloatRect m_boundingBox; > > > > If m_boundingBox is the .boundingBox() of m_transformedBounds, would it make sense to name the two of them in a very similar way? Such as making it m_transformedBoundingBox, for instance. > > > > > Source/WebCore/platform/graphics/blackberry/LayerRendererSurface.h:57 > > > - FloatRect drawRect() const; > > > + // These use normalized device coordinates > > > + FloatRect boundingBox() const; > > > FloatQuad transformedBounds() const; > > > > Same question as for the comment above. > > There were two reasons I didn't name it "m_transformedBoundingBox" - I think the name is too long, and it's unclear whether it's the transformed bounding box, or the bounding box of the transformed bounding rect (when it is in fact the latter). I would rather remove the "transformed" moniker from the other accessors in the future, in favor of something shorter, than adding it to more places. In an upcoming patch, "geometry" might be a more suitable name for the transformedBounds(), since it can be a polygon in some cases.
I've decided not to renable boundingBox() at this time, instead "transformedBounds()" will be renamed to "geometry" once it's a vector of 4D points (a polygon in homogeneous space).
(In reply to comment #14) > I've decided not to renable boundingBox() at this time, instead "transformedBounds()" will be renamed to "geometry" once it's a vector of 4D points (a polygon in homogeneous space). renable? I meant "rename". Also, homogeneous space is something else than what I meant, should say "vector of points, expressed in homogeneous coordinates".
Created attachment 203035 [details] Patch Rebased on top of changed dependency patch
Comment on attachment 203035 [details] Patch Nice!
Comment on attachment 203035 [details] Patch This should apply now that the dependency has landed.
Comment on attachment 203035 [details] Patch Clearing flags on attachment: 203035 Committed r150817: <http://trac.webkit.org/changeset/150817>
All reviewed patches have been landed. Closing bug.