Bug 51799 - ContextShadow can avoid code duplication for getting the CTM
: ContextShadow can avoid code duplication for getting the CTM
Status: RESOLVED FIXED
: WebKit
Canvas
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-01-02 00:55 PST by
Modified: 2011-01-03 17:37 PST (History)


Attachments
Untested with GTK (30.28 KB, patch)
2011-01-02 01:03 PST, Helder Correia
no flags Review Patch | Details | Formatted Diff | Diff
Stick to elimination of getTransformationMatrixFromContext() (30.12 KB, patch)
2011-01-02 20:00 PST, Helder Correia
no flags Review Patch | Details | Formatted Diff | Diff
Attept to fix Cairo-based build (30.87 KB, patch)
2011-01-03 00:45 PST, Helder Correia
no flags Review Patch | Details | Formatted Diff | Diff
Attempt 2 to fix Cairo-based build (31.47 KB, patch)
2011-01-03 00:54 PST, Helder Correia
no flags Review Patch | Details | Formatted Diff | Diff
Fix GTK build (31.92 KB, patch)
2011-01-03 16:48 PST, Helder Correia
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-01-02 00:55:54 PST
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().
------- Comment #1 From 2011-01-02 01:03:01 PST -------
Created an attachment (id=77773) [details]
Untested with GTK
------- Comment #2 From 2011-01-02 08:47:07 PST -------
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.
------- Comment #3 From 2011-01-02 11:10:47 PST -------
(From update of attachment 77773 [details])
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.
------- Comment #4 From 2011-01-02 11:13:57 PST -------
(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.
------- Comment #5 From 2011-01-02 11:30:03 PST -------
> 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.
------- Comment #6 From 2011-01-02 11:45:29 PST -------
(From update of attachment 77773 [details])
According to the comments above the review of the patch should be denied.

Is this bug still valid?
------- Comment #7 From 2011-01-02 17:20:51 PST -------
(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?
------- Comment #8 From 2011-01-02 20:00:04 PST -------
Created an attachment (id=77788) [details]
Stick to elimination of getTransformationMatrixFromContext()
------- Comment #9 From 2011-01-02 22:38:44 PST -------
(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.
------- Comment #10 From 2011-01-03 00:45:06 PST -------
Created an attachment (id=77794) [details]
Attept to fix Cairo-based build
------- Comment #11 From 2011-01-03 00:54:50 PST -------
Created an attachment (id=77795) [details]
Attempt 2 to fix Cairo-based build
------- Comment #12 From 2011-01-03 16:21:24 PST -------
(From update of attachment 77795 [details])
Clearing flags on attachment: 77795

Committed r74947: <http://trac.webkit.org/changeset/74947>
------- Comment #13 From 2011-01-03 16:21:29 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #14 From 2011-01-03 16:41:12 PST -------
http://trac.webkit.org/changeset/74947 might have broken GTK Linux 32-bit Release and GTK Linux 64-bit Debug
------- Comment #15 From 2011-01-03 16:48:29 PST -------
Created an attachment (id=77853) [details]
Fix GTK build
------- Comment #16 From 2011-01-03 17:04:22 PST -------
(From update of attachment 77853 [details])
Looks like the EWS can’t apply this patch, so I suspect it’s not going to make it in the commit queue.
------- Comment #17 From 2011-01-03 17:37:24 PST -------
(From update of attachment 77853 [details])
I made this build fix in r 74950