Clean up some of the SVG namespace checks

RESOLVED FIXED in mozilla11

Status

()

Core
SVG
--
enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bokeefe, Assigned: bokeefe)

Tracking

unspecified
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 1

6 years ago
Created attachment 580198 [details] [diff] [review]
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?
Attachment #580198 - Flags: feedback?(longsonr)

Comment 2

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

Updated

6 years ago
Assignee: nobody → bokeefe
> 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

6 years ago
IsSVG with an atom sounds better to me too.
(Assignee)

Comment 5

6 years ago
Created attachment 580441 [details] [diff] [review]
Clean up SVG namespace checks, v2

Slightly revised, with a new IsSVG(atom) function.
Attachment #580198 - Attachment is obsolete: true
Attachment #580198 - Flags: feedback?(longsonr)
Attachment #580441 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Attachment #580441 - Attachment is patch: true

Comment 6

6 years ago
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

6 years ago
(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.
(Assignee)

Comment 10

6 years ago
(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.
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Attachment #580441 - Flags: review?(bzbarsky)
(Assignee)

Comment 11

6 years ago
Created attachment 580519 [details] [diff] [review]
Clean up SVG namespace checks, v3 (against mozilla-inbound)

New patch, for mozilla-inbound, with both of those fixes.
Attachment #580441 - Attachment is obsolete: true
Attachment #580519 - Flags: review?(longsonr)

Comment 12

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

great stuff
Attachment #580519 - Flags: review?(longsonr) → review+
(Assignee)

Comment 13

6 years ago
(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 =)
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/5df2a353f393
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/5df2a353f393
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.