Bug 135046 - Turn width/height to presentation attributes
Summary: Turn width/height to presentation attributes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 135045
  Show dependency treegraph
 
Reported: 2014-07-18 01:40 PDT by Dirk Schulze
Modified: 2019-09-21 09:05 PDT (History)
4 users (show)

See Also:


Attachments
Patch (65.34 KB, patch)
2014-07-18 02:35 PDT, Dirk Schulze
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (640.97 KB, application/zip)
2014-07-18 04:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (643.43 KB, application/zip)
2014-07-18 05:22 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (721.98 KB, application/zip)
2014-07-18 06:27 PDT, Build Bot
no flags Details
Patch (66.46 KB, patch)
2014-07-18 11:39 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (66.46 KB, patch)
2014-07-18 12:31 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (66.47 KB, patch)
2014-07-18 15:19 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Same patch again to see if Win passes now. (66.47 KB, patch)
2014-07-20 02:31 PDT, Dirk Schulze
dino: review+
Details | Formatted Diff | Diff
Performance test (8.18 KB, text/html)
2014-07-22 05:47 PDT, Dirk Schulze
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2014-07-18 01:40:26 PDT
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.
Comment 1 Dirk Schulze 2014-07-18 02:35:23 PDT
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.
Comment 2 Build Bot 2014-07-18 04:18:30 PDT
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
Comment 3 Build Bot 2014-07-18 04:18:32 PDT
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
Comment 4 Build Bot 2014-07-18 05:22:44 PDT
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
Comment 5 Build Bot 2014-07-18 05:22:48 PDT
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
Comment 6 Build Bot 2014-07-18 06:27:19 PDT
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
Comment 7 Build Bot 2014-07-18 06:27:23 PDT
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
Comment 8 Dirk Schulze 2014-07-18 11:39:42 PDT
Created attachment 235133 [details]
Patch

See previous comment.
Comment 9 Dirk Schulze 2014-07-18 12:31:40 PDT
Created attachment 235138 [details]
Patch

more tries to get GTK building...
Comment 10 Dirk Schulze 2014-07-18 15:18:42 PDT
Stupid EFL build system.
Comment 11 Dirk Schulze 2014-07-18 15:19:17 PDT
Created attachment 235151 [details]
Patch

This time!
Comment 12 Dirk Schulze 2014-07-19 09:50:38 PDT
(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.
Comment 13 Dirk Schulze 2014-07-20 02:31:51 PDT
Created attachment 235181 [details]
Same patch again to see if Win passes now.
Comment 14 Dean Jackson 2014-07-21 15:54:30 PDT
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?
Comment 15 Dirk Schulze 2014-07-22 05:47:53 PDT
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.
Comment 16 Dirk Schulze 2014-07-22 06:28:07 PDT
Committed r171341: <http://trac.webkit.org/changeset/171341>