Bug 116604 – [BlackBerry] Fix style issues in BlackBerry accelerated compositing backend

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Arvid Nilsson]

Remove "get" prefix from getters and remove dead code.

[Arvid Nilsson]

Created attachment 202516 [details]
Patch

[Carlos Garcia Campos]

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()

[Arvid Nilsson]

Created attachment 202529 [details]
Patch

[Arvid Nilsson]

(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.

[Arvid Nilsson]

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...

[Arvid Nilsson]

(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...

[Arvid Nilsson]

Alright, I'll need to go and fix bug #116616 first =)

[Arvid Nilsson]

Created attachment 202803 [details]
Patch

[Carlos Garcia Campos]

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?

[Arvid Nilsson]

(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.

[Jakob Petsovits]

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.

[Arvid Nilsson]

(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).

[Arvid Nilsson]

(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.

[Arvid Nilsson]

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).

[Arvid Nilsson]

(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".

[Arvid Nilsson]

Created attachment 203035 [details]
Patch

Rebased on top of changed dependency patch

[Carlos Garcia Campos]

Comment on attachment 203035 [details]
Patch

Nice!

[Arvid Nilsson]

Comment on attachment 203035 [details]
Patch

This should apply now that the dependency has landed.

[WebKit Commit Bot]

Comment on attachment 203035 [details]
Patch

Clearing flags on attachment: 203035

Committed r150817: <http://trac.webkit.org/changeset/150817>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.