Bug 32758

Summary:

SVGAngle / SVGPreserveAspectRatio should be POD types

Product:

WebKit

Reporter:

Nikolas Zimmermann <zimmermann>

Component:

SVG

Assignee:

Nikolas Zimmermann <zimmermann>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, eric, krit, staikos, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Initial patch	none
Updated patch	zimmermann: commit-queue-
Updated patch v2	eric: review+, zimmermann: commit-queue-

Description Nikolas Zimmermann 2009-12-18 17:01:58 PST

The last two types which remain to be converted to POD types are SVGAngle/SVGPreserveAspectRatio. Just like it has been done for SVGLength before, change that, as there's no need to have these simple objects refcounted.

Also this is a blocker for my upcoming patch: remove DOMObjectWithSVGContext, to basically rewrite SVG JS bindings to be more consistent with the HTML style of creating bindings (no special SVG context needed anymore soon...)

Comment 1 Nikolas Zimmermann 2009-12-18 18:29:32 PST

Created attachment 45217 [details]
Initial patch

Mechanical patch making SVGAngle / SVGPreserveAspectRatio non-refcounted types.

Comment 2 WebKit Review Bot 2009-12-18 18:32:10 PST

style-queue ran check-webkit-style on attachment 45217 [details] without any errors.

Comment 3 Nikolas Zimmermann 2009-12-18 19:33:48 PST

Created attachment 45220 [details]
Updated patch

Incorporated Eric's comments from IRC (remove SVGPreserveAspectRatio code duplication)

Comment 4 WebKit Review Bot 2009-12-18 19:38:35 PST

style-queue ran check-webkit-style on attachment 45220 [details] without any errors.

Comment 5 Nikolas Zimmermann 2009-12-18 20:20:17 PST

Created attachment 45222 [details]
Updated patch v2

Yet another round, this time it should be ready for trunk.

Comment 6 WebKit Review Bot 2009-12-18 20:24:15 PST

style-queue ran check-webkit-style on attachment 45222 [details] without any errors.

Comment 7 Eric Seidel (no email) 2009-12-18 20:29:38 PST

Comment on attachment 45222 [details]
Updated patch v2

Why remove:
184          if (orientAngle())
?

In general this looks great though.   Does it break v8?  The Chromium EWS should tell us.

Comment 8 Nikolas Zimmermann 2009-12-18 20:35:41 PST

(In reply to comment #7)
> (From update of attachment 45222 [details])
> Why remove:
> 184          if (orientAngle())
> ?
> 
> In general this looks great though.   Does it break v8?  The Chromium EWS
> should tell us.

Landed in r52373. As the chromium ews bot is down, I'll just risk it, and watch the bots :-)

Comment 9 Adam Barth 2009-12-18 21:04:51 PST

> Landed in r52373. As the chromium ews bot is down, I'll just risk it, and watch
> the bots :-)

We need a better dashboard for seeing success.  Turns out this patch passed:
http://webkit-commit-queue.appspot.com/patch-status/chromium-ews/45222