Bug 116604

Summary: [BlackBerry] Fix style issues in BlackBerry accelerated compositing backend
Product: WebKit Reporter: Arvid Nilsson <anilsson>
Component: WebKit BlackBerryAssignee: Arvid Nilsson <anilsson>
Status: RESOLVED FIXED    
Severity: Normal CC: anilsson, anlo, cgarcia, commit-queue, jpetsovits, rwlbuis, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 116616    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Arvid Nilsson 2013-05-22 02:57:31 PDT
Remove "get" prefix from getters and remove dead code.
Comment 1 Arvid Nilsson 2013-05-22 03:25:01 PDT
Created attachment 202516 [details]
Patch
Comment 2 Carlos Garcia Campos 2013-05-22 03:45:42 PDT
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()
Comment 3 Arvid Nilsson 2013-05-22 05:23:30 PDT
Created attachment 202529 [details]
Patch
Comment 4 Arvid Nilsson 2013-05-22 05:26:02 PDT
(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 5 Arvid Nilsson 2013-05-22 06:38:48 PDT
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...
Comment 6 Arvid Nilsson 2013-05-22 06:57:00 PDT
(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...
Comment 7 Arvid Nilsson 2013-05-22 07:19:34 PDT
Alright, I'll need to go and fix bug #116616 first =)
Comment 8 Arvid Nilsson 2013-05-24 04:33:45 PDT
Created attachment 202803 [details]
Patch
Comment 9 Carlos Garcia Campos 2013-05-24 04:41:22 PDT
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?
Comment 10 Arvid Nilsson 2013-05-24 04:45:58 PDT
(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 11 Jakob Petsovits 2013-05-27 08:05:53 PDT
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.
Comment 12 Arvid Nilsson 2013-05-27 09:00:01 PDT
(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).
Comment 13 Arvid Nilsson 2013-05-27 09:02:31 PDT
(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.
Comment 14 Arvid Nilsson 2013-05-28 02:39:32 PDT
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).
Comment 15 Arvid Nilsson 2013-05-28 02:41:14 PDT
(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".
Comment 16 Arvid Nilsson 2013-05-28 03:32:26 PDT
Created attachment 203035 [details]
Patch

Rebased on top of changed dependency patch
Comment 17 Carlos Garcia Campos 2013-05-28 10:07:19 PDT
Comment on attachment 203035 [details]
Patch

Nice!
Comment 18 Arvid Nilsson 2013-05-28 10:57:58 PDT
Comment on attachment 203035 [details]
Patch

This should apply now that the dependency has landed.
Comment 19 WebKit Commit Bot 2013-05-28 11:40:40 PDT
Comment on attachment 203035 [details]
Patch

Clearing flags on attachment: 203035

Committed r150817: <http://trac.webkit.org/changeset/150817>
Comment 20 WebKit Commit Bot 2013-05-28 11:40:43 PDT
All reviewed patches have been landed.  Closing bug.