Bug 12936

Summary:

Fix most remaining <use> bugs.

Product:

WebKit

Reporter:

Nikolas Zimmermann <zimmermann>

Component:

SVG

Assignee:

Nikolas Zimmermann <zimmermann>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

a.neumann, jay, kyle.rove, ml, rwlbuis

Priority:

Version:

523.x (Safari 3)

Hardware:

Mac

OS:

OS X 10.4

Attachments:

Description	Flags
Initial patch	darin: review+

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.