Bug 12936 - Fix most remaining <use> bugs.
Summary: Fix most remaining <use> bugs.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
: 12267 12916 12917 12926 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-03-01 10:04 PST by Nikolas Zimmermann
Modified: 2007-03-13 02:41 PDT (History)
5 users (show)

See Also:


Attachments
Initial patch (24.10 KB, patch)
2007-03-01 10:16 PST, Nikolas Zimmermann
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2007-03-01 10:04:10 PST
Filing one master bug, whose attached patch fixes several bugs:
12926, 12267, 12916, 12917, 12838

Attaching patch soon.
Comment 1 Nikolas Zimmermann 2007-03-01 10:05:17 PST
*** Bug 12926 has been marked as a duplicate of this bug. ***
Comment 2 Nikolas Zimmermann 2007-03-01 10:05:43 PST
*** Bug 12267 has been marked as a duplicate of this bug. ***
Comment 3 Nikolas Zimmermann 2007-03-01 10:06:27 PST
*** Bug 12916 has been marked as a duplicate of this bug. ***
Comment 4 Nikolas Zimmermann 2007-03-01 10:06:48 PST
*** Bug 12917 has been marked as a duplicate of this bug. ***
Comment 5 Nikolas Zimmermann 2007-03-01 10:07:33 PST
*** Bug 12838 has been marked as a duplicate of this bug. ***
Comment 6 Nikolas Zimmermann 2007-03-01 10:16:45 PST
Created attachment 13437 [details]
Initial patch

Doesn't contain any layout test updates yet - as I'm on a "--svg-experimental" build. If this patch gets r+ I'll create new test results with a fresh ToT build + my patch applied.
Comment 7 Darin Adler 2007-03-04 23:08:34 PST
Comment on attachment 13437 [details]
Initial patch

+    return svgElement;

Avoid reference count churn by saying return svgElement.release(), which allows the RefPtr inside the function to give up the pointer without doing a ref followed by a deref.

+    String widthString = String::number(width().value());
+    String heightString = String::number(height().value());

A shame to compute these both even when there's no width or height attribute. I sugestmoving these expressions inside the if statements. You probably don't need the local variables.

+                    String transformString = String::format("translate(%f, %f)", use->x().value(), use->y().value());

I see this pattern repeated multiple times. I suggest some sort of helper function to do this. Maybe a new function appendTransform(const FloatSize&) as a member or just a helper function.

Do the two layout tests really cover all the problems?

r=me
Comment 8 Nikolas Zimmermann 2007-03-05 17:49:04 PST
Landed in r19972.
Comment 9 Nikolas Zimmermann 2007-03-05 17:50:54 PST
Hi Darin,

forgot to fix the issue you mentioned before comitting.
Need to get some sleep now will fix it tomorrow.

> (From update of attachment 13437 [details] [edit])
> +    return svgElement;
> 
> Avoid reference count churn by saying return svgElement.release(), which allows
> the RefPtr inside the function to give up the pointer without doing a ref
> followed by a deref.
okay

> 
> +    String widthString = String::number(width().value());
> +    String heightString = String::number(height().value());
> 
> A shame to compute these both even when there's no width or height attribute. I
> sugestmoving these expressions inside the if statements. You probably don't
> need the local variables.
Okay.

> 
> +                    String transformString = String::format("translate(%f,
> %f)", use->x().value(), use->y().value());
> 
> I see this pattern repeated multiple times. I suggest some sort of helper
> function to do this. Maybe a new function appendTransform(const FloatSize&) as
> a member or just a helper function.
Sounds reasonable.
 
> Do the two layout tests really cover all the problems?
Yes, I think so - will need to check all individual bug reports again.

Niko
Comment 10 jay 2007-03-13 02:41:23 PDT
niko,

reopening 12838 as broken, also widening scope as CSS styles are effected irrespective of scripting.