DeCOMtaminate nsIContent more...

RESOLVED FIXED

Status

()

RESOLVED FIXED
16 years ago
14 years ago

People

(Reporter: jst, Assigned: jst)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

16 years ago
nsIContent's ChildAt, ChildCount, GetNodeInfo, etc should be decomtaminated.
Huge patch coming up.
(Assignee)

Comment 1

16 years ago
Created attachment 129690 [details]
fix (gzip)
(Assignee)

Comment 2

16 years ago
Created attachment 129701 [details]
diff -w of the above (gzip)
(Assignee)

Comment 3

16 years ago
Taking bug.
Assignee: dom_bugs → jst

Comment 4

16 years ago
should help out with minimo to the tune of 700kb or so..
Blocks: 215636
(Assignee)

Updated

16 years ago
Attachment #129701 - Flags: review?(caillon)
woot!

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

Comment 6

16 years ago
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.
Status: NEW → ASSIGNED
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

16 years ago
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?
(Assignee)

Comment 9

16 years ago
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.
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.
(Assignee)

Comment 11

16 years ago
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

16 years ago
I guess I'd prefer to have CONTENT_NO_INDEX rather than to have two different ints
in nsIContent. 2cts.
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.
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.
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.
- 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 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?
Attachment #129701 - Flags: review?(caillon) → review-
(Assignee)

Comment 18

16 years ago
> >+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!
(Assignee)

Comment 19

16 years ago
Created attachment 131912 [details]
Updated diff -w (fixes the problems found by caillon)
(Assignee)

Updated

16 years ago
Attachment #131912 - Attachment is patch: false
Attachment #131912 - Attachment mime type: text/plain → application/x-gzip
(Assignee)

Updated

16 years ago
Attachment #131912 - Flags: superreview?(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 on attachment 131912 [details]
Updated diff -w (fixes the problems found by caillon)

See my comments.
Attachment #131912 - Flags: superreview?(peterv) → superreview+
(Assignee)

Comment 22

16 years ago
> > @@ -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

16 years ago
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.
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
(Assignee)

Comment 25

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

Comment 26

16 years ago
> nsIContent::IsContentOfType(nsIContent::eTEXT)

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

Comment 27

16 years ago
FIXED. Let's open new bugs for further deCOMtamination of nsIContent n' friends.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 29

16 years ago
Oops, ignore my post above. I see what you made...
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?
(Assignee)

Comment 31

16 years ago
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.
You need to log in before you can comment on or make changes to this bug.