225837 - DeCOMtaminate nsIContent::GetTag()

Status: RESOLVED FIXED

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

Description

[Johnny Stenback (:jst)]

GetTag() should be renamed to Tag(), and it should simply return a weak
reference to an nsIAtom. Huge patch coming up.

Comment 1

[Johnny Stenback (:jst)]

Taking.

Comment 2

[Johnny Stenback (:jst)]

Attachment: diff -w, deCOMtaminate nsIContent::GetTag, GetRangeList, GetContentID.

This is a diff -w, so ignore any bad looking indentation.

Comment 3

[Johnny Stenback (:jst)]

Oh, and ignore the changes to cygwin-wrapper, those won't be checked in (forgot
to take those out of the diff).

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 135609 [details] [diff] [review]
diff -w, deCOMtaminate nsIContent::GetTag, GetRangeList, GetContentID.



>   NS_IMETHOD RangeRemove(nsIDOMRange* aRange) = 0;
>   /**
>    * Get the list of ranges that have either endpoint in this content item
>-   * @param aResult the list of ranges owned partially by this content [OUT]
>+   * @returns the list of ranges owned partially by this content
>    */
>-  NS_IMETHOD GetRangeList(nsVoidArray** aResult) const = 0;
>+  virtual nsVoidArray *GetRangeList() const = 0;

Please add a comment on who is the owner of this list? Will the caller have to
delete it?

>Index: content/base/src/mozSanitizingSerializer.cpp
> mozSanitizingHTMLSerializer::Init(PRUint32 aFlags, PRUint32 dummy,
>                                   const char* aCharSet, PRBool aIsCopying)
> {
>+  nsIParserService* parserService = nsContentUtils::GetParserServiceWeakRef();
>+
>+  NS_ENSURE_TRUE(parserService, NS_ERROR_UNEXPECTED);

Optional Nit: You don't really need the temporary, same in
nsPlainTextSerializer::Init

>+mozSanitizingHTMLSerializer::GetIdForContent(nsIContent* aContent)
...
>+  PRInt32 id;
>+  nsresult rv = parserService->HTMLAtomTagToId(aContent->Tag(), &id);
>+  NS_ASSERTION(NS_SUCCEEDED(rv), "Can't map HTML tag to id!");

The old code used |mContent| rather then |aContent| here. Are they the same?

>Index: content/base/src/nsDocumentEncoder.cpp
...
> nsHTMLCopyEncoder::IncludeInContext(nsIDOMNode *aNode)
...
>+  if (tag == nsHTMLAtoms::b        ||
>+      tag == nsHTMLAtoms::i        ||
...
>+      tag == nsHTMLAtoms::h5       ||
>+      tag == nsHTMLAtoms::h6) 
>   {
>     return PR_TRUE;
>   }
> 
>   return PR_FALSE;
> }

Feel free to make that just |return tag == nsHTMLAtoms::b ||  | etc. 


>Index: content/base/src/nsSelection.cpp
>+static
>+nsIAtom *GetTag(nsIDOMNode *aNode)
> {
>-  nsCOMPtr<nsIAtom> atom;
>-  
>-  if (!aNode) 
>+  nsCOMPtr<nsIContent> content = do_QueryInterface(aNode);
>+  if (!content) 
>   {
>-    NS_NOTREACHED("null node passed to GetTag()");
>-    return atom;
>+    NS_NOTREACHED("bad node passed to Tag()");
>+    return nsnull;

Should this still say "GetTag"?

>@@ -4253,39 +4247,43 @@ nsTypedSelection::GetTableSelectionType(
>+  if (!content->IsContentOfType(nsIContent::eHTML)) {
>+    return result;
>+  }

I'd prefer if this returned |NS_OK| instead. Should be more safe in case
additional code is inserted above.

>+  nsIAtom *tag = content->Tag();
>+  if (!tag) return NS_ERROR_FAILURE;

This should never return null, right?


>@@ -6129,20 +6126,19 @@ nsTypedSelection::FixupSelectionPoints(n
...
>-  nsCOMPtr<nsIAtom> atom;
>-  atom = GetTag(endNode);
>+  nsIAtom *atom = Tag(endNode);

Shouldn't this still call GetTag?

>@@ -6713,36 +6709,34 @@ nsTypedSelection::Extend(nsIDOMNode* aPa
...
>-    nsCOMPtr<nsIAtom> tag;
>-    content->GetTag(getter_AddRefs(tag));
>+    nsIAtom *tag = content->Tag();
>     if (tag)
>     {

No need to test for null |tag| any more.


>Index: content/events/src/nsEventStateManager.cpp
>@@ -1201,65 +1191,71 @@ nsEventStateManager :: FireContextClick 
...
>+        else if (tag == nsHTMLAtoms::applet ||
>+                 tag == nsHTMLAtoms::embed)
>           allowedToDispatch = PR_FALSE;

Don't we support <object> too?


>Index: content/html/content/src/nsAttributeContent.cpp
>-  NS_IMETHOD GetTag(nsIAtom** aResult) const
>+  nsIAtom *Tag() const
>   {
>-    *aResult = nsnull;
>-    return NS_OK;
>+    return nsnull;
>   }

This is a nono according to the latest namingconvention. Make it return
whatever textnodes return (or some such).

>Index: content/html/document/src/nsHTMLContentSink.cpp
> #ifdef NS_DEBUG
>+      {
>       // Tracing code
>-      nsCOMPtr<nsIAtom> tag;
>-      mStack[mStackPos].mContent->GetTag(getter_AddRefs(tag));
>+        nsIAtom *tag = mStack[mStackPos].mContent->Tag();
>       const char *tagStr;
>       tag->GetUTF8String(&tagStr);

no need for the temporary |tag| here. (which means that you could remove the {}
if you want). Same in SinkContext::FlushTags

>Index: content/xbl/src/nsBindingManager.cpp
>+nsBindingManager::ResolveTag(nsIContent* aContent, PRInt32* aNameSpaceID,
...
>-  aContent->GetNameSpaceID(aNameSpaceID);
>-  return aContent->GetTag(aResult);
>+  nsINodeInfo *ni = aContent->GetNodeInfo();
>+
>+  if (ni) {
>+    *aNameSpaceID = ni->NamespaceID();
>+    NS_ADDREF(*aResult = ni->NameAtom());
>+  } else {
>+    *aNameSpaceID = kNameSpaceID_None;
>+    *aResult = nsnull;
>+  }

Are you sure this is safe since html-elements will return a different thing?

>Index: content/xbl/src/nsXBLPrototypeBinding.cpp
>@@ -893,22 +892,23 @@ PRBool PR_CALLBACK SetAttrs(nsHashKey* a
>-        nsCOMPtr<nsIAtom> tag;
>-        realElement->GetTag(getter_AddRefs(tag));
>+
>         if (dst == nsXBLAtoms::xbltext ||
>-            (tag == nsHTMLAtoms::html) && (dst == nsHTMLAtoms::value) && !value.IsEmpty()) {
>+            (realElement->Tag() == nsHTMLAtoms::html &&
>+             realElement->IsContentOfType(nsIContent::eHTML)) &&
>+            (dst == nsHTMLAtoms::value) && !value.IsEmpty()) {

no need for () around the Tag and IsContentOfType calls. And around the
dst==value test.

Note to self: You're at nsXBLService::LoadBindingDocumentInfo

Comment 5

[Brian Ryner (not reading)]

Comment on attachment 135609 [details] [diff] [review]
diff -w, deCOMtaminate nsIContent::GetTag, GetRangeList, GetContentID.

>Index: content/base/src/nsSelection.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsSelection.cpp,v
>retrieving revision 3.161
>diff -u -9 -p -w -r3.161 nsSelection.cpp
>--- content/base/src/nsSelection.cpp	22 Oct 2003 06:09:29 -0000	3.161
>+++ content/base/src/nsSelection.cpp	15 Nov 2003 23:23:42 -0000
>@@ -4253,39 +4247,43 @@ nsTypedSelection::GetTableSelectionType(
>   result = aRange->GetEndOffset(&endOffset);
>   if (NS_FAILED(result)) return result;
>   result = aRange->GetStartOffset(&startOffset);
>   if (NS_FAILED(result)) return result;
> 
>   // Not a single selected node
>   if ((endOffset - startOffset) != 1)
>     return NS_OK;
> 
>-  nsCOMPtr<nsIAtom> atom;
>-  content->GetTag(getter_AddRefs(atom));
>-  if (!atom) return NS_ERROR_FAILURE;
>+  if (!content->IsContentOfType(nsIContent::eHTML)) {
>+    return result;
>+  }
>+
>+  nsIAtom *tag = content->Tag();
>+  if (!tag) return NS_ERROR_FAILURE;

You shouldn't need this null check if Tag() never returns null.

>@@ -5939,20 +5937,19 @@ nsTypedSelection::Collapse(nsIDOMNode* a
>     return result;
> 
> #ifdef DEBUG_SELECTION
>   if (aParentNode)
>   {
>     nsCOMPtr<nsIContent>content;
>     content = do_QueryInterface(aParentNode);
>     if (!content)
>       return NS_ERROR_FAILURE;
>-    nsCOMPtr<nsIAtom> tag;
>-    content->GetTag(getter_AddRefs(tag));
>+    nsIAtom *tag = content->Tag();
>     if (tag)

Same here.

>@@ -6713,36 +6709,34 @@ nsTypedSelection::Extend(nsIDOMNode* aPa
>       selectFrames(range, PR_TRUE);
>
> #endif
>     }
>     else {
>       selectFrames(presContext, difRange, PR_TRUE);
>     }
>     res = CopyRangeToAnchorFocus(range);
>     if (NS_FAILED(res))
>       return res;
>-
>   }
> 
>   DEBUG_OUT_RANGE(range);
> #if 0
>   if (eDirNext == mDirection)
>     printf("    direction = 1  LEFT TO RIGHT\n");
>   else
>     printf("    direction = 0  RIGHT TO LEFT\n");
> #endif
>   SetDirection(dir);
> #ifdef DEBUG_SELECTION
>   if (aParentNode)
>   {
>     nsCOMPtr<nsIContent>content;
>     content = do_QueryInterface(aParentNode);
>-    nsCOMPtr<nsIAtom> tag;
>-    content->GetTag(getter_AddRefs(tag));
>+    nsIAtom *tag = content->Tag();
>     if (tag)

and here.

>Index: content/events/src/nsEventStateManager.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v
>retrieving revision 1.463
>diff -u -9 -p -w -r1.463 nsEventStateManager.cpp
>--- content/events/src/nsEventStateManager.cpp	13 Nov 2003 18:38:41 -0000	1.463
>+++ content/events/src/nsEventStateManager.cpp	15 Nov 2003 23:23:51 -0000
>@@ -2270,25 +2271,26 @@ nsEventStateManager::GetNearestScrolling
>     return GetNearestScrollingView(parent);
>   }
> 
>   return nsnull;
> }
> 
> PRBool
> nsEventStateManager::CheckDisabled(nsIContent* aContent)
> {
>-  nsCOMPtr<nsIAtom> tag;
>-  aContent->GetTag(getter_AddRefs(tag));
>+  nsIAtom *tag = aContent->Tag();
> 
>-  if (tag == nsHTMLAtoms::input    ||
>+  if ((tag == nsHTMLAtoms::input    ||
>       tag == nsHTMLAtoms::select   ||
>       tag == nsHTMLAtoms::textarea ||
>-      tag == nsHTMLAtoms::button) {
>+       tag == nsHTMLAtoms::button) &&
>+      (aContent->IsContentOfType(nsIContent::eHTML) ||
>+       aContent->IsContentOfType(nsIContent::eXUL))) {
>     return aContent->HasAttr(kNameSpaceID_None, nsHTMLAtoms::disabled);
>   }

This is nitpicking, but of these tags, button is the only one that exists in
XUL.


>Index: content/html/content/src/nsAttributeContent.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/html/content/src/nsAttributeContent.cpp,v
>retrieving revision 1.67
>diff -u -9 -p -w -r1.67 nsAttributeContent.cpp
>--- content/html/content/src/nsAttributeContent.cpp	7 Nov 2003 09:47:13 -0000	1.67
>+++ content/html/content/src/nsAttributeContent.cpp	15 Nov 2003 23:23:51 -0000
>@@ -86,22 +86,21 @@ public:
>   NS_IMETHOD_(PRBool) IsNativeAnonymous() const { return PR_TRUE; }
>   NS_IMETHOD_(void) SetNativeAnonymous(PRBool aAnonymous) { }
> 
>   NS_IMETHOD GetNameSpaceID(PRInt32* aID) const
>   {
>     *aID = kNameSpaceID_None;
>     return NS_OK;
>   }
> 
>-  NS_IMETHOD GetTag(nsIAtom** aResult) const
>+  nsIAtom *Tag() const
>   {
>-    *aResult = nsnull;
>-    return NS_OK;
>+    return nsnull;
>   }

This shouldn't return null if the method is called Tag().

>Index: content/xbl/src/nsXBLBinding.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xbl/src/nsXBLBinding.cpp,v
>retrieving revision 1.168
>diff -u -9 -p -w -r1.168 nsXBLBinding.cpp
>--- content/xbl/src/nsXBLBinding.cpp	22 Oct 2003 06:09:31 -0000	1.168
>+++ content/xbl/src/nsXBLBinding.cpp	15 Nov 2003 23:24:11 -0000
>@@ -547,21 +547,23 @@ nsXBLBinding::GenerateAnonymousContent()
>     children->GetLength(&length);
>     if (length > 0 && !hasInsertionPoints) {
>       // There are children being placed underneath us, but we have no specified
>       // insertion points, and therefore no place to put the kids.  Don't generate
>       // anonymous content.
>       // Special case template and observes.
>       for (PRUint32 i = 0; i < length; i++) {
>         children->Item(i, getter_AddRefs(node));
>         childContent = do_QueryInterface(node);
>-        nsCOMPtr<nsIAtom> tag;
>-        childContent->GetTag(getter_AddRefs(tag));
>-        if (tag != nsXULAtoms::observes && tag != nsXULAtoms::templateAtom) {
>+
>+        nsINodeInfo *ni = childContent->GetNodeInfo();
>+
>+        if (!ni || (!ni->Equals(nsXULAtoms::observes, kNameSpaceID_XBL) &&
>+                    !ni->Equals(nsXULAtoms::templateAtom, kNameSpaceID_XBL))) {

I think these should be kNameSpaceID_XUL.

>@@ -625,21 +627,24 @@ nsXBLBinding::GenerateAnonymousContent()
>                   break;
>                 insertionPoint = nsnull;
>               }
> 
>               if (insertionPoint) 
>                 insertionPoint->AddChild(childContent);
>               else {
>                 // We were unable to place this child.  All anonymous content
>                 // should be thrown out.  Special-case template and observes.
>-                nsCOMPtr<nsIAtom> tag;
>-                childContent->GetTag(getter_AddRefs(tag));
>-                if (tag != nsXULAtoms::observes && tag != nsXULAtoms::templateAtom) {
>+
>+                nsINodeInfo *ni = childContent->GetNodeInfo();
>+
>+                if (!ni ||
>+                    (!ni->Equals(nsXULAtoms::observes, kNameSpaceID_XBL) &&
>+                     !ni->Equals(nsXULAtoms::templateAtom, kNameSpaceID_XBL))) {

Same here.

I'll finish this later, starting at nsComposeTxtSrvFilter.cpp

Comment 6

[Boris Zbarsky [:bzbarsky]]

> This shouldn't return null if the method is called Tag().

Note that nsAttributeContent needs some removing in general...

Comment 7

[Johnny Stenback (:jst)]

Jonas Sicking said:
>>+  virtual nsVoidArray *GetRangeList() const = 0;
> 
> Please add a comment on who is the owner of this list? Will the caller have to
> delete it?

Done. The caller shouldn't delete it, I made it return a const nsVoidArray *.

>>+mozSanitizingHTMLSerializer::GetIdForContent(nsIContent* aContent)
>>+  nsresult rv = parserService->HTMLAtomTagToId(aContent->Tag(), &id);
> 
> The old code used |mContent| rather then |aContent| here. Are they the same?

Yeah, all callers pass in mContent.

>>@@ -1201,65 +1191,71 @@ nsEventStateManager :: FireContextClick 
>>+        else if (tag == nsHTMLAtoms::applet ||
>>+                 tag == nsHTMLAtoms::embed)
> Don't we support <object> too?

Yup, added that.

>>Index: content/html/content/src/nsAttributeContent.cpp
>>-  NS_IMETHOD GetTag(nsIAtom** aResult) const
>>+  nsIAtom *Tag() const
>>  {
>>-    *aResult = nsnull;
>>-    return NS_OK;
>>+    return nsnull;
>>  }
> This is a nono according to the latest namingconvention. Make it return
> whatever textnodes return (or some such).

Done.

>>+nsBindingManager::ResolveTag(nsIContent* aContent, PRInt32* aNameSpaceID,
>>-  aContent->GetNameSpaceID(aNameSpaceID);
>>-  return aContent->GetTag(aResult);
>>+  nsINodeInfo *ni = aContent->GetNodeInfo();
>>+
>>+  if (ni) {
>>+    *aNameSpaceID = ni->NamespaceID();
>>+    NS_ADDREF(*aResult = ni->NameAtom());
>>+  } else {
>>+    *aNameSpaceID = kNameSpaceID_None;
>>+    *aResult = nsnull;
>>+  }
> Are you sure this is safe since html-elements will return a different thing?

No, I can't say I'm sure, so I changed it back to what it was until we figure
out the whole GetNameSpaceID() thing. The only reason I touched that to start
with was that I could do that with one virtual call in stead of two using
nsINodeInfo...

The rest of your suggested changes (except some optional ones) are also done.

Comment 8

[Johnny Stenback (:jst)]

All issues found by bryner so far are now corrected too.

Comment 9

[Brian Ryner (not reading)]

Comment on attachment 135609 [details] [diff] [review]
diff -w, deCOMtaminate nsIContent::GetTag, GetRangeList, GetContentID.

>Index: embedding/browser/webBrowser/nsDocShellTreeOwner.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/embedding/browser/webBrowser/nsDocShellTreeOwner.cpp,v
>retrieving revision 1.78
>diff -u -9 -p -w -r1.78 nsDocShellTreeOwner.cpp
>--- embedding/browser/webBrowser/nsDocShellTreeOwner.cpp	4 Sep 2003 02:23:18 -0000	1.78
>+++ embedding/browser/webBrowser/nsDocShellTreeOwner.cpp	15 Nov 2003 23:25:31 -0000
>@@ -1526,105 +1566,103 @@ ChromeContextMenuListener :: ContextMenu
>       return NS_ERROR_OUT_OF_MEMORY;
>     menuInfo = menuInfoImpl; 
>   }
> 
>   // Find the first node to be an element starting with this node and
>   // working up through its parents.
> 
>   PRUint32 flags = nsIContextMenuListener::CONTEXT_NONE;
>   PRUint32 flags2 = nsIContextMenuListener2::CONTEXT_NONE;
>-  nsCOMPtr<nsIDOMHTMLElement> element;
>+  nsCOMPtr<nsIContent> content;
>   do {
>     // XXX test for selected text
>-    element = do_QueryInterface(node);
>-    if (element)
>-    {
>-      nsAutoString tag;
>-      element->GetTagName(tag);
>+    content = do_QueryInterface(node);
>+    if (content && content->IsContentOfType(nsIContent::eHTML)) {
>+      nsIAtom *tag = content->Tag();
>+      const char *tagStr;
>+      tag->GetUTF8String(&tagStr);

Looks like you could avoid the 'tag' temporary here.

>Index: embedding/components/find/src/nsFind.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/embedding/components/find/src/nsFind.cpp,v
>retrieving revision 1.23
>diff -u -9 -p -w -r1.23 nsFind.cpp
>--- embedding/components/find/src/nsFind.cpp	27 Sep 2003 04:18:16 -0000	1.23
>+++ embedding/components/find/src/nsFind.cpp	15 Nov 2003 23:25:32 -0000
>@@ -425,50 +425,50 @@ nsFind::NextNode(nsIDOMRange* aSearchRan
> 
> #ifdef DEBUG_FIND
>   printf("Iterator gave: "); DumpNode(mIterNode);
> #endif
>   return NS_OK;
> }
> 
> PRBool nsFind::IsBlockNode(nsIContent* aContent)
> {
>-  nsCOMPtr<nsIAtom> atom;
>-  aContent->GetTag(getter_AddRefs(atom));
>+  // XXX jst: This is a little too HTMLy for my liking...
> 
>-  if (atom.get() == sImgAtom || atom.get() == sHRAtom
>-      || atom.get() == sThAtom || atom.get() == sTdAtom)
>+  nsIAtom *atom = aContent->Tag();
>+
>+  if (atom == sImgAtom ||
>+      atom == sHRAtom ||
>+      atom == sThAtom ||
>+      atom == sTdAtom)
>     return PR_TRUE;
> 
>   if (!mParserService) {
>     nsresult rv;
>     mParserService = do_GetService(kParserServiceCID, &rv);
>-    if (NS_FAILED(rv) || !mParserService) return PR_FALSE;
>+    if (NS_FAILED(rv))
>+      return PR_FALSE;
>   }

Since you're not passing back rv, you could just skip it and return PR_FALSE if
mParserService is null.

>@@ -485,71 +485,69 @@ PRBool nsFind::IsVisibleNode(nsIDOMNode 
>     // No frame! Not visible then.
>     return PR_FALSE;
>   }
> 
>   return frame->GetStyleVisibility()->IsVisible();
> }
> 
> PRBool nsFind::SkipNode(nsIContent* aContent)
> {
>-  nsCOMPtr<nsIAtom> atom;
>+  nsIAtom *atom;
> 
> #ifdef HAVE_BIDI_ITERATOR
>-  aContent->GetTag(getter_AddRefs(atom));
>-  if (!atom)
>-    return PR_TRUE;
>-  nsIAtom *atomPtr = atom.get();
>+  atom = aContent->Tag();
> 
>   // We may not need to skip comment nodes,
>   // now that IsTextNode distinguishes them from real text nodes.
>-  return (sScriptAtom == atomPtr || sCommentAtom == atomPtr
>-          || sNoframesAtom == atomPtr
>-          || sSelectAtom == atomPtr || sTextareaAtom == atomPtr)
>+  return (!atom ||

!atom should not be possible.

>+          atom == sScriptAtom ||
>+          atom == sCommentAtom ||
>+          atom == sNoframesAtom ||
>+          atom == sSelectAtom ||
>+          atom == sTextareaAtom);
> 
> #else /* HAVE_BIDI_ITERATOR */
>   // Temporary: eventually we will have an iterator to do this,
>   // but for now, we have to climb up the tree for each node
>   // and see whether any parent is a skipped node,
>   // and take the performance hit.
> 
>-  nsCOMPtr<nsIDOMNode> node (do_QueryInterface(aContent));
>-  while (node)
>+  nsIContent *content = aContent;
>+  while (content)
>   {
>-    nsCOMPtr<nsIContent> content (do_QueryInterface(node));
>-    if (!content) return PR_FALSE;
>-    content->GetTag(getter_AddRefs(atom));
>+    atom = content->Tag();
>     if (!atom)
>       return PR_FALSE;

Same here.

>+#if 0
>     nsAutoString atomName;
>     atom->ToString(atomName);
>     //printf("Atom name is %s\n", 
>     //       NS_LossyConvertUCS2toASCII(atomName).get());
>-    nsIAtom *atomPtr = atom.get();
>-    if (atomPtr == sScriptAtom || atomPtr == sCommentAtom
>-        || sNoframesAtom == atomPtr
>-        || sSelectAtom == atomPtr || sTextareaAtom == atomPtr)
>+#endif

You might clean this up. (remove the parts in the #if 0 block that you've
replaced below, and probably comment out the ToString() call to go with the
printf (or remove all of it).

>Index: layout/html/base/src/nsFrame.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/html/base/src/nsFrame.cpp,v
>retrieving revision 3.457
>diff -u -9 -p -w -r3.457 nsFrame.cpp
>--- layout/html/base/src/nsFrame.cpp	10 Nov 2003 23:36:06 -0000	3.457
>+++ layout/html/base/src/nsFrame.cpp	15 Nov 2003 23:26:01 -0000
>@@ -2701,21 +2701,20 @@ NS_IMETHODIMP
> nsFrame::GetFrameName(nsAString& aResult) const
> {
>   return MakeFrameName(NS_LITERAL_STRING("Frame"), aResult);
> }
> 
> nsresult
> nsFrame::MakeFrameName(const nsAString& aType, nsAString& aResult) const
> {
>   aResult = aType;
>-  if (nsnull != mContent) {
>-    nsCOMPtr<nsIAtom> tag;
>-    mContent->GetTag(getter_AddRefs(tag));
>+  if (mContent) {
>+    nsIAtom *tag = mContent->Tag();
>     if (tag && tag != nsLayoutAtoms::textTagName) {

No need to null check.

>@@ -4753,20 +4752,19 @@ NS_IMETHODIMP nsFrame::SetBidiProperty(n
> 
> #ifdef NS_DEBUG
> static void
> GetTagName(nsFrame* aFrame, nsIContent* aContent, PRIntn aResultSize,
>            char* aResult)
> {
>   char namebuf[40];
>   namebuf[0] = 0;
>   if (aContent) {
>-    nsCOMPtr<nsIAtom> tag;
>-    aContent->GetTag(getter_AddRefs(tag));
>+    nsIAtom *tag = aContent->Tag();
>     if (tag) {

Ditto.

>Index: layout/html/base/src/nsImageFrame.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/html/base/src/nsImageFrame.cpp,v
>retrieving revision 1.297
>diff -u -9 -p -w -r1.297 nsImageFrame.cpp
>--- layout/html/base/src/nsImageFrame.cpp	6 Nov 2003 16:03:34 -0000	1.297
>+++ layout/html/base/src/nsImageFrame.cpp	15 Nov 2003 23:26:21 -0000
>@@ -1202,25 +1202,23 @@ nsImageFrame::DisplayAltFeedback(nsIPres
>     PRInt32 iconWidth = NSIntPixelsToTwips(ICON_SIZE + ICON_PADDING, p2t);
>     inner.x += iconWidth;
>     inner.width -= iconWidth;
>   }
> 
>   // If there's still room, display the alt-text
>   if (!inner.IsEmpty()) {
>     nsIContent* content = GetContent();
>     if (content) {
>-      nsCOMPtr<nsIAtom> tag;
>-      content->GetTag(getter_AddRefs(tag));
>-      if (tag) {
>+      nsIAtom *tag = content->Tag();
>+
>         nsAutoString altText;
>         nsCSSFrameConstructor::GetAlternateTextFor(content, tag, altText);

You could avoid the temporary here.  (and maybe in nsObjectFrame? not enough
context to see if the variables are referenced again).

>Index: layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp,v
>retrieving revision 1.192
>diff -u -9 -p -w -r1.192 nsTreeBodyFrame.cpp
>--- layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp	22 Oct 2003 06:09:45 -0000	1.192
>+++ layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp	15 Nov 2003 23:27:56 -0000
>@@ -413,23 +413,23 @@ nsTreeBodyFrame::Init(nsIPresContext* aP
> }
> 
> NS_IMETHODIMP
> nsTreeBodyFrame::GetMinSize(nsBoxLayoutState& aBoxLayoutState, nsSize& aSize)
> {
>   EnsureView();
> 
>   nsCOMPtr<nsIContent> baseElement;
>   GetBaseElement(getter_AddRefs(baseElement));
>-  nsCOMPtr<nsIAtom> tag;
>-  baseElement->GetTag(getter_AddRefs(tag));
>+
>+  nsINodeInfo *ni = baseElement->GetNodeInfo();
> 
>   PRInt32 desiredRows;
>-  if (tag == nsHTMLAtoms::select) {
>+  if (ni && ni->Equals(nsHTMLAtoms::select, kNameSpaceID_XUL)) {

This is supposed to be an HTML select tag (XBL form control support).  So,
probably should be (baseElement->Tag() == nsHTMLAtoms::select &&
baseElement->IsContentOfType(nsIContent::eHTML))

>@@ -1633,21 +1633,22 @@ nsTreeBodyFrame::IsCellCropped(PRInt32 a
>   
>   return NS_OK;
> }
> 
> void
> nsTreeBodyFrame::MarkDirtyIfSelect()
> {
>   nsCOMPtr<nsIContent> baseElement;
>   GetBaseElement(getter_AddRefs(baseElement));
>-  nsCOMPtr<nsIAtom> tag;
>-  baseElement->GetTag(getter_AddRefs(tag));
>-  if (tag == nsHTMLAtoms::select) {
>+
>+  nsINodeInfo *ni = baseElement->GetNodeInfo();
>+
>+  if (ni && ni->Equals(nsHTMLAtoms::select, kNameSpaceID_XUL)) {

Ditto.

>@@ -3422,21 +3423,20 @@ nsTreeBodyFrame::EnsureColumns()
> 
>     if (!parent)
>       return;
> 
>     nsCOMPtr<nsIPresShell> shell; 
>     mPresContext->GetShell(getter_AddRefs(shell));
> 
>     // Note: this is dependent on the anonymous content for select
>     // defined in select.xml
>-    nsCOMPtr<nsIAtom> parentTag;
>-    parent->GetTag(getter_AddRefs(parentTag));
>-    if (parentTag == nsHTMLAtoms::select) {
>+    nsINodeInfo *ni = parent->GetNodeInfo();
>+    if (ni && ni->Equals(nsHTMLAtoms::select, kNameSpaceID_XUL)) {

Ditto.

>@@ -3488,23 +3488,24 @@ nsTreeBodyFrame::EnsureColumns()
> 
>       colBox->GetNextBox(&colBox);
>     }
>   }
> }
> 
> nsresult
> nsTreeBodyFrame::GetBaseElement(nsIContent** aContent)
> {
>-  nsCOMPtr<nsIAtom> tag;
>+  nsINodeInfo *ni;
>   nsIContent* parent;
>   for (parent = mContent;
>-       parent && NS_SUCCEEDED(parent->GetTag(getter_AddRefs(tag)))
>-         && tag != nsXULAtoms::tree && tag != nsHTMLAtoms::select;
>+       parent && (ni = parent->GetNodeInfo()) &&
>+         !ni->Equals(nsXULAtoms::tree, kNameSpaceID_XUL) &&
>+         !ni->Equals(nsHTMLAtoms::select, kNameSpaceID_XUL);

Here too.

sr=bryner with those changes.

Comment 10

[Johnny Stenback (:jst)]

Done.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 135609 [details] [diff] [review]
diff -w, deCOMtaminate nsIContent::GetTag, GetRangeList, GetContentID.

>Index: content/xul/document/src/nsElementMap.cpp
>@@ -193,26 +193,22 @@ nsElementMap::Add(const nsAString& aID, 
...
>-                    rv = tag->ToString(tagname);
>+                    nsresult rv = tag->ToString(tagname);

You could get rid of the |tag|-temporary. Same in nsElementMap::Add and
nsElementMap::Remove

>Index: embedding/components/find/src/nsFind.cpp
> PRBool nsFind::SkipNode(nsIContent* aContent)
> {
...
>+  while (content)
>   {
>-    nsCOMPtr<nsIContent> content (do_QueryInterface(node));
>-    if (!content) return PR_FALSE;
>-    content->GetTag(getter_AddRefs(atom));
>+    atom = content->Tag();
>     if (!atom)
>       return PR_FALSE;

You can remove this test.

>Index: layout/xul/base/src/nsXULTooltipListener.cpp
>@@ -580,20 +578,19 @@ nsXULTooltipListener::GetTooltipFor(nsIC
...
>-            GetImmediateChild(aTarget, nsXULAtoms::tooltip, aTooltip); // this addrefs
>-            NS_IF_ADDREF(*aTooltip);
>+            GetImmediateChild(aTarget, nsXULAtoms::tooltip, aTooltip);
>             return NS_OK;
>           } else {

Why remove the addref?

>Index: layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
>@@ -3488,23 +3488,24 @@ nsTreeBodyFrame::EnsureColumns()
...
> nsTreeBodyFrame::GetBaseElement(nsIContent** aContent)
> {
>-  nsCOMPtr<nsIAtom> tag;
>+  nsINodeInfo *ni;
>   nsIContent* parent;
>   for (parent = mContent;
>-       parent && NS_SUCCEEDED(parent->GetTag(getter_AddRefs(tag)))
>-         && tag != nsXULAtoms::tree && tag != nsHTMLAtoms::select;
>+       parent && (ni = parent->GetNodeInfo()) &&
>+         !ni->Equals(nsXULAtoms::tree, kNameSpaceID_XUL) &&
>+         !ni->Equals(nsHTMLAtoms::select, kNameSpaceID_XUL);
>        parent = parent->GetParent()) {
>     // Do nothing; we just go up till we hit the right tag or run off the tree
>   }

In other places like this you test |!ni || !ni->Equals(...)|. You might need to
do that here too in case the initial aContent is a textnode. Don't forget to
parenthesize accordingly.

>Index: layout/xul/base/src/tree/src/nsTreeContentView.cpp
>@@ -711,61 +713,59 @@ nsTreeContentView::ContentStatesChanged(
...
> nsTreeContentView::AttributeChanged(nsIDocument *aDocument,
...
>   // If we have a legal tag, go up to the tree and make sure that it's ours.
>   nsCOMPtr<nsIContent> parent = aContent;
>-  nsCOMPtr<nsIAtom> parentTag;
>+  nsINodeInfo *ni = nsnull;
>   do {
>     parent = parent->GetParent();
>     if (parent)
>-      parent->GetTag(getter_AddRefs(parentTag));
>-  } while (parent && parentTag != nsXULAtoms::tree);
>+      ni = parent->GetNodeInfo();
>+  } while (parent && ni && !ni->Equals(nsXULAtoms::tree, kNameSpaceID_XUL));
>+

Same thing here.

With that: r=sicking

Comment 12

[Johnny Stenback (:jst)]

The addref was moved into GetImmediateChild(), the rest of the issues are fixed.
Thank you very much for the r= and sr=!

Comment 13

[Johnny Stenback (:jst)]

Attachment: Patch that was checked in.

Comment 14

[Kathleen :Brade]

jst--I noticed two warnings (Unused variable `nsINodeInfo*ni') appeared with
this checkin; can the lines be removed entirely?

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp&mark=1643#1633
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp&mark=3433#3423

Comment 15

[Johnny Stenback (:jst)]

Ah, good catch brade! I just removed those.

Marking this FIXED now that it's in and things seem to be working...

Comment 16

[Boris Zbarsky [:bzbarsky]]

I'm a little confused by some of these changes.

The change at
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/content/xbl/src&command=DIFF&root=/cvsroot&file=nsXBLService.cpp&rev1=1.182&rev2=1.183#4

Why's that testing !aBoundElement->IsContentOfType(nsIContent::eHTML) ?  Testing
for non-html elements named not select and not input doesn't seem to make much
sense...

The change at
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/content/xbl/src&command=DIFF&root=/cvsroot&file=nsXBLPrototypeBinding.cpp&rev1=1.79&rev2=1.80#0

That should be testing for XUL, not HTML as the nodetype -- see the comment
there.  Same for the change at line 900 or so.

Comment 17

[Johnny Stenback (:jst)]

Attachment: How about this then?

Some maybe something like this?

FYI, I did talk this over with Bryner and Ben and we came to the conclusion
that there is no <html> tag in XUL (didn't see that comment though, I must
admit), so maybe that code is bogus?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 136324 [details] [diff] [review]
How about this then?

Yeah, that makes a lot more sense.  As for <html> in XUL, it used to exist,
apparently.  See
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/components/autocomplet
e/test/autocomplete_test.xul&mark=162,173,181#162 and
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/html/tests/xbl/insan
eInsertion.xul&mark=9

No idea what the state of it is now...

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 136324 [details] [diff] [review]
How about this then?

>Index: content/xbl/src/nsXBLPrototypeBinding.cpp

>       if (realElement) {
>         realElement->SetAttr(kNameSpaceID_None, dst, value, PR_FALSE);
> 
>         if (dst == nsXBLAtoms::xbltext ||
>-            (realElement->Tag() == nsHTMLAtoms::html &&
>-             realElement->IsContentOfType(nsIContent::eHTML) &&
>+            realElement->GetNodeInfo()->Equals(nsHTMLAtoms::html,
>+                                               kNameSpaceID_XUL) &&
>              dst == nsHTMLAtoms::value && !value.IsEmpty())) {

Note that you still need a '(' before the realElement->GetNodeInfo() call --
this patch does not compile as-is.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 136324 [details] [diff] [review]
How about this then?

Could this please be approved for 1.6b?  It corrects a crash when attaching XBL
bindings to HTML nodes (bug 226744) and generally restores this logic to what
it used to be before the patches for this bug.

Comment 21

[Johnny Stenback (:jst)]

Fix checked in.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Um.... jst?  You didn't check in the nsXBLService part of that patch, which was
the crash-fix part....  Your list of checkins for the day is:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=jst&whotype=regexp&sortby=Date&hours=2&date=explicit&mindate=2003-12-02+00%3A00%3A00&maxdate=2003-12-03+00%3A00%3A00&cvsroot=%2Fcvsroot

I've taken the liberty of assuming a=dbaron applies to 1.6final too and checking
it in.

Comment 23

[Johnny Stenback (:jst)]

Duh! Sorry about that, and thanks for checking it in!