Bug 48204 – Convert SVGRect to the new SVGPropertyTearOff concept

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Convert SVGRect to the new SVGPropertyTearOff concept. Requires no new generator features, will be a simple patch.

[Nikolas Zimmermann]

Created attachment 71880 [details]
Patch

Patch is larger than previously planned. I needed to refactor the SVGPropertyTearOff type handling from all generators into CodeGenerator.pm, to avoid adding hacks like s/SVGRect/FloatRect/ in all generators.
This also removes the need again for the temporary SVGAnimatedProperty/SVGProperty/SVGListProperty markers in the idl files.

Tested with a Mac Leopard debug build, and a Chrome build on Snow Leopard.

[Eric Seidel (no email)]

Attachment 71880 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4792016

[Nikolas Zimmermann]

It built fine for me.... retrying build from scratch after gclient sync.

[Nikolas Zimmermann]

Created attachment 71918 [details]
Patch v2

Fix chromium release builds - an unused variable was generated in the v8 build.

[Rob Buis]

Comment on attachment 71918 [details]
Patch v2

The only problem was const_cast usage in contextElement, WildFox said he'd fix it so r=me.

[Nikolas Zimmermann]

Landed in r70573.

[Dimitri Glazkov (Google)]

Reverted r70573 for reason:

Broke 39 tests on Chromium

Committed r70581: <http://trac.webkit.org/changeset/70581>

[Dimitri Glazkov (Google)]

(In reply to comment #7)
> Reverted r70573 for reason:
> 
> Broke 39 tests on Chromium
> 
> Committed r70581: <http://trac.webkit.org/changeset/70581>

Apologies for rolling this out, I couldn't find you on #webkit. Here are the failures (they look real):

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&useWebKitCanary=true&tests=svg%2Fcustom%2Fclone-element-with-animated-svg-properties.html%2Csvg%2Fcustom%2Fmouse-move-on-svg-root-standalone.svg%2Csvg%2Fcustom%2Fmouse-move-on-svg-root.xhtml%2Csvg%2Fcustom%2Fsvg-curve-with-relative-cordinates.html%2Csvg%2Fcustom%2Fuse-property-changes-through-svg-dom.svg%2Csvg%2Fcustom%2Fviewport-update2.svg%2Csvg%2Fdynamic-updates%2FSVGCircleElement-svgdom-cx-prop.html%2Csvg%2Fdynamic-updates%2FSVGCircleElement-svgdom-cy-prop.html%2Csvg%2Fdynamic-updates%2FSVGCircleElement-svgdom-r-prop.html%2Csvg%2Fdynamic-updates%2FSVGEllipseElement-svgdom-cx-prop.html%2Csvg%2Fdynamic-updates%2FSVGEllipseElement-svgdom-cy-prop.html%2Csvg%2Fdynamic-updates%2FSVGEllipseElement-svgdom-rx-prop.html%2Csvg%2Fdynamic-updates%2FSVGEllipseElement-svgdom-ry-prop.html%2Csvg%2Fdynamic-updates%2FSVGForeignObjectElement-svgdom-height-prop.html%2Csvg%2Fdynamic-updates%2FSVGForeignObjectElement-svgdom-width-prop.html%2Csvg%2Fdynamic-updates%2FSVGForeignObjectElement-svgdom-x-prop.html%2Csvg%2Fdynamic-updates%2FSVGForeignObjectElement-svgdom-y-prop.html%2Csvg%2Fdynamic-updates%2FSVGImageElement-svgdom-height-prop.html%2Csvg%2Fdynamic-updates%2FSVGImageElement-svgdom-width-prop.html%2Csvg%2Fdynamic-updates%2FSVGImageElement-svgdom-x-prop.html%2Csvg%2Fdynamic-updates%2FSVGImageElement-svgdom-y-prop.html%2Csvg%2Fdynamic-updates%2FSVGLineElement-svgdom-x1-prop.html%2Csvg%2Fdynamic-updates%2FSVGLineElement-svgdom-x2-prop.html%2Csvg%2Fdynamic-updates%2FSVGLineElement-svgdom-y1-prop.html%2Csvg%2Fdynamic-updates%2FSVGLineElement-svgdom-y2-prop.html%2Csvg%2Fdynamic-updates%2FSVGMarkerElement-svgdom-markerHeight-prop.html%2Csvg%2Fdynamic-updates%2FSVGMarkerElement-svgdom-markerWidth-prop.html%2Csvg%2Fdynamic-updates%2FSVGMarkerElement-svgdom-orientAngle-prop.html%2Csvg%2Fdynamic-updates%2FSVGMarkerElement-svgdom-refX-prop.html%2Csvg%2Fdynamic-updates%2FSVGMarkerElement-svgdom-refY-prop.html%2Csvg%2Fdynamic-updates%2FSVGMaskElement-svgdom-height-prop.html%2Csvg%2Fdynamic-updates%2FSVGMaskElement-svgdom-width-prop.html%2Csvg%2Fdynamic-updates%2FSVGMaskElement-svgdom-x-prop.html%2Csvg%2Fdynamic-updates%2FSVGMaskElement-svgdom-y-prop.html%2Csvg%2Fdynamic-updates%2FSVGRectElement-svgdom-height-prop.html%2Csvg%2Fdynamic-updates%2FSVGRectElement-svgdom-width-prop.html%2Csvg%2Fdynamic-updates%2FSVGRectElement-svgdom-x-prop.html%2Csvg%2Fdynamic-updates%2FSVGRectElement-svgdom-y-prop.html%2Csvg%2Fcustom%2Fuse-instanceRoot-event-bubbling.xhtml

[Nikolas Zimmermann]

(In reply to comment #7)
> Reverted r70573 for reason:
> 
> Broke 39 tests on Chromium
> 
> Committed r70581: <http://trac.webkit.org/changeset/70581>

Oh, that's a pity. Will look into how I can run tests on my own with my local chrome build.
I only tested building and running the svg/dom tests manually with TestShell.app.
Going to read the docs and try testing on Chrome. It worked fine on Leopard & Snow Leopard, so it has to be related to the V8 changes...

[Nikolas Zimmermann]

(In reply to comment #9)
> (In reply to comment #7)
> > Reverted r70573 for reason:
> > 
> > Broke 39 tests on Chromium
> > 
> > Committed r70581: <http://trac.webkit.org/changeset/70581>
> 
> Oh, that's a pity. Will look into how I can run tests on my own with my local chrome build.
> I only tested building and running the svg/dom tests manually with TestShell.app.
> Going to read the docs and try testing on Chrome. It worked fine on Leopard & Snow Leopard, so it has to be related to the V8 changes...

Running tests on Chromium was trivial, easily spotted the problem within minutes:

-    } elsif ($svgPropertyType) {
+    } elsif ($svgNativeType) {
         if ($useExceptions) {
             push(@implContentDecls, "    if (!ec)\n");
             push(@implContentDecls, "        wrapper->commitChange();\n");

That fixes Chrome, stupid me!
We were not commiting changes, after "foo.viewBox.x.baseVal = 100". The underlying variables have been changed, but layout was not triggered, nor was 'foo' marked for XML-DOM synchronization.

[Nikolas Zimmermann]

Relanded in r70631.