Bug 12936 – Fix most remaining <use> bugs.

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Filing one master bug, whose attached patch fixes several bugs:
12926, 12267, 12916, 12917, 12838

Attaching patch soon.

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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.

[Darin Adler]

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

[Nikolas Zimmermann]

Landed in r19972.

[Nikolas Zimmermann]

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

[jay]

niko,

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