Summary: | ContextShadow can avoid code duplication for getting the CTM | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Helder Correia <helder> | ||||||||||||
Component: | Canvas | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, ariya.hidayat, commit-queue, eric, mdelaney7, mrobinson, simon.fraser, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | OS X 10.5 | ||||||||||||||
Attachments: |
|
Description
Helder Correia
2011-01-02 00:55:54 PST
Created attachment 77773 [details]
Untested with GTK
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 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. (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. > 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 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?
(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? Created attachment 77788 [details]
Stick to elimination of getTransformationMatrixFromContext()
(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. Created attachment 77794 [details]
Attept to fix Cairo-based build
Created attachment 77795 [details]
Attempt 2 to fix Cairo-based build
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> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/74947 might have broken GTK Linux 32-bit Release and GTK Linux 64-bit Debug Created attachment 77853 [details]
Fix GTK build
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 on attachment 77853 [details]
Fix GTK build
I made this build fix in r 74950
|