Bug 90716 - Move zero-length-subpaths from RenderSVGShape to RenderSVGPath
Summary: Move zero-length-subpaths from RenderSVGShape to RenderSVGPath
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philip Rogers
URL:
Keywords:
Depends on:
Blocks: 88231
  Show dependency treegraph
 
Reported: 2012-07-06 19:05 PDT by Philip Rogers
Modified: 2012-07-17 14:43 PDT (History)
3 users (show)

See Also:


Attachments
Preliminary patch (25.08 KB, patch)
2012-07-06 19:35 PDT, Philip Rogers
no flags Details | Formatted Diff | Diff
Move zero-length-subpaths from RenderSVGShape to RenderSVGPath (20.71 KB, patch)
2012-07-12 12:33 PDT, Philip Rogers
no flags Details | Formatted Diff | Diff
Update per reviewer comments (21.16 KB, patch)
2012-07-12 13:38 PDT, Philip Rogers
zimmermann: review+
zimmermann: commit-queue-
Details | Formatted Diff | Diff
Update per reviewer comments (2) (21.20 KB, patch)
2012-07-17 13:56 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-07-06 19:05:22 PDT
The SVG spec only allows zero-length-subpaths in paths but our current codebase has the code in RenderSVGShape, the superclass of RenderSVGRect and RenderSVGEllipse. Neither rects nor ellipses can contain zero-length-subpaths so we should move the zero-length-subpath code into RenderSVGPath for better code clarity and rendering performance.
Comment 1 Philip Rogers 2012-07-06 19:35:47 PDT
Created attachment 151129 [details]
Preliminary patch

This patch depends on both WK90514 and WK90655 so it's just a preview until those land.
Comment 2 Philip Rogers 2012-07-12 12:33:05 PDT
Created attachment 152024 [details]
Move zero-length-subpaths from RenderSVGShape to RenderSVGPath
Comment 3 Stephen Chenney 2012-07-12 13:16:25 PDT
Comment on attachment 152024 [details]
Move zero-length-subpaths from RenderSVGShape to RenderSVGPath

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

Looks good to me.

> Source/WebCore/ChangeLog:10
> +        to RenderSVGPath.

I would suggest adding what you gain by this.
Comment 4 Philip Rogers 2012-07-12 13:38:56 PDT
Created attachment 152046 [details]
Update per reviewer comments

Updated ChangeLog to summarize why this is awesome.

Also added OVERRIDEs to RenderSVGPath per schenney's comments in wkb.ug/88231
Comment 5 Philip Rogers 2012-07-16 14:08:33 PDT
Nikolas, I think you may have the most context for a review here. Friendly ping for a review :)
Comment 6 Nikolas Zimmermann 2012-07-16 23:46:51 PDT
Comment on attachment 152046 [details]
Update per reviewer comments

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

Looks great, r=me with a minor comment.

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:56
> +FloatRect RenderSVGPath::updateStrokeBoundingBox() const

I think updateStrokeBoundingBox is a misleading name, as it returns a new strokeBoundingBox, instead of updating the m_strokeBoundingBox.
Maybe you can come up with a better name not involving update...

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:96
> +    context->save();

Use GraphicsContextStateSaver stateSaver(*context, true) here.
Comment 7 Philip Rogers 2012-07-17 13:56:40 PDT
Created attachment 152821 [details]
Update per reviewer comments (2)
Comment 8 WebKit Review Bot 2012-07-17 14:43:15 PDT
Comment on attachment 152821 [details]
Update per reviewer comments (2)

Clearing flags on attachment: 152821

Committed r122875: <http://trac.webkit.org/changeset/122875>
Comment 9 WebKit Review Bot 2012-07-17 14:43:20 PDT
All reviewed patches have been landed.  Closing bug.