The default bug view has changed. See this FAQ.

Some namespaces should be checked in nsMathMLContainerFrame.cpp

RESOLVED FIXED in mozilla7

Status

()

Core
MathML
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: fredw, Assigned: Daniel Cazacu)

Tracking

Trunk
mozilla7
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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 ||
Er.... kNameSpaceID_MathML, no?
(Reporter)

Comment 2

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

Yes :-)
(Reporter)

Updated

6 years ago
Assignee: nobody → cazacu.daniell
(Reporter)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

6 years ago
Created attachment 541669 [details] [diff] [review]
The .diff file for the modifications where needed.

Attempt at solving first bug.
(Assignee)

Updated

6 years ago
Attachment #541669 - Flags: review?(karlt)
(Reporter)

Comment 4

6 years ago
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
Attachment #541669 - Flags: review?(karlt)
(Assignee)

Comment 5

6 years ago
Created attachment 542102 [details] [diff] [review]
Revised patch, open for review
Attachment #541669 - Attachment is obsolete: true
Attachment #542102 - Flags: review?(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?
Attachment #542102 - Flags: review?(karlt) → review+
(Assignee)

Comment 7

6 years ago
Created attachment 542398 [details] [diff] [review]
Revised patch, already reviewed

Now with the extra space added.
Attachment #542102 - Attachment is obsolete: true
(Reporter)

Comment 8

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

Comment 9

6 years ago
Created attachment 542767 [details] [diff] [review]
Final patch

Rebuilt the patch to fix the syntax error.
Attachment #542398 - Attachment is obsolete: true
(Reporter)

Comment 10

6 years ago
(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.
Keywords: checkin-needed
(Reporter)

Updated

6 years ago
Attachment #542767 - Attachment description: Revised, hopefully fixed corrupt sintax. → Final patch
Attachment #542767 - Flags: review+

Updated

6 years ago
Flags: in-testsuite?
http://hg.mozilla.org/mozilla-central/rev/4c042c127a02
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.