Closed
Bug 488249
Opened 15 years ago
Closed 15 years ago
Replace IsNodeOfType(eHTML) checks with namespace checks
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a1
People
(Reporter: hsivonen, Assigned: dzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 3 obsolete files)
147.82 KB,
patch
|
Details | Diff | Splinter Review | |
2.55 KB,
patch
|
Details | Diff | Splinter Review | |
2.73 KB,
patch
|
Details | Diff | Splinter Review |
Replace calls to virtual IsNodeOfType(eHTML) with non-virtual namespace id checks once both HTML and XHTML nodes have the same namespace.
Reporter | ||
Comment 1•15 years ago
|
||
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?
Comment 2•15 years ago
|
||
IMHO, we could even have isHTML(), isSVG(), isXUL() and isMathML() helper methods in nsIContent.
Comment 3•15 years ago
|
||
Yeah, I'd be pretty happy with having those methods and IsInNamespace().
Assignee | ||
Comment 4•15 years ago
|
||
Assignee | ||
Updated•15 years ago
|
Attachment #391530 -
Flags: review?(jst)
Comment 5•15 years ago
|
||
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+
Assignee | ||
Comment 6•15 years ago
|
||
Assignee | ||
Comment 7•15 years ago
|
||
Attachment #394323 -
Attachment is obsolete: true
Updated•15 years ago
|
Attachment #391530 -
Attachment is obsolete: true
Comment 8•15 years ago
|
||
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
Comment 9•15 years ago
|
||
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?).
Comment 10•15 years ago
|
||
Also pushed http://hg.mozilla.org/mozilla-central/rev/eb32cbfba7f5 to fix accessibility build bustage.
Assignee | ||
Comment 11•15 years ago
|
||
Thanks, I had those fixed in my mq but I didn't upload because it was still building =(
Comment 12•15 years ago
|
||
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.
Comment 14•15 years ago
|
||
This still has the SVG bustage.
Attachment #394387 -
Attachment is obsolete: true
Comment 15•15 years ago
|
||
Comment 16•15 years ago
|
||
Some more eHTML consumers got added....
Comment 17•15 years ago
|
||
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 ago → 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Assignee: nobody → dzbarsky
Comment 18•15 years ago
|
||
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.
Comment 19•15 years ago
|
||
Looks like a 1.2% Txul win on WinXP, at least.
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•