Closed Bug 488249 Opened 15 years ago Closed 15 years ago

Replace IsNodeOfType(eHTML) checks with namespace checks

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: hsivonen, Assigned: dzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

Replace calls to virtual IsNodeOfType(eHTML) with non-virtual namespace id checks once both HTML and XHTML nodes have the same namespace.
Component: Content → DOM
QA Contact: content → general
Would it be reasonable to add a non-virtual inline convenience method IsInNamespace() on nsINode and then replace IsNodeOfType(eHTML) with IsInNamespace(kNameSpaceID_XHTML) and similarly also for SVGness and XULness checks?
IMHO, we could even have isHTML(), isSVG(), isXUL() and isMathML() helper
methods in nsIContent.
Yeah, I'd be pretty happy with having those methods and IsInNamespace().
Blocks: 507316
Attached patch Patch (obsolete) — Splinter Review
Attachment #391530 - Flags: review?(jst)
Comment on attachment 391530 [details] [diff] [review]
Patch

This looks good! A couple of things to note below. Feel free to update the patch if you can, or let me know and I can adjust and check in if you want. r+sr=jst with the notes below taken into account. Thanks for the patch!

- In AdjustRangeForSelection():

   nsINode* brNode = node->GetChildAt(offset - 1);
-  while (brNode && brNode->IsNodeOfType(nsINode::eHTML)) {
-    nsIContent* brContent = static_cast<nsIContent*>(brNode);
+
+  nsIContent* brContent;
+  while (brNode && brNode->IsNodeOfType(nsINode::eELEMENT) && 
+         (brContent = static_cast<nsIContent*>(brNode))->IsHTML()) {
     if (brContent->Tag() != nsGkAtoms::br || IsContentBR(brContent))
       break;
     brNode = node->GetChildAt(--offset - 1);
   }

brNode could simply be an nsIContent* here as GetChildAt() returns nsIContent*, that way you wouldn't need the brNode->IsNodeOfType(nsINode::eELEMENT) check at all.

- In content/html/content/src/nsGenericHTMLElement.cpp:

 IsBody(nsIContent *aContent)
 {
-  return (aContent->NodeInfo()->Equals(nsGkAtoms::body) &&
-          aContent->IsNodeOfType(nsINode::eHTML));
+  return aContent->NodeInfo()->Equals(nsGkAtoms::body) &&
+          aContent->IsHTML();

Fix the indentation, or leave the parens. I'd vote for leaving the parens.

- In nsGenericHTMLElement::GetSpellcheck():

-      switch (static_cast<nsIContent*>(node)->
-              FindAttrValueIn(kNameSpaceID_None, nsGkAtoms::spellcheck,
+      switch (node->FindAttrValueIn(kNameSpaceID_None, nsGkAtoms::spellcheck,
                               strings, eCaseMatters)) {

Fix indentation.

- In nsGenericHTMLFormElement::IsNodeOfType():

+  return !(aFlags & ~(eCONTENT | eELEMENT |/* eHTML |*/ eHTML_FORM_CONTROL));

Any reason to keep eHTML commented out there? I'd vote for removing it...

- In txXPathNodeUtils::getLocalName():

             nodeInfo->GetLocalName(aLocalName);
-
-            // Check for html
-            if (nodeInfo->NamespaceEquals(kNameSpaceID_None) &&
-                aNode.mNode->IsNodeOfType(nsINode::eHTML)) {
-                ToUpperCase(aLocalName);
-            }

Someone who knows our XPath code should look at this one... I just ran this by sicking and he said that's exactly right (apparently this change was part of another bug at some point as well, or something).

- In nsNodeSH::PreCreate():

-    if (node->IsNodeOfType(nsINode::eELEMENT |
-                           nsIContent::eHTML |
-                           nsIContent::eHTML_FORM_CONTROL)) {
+    if (node->IsNodeOfType(nsINode::eELEMENT)    &&
+        static_cast<nsIContent*>(node)->IsHTML() &&
+        node->IsNodeOfType(nsINode::eHTML_FORM_CONTROL)) {

Only HTML elements can be of node type eHTML_FORM_CONTROL, so no need to check if it's an element, or if it's HTML here, just the eHTML_FORM_CONTROL will be enough. And additionally, this could be optimized a bit if you remember whether the node is an element (checked further up) there's no need to call IsNodeOfType() here if it's not an element, so you'd save a virtual call for non-element nodes. Not a big deal tho...

- In widget/src/gtk2/nsNativeThemeGTK.cpp:

-static PRBool IsFrameContentNodeOfType(nsIFrame *aFrame, PRUint32 aFlags)
+static PRBool IsFrameContentNodeInNamespace(nsIFrame *aFrame, PRUint32 aFlag)

Please rename aFlag to aNamespace.
Attachment #391530 - Flags: superreview+
Attachment #391530 - Flags: review?(jst)
Attachment #391530 - Flags: review+
Attached patch Patch for check-in (obsolete) — Splinter Review
Attached patch Check-in, previous missed a spot (obsolete) — Splinter Review
Attachment #394323 - Attachment is obsolete: true
Attachment #391530 - Attachment is obsolete: true
Pushed that last diff as http://hg.mozilla.org/mozilla-central/rev/4aa19414e651
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Pushed http://hg.mozilla.org/mozilla-central/rev/59ae87416f96 to fix some Mac native theme bustage and some cross-platform bustage due to an IsNodeOfType call in the frame constructor that wasn't fixed in that last patch (maybe because it landed after the patch was posted?).
Also pushed http://hg.mozilla.org/mozilla-central/rev/eb32cbfba7f5 to fix accessibility build bustage.
Thanks, I had those fixed in my mq but I didn't upload because it was still building =(
Backed out all three changesets to fix test orange.  There were at least the following failures:

1) "46386 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_bug507902.html | Window drawn on broken load should match broken icon reference" on mochitest on all three platforms.
2) '77 ERROR TEST-UNEXPECTED-FAIL |
chrome://mochikit/content/chrome/content/base/test/chrome/test_title.xul |
'title': Setting 'title' works with a <head> - got "", expected "Hello"' on mochitest-chrome on all three platforms.
3) crashes during crashtest, for example when loading or leaving layout/svg/crashtests/492186-1.svg

I looked into this last one, and it looks like one issue is that NS_NewSVGElement will fall through to NS_NewXMLElement, thus creating nodes that are in the SVG namespace but NOT subclasses of nsSVGElement.  We used to test false on IsNodeOfType(eSVG) for these, but the namespace-based IsSVG code returns true; then we end up casting them to nsSVGElement and jumping into lala-land when trying to call a virtual method on them.

Probably the best way to go forward is to leave eSVG and implement IsSVG in terms of it or something...  and maybe file a followup bug on the SVG DOM impl if we think creating non-nsSVGElements in the SVG namespace is wrong.  Then try-server the patch and see what's still failing at that point.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Any updates here? I'd expect the patch to bit-rot pretty quickly if we don't check it in again. I agree with bz's suggestion in the previous comment, but definitely think it's worth filing another bug.
Some more eHTML consumers got added....
Pushed attachment 403943 [details] [diff] [review], attachment 403946 [details] [diff] [review], attachment 403961 [details] [diff] [review] plus an nsINode iid rev as http://hg.mozilla.org/mozilla-central/rev/95f56721c813
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Assignee: nobody → dzbarsky
This caused some test failures because I'm a doofus and didn't investigate items 1 and 2 from comment 12 more thoroughly.  There were 4 spots that bug 468708 missed: new Image(), new Audio(), setting .title on a document, and poster frames for videos.  Pushed http://hg.mozilla.org/mozilla-central/rev/4364f81cf017 with those fixes.
Looks like a 1.2% Txul win on WinXP, at least.
Depends on: 566629
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.