RESOLVED FIXED 12936
Fix most remaining <use> bugs.
https://bugs.webkit.org/show_bug.cgi?id=12936
Summary Fix most remaining <use> bugs.
Nikolas Zimmermann
Reported 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.
Attachments
Initial patch (24.10 KB, patch)
2007-03-01 10:16 PST, Nikolas Zimmermann
darin: review+
Nikolas Zimmermann
Comment 1 2007-03-01 10:05:17 PST
*** Bug 12926 has been marked as a duplicate of this bug. ***
Nikolas Zimmermann
Comment 2 2007-03-01 10:05:43 PST
*** Bug 12267 has been marked as a duplicate of this bug. ***
Nikolas Zimmermann
Comment 3 2007-03-01 10:06:27 PST
*** Bug 12916 has been marked as a duplicate of this bug. ***
Nikolas Zimmermann
Comment 4 2007-03-01 10:06:48 PST
*** Bug 12917 has been marked as a duplicate of this bug. ***
Nikolas Zimmermann
Comment 5 2007-03-01 10:07:33 PST
*** Bug 12838 has been marked as a duplicate of this bug. ***
Nikolas Zimmermann
Comment 6 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.
Darin Adler
Comment 7 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
Nikolas Zimmermann
Comment 8 2007-03-05 17:49:04 PST
Landed in r19972.
Nikolas Zimmermann
Comment 9 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
jay
Comment 10 2007-03-13 02:41:23 PDT
niko, reopening 12838 as broken, also widening scope as CSS styles are effected irrespective of scripting.
Note You need to log in before you can comment on or make changes to this bug.