708846 - Clean up some of the SVG namespace checks

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Description

[Brian O'Keefe [:bokeefe]]

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.

Comment 1

[Brian O'Keefe [:bokeefe]]

Attachment: Clean up SVG namespace checks

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?

Comment 2

[Robert Longson [:longsonr]]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 4

[Robert Longson [:longsonr]]

IsSVG with an atom sounds better to me too.

Comment 5

[Brian O'Keefe [:bokeefe]]

Attachment: Clean up SVG namespace checks, v2

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

Comment 6

[Robert Longson [:longsonr]]

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.

Comment 7

[Robert Longson [:longsonr]]

(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 8

[Jonathan Watt [:jwatt]]

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.

Comment 9

[Jonathan Watt [:jwatt]]

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

Comment 10

[Brian O'Keefe [:bokeefe]]

(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.

Comment 11

[Brian O'Keefe [:bokeefe]]

Attachment: Clean up SVG namespace checks, v3 (against mozilla-inbound)

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

Comment 12

[Robert Longson [:longsonr]]

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

great stuff

Comment 13

[Brian O'Keefe [:bokeefe]]

(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 =)

Comment 14

[Ed Morley [:emorley]]

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

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5df2a353f393

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5df2a353f393