Bug 135046 – Turn width/height to presentation attributes

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Dirk Schulze]

The width and height attributes for <svg>, <image>, <pattern>, <mask>, <filter> and <foreignObject> will be CSS propeties. SVG DOM and SVG animation behavior must be preserved.

[Dirk Schulze]

Created attachment 235119 [details]
Patch

The patch has a detailed description in the change log as base for future patches that will have less descriptions.

[Build Bot]

Comment on attachment 235119 [details]
Patch

Attachment 235119 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6153426966675456

New failing tests:
svg/zoom/page/zoom-foreignObject.svg

[Build Bot]

Created attachment 235122 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 235119 [details]
Patch

Attachment 235119 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6111123417858048

New failing tests:
svg/zoom/page/zoom-foreignObject.svg

[Build Bot]

Created attachment 235123 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 235119 [details]
Patch

Attachment 235119 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6537957267734528

New failing tests:
svg/zoom/page/zoom-foreignObject.svg

[Build Bot]

Created attachment 235124 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Dirk Schulze]

Created attachment 235133 [details]
Patch

See previous comment.

[Dirk Schulze]

Created attachment 235138 [details]
Patch

more tries to get GTK building...

[Dirk Schulze]

Stupid EFL build system.

[Dirk Schulze]

Created attachment 235151 [details]
Patch

This time!

[Dirk Schulze]

(In reply to comment #11)
> Created an attachment (id=235151) [details]
> Patch
> 
> This time!

The build failures seem to be unrelated to the bug. At least I don't see any evidence in the build output. The previous patch built on windows before.

[Dirk Schulze]

Created attachment 235181 [details]
Same patch again to see if Win passes now.

[Dean Jackson]

Comment on attachment 235181 [details]
Same patch again to see if Win passes now.

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

> Source/WebCore/svg/SVGElement.cpp:544
> +    auto svgIt = cssPropertyWithSVGDOMMap.find(attributeName.impl());
> +    if (svgIt != cssPropertyWithSVGDOMMap.end())

Nit: I think if you use auto you should probably call the variable svgIterator, or svgPropertyIterator or something.

> Source/WebCore/svg/SVGElement.h:104
> +        // Trigger style recalculation for "elements as resource" (e.g. referenced by feImage).
> +        setNeedsStyleRecalc(InlineStyleChange);

Is this going to be a big performance hit?

[Dirk Schulze]

Created attachment 235285 [details]
Performance test

(In reply to comment #14)
> (From update of attachment 235181 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235181&action=review
> 
> > Source/WebCore/svg/SVGElement.h:104
> > +        // Trigger style recalculation for "elements as resource" (e.g. referenced by feImage).
> > +        setNeedsStyleRecalc(InlineStyleChange);
> 
> Is this going to be a big performance hit?

I attached a performance test. I commented out the line in question and it did not have a measurable performance impact. In general with the patch we have 30%-40% more runs/s than without the patch (tested on tip, both with debug build). I wouldn't take this result too serious though. Even if the results are stable (~390 runs/s w/o patch ~610 runs/s with patch), it just might be that we bypassed some special testing that we just compile for debug builds. Testing release builds would make more sense.

For now it is just important that we do not regress with setNeedsStyleRecalc on performance.

To the test: It switches the relative width of a couple of dozens rectangles between 98% and 99%. Just elements that are NOT resources by other elements are from interest for this line.

[Dirk Schulze]

Committed r171341: <http://trac.webkit.org/changeset/171341>