Bug 48204 - Convert SVGRect to the new SVGPropertyTearOff concept
Summary: Convert SVGRect to the new SVGPropertyTearOff concept
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 47905
  Show dependency treegraph
 
Reported: 2010-10-24 04:29 PDT by Nikolas Zimmermann
Modified: 2010-10-27 14:32 PDT (History)
7 users (show)

See Also:


Attachments
Patch (71.38 KB, patch)
2010-10-26 07:34 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v2 (71.62 KB, patch)
2010-10-26 11:39 PDT, Nikolas Zimmermann
rwlbuis: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2010-10-24 04:29:43 PDT
Convert SVGRect to the new SVGPropertyTearOff concept. Requires no new generator features, will be a simple patch.
Comment 1 Nikolas Zimmermann 2010-10-26 07:34:45 PDT
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.
Comment 2 Eric Seidel (no email) 2010-10-26 10:38:04 PDT
Attachment 71880 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4792016
Comment 3 Nikolas Zimmermann 2010-10-26 10:49:33 PDT
It built fine for me.... retrying build from scratch after gclient sync.
Comment 4 Nikolas Zimmermann 2010-10-26 11:39:30 PDT
Created attachment 71918 [details]
Patch v2

Fix chromium release builds - an unused variable was generated in the v8 build.
Comment 5 Rob Buis 2010-10-26 13:31:15 PDT
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.
Comment 6 Nikolas Zimmermann 2010-10-26 14:02:58 PDT
Landed in r70573.
Comment 7 Dimitri Glazkov (Google) 2010-10-26 15:35:14 PDT
Reverted r70573 for reason:

Broke 39 tests on Chromium

Committed r70581: <http://trac.webkit.org/changeset/70581>
Comment 8 Dimitri Glazkov (Google) 2010-10-26 15:36:37 PDT
(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
Comment 9 Nikolas Zimmermann 2010-10-26 23:51:41 PDT
(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...
Comment 10 Nikolas Zimmermann 2010-10-27 01:01:01 PDT
(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.
Comment 11 Nikolas Zimmermann 2010-10-27 03:27:38 PDT
Relanded in r70631.