Bug 33776 – Rewrite SVG <use> support in a modern-fashion

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

[Nikolas Zimmermann]

SVGUseElement was designed in a time where our SVG render tree had zero integration with the rest and where the shadow tree concept has recently been introduced.
It suffers from several design problems. The most important thing to fix is the relationship between the render tree & SVGUseElement - who owns what, who reacts to style updates etc. (SVGUseElement was hacking around the fact there was no renderer managing the shadow tree -> fix that by introducing a RenderSVGShadowTreeContainer). Even more important is to recreate the shadow tree in a _lazy_ way. Currently every attribute change, of every element referenced by a <use> element causes a full recloning (just think about animations ;-)

I've rewritten the <use> support and it's __a lot__ faster now, and fixes tons of bugs, mostly related to relayouting <use> content with percentual values in-use.
Uploading a patch soon, after identifying which bug reports are fixed now :-)

[Nikolas Zimmermann]

*** Bug 25035 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 17424 has been marked as a duplicate of this bug. ***

[Dirk Schulze]

Still waiting for the patch ;-)

[Nikolas Zimmermann]

Created attachment 46843 [details]
Almost a patch

Adding the WebCore part, does not contain a ChangeLog, not marking for review yet. Just for Dirk to have a look before I proceed...

[Nikolas Zimmermann]

Created attachment 46860 [details]
Updated patch

A real patch, discussed with Dirk on IRC.

[WebKit Review Bot]

Attachment 46860 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/svg/SVGUseElement.cpp:171:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/svg/SVGUseElement.cpp:172:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderSVGTransformableContainer.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

Created attachment 46869 [details]
Updated patch v2

Fix last-minute regressions, in changes I made on the fly while working on Dirks comments :/ Passes pixel tests without regressions with tolerance=0 here.
Testing SVG animations is a joke though, as the W3C-SVG-1.1 are dumped before the anim even starts - unfortunately Antti did not work on that while creating SVG animations support.. anyhow I've manually checked the anims work fine.

[WebKit Review Bot]

Attachment 46869 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/RenderSVGTransformableContainer.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/rendering/RenderSVGTransformableContainer.cpp:54:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

Created attachment 46870 [details]
Updated patch v3

[Dirk Schulze]

Comment on attachment 46870 [details]
Updated patch v3

Niko an I had a long discussen and went through the code step by step. Niko already fixed all issues.
The zero check is neccessary here. Negative offsets are allowed. Niko will change the  alphabetic order before landing.
 r=me
Great patch Niko!

[WebKit Review Bot]

Attachment 46870 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/RenderSVGTransformableContainer.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/rendering/RenderSVGTransformableContainer.cpp:54:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

(In reply to comment #11)
> Attachment 46870 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebCore/rendering/RenderSVGTransformableContainer.cpp:28:  Alphabetical sorting
> problem.  [build/include_order] [4]

Will fix before landing.

> WebCore/rendering/RenderSVGTransformableContainer.cpp:54:  Tests for
> true/false, null/non-null, and zero/non-zero should all be done without
> equality comparisons.  [readability/comparison_to_zero] [5]

This is expected, we want to check for width/height exactly to be 0, they can be negative so !width, won't help. Shall I fix a bug? Eric?

[Eric Seidel (no email)]

I don't deal much with the style bot, but yes.  The general answer is you should always file a bug. :)  false positives are very-bad as they make people not trust the bot.

[Nikolas Zimmermann]

Committed r53446: <http://trac.webkit.org/changeset/53446>

[Shinichiro Hamaji]

> This is expected, we want to check for width/height exactly to be 0, they can
> be negative so !width, won't help. Shall I fix a bug? Eric?

I don't know well about this area well, but I tried the following code and I didn't see any difference between f==0 and !f . I guess we need to use signbit() to check if a float is a positive-zero.

#include <stdio.h>
#include <math.h>

void isPositiveZero(float f) {
  printf("=== %f ===\n", f);
  printf("f == 0: %d\n", f == 0);
  printf("f == 0.0f: %d\n", f == 0.0f);
  printf("f == 0.0: %d\n", f == 0.0);
  printf("!f: %d\n", !f);
  // The above 4 checks return 1 for both 0.0f and -0.0f.
  printf("!f && !signbit(f): %d\n", !f && !signbit(f));
}

int main() {
  isPositiveZero(0.0f);
  isPositiveZero(-0.0f);
}

[Dirk Schulze]

(In reply to comment #15)
> > This is expected, we want to check for width/height exactly to be 0, they can
> > be negative so !width, won't help. Shall I fix a bug? Eric?
> 
> I don't know well about this area well, but I tried the following code and I
> didn't see any difference between f==0 and !f . I guess we need to use
> signbit() to check if a float is a positive-zero.
> 
> #include <stdio.h>
> #include <math.h>
> 
> void isPositiveZero(float f) {
>   printf("=== %f ===\n", f);
>   printf("f == 0: %d\n", f == 0);
>   printf("f == 0.0f: %d\n", f == 0.0f);
>   printf("f == 0.0: %d\n", f == 0.0);
>   printf("!f: %d\n", !f);
>   // The above 4 checks return 1 for both 0.0f and -0.0f.
>   printf("!f && !signbit(f): %d\n", !f && !signbit(f));
> }
> 
> int main() {
>   isPositiveZero(0.0f);
>   isPositiveZero(-0.0f);
> }
You may have misunaderstood Niko. It's not the question of 0 or -0 or other combinations of zero. It's just, that we don't make something if the offset is zero, but make translations if the offset gets negative like (-3,-4). That is not covered with !offset. So the style bot is a bit to strict on this rule.