Closed Bug 410811 Opened 12 years ago Closed 12 years ago

Implement GetNearestViewportElement and GetFarthestViewportElement

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: longsonr, Assigned: longsonr)

References

()

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
As noted in bug 409376 comment 12
Attachment #295382 - Flags: review?(tor)
Comment on attachment 295382 [details] [diff] [review]
patch

on second thoughts I'll move the implementation to nsSVGUtils then I only need one copy of each function.
Attachment #295382 - Attachment is obsolete: true
Attachment #295382 - Flags: review?(tor)
Attached patch updated patch (obsolete) — Splinter Review
same logic as previously but moved to nsSVGUtils so there is only one copy of each function
Attachment #295420 - Flags: review?(tor)
Comment on attachment 295420 [details] [diff] [review]
updated patch

For a future bug/patch, it might be interested to see how these nsSVGUtil methods could be generalized to be used by other code in content/svg that does similar lookups.
Attachment #295420 - Flags: review?(tor) → review+
Attachment #295420 - Flags: superreview?(roc)
+      *aNearestViewportElement = SVGElement;
+      NS_ADDREF(*aNearestViewportElement);

use .swap()

+      nsCOMPtr<nsIDOMSVGElement> SVGElement = do_QueryInterface(element);
+      *aFarthestViewportElement = SVGElement;

hoist SVGElement to the outermost scope and use .swap() at the end of the function.

This looks OK but is this the right thing to be working on for 1.9?
I'm doing this so that the litmus test for bug 409376 causes less confusion to testers see bug 409376 comment 12. At the moment when you look at the test and the reference image they are different and also you get errors in the error console. This patch corrects both issues so I'm hoping it will mean less work in the long run.
Attachment #295420 - Attachment is obsolete: true
Attachment #296246 - Flags: superreview?(roc)
Attachment #295420 - Flags: superreview?(roc)
Comment on attachment 296246 [details] [diff] [review]
address sr comments

OK, but we could create a testcase that doesn't have those problems.
Attachment #296246 - Flags: superreview?(roc) → superreview+
Comment on attachment 296246 [details] [diff] [review]
address sr comments

Low risk. The new methods are only called by the SVG DOM so this implementation can't break any existing functionality.
Attachment #296246 - Flags: approval1.9?
Comment on attachment 296246 [details] [diff] [review]
address sr comments

a=beltzner for 1.9
Attachment #296246 - Flags: approval1.9? → approval1.9+
checked in and in and in. Must try harder.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
bug 483389 includes mochitests for this.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.