Last Comment Bug 648140 - Some namespaces should be checked in nsMathMLContainerFrame.cpp
: Some namespaces should be checked in nsMathMLContainerFrame.cpp
Status: RESOLVED FIXED
[good first bug]
:
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Daniel Cazacu
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-06 15:25 PDT by Frédéric Wang (:fredw)
Modified: 2011-07-01 05:44 PDT (History)
2 users (show)
Ms2ger: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
The .diff file for the modifications where needed. (1.92 KB, patch)
2011-06-24 06:41 PDT, Daniel Cazacu
no flags Details | Diff | Splinter Review
Revised patch, open for review (2.16 KB, patch)
2011-06-27 00:42 PDT, Daniel Cazacu
karlt: review+
Details | Diff | Splinter Review
Revised patch, already reviewed (2.16 KB, patch)
2011-06-28 01:33 PDT, Daniel Cazacu
no flags Details | Diff | Splinter Review
Final patch (2.11 KB, patch)
2011-06-29 04:15 PDT, Daniel Cazacu
fred.wang: review+
Details | Diff | Splinter Review

Description Frédéric Wang (:fredw) 2011-04-06 15:25:28 PDT
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 [:bz] (still a bit busy) 2011-04-06 15:37:34 PDT
Er.... kNameSpaceID_MathML, no?
Comment 2 Frédéric Wang (:fredw) 2011-04-06 22:54:06 PDT
(In reply to comment #1)
> Er.... kNameSpaceID_MathML, no?

Yes :-)
Comment 3 Daniel Cazacu 2011-06-24 06:41:03 PDT
Created attachment 541669 [details] [diff] [review]
The .diff file for the modifications where needed.

Attempt at solving first bug.
Comment 4 Frédéric Wang (:fredw) 2011-06-24 06:53:02 PDT
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 2011-06-27 00:42:21 PDT
Created attachment 542102 [details] [diff] [review]
Revised patch, open for review
Comment 6 Karl Tomlinson (:karlt) 2011-06-27 14:23:33 PDT
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 2011-06-28 01:33:42 PDT
Created attachment 542398 [details] [diff] [review]
Revised patch, already reviewed

Now with the extra space added.
Comment 8 Frédéric Wang (:fredw) 2011-06-28 10:09:22 PDT
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 2011-06-29 04:15:20 PDT
Created attachment 542767 [details] [diff] [review]
Final patch

Rebuilt the patch to fix the syntax error.
Comment 10 Frédéric Wang (:fredw) 2011-06-29 04:20:51 PDT
(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] 2011-07-01 05:44:17 PDT
http://hg.mozilla.org/mozilla-central/rev/4c042c127a02

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