Bug 88231 - Refactor RenderSVG{Shape,Rect,Ellipse,Path} to be less coupled.
Summary: Refactor RenderSVG{Shape,Rect,Ellipse,Path} to be less coupled.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 91131 90514 90655 90716
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-04 07:18 PDT by Philip Rogers
Modified: 2012-07-12 11:56 PDT (History)
2 users (show)

See Also:


Attachments
Preview patch (45.62 KB, patch)
2012-06-04 07:23 PDT, Philip Rogers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Rogers 2012-06-04 07:18:30 PDT
RenderSVG{Shape,Rect,Ellipse,Path} are too tightly coupled.
Comment 1 Philip Rogers 2012-06-04 07:23:23 PDT
Created attachment 145585 [details]
Preview patch

Preview patch for the direction I'd like to go. Looking for initial impressions.
Comment 2 Stephen Chenney 2012-06-04 08:07:31 PDT
Comment on attachment 145585 [details]
Preview patch

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

> Source/WebCore/rendering/svg/RenderSVGEllipse.h:59
> +    bool usePathStrokeContains;

I think that m_shouldUsePathStrokeContains would better fit WebKit style.

> Source/WebCore/rendering/svg/RenderSVGPath.h:41
> +    virtual void applyNonScalingStrokeTransform(const AffineTransform*, bool);

Should any or all of the new methods be OVERRIDE?

> Source/WebCore/rendering/svg/RenderSVGRect.h:59
> +    bool usePathStrokeContains;

m_shouldUsePathStrokeContains

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:190
> +void RenderSVGShape::applyNonScalingStrokeTransform(const AffineTransform* strokeTransform, bool inverse)

I'm not entirely happy with this. I can see that you want to cache the pre transform path and also take care of clearing the path, but it seems like there is a cleaner way to do this. I would most like to get rid of the "new" in the unapply case.

> Source/WebCore/rendering/svg/RenderSVGShape.h:89
> +    virtual FloatRect strokeBoundingBox() const;

If your adding this, it would be good to add the OVERRIDE to all of these methods.