Summary: | Rewrite SVG <use> support in a modern-fashion | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikolas Zimmermann <zimmermann> | ||||||||||
Component: | SVG | Assignee: | Nikolas Zimmermann <zimmermann> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, bdakin, eric, hamaji, jay, jeffschiller, krit, oliver, staikos, webkit.review.bot | ||||||||||
Priority: | P1 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Nikolas Zimmermann
2010-01-17 16:49:01 PST
*** Bug 25035 has been marked as a duplicate of this bug. *** *** Bug 17424 has been marked as a duplicate of this bug. *** Still waiting for the patch ;-) 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...
Created attachment 46860 [details]
Updated patch
A real patch, discussed with Dirk on IRC.
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.
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.
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.
Created attachment 46870 [details]
Updated patch v3
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!
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.
(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? 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. Committed r53446: <http://trac.webkit.org/changeset/53446> > 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);
}
(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. |