Last Comment Bug 708846 - Clean up some of the SVG namespace checks
: Clean up some of the SVG namespace checks
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla11
Assigned To: Brian O'Keefe [:bokeefe]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-08 14:27 PST by Brian O'Keefe [:bokeefe]
Modified: 2012-02-01 13:58 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Clean up SVG namespace checks (16.39 KB, patch)
2011-12-08 14:37 PST, Brian O'Keefe [:bokeefe]
no flags Details | Diff | Splinter Review
Clean up SVG namespace checks, v2 (17.72 KB, patch)
2011-12-09 09:10 PST, Brian O'Keefe [:bokeefe]
no flags Details | Diff | Splinter Review
Clean up SVG namespace checks, v3 (against mozilla-inbound) (18.01 KB, patch)
2011-12-09 13:06 PST, Brian O'Keefe [:bokeefe]
longsonr: review+
Details | Diff | Splinter Review

Description Brian O'Keefe [:bokeefe] 2011-12-08 14:27:24 PST
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] 2011-12-08 14:37:20 PST
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?
Comment 2 Robert Longson 2011-12-08 14:42:16 PST
obj->NodeInfo()->Equals([some atom], kNameSpaceID_SVG)
Comment 3 Boris Zbarsky [:bz] 2011-12-08 15:17:56 PST
> 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 2011-12-09 01:03:48 PST
IsSVG with an atom sounds better to me too.
Comment 5 Brian O'Keefe [:bokeefe] 2011-12-09 09:10:59 PST
Created attachment 580441 [details] [diff] [review]
Clean up SVG namespace checks, v2

Slightly revised, with a new IsSVG(atom) function.
Comment 6 Robert Longson 2011-12-09 09:33:44 PST
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 2011-12-09 09:35:33 PST
(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] (back in October - email directly if necessary) 2011-12-09 09:37:35 PST
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] (back in October - email directly if necessary) 2011-12-09 09:38:30 PST
Meh, somehow didn't see that Robert already said that.
Comment 10 Brian O'Keefe [:bokeefe] 2011-12-09 10:37:23 PST
(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] 2011-12-09 13:06:08 PST
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.
Comment 12 Robert Longson 2011-12-09 13:23:19 PST
Comment on attachment 580519 [details] [diff] [review]
Clean up SVG namespace checks, v3 (against mozilla-inbound)

great stuff
Comment 13 Brian O'Keefe [:bokeefe] 2011-12-09 13:45:54 PST
(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] 2011-12-15 02:34:04 PST
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 16 Ed Morley [:emorley] 2011-12-16 06:19:10 PST
https://hg.mozilla.org/mozilla-central/rev/5df2a353f393

Note You need to log in before you can comment on or make changes to this bug.