215981 - DeCOMtaminate nsIContent more...

Status: RESOLVED FIXED

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

Description

[Johnny Stenback (:jst)]

nsIContent's ChildAt, ChildCount, GetNodeInfo, etc should be decomtaminated.
Huge patch coming up.

Comment 1

[Johnny Stenback (:jst)]

Attachment: fix (gzip)

Comment 2

[Johnny Stenback (:jst)]

Attachment: diff -w of the above (gzip)

Comment 3

[Johnny Stenback (:jst)]

Taking bug.

Comment 4

[chris hofmann]

should help out with minimo to the tune of 700kb or so..

Comment 5

[Doug Turner (:dougt)]

woot!

jst, what are your plans with this patch?  is this something you hope to land
when the tree reopens?  

Comment 6

[Johnny Stenback (:jst)]

Just to clarify here, this is a 700k+ diff, what it does for code size I don't
know. It should help, no doubt, but how much? I don't know that yet.

The plan is to land this ASAP, and I guess that means as soon as the tree reopens.

Comment 7

[Doug Turner (:dougt)]

i don't see much of any different in minimo:

disk space deltas (du -s)
BEFORE: 14112
AFTER:  14096

I would have to do more work to get in memory numbers, but I don't see too much
of a different on a couple of page runs.

In minimo, xul, svg, accessablity, html-editor, and most extensions are all
disabled.  I am sure that this will make a bigger splash with the full browser.
 In any case, good work!

Comment 8

[Axel Hecht]

is the conflict between the PRInt32 in 
nsIContent::IndexOf
and the PRUint32 in 
nsIContent::ChildAt and friends a good idea? Or does it just shovel the casts to
different places?

Comment 9

[Johnny Stenback (:jst)]

IMO it is. IndexOf() returns the index, and uses -1 to flag that the child
wasn't found. But passing a signed integer to ChildAt() and friends makes little
sense to me since indexes start from 0 and go up. Period.

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Some general comments:

- nsIAccessNode.idl is marked as under review, and basically are wrappers around
some of the methods you changed from signed to unsigned on nsIContent, for
example nsIAccessNode::GetChildNodeAt() -> nsIContent::GetChildAt() and
nsIAccessNode::GetNumChildren() -> nsIContent::GetChildCount() are the ones that
stuck out the most at me.  You might want to ping to aaronl and have him change
those as well before this interface is actually frozen.

- I think there will be a bunch of new compiler warnings with this patch
complaining about signed vs. unsigned.  The compiler warning people will
probably go nutso.  I suppose it's not that big of a deal really, but it would
be nice to keep the warnings down where possible.  It does kind of suck that
IndexOf() returns a signed int.  Maybe we should consider changing IndexOf's
signature to |PRBool IndexOf(nsIContent*,PRUint32*)| where it returns false
where it previously returned -1, and true otherwise.  Either that, or require
the nsIContent* parameter passed into to be a child of the content and start
asserting if that's not the case.  We could then change the return value to
PRUint32.


-In nsAccessNode::GetNumChildren(PRInt32 *aNumChildren)

+  if (content) {
+    *aNumChildren = content->GetChildCount();
+  } else {
+    *aNumChildren = nsnull;
+
+    NS_ERROR_NULL_POINTER;

Oops?  :-)  Also, if you're going to do an early return, use |if (!content)|.


Index: build/cygwin-wrapper
===================================================================
RCS file: /cvsroot/mozilla/build/cygwin-wrapper,v

I'm not sure this file is supposed to be part of the diff.  If it is, I think
you need r= from cls or leaf.


More later.

Comment 11

[Johnny Stenback (:jst)]

Yeah, ignore those cygwin-wrapper changes (there's two of those files changed in
the diff), they're my local changes that were not supposed to be left in the
diff (I removed them from the initial diff, but forgot to do so for the -w diff).

I'm not sure what to do about IndexOf(). I don't like changing the signature to
involve more argument passing, and I don't think we can require that the node be
a child of the content object since we use IndexOf() to find out if a content
object is an anonymous child of its parent, and so on.

I guess one idea would be to make IndexOf() return PRUint32 and define a
CONTENT_NO_INDEX (or whatever name) constant that would be ((PRUint32)-1) or and
require that callers check against that if they want to know if the child wasn't
found. This would work, but it would require some more code changes, which I'm
willing to do if others think it's worth it.

On that same note, we also have the same problem with nsVoidArray and friends.
The methods that take indexes on those classes should also (IMO, and alecf said
that he agrees with me on that) get the same treatment, and being consistent
would of course be nice...

Comment 12

[Axel Hecht]

I guess I'd prefer to have CONTENT_NO_INDEX rather than to have two different ints
in nsIContent. 2cts.

Comment 13

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

I've found that more often then not it's easier to go with PRInt32 although i do
agree it's a bit uglier. The reason is that often enough you need to have signed
values somewhere (like in the case of IndexOf for example) whereas you very
rarly loose functionality by overusing signed (as is the case here since i doubt
we'll ever have enough childrent to not fit in a signed).

It's not really a big deal though, but I do suspect we'll end up with more casts.

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Well, we can decide what to do with IndexOf later.  I will continue my review
this weekend, ignoring any signed vs. unsigned warnings, but I'll still look for
logical errors there. I think Johnny also said he would compile on linux which
would let him see the warnings and try and do some more warnings reduction.

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

General nit (not your fault).  It would be nice if you picked one way of saying
"we have some foos".  We currently have |Count() != 0|, |Count() > 0|, and
|!!Count()|.  (Personally, I prefer > 0, but whatever)


- In nsresult nsContentIterator::Init(nsIDOMRange* aRange)

|cChild| can be made into a raw pointer now.

- In nsContentIterator::GetDeepFirstChild() and GetDeepLastChild().

|cN| could be turned into a raw pointer.  Bonus points for fixing the crap
signatures.


-In nsContentIterator::NextNode()

>-    PRInt32 numChildren;
>-  
>-    cN->ChildCount(numChildren);
>+    PRInt32 numChildren = cN->GetChildCount();
>   
>     // if it has children then next node is first child
>     if (numChildren)

Aside from using a PRInt32 here(!), this is the only thing that numChildren is
used for, so just do |if (ContentHasChildren())|

- In nsContentUtils::BelongsInForm()

>-  PRInt32 count = 0;
>-
>-  form->ChildCount(count);
>+  PRInt32 count = form->GetChildCount();
> 
>   if (count > 0) {

|if (form->GetChildCount() > 0)|


- In GetElementByAttribute()   (nsDocument.cpp)

+  PRInt32 childCount = aContent->GetChildCount();

PRUint32


- In nsDocumentFragment::ReconnectChildren()

>-  nsCOMPtr<nsIContent> child, parent;
...
>+  nsIContent *child, *parent;

Now that you got rid of the nsCOMPtrs, you can move those declarations into the
loop while initializing them.

- In nsGeneratedContentIterator::PrevNode()

+  PRInt32 numChildren = cN->GetChildCount();

PRUint32

- nsDOMEventRTTearoff::QueryInterface()

Since you're changing this, can't you just use the NS_INTERFACE_MAP_INHERITING
macros?

- In nsGenericElement::doInsertBefore()

>-    PRInt32 count, old_count, i;
>+    PRUint32 i;

Nit: move |i| down to where it's actually being used?

- In  nsGenericContainerElement::CopyInnerTo()

+      if (NS_OK != rv) {
...
+      if (NS_OK != rv) {

Wanna fix both of those in this method?  Surprised you didn't, actually.

- In nsSelection::GetRootForContentSubtree()

>     PRInt32 childIndex = 0;
...
>+    childIndex = parent->IndexOf(child);

PRInt32 childIndex = parent->IndexOf(child);


That's all for now.  Will continue later.  The last file I reviewed was
nsEventListenerManager.cpp.

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

- In nsresult nsEventStateManager::GetDocSelectionLocation()

>       typedef PRInt32* PRInt32_ptr;
>       domRange->GetStartOffset(PRInt32_ptr(aStartOffset));

Sorry to nit about that, but since it's in the diff :) can you kill the typedef
and use NS_STATIC_CAST?

+static inline PRBool
+MatchTag(nsIContent *aContent, nsIAtom *aTag)

Maybe that would be better named MatchHTMLTag?

- In  GenericElementCollection::Item(PRUint32 aIndex, nsIDOMNode** aReturn)

>-    while (child) {
>-      nsCOMPtr<nsIAtom> childTag;
>-      child->GetTag(getter_AddRefs(childTag));
>-      if (mTag == childTag) {
>+    while ((child = mParent->GetChildAt(childIndex))) {
>+      if (MatchTag(child, mTag)) {
>         if (aIndex == theIndex) {
>           CallQueryInterface(child, aReturn);
>           NS_ASSERTION(aReturn, "content element must be an nsIDOMNode");
> 
>           return NS_OK;
>         }
>         ++theIndex;
>       }
>-      mParent->ChildAt(++childIndex, getter_AddRefs(child));
>     }


Um, this is wrong.  You will hit an infinite loop if the tags don't match. 
Looks like you need to post-increment in the while statement.

- In nsGenericHTMLElement::CopyInnerTo()

>+  PRInt32 id = PR_UINT32_MAX;
>+  if (doc) {
>+    doc->GetAndIncrementContentID(&id);
>   }
> 
>   aDst->SetContentID(id);

I think you meant PR_INT32_MAX

>+static PRBool
>+IsBody(nsIContent *aContent)

Shouldn't this be an inline method?

>+static PRBool
>+IsOffsetParent(nsIContent *aContent)

And this?


- Random comment:

>       nsIHTMLStyleSheet*  sheet = GetAttrStyleSheet(mDocument);
>       if (sheet) {
>         mAttributes->SetStyleSheet(sheet);
>-        //      sheet->SetAttributesFor(htmlContent, mAttributes); // sync
attributes with sheet
>         NS_RELEASE(sheet);
>       }

Oh man, it sucks that GetAttrStyleSheet addrefs but doesn't advertise it.  There
are a fair amount of callers so I won't ask you to fix it in this patch.  I'll
file a new bug and patch it there, after this lands.

- In nsGenericHTMLElement::FindForm()

>     if (content->IsContentOfType(nsIContent::eHTML)) {
>-      content->GetTag(getter_AddRefs(tag));
>-
>       // If the current ancestor is a form, return it as our form
>-      if (tag == nsHTMLAtoms::form) {
>+      if (content->GetNodeInfo()->Equals(nsHTMLAtoms::form)) {
>         return CallQueryInterface(content, aForm);
>       }
>     }

Wanna combine the ifs there?

>+static PRBool
>+IsArea(nsIContent *aContent)

Hmm, this too and you have several more of these which could probably be inlined
in nsGenericHTMLElement.cpp.

- In nsGenericHTMLContainerElement::ReplaceContentsWithText()

>+  PRInt32 children = GetChildCount();
...
>+  for (PRInt32 i = children - 1; i >= lastChild; --i) {

PRUint32 in both places.  Also since you are already touching all of the lines
that children is used, how about calling it numChildren or something more
meaningful?

Same for nsGenericHTMLContainerElement::GetContentsAsText()


- In nsHTMLSelectElement::RemoveOptionsFromList()

>-      for (int i=aListIndex;i<aListIndex+numRemoved;i++) {
>+      for (int i = aListIndex; i < aListIndex + numRemoved ; ++i) {

Nit: Fix the whitespace after numRemoved


- In HTMLContentSink::~HTMLContentSink()

>   PRInt32 numContexts = mContextStack.Count();
>-  if (mCurrentContext == mHeadContext) {
>+  if (mCurrentContext == mHeadContext && numContexts) {

Ugh.  Please fix nsVoidArray (in another bug) like you said in comment 11 :-)


- In HTMLContentSink::ProcessSCRIPTTag()

>+  // To prevent script evaluation in a frameset document we suspended
>+  // the script loader. Now that the script content has been handled
>+  // let's resume the script loader.

Commas go after "document" and "handled".


- In nsHTMLDocument::ResolveName()

>+    if (child->IsContentOfType(nsIContent::eHTML) &&
>+        child->GetNodeInfo()->Equals(nsHTMLAtoms::body,
>+                                     mDefaultNamespaceID)) {

It would be nice if you could turn IsBody() from way back in the review into
something that could be called from here.

- In SelectorMatches()  (nsCSSStyleSheet.cpp)

>-        nsCOMPtr<nsIContent> firstChild;
>-        nsCOMPtr<nsIContent> parent = data.mParentContent;
>+        nsIContent *firstChild = nsnull;
>+        nsIContent *parent = data.mParentContent;
>         if (parent) {
>           PRBool acceptNonWhitespace =
>             nsCSSPseudoClasses::firstNode == pseudoClass->mAtom;
>           PRInt32 index = -1;
>           do {
>-            parent->ChildAt(++index, getter_AddRefs(firstChild));
>+            firstChild = parent->GetChildAt(++index);

Wanna move the declaration of |firstChild| into the loop?  It's not used outside
of it, and we don't care anymore about nsCOMPtr's ctor/dtor being called from
within it.  Same for the next couple of things in the ensuing |else if| statements.

- In nsDOMCSSAttributeDeclaration::GetCSSParsingEnvironment()

>+  nsresult result = NS_OK;
>+
>+  nsINodeInfo *nodeInfo = mContent->GetNodeInfo();
>   if (NS_FAILED(result)) {
>     return result;
>   }

You can probably move |result|'s declaration down a bit in the function.

- In nsXULElement::GetChildAt(PRUint32 aIndex) const

>     nsresult rv;
>     if (NS_FAILED(rv = EnsureContentsGenerated())) {
>-        *aResult = nsnull;
>-        return rv;
>+        return nsnull;

|rv| is no longer needed.  Same with IndexOf()

- In nsXULElement::Focus()

>   // Retrieve the context
>   nsCOMPtr<nsIPresContext> aPresContext;
>+  if (shell) {
>   shell->GetPresContext(getter_AddRefs(aPresContext));
>+  }
> 
>   // Set focus
>   return SetFocus(aPresContext);

So, you now add a way (in theory) for aPresContext to be null.  Passing a null
pres context to SetFocus() will crash.  Of course, before, we would have crashed
with a null shell.  So you're really moving this crash around, but still it
would be nice to not do this.  Maybe add another check?  Or null check in
SetFocus?  The check can probably just be removed, but don't make us do more
work just to crash later on.  Crash speed is important :-)

Same comments apply to Blur().


Index: content/xul/content/src/nsXULPopupListener.cpp

>-/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Change the tab-width, too?


More later.  The last file I looked at was nsXULCommandDispatcher.cpp.

Comment 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 129701 [details]
diff -w of the above (gzip)

So, in addition to my previous comments:

- In nsXULDocument::ContentAppended()

Move rv's declaration into the for-loop.

- In nsXULContentBuilder::RemoveGeneratedContent()

>+        PRUint32 i = element->GetChildCount();
> 
>+        while (i-- > 0) {
>+            nsIContent *child = element->GetChildAt(i);
>             NS_ASSERTION(child != nsnull, "huh? no child?");
>             if (! child)
>                 continue;

The assertion and null check can probably go.

- In XULSortServiceImpl::SortContainer()

You removed the early users of |rv|.  You can move it down quite a ways in the
function now.

- In nsXULTemplateBuilder::CompileRules()

>+    for (PRUint32 i = 0; i < count; i++) {
>+        nsIContent *rule = tmpl->GetChildAt(i);
>         if (! rule)
>             break;

This null check can die...

>+        if (!rule->IsContentOfType(nsIContent::eXUL))
>             continue;
> 
>         nsCOMPtr<nsIAtom> tag;
>         rule->GetTag(getter_AddRefs(tag));
> 
>-        if (tag.get() == nsXULAtoms::rule) {
>+        if (tag == nsXULAtoms::rule) {

And this looks like a fine candidate for
|rule->GetNodeInfo()->Equals(nsXULAtoms::rule, kNameSpaceID_XUL);| to save the
atom addrefing and nsCOMPtr ctor/dtor in this loop.

- In nsXULTemplateBuilder::AddSimpleRuleBindings()

>     while (elements.Count()) {
>         // Pop the next element off the stack
>-        PRInt32 i = elements.Count() - 1;
>+        PRUint32 i = (PRUint32)(elements.Count() - 1);

Not your fault, but could fix this loop to not suck as much?  Changing it to a
for-loop would be nice.

- In nsWSRunObject::GetPreviousWSNode()

>-  nsCOMPtr<nsIContent> priorContent, startContent( do_QueryInterface(aStartNode) );
>+  nsCOMPtr<nsIContent> startContent( do_QueryInterface(aStartNode) );
>   if (!aOffset)
>   {
>     if (aStartNode==aBlockParent)
>     {
>       // we are at start of the block.
>       return NS_OK;
>     }
>     else
>     {
>       // we are at start of non-block container
>       return GetPreviousWSNode(aStartNode, aBlockParent, aPriorNode);
>     }
>   }
>   
>-  nsresult res = startContent->ChildAt(aOffset - 1, getter_AddRefs(priorContent));
>-  NS_ENSURE_SUCCESS(res, res);
>+  nsIContent *priorContent = startContent->GetChildAt(aOffset - 1);

Unless my eyes decieve me, you could move the QI above way down here.  And kill
the else-after-return, too.  Same for GetNextWSNode()

- In nsImgManager::ShouldLoad()

>+        if (!nodeinfo) return NS_OK;
> 
>         doc = nodeinfo->GetDocument();
>         // XXX what should this code do if there is really no document?
>-        if (!doc) return NS_OK;
>+        if (!doc)
>+          return NS_OK;

Sorry to nit here, but you change it, I review it.  Why make this change, but
not update the nodeinfo check above?


In Index: gfx/src/shared/nsNativeTheme.cpp

You changed the other native theme methods from |void GetPrimaryShell(nsIFrame*
aFrame, nsIPresShell** aResult)| to |nsIPresShell*
GetPrimaryPresShell(nsIFrame* aFrame)| but not this one.  For consistency's
sake, please do so as well.

- In nsImageMap::UpdateAreas()

>-    nsCOMPtr<nsIDOMHTMLElement> element = do_QueryInterface(child);
>-    if (! element)
>+    if (!child->IsContentOfType(nsIContent::eELEMENT))

You mean nsIContent::eHTML, right?


One more comment I have in general: several places you change things from
iterating over all presshells to just GetShellAt(0) and in many other places,
you don't.  Is there any reasoning behind that?

Comment 18

[Johnny Stenback (:jst)]

> >+static PRBool
> >+IsBody(nsIContent *aContent)
>  
> Shouldn't this be an inline method?

You commented about this for a bunch of these little helpers, but I don't think
it's worth inlining those. They're not used in extremely performance critical
sections of code, and they're more than a few instructions, so I'd rather not
duplicate that code in all places where that helper is called. Not that it
really matters, but I think the code will be smaller this way w/o any even
significant performance tradeoffs.

> - In SelectorMatches()  (nsCSSStyleSheet.cpp)
...
> Wanna move the declaration of |firstChild| into the loop?  It's not used
> outside of it, and we don't care anymore about nsCOMPtr's ctor/dtor being
> called from within it.  Same for the next couple of things in the ensuing
> |else if| statements.

It is used outside the loop, and besides, you can't reference variables declared
inside a do { } while () block in the while condition.

> >         nsCOMPtr<nsIAtom> tag;
> >         rule->GetTag(getter_AddRefs(tag));
> > 
> >-        if (tag.get() == nsXULAtoms::rule) {
> >+        if (tag == nsXULAtoms::rule) {
> 
> And this looks like a fine candidate for
> |rule->GetNodeInfo()->Equals(nsXULAtoms::rule, kNameSpaceID_XUL);| to
> save the atom addrefing and nsCOMPtr ctor/dtor in this loop.

The problem here is that we don't know that rule has nodeinfo, it could be a
text node. I'll leave it as is for now, once the tag stuff is decomtaminated,
we'll eliminate the addref/release.

> One more comment I have in general: several places you change things from
> iterating over all presshells to just GetShellAt(0) and in many other
> places, you don't.  Is there any reasoning behind that?

Yeah, the places where I did that it didn't make sense to iterate over the
shells (which is probably the case for most of the remaining places where we do
iterate over the shells too, but that's another bug), like when firing DOM
events, those should only ever fire once, no matter how many shells you have.
The only other case I saw in the diff was a case in the sink where there won't
be more than one shell, so no need to iterate there either.

All other changes made (except probably a few that weren't required or needed).
Thanks for digging through this!

Comment 19

[Johnny Stenback (:jst)]

Attachment: Updated diff -w (fixes the problems found by caillon)

Comment 20

[Peter Van der Beken [:peterv]]

> Index: accessible/src/atk/nsHTMLTableAccessibleWrap.cpp
> ===================================================================

> @@ -501,21 +501,19 @@ nsHTMLTableAccessibleWrap::GetTableLayou

>    nsCOMPtr<nsIContent> content(do_QueryInterface(tableNode));
>    NS_ENSURE_TRUE(content, NS_ERROR_FAILURE);
>  
> -  nsCOMPtr<nsIPresShell> presShell;
> -  rv = content->GetDocument()->GetShellAt(0, getter_AddRefs(presShell));
> -  NS_ENSURE_SUCCESS(rv, rv);
> +  nsIPresShell *presShell = content->GetDocument()->GetShellAt(0);

Null-check?

> Index: accessible/src/base/nsAccessible.cpp
> ===================================================================

>  nsresult nsAccessible::AppendFlatStringFromSubtreeRecurse(nsIContent
*aContent, nsAString *aFlatString)
>  {
>    // Depth first search for all text nodes that are decendants of content node.
>    // Append all the text into one flat string
>  
> -  PRInt32 numChildren = 0;
> +  PRUint32 numChildren = aContent->GetChildCount();
>  
> -  aContent->ChildCount(numChildren);
>    if (numChildren == 0) {
>      AppendFlatStringFromContentNode(aContent, aFlatString);
>      return NS_OK;
>    }
>  
> -  nsCOMPtr<nsIContent> contentWalker;
> -  PRInt32 index;
> -  for (index = 0; index < numChildren; index++) {
> -    aContent->ChildAt(index, getter_AddRefs(contentWalker));
> -    AppendFlatStringFromSubtree(contentWalker, aFlatString);
> +  for (PRUint32 index = 0; index < numChildren; index++) {
> +    AppendFlatStringFromSubtree(aContent->GetChildAt(index), aFlatString);
>    }

I don't like the declaration of index inside the condition and it should be
++index, but oh well (there's so many others in the patch ;-)).

> Index: accessible/src/base/nsDocAccessible.cpp
> ===================================================================

>  void nsDocAccessible::GetEventShell(nsIDOMNode *aNode, nsIPresShell
**aEventShell)

>    nsCOMPtr<nsIDOMDocument> domDocument;
>    aNode->GetOwnerDocument(getter_AddRefs(domDocument));
>    nsCOMPtr<nsIDocument> doc(do_QueryInterface(domDocument));
> -  if (doc)
> -    doc->GetShellAt(0, aEventShell);
> +  if (doc) {
> +    *aEventShell = doc->GetShellAt(0);
> +    NS_IF_ADDREF(*aEventShell);

NS_IF_ADDREF(*aEventShell = doc->GetShellAt(0));

> Index: accessible/src/base/nsRootAccessible.cpp
> ===================================================================

>  void nsRootAccessible::GetEventShell(nsIDOMNode *aNode, nsIPresShell
**aEventShell)

>    nsCOMPtr<nsIDocument> doc(do_QueryInterface(domDocument));
>    if (!doc) {   // This is necessary when the node is the document node
>      doc = do_QueryInterface(aNode);
>    }
> -  if (doc)
> -    doc->GetShellAt(0, aEventShell);
> +  if (doc) {
> +    *aEventShell = doc->GetShellAt(0);
> +    NS_IF_ADDREF(*aEventShell);

NS_IF_ADDREF(*aEventShell = doc->GetShellAt(0));

> Index: content/base/src/nsContentAreaDragDrop.cpp
> ===================================================================

> @@ -1417,25 +1429,26 @@ nsresult nsTransferableFactory::GetDragg

> -              PRInt32 childOffset = (anchorOffset < focusOffset) ?
anchorOffset : focusOffset;
> -              nsCOMPtr<nsIContent> childContent;
> -              selStartContent->ChildAt(childOffset,
getter_AddRefs(childContent));
> +              PRInt32 childOffset =
> +                (anchorOffset < focusOffset) ? anchorOffset : focusOffset;
> +              nsIContent *childContent =
> +                selStartContent->GetChildAt(childOffset);
>                // if we find an image, we'll fall into the node-dragging code,
>                // rather the the selection-dragging code
> -              nsCOMPtr<nsIDOMHTMLImageElement> selectedImage;
> -              selectedImage = do_QueryInterface(childContent);
> +              nsCOMPtr<nsIDOMHTMLImageElement> selectedImage =
> +                do_QueryInterface(childContent);

              nsCOMPtr<nsIDOMHTMLImageElement> selectedImage =
                do_QueryInterface(selStartContent->GetChildAt(childOffset));

> Index: content/base/src/nsNodeInfoManager.cpp
> ===================================================================

>  NS_IMETHODIMP
>  nsNodeInfoManager::GetNodeInfo(const nsAString& aQualifiedName,
>                                 const nsAString& aNamespaceURI,
>                                 nsINodeInfo** aNodeInfo)
>  {
>    NS_ENSURE_ARG(!aQualifiedName.IsEmpty());
>  
> -  nsAutoString name(aQualifiedName);
> -  nsAutoString prefix;
> -  PRInt32 nsoffset = name.FindChar(':');
> -  if (-1 != nsoffset) {
> -    name.Left(prefix, nsoffset);
> -    name.Cut(0, nsoffset+1);
> -  }
> -
> -  nsCOMPtr<nsIAtom> nameAtom = do_GetAtom(name);
> -  NS_ENSURE_TRUE(nameAtom, NS_ERROR_OUT_OF_MEMORY);
> +  nsAString::const_iterator start, end;
> +  aQualifiedName.BeginReading(start);
> +  aQualifiedName.EndReading(end);
>  
>    nsCOMPtr<nsIAtom> prefixAtom;
>  
> -  if (!prefix.IsEmpty()) {
> -    prefixAtom = do_GetAtom(prefix);
> +  nsAString::const_iterator iter(start);
> +
> +  if (FindCharInReadable(':', iter, end)) {
> +    ++iter; // step over the ':'
> +
> +    if (iter == end) {
> +      // No data after the ':'.
> +
> +      return NS_ERROR_INVALID_ARG;
> +    }
> +
> +    prefixAtom = do_GetAtom(Substring(iter, end));
>      NS_ENSURE_TRUE(prefixAtom, NS_ERROR_OUT_OF_MEMORY);
>    }
>  
> +  nsCOMPtr<nsIAtom> nameAtom = do_GetAtom(Substring(start, iter));
> +  NS_ENSURE_TRUE(nameAtom, NS_ERROR_OUT_OF_MEMORY);
> +

This doesn't seem right, it looks like you're setting prefix to localname and
vice versa.

> Index: content/html/content/src/nsHTMLSelectElement.cpp
> ===================================================================

> @@ -698,19 +694,19 @@ nsHTMLSelectElement::RemoveOptionsFromLi
>                                               aDepth);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    if (numRemoved) {
>      // Tell the widget we removed the options
>      nsISelectControlFrame* selectFrame = GetSelectFrame();
>      if (selectFrame) {
>        nsCOMPtr<nsIPresContext> presContext;
>        GetPresContext(this, getter_AddRefs(presContext));
> -      for (int i=aListIndex;i<aListIndex+numRemoved;i++) {
> +      for (int i = aListIndex; i < aListIndex + numRemoved; ++i) {

PRInt32

> Index: layout/html/base/src/nsFrame.cpp
> ===================================================================

> @@ -1995,37 +1991,31 @@ nsresult nsFrame::GetContentAndOffsetsFr
>        if (!skipThisKid) {
>          // The frame's content is not generated. Now check
>          // if it is anonymous content!
>  
>          nsIContent* kidContent = kid->GetContent();
>          if (kidContent) {
>            nsCOMPtr<nsIContent> content = kidContent->GetParent();
>  
>            if (content) {
> -            PRInt32 kidCount = 0;
> -
> -            result = content->ChildCount(kidCount);
> -            if (NS_SUCCEEDED(result)) {
> -
> -              PRInt32 kidIndex = 0;
> -              result = content->IndexOf(kidContent, kidIndex);
> +            PRInt32 kidCount = content->ChildCount(kidCount);

Huh?

> Index: uriloader/base/nsDocLoader.cpp
> ===================================================================

>  NS_IMETHODIMP
>  nsDocLoaderImpl::GetContentViewerContainer(nsISupports* aDocumentID,
>                                             nsIContentViewerContainer** aResult)
>  {

> -          rv = supp->QueryInterface(kIContentViewerContainerIID,
(void**)aResult);          

Remove the definition of kIContentViewerContainerIID on line 94.

Comment 21

[Peter Van der Beken [:peterv]]

Comment on attachment 131912 [details]
Updated diff -w (fixes the problems found by caillon)

See my comments.

Comment 22

[Johnny Stenback (:jst)]

> > @@ -501,21 +501,19 @@ nsHTMLTableAccessibleWrap::GetTableLayou
[...]
> > +  nsIPresShell *presShell = content->GetDocument()->GetShellAt(0);
> 
> Null-check?

No, the code already relies on the presshell always being there, and I don't
want to go through all this code to make it safe against this when it appears
like it doesn't need to be. If it does, that's another bug.

The rest of the changes are made, and I'll try to land these changes soon.

Comment 23

[rbs]

jst, I think the following simplifications will have undesirable side-effects:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/layout/mathml/base/src&command=DIFF_FRAMESET&file=nsMathMLTokenFrame.cpp&rev1=1.12&rev2=1.13&root=/cvsroot
(and similar elsewhere).

The idea was to avoid comment nodes, e.g., in <mtext><!-- comment -->text</mtext>.
How to differentiate beteewn the comment_child and the text_child, that is the
question... And so I used nsIDOMText, which added another level of checking, but
achieved the intended correctness.

Comment 24

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

yep, that seems indeed to break things. Though a better way of ensuring that you
only get textnodes is to call nsIContent::IsContentOfType(nsIContent::eTEXT)
rather then QI-ing to nsIDOMText

Comment 25

[Johnny Stenback (:jst)]

rbs, good catch! Issue fixed (using IsContentOfType()).

Comment 26

[rbs]

> nsIContent::IsContentOfType(nsIContent::eTEXT)

Yea. Unfortunately, that relatively newish thing wasn't there at the time.

Comment 27

[Johnny Stenback (:jst)]

FIXED. Let's open new bugs for further deCOMtamination of nsIContent n' friends.

Comment 28

[rbs]

BTW, there are two other spots that missed the fix (where child texts get
concatenated to figure out the actual length of the textual content).
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/layout/mathml/base/src&command=DIFF_FRAMESET&file=nsMathMLTokenFrame.cpp&rev1=1.12&rev2=1.13&root=/cvsroot
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/layout/mathml/base/src&command=DIFF_FRAMESET&file=nsMathMLmoFrame.cpp&rev1=1.56&rev2=1.57&root=/cvsroot

Comment 29

[rbs]

Oops, ignore my post above. I see what you made...

Comment 30

[Boris Zbarsky [:bzbarsky]]

For the record, this patch caused crashes in bug 220464 and bug 220516, and also
caused bug 220519.

And what's up with the change to nsHTMLReflowState.cpp?  Was that really part of
this patch?

Comment 31

[Johnny Stenback (:jst)]

The change to nsHTMLReflowState.cpp is simply fixing a warning that I found
while chasing warnings intriduced by this change. So it's unrelated, it just
went in with the same checkin.

Comment 32

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

this also caused bug 221323