Summary: | Turn width/height to presentation attributes | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Schulze <krit> | ||||||||||||||||||||
Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | buildbot, dino, rniwa, zimmermann | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||
Bug Blocks: | 135045 | ||||||||||||||||||||||
Attachments: |
|
Description
Dirk Schulze
2014-07-18 01:40:26 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 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 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 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 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 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 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
Created attachment 235133 [details]
Patch
See previous comment.
Created attachment 235138 [details]
Patch
more tries to get GTK building...
Stupid EFL build system. Created attachment 235151 [details]
Patch
This time!
(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. Created attachment 235181 [details]
Same patch again to see if Win passes now.
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? 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. Committed r171341: <http://trac.webkit.org/changeset/171341> |