648140 - Some namespaces should be checked in nsMathMLContainerFrame.cpp

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Frédéric Wang (:fredw)]

David wrote XXX comments where he indicated we should check namespaces.
This can be done by using nsIContent::GetNameSpaceID and seeing if the result is kNameSpaceID_None.

fred@debian:~/mozilla/src/layout/mathml$ grep -n -C2 "XXX" *.cpp | grep -C2 namespace
nsMathMLContainerFrame.cpp-744-    if (!content)
nsMathMLContainerFrame.cpp-745-      break;
nsMathMLContainerFrame.cpp:746:    // XXXldb This should check namespaces too.
nsMathMLContainerFrame.cpp-747-    if (content->Tag() == nsGkAtoms::math)
nsMathMLContainerFrame.cpp-748-      break;
--
nsMathMLContainerFrame.cpp-1417-    return 0;
nsMathMLContainerFrame.cpp-1418-  }
nsMathMLContainerFrame.cpp:1419:  // XXXldb This should check namespaces too.
nsMathMLContainerFrame.cpp-1420-  nsIAtom *parentTag = parentContent->Tag();
nsMathMLContainerFrame.cpp-1421-  if (parentTag == nsGkAtoms::math ||

Comment 1

[Boris Zbarsky [:bzbarsky]]

Er.... kNameSpaceID_MathML, no?

Comment 2

[Frédéric Wang (:fredw)]

(In reply to comment #1)
> Er.... kNameSpaceID_MathML, no?

Yes :-)

Comment 3

[Daniel Cazacu]

Attachment: The .diff file for the modifications where needed.

Attempt at solving first bug.

Comment 4

[Frédéric Wang (:fredw)]

Comment on attachment 541669 [details] [diff] [review]
The .diff file for the modifications where needed.

Daniel, I think your patch is incorrect. It seems that it is already applied on top of other changes adding "content->GetNameSpaceID()==kNameSpaceID_MathML".

You should remove the XXXldb comment, since your patch is supposed to address it.

You should also add spaces around the == operators. Please read the link I sent to you: https://developer.mozilla.org/En/Developer_Guide/Coding_Style

Comment 5

[Daniel Cazacu]

Attachment: Revised patch, open for review

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 542102 [details] [diff] [review]
Revised patch, open for review

>+  if (parentContent->GetNameSpaceID() == kNameSpaceID_MathML && 
>+     (parentTag == nsGkAtoms::math || parentTag == nsGkAtoms::mtd_)) {

Can you indent the second line one more space, please, so that it lies within the parentheses of the condition test?

Comment 7

[Daniel Cazacu]

Attachment: Revised patch, already reviewed

Now with the extra space added.

Comment 8

[Frédéric Wang (:fredw)]

I can not apply the second and third versions. It seems that the

@@ -1399,20 +1399,20 @@

in the second hunk should be replaced by

@@ -1399,20 +1399,19 @@

Comment 9

[Daniel Cazacu]

Attachment: Final patch

Rebuilt the patch to fix the syntax error.

Comment 10

[Frédéric Wang (:fredw)]

(In reply to comment #9)
> Created attachment 542767 [details] [diff] [review] [review]
> Revised, hopefully fixed corrupt sintax.
> 
> Rebuilt the patch to fix the syntax error.

OK, this applies correctly, thanks.

Comment 11

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/4c042c127a02