Bug 51799 – ContextShadow can avoid code duplication for getting the CTM

• Component: Canvas
• Status: RESOLVED FIXED

[Helder Correia]

Currently, ContextShadow defines its own getTransformationMatrixFromContext(), which does exactly the same as GraphicsContext::getCTM(). This can be avoided by passing GraphicsContext instead of PlatformContext to beginShadowLayer(). Additionally, there are two calls to getTransformation...() in beginShadowLayer(): one in adjustBlurDistance() and another in calculateLayerBoundingRect(). There's no reason for this and it can be optimized. Finally, endShadowLayer() would be cleaner from an API POV if no argument was required. There's no need to pass the context once again shortly after the user has called beginShadowLayer() and drawn some primitives. All it takes is to store the pointer to the context in beginShadowLayer().

[Helder Correia]

Created attachment 77773 [details]
Untested with GTK

[Ariya Hidayat]

endShadowLayer() change is probably better splitted in another patch, it has nothing to do with the other changes.

I'm undecided whether adding more member variables to ContextShadow class is justified in this case. The size of the class impacts the performance of Graphics Context, especially during save/restore, which happens way more often than just dealing with shadow.

[Simon Fraser (smfr)]

Comment on attachment 77773 [details]
Untested with GTK

View in context: https://bugs.webkit.org/attachment.cgi?id=77773&action=review

> WebCore/platform/graphics/ContextShadow.cpp:86
> -bool ContextShadow::mustUseContextShadow(PlatformContext context)
> +bool ContextShadow::mustUseContextShadow(const GraphicsContext* context)

I think this method belongs in platform-specific code. Different platforms may make different decisions about when to use ContextShadow.

> WebCore/platform/graphics/ContextShadow.cpp:178
> -void ContextShadow::adjustBlurDistance(const PlatformContext context)
> +void ContextShadow::adjustBlurDistance(const AffineTransform& transform)
>  {
>      // Adjust blur if we're scaling, since the radius must not be affected by transformations.
> -    const AffineTransform transform(getTransformationMatrixFromContext(context));
> -
>      if (transform.isIdentity())
>          return;

This method is wrong. It should return false if shadows don't ignore transforms.

> WebCore/platform/graphics/ContextShadow.cpp:-207
> -    const AffineTransform transform(getTransformationMatrixFromContext(context));

Getting the CTM is pretty cheap, so I don't really see the need to pass the transform around.

> WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:92
> -PlatformContext ContextShadow::beginShadowLayer(PlatformContext context, const FloatRect& layerArea)
> +PlatformContext ContextShadow::beginShadowLayer(const GraphicsContext* context, const FloatRect& layerArea)

We don't normally treat the GraphicsContext* as const.

[Simon Fraser (smfr)]

(In reply to comment #2)

> I'm undecided whether adding more member variables to ContextShadow class is justified in this case. The size of the class impacts the performance of Graphics Context, especially during save/restore, which happens way more often than just dealing with shadow.

It's not clear to me why ContextShadow has to be part of the state, copied by value.

[Ariya Hidayat]

> It's not clear to me why ContextShadow has to be part of the state, copied by value.

Because back then when it was really small, the easiest thing to do was to attach it to the state.

I guess right now it's just easier to instantiate it as it is needed.

[Dirk Schulze]

Comment on attachment 77773 [details]
Untested with GTK

According to the comments above the review of the patch should be denied.

Is this bug still valid?

[Helder Correia]

(In reply to comment #3) 
> I think this method belongs in platform-specific code. Different platforms may make different decisions about when to use ContextShadow.

I agree. I can prepare a separate patch fixing that. 

> This method is wrong. It should return false if shadows don't ignore transforms.

You mean the blur distance should only be adjusted when we're working on a canvas?

[Helder Correia]

Created attachment 77788 [details]
Stick to elimination of getTransformationMatrixFromContext()

[Simon Fraser (smfr)]

(In reply to comment #7)
> > This method is wrong. It should return false if shadows don't ignore transforms.
> 
> You mean the blur distance should only be adjusted when we're working on a canvas?

Right. I make this change in the patch in bug 22102.

[Helder Correia]

Created attachment 77794 [details]
Attept to fix Cairo-based build

[Helder Correia]

Created attachment 77795 [details]
Attempt 2 to fix Cairo-based build

[WebKit Commit Bot]

Comment on attachment 77795 [details]
Attempt 2 to fix Cairo-based build

Clearing flags on attachment: 77795

Committed r74947: <http://trac.webkit.org/changeset/74947>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/74947 might have broken GTK Linux 32-bit Release and GTK Linux 64-bit Debug

[Helder Correia]

Created attachment 77853 [details]
Fix GTK build

[Darin Adler]

Comment on attachment 77853 [details]
Fix GTK build

Looks like the EWS can’t apply this patch, so I suspect it’s not going to make it in the commit queue.

[Simon Fraser (smfr)]

Comment on attachment 77853 [details]
Fix GTK build

I made this build fix in r 74950