Closed Bug 484966 Opened 11 years ago Closed 9 years ago

Rename and move nsSVGUtils::GetParentElement

Categories

(Core :: SVG, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: jwatt, Assigned: juca)

Details

Attachments

(2 files)

It was completely unclear to me from the name that nsSVGUtils::GetParentElement gets the flattened tree parent. I had to do a bit of digging before the change I was looking at made sense. How about calling it GetFlatTreeParent, and how about also moving it to nsIContent just after GetParent? That seems like a better name and location to me.

If that's okay, this seems like a good first bug, and I have someone in mind. So no patches, okay. ;-)
Whiteboard: [good first bug]
Some kind of iterator class that allowed you to go up through the flat tree parents would be nice too. You would give it a starting element and it would call a virtual method for each ancestor till you told it to stop (return false from the virtual method) or it ran out of parents.
GetNearestViewport/GetFarthestViewport could be redone to use the iterator class.
I think we have an existing bug about having methods for iterating the flattened tree (needed for the DOM parts of XBL2), for what it's worth.  Probably assigned to bryner or some such...

I would probably be fine with putting something like this on nsIContent.
(In reply to comment #0)
> It was completely unclear to me from the name that nsSVGUtils::GetParentElement
> gets the flattened tree parent. I had to do a bit of digging before the change
> I was looking at made sense. How about calling it GetFlatTreeParent, and how
> about also moving it to nsIContent just after GetParent? That seems like a
> better name and location to me.

I just noticed that currently we already have nsIContent* nsIContent::GetFlattenedTreeParent() implemented in src/content/base/src/nsGenericElement.cpp but Element* nsSVGUtils::GetParentElement(nsIContent *aContent) has not been removed yet. So it seems that this bug has been partially fixed already.

I am not sure if it is the correct way of doing it, but I am submitting a patch for review. I would like to get some feedback on it and in case it is wrong, I'd like to learn why.
Status: NEW → ASSIGNED
Attached patch first try...Splinter Review
Assignee: nobody → felipe.sanches
Attachment #472794 - Flags: review?
You should pick a reviewer by entering an email address after you change the review flag to ?. In this case probably me or jwatt (http://www.mozilla.org/about/owners.html#svg).
Attachment #472794 - Flags: review? → review?(jwatt)
Comment on attachment 472794 [details] [diff] [review]
first try...

Looks good - thanks!
Attachment #472794 - Flags: review?(jwatt) → review+
Gah.  Why didn't someone push this back in September?

I'm going to take this temporarily so I remember to push it once Firefox 4 ships....
Assignee: juca → bzbarsky
Priority: -- → P1
Whiteboard: [good first bug] → [need gk2 ship][good first bug]
This bitrotted some time ago. It needs additional changes in nsSVGFilters.cpp at least now.
Like I said, "Gah".

Felipe, do you want to update the patch yourself?  If not, I'm happy to do that...
:/ Dunno why I didn't push it. Were we needing approval that far back? Anyways, I'm happy to update the patch since I'm mostly to blame here.
Felipe/bz: ping?
I'm planning to merge and push this; it just hasn't happened yet.
Attachment 524577 [details] [diff] is just the merge to tip.  I pushed that as http://hg.mozilla.org/projects/cedar/rev/f6cb911d37da

Reassigning back to Felipe, since this is his patch.
Assignee: bzbarsky → juca
Flags: in-testsuite-
Whiteboard: [need gk2 ship][good first bug] → fixed-in-cedar
Target Milestone: --- → mozilla2.2
http://hg.mozilla.org/mozilla-central/rev/f6cb911d37da
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Thanks for the patch Felipe, and thanks for catching this Boris!
You need to log in before you can comment on or make changes to this bug.