Bug 51799 - ContextShadow can avoid code duplication for getting the CTM
: ContextShadow can avoid code duplication for getting the CTM
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Canvas
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-02 00:55 PST by Helder Correia
Modified: 2011-01-03 17:37 PST (History)
8 users (show)

See Also:


Attachments
Untested with GTK (30.28 KB, patch)
2011-01-02 01:03 PST, Helder Correia
no flags Details | Formatted Diff | Diff
Stick to elimination of getTransformationMatrixFromContext() (30.12 KB, patch)
2011-01-02 20:00 PST, Helder Correia
no flags Details | Formatted Diff | Diff
Attept to fix Cairo-based build (30.87 KB, patch)
2011-01-03 00:45 PST, Helder Correia
no flags 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 Details | Formatted Diff | Diff
Fix GTK build (31.92 KB, patch)
2011-01-03 16:48 PST, Helder Correia
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Helder Correia 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 Helder Correia 2011-01-02 01:03:01 PST
Created attachment 77773 [details]
Untested with GTK
Comment 2 Ariya Hidayat 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 Simon Fraser (smfr) 2011-01-02 11:10:47 PST
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.
Comment 4 Simon Fraser (smfr) 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 Ariya Hidayat 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 Dirk Schulze 2011-01-02 11:45:29 PST
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?
Comment 7 Helder Correia 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 Helder Correia 2011-01-02 20:00:04 PST
Created attachment 77788 [details]
Stick to elimination of getTransformationMatrixFromContext()
Comment 9 Simon Fraser (smfr) 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 Helder Correia 2011-01-03 00:45:06 PST
Created attachment 77794 [details]
Attept to fix Cairo-based build
Comment 11 Helder Correia 2011-01-03 00:54:50 PST
Created attachment 77795 [details]
Attempt 2 to fix Cairo-based build
Comment 12 WebKit Commit Bot 2011-01-03 16:21:24 PST
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>
Comment 13 WebKit Commit Bot 2011-01-03 16:21:29 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 WebKit Review Bot 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 Helder Correia 2011-01-03 16:48:29 PST
Created attachment 77853 [details]
Fix GTK build
Comment 16 Darin Adler 2011-01-03 17:04:22 PST
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.
Comment 17 Simon Fraser (smfr) 2011-01-03 17:37:24 PST
Comment on attachment 77853 [details]
Fix GTK build

I made this build fix in r 74950