Since bug 589640 is fixed, and removed the eSVG type checks, nsIContent::IsSVG() does a namespace check like nsIContent::IsHTML(). As a followup to that, we can clean up the places that do (nsIContent object)->GetNameSpaceID() == kNameSpaceID_SVG.

This (mostly) just replaces the namespace checks with calls to IsSVG(). I left a few namespace checks where the namespace ID was compared to multiple different namespaces (some of the tree building code does that). I did make two non-SVG changes along the same lines: nsAccessibilityService.cpp had a namespace check for MathML right below an SVG one, so I changed both. nsXMLFragmentContentSink.cpp did the same with the XHTML namespace. There were a handful of of places that did |obj->GetNamespaceID() == kNameSpaceID_SVG && obj->Tag() == [some atom]|. I changed the GetNamespaceID() parts of those, but is there a better way to do that check?

obj->NodeInfo()->Equals([some atom], kNameSpaceID_SVG)

> I changed the GetNamespaceID() parts of those, but is there a better way to do that check? Yes. Add an IsSVG signature that takes an atom as argument. See the corresponding IsHTML code. What comment 2 suggests would work, but is much less readable than the better IsSVG signature, imo.

IsSVG with an atom sounds better to me too.

Slightly revised, with a new IsSVG(atom) function.

Comment on attachment 580441 [details] [diff] [review] Clean up SVG namespace checks, v2 >- else if (content->GetNameSpaceID() == kNameSpaceID_MathML && >- content->Tag() == nsGkAtoms::math) { >+ else if (content->NodeInfo()->Equals(nsGkAtoms::math, kNameSpaceID_SVG)) { You want MathML as the namespace here. Or better, create a IsMathML method that takes a tag. >diff --git a/image/src/SVGDocumentWrapper.cpp b/image/src/SVGDocumentWrapper.cpp >--- a/image/src/SVGDocumentWrapper.cpp >+++ b/image/src/SVGDocumentWrapper.cpp >@@ -460,19 +460,17 @@ SVGDocumentWrapper::GetRootSVGElem() > if (!mViewer) > return nsnull; // Can happen during destruction > > nsIDocument* doc = mViewer->GetDocument(); > if (!doc) > return nsnull; // Can happen during destruction > > Element* rootElem = mViewer->GetDocument()->GetRootElement(); >- if (!rootElem || >- rootElem->GetNameSpaceID() != kNameSpaceID_SVG || >- rootElem->Tag() != SVGDocumentWrapper::kSVGAtom) { >+ if (!rootElem || !rootElem->IsSVG(SVGDocumentWrapper::kSVGAtom)) { > return nsnull; > } kSVGAtom is disappearing in bug 708888. That's landed on inbound but not on central yet. You'll need to fix up this patch once it does or do a patch against mozilla-inbound.

(In reply to Robert Longson from comment #6) > > You want MathML as the namespace here. Or better, create a IsMathML method > that takes a tag. s/a tag/an atom/

Comment on attachment 580441 [details] [diff] [review] Clean up SVG namespace checks, v2 This looks good, except for the following: >+++ b/accessible/src/base/nsAccessibilityService.cpp >@@ -1194,23 +1194,21 @@ nsAccessibilityService::GetOrCreateAcces > if (!newAcc) { > // Elements may implement nsIAccessibleProvider via XBL. This allows them to > // say what kind of accessible to create. > newAcc = CreateAccessibleByType(content, aWeakShell); > } > > if (!newAcc) { > // Create generic accessibles for SVG and MathML nodes. >- if (content->GetNameSpaceID() == kNameSpaceID_SVG && >- content->Tag() == nsGkAtoms::svg) { >+ if (content->IsSVG(nsGkAtoms::svg)) { > newAcc = new nsEnumRoleAccessible(content, aWeakShell, > nsIAccessibleRole::ROLE_DIAGRAM); > } >- else if (content->GetNameSpaceID() == kNameSpaceID_MathML && >- content->Tag() == nsGkAtoms::math) { >+ else if (content->NodeInfo()->Equals(nsGkAtoms::math, kNameSpaceID_SVG)) { You seem to have accidentally changed kNameSpaceID_MathML to kNameSpaceID_SVG.

Meh, somehow didn't see that Robert already said that.

(In reply to Robert Longson from comment #6) > You want MathML as the namespace here. Or better, create a IsMathML method > that takes a tag. Whoops, chalk that up to copy/paste failures. > kSVGAtom is disappearing in bug 708888. That's landed on inbound but not on > central yet. You'll need to fix up this patch once it does or do a patch > against mozilla-inbound. I'll rebase the patch mozilla-inbound (as soon as hg clone finishes) and post the new patch.

New patch, for mozilla-inbound, with both of those fixes.

Comment on attachment 580519 [details] [diff] [review] Clean up SVG namespace checks, v3 (against mozilla-inbound) great stuff

(In reply to Robert Longson from comment #12) > Comment on attachment 580519 [details] [diff] [review] > Clean up SVG namespace checks, v3 (against mozilla-inbound) > > great stuff Thanks for reviewing =)

In my queue with a few other checkin-neededs that are being sent to try first and then onto inbound :-) https://tbpl.mozilla.org/?tree=Try&rev=fd440327d5e4