671672 - Reduce a bunch of console spam in debug builds caused by the HTML editor

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[(no longer active)]

A bunch of stuff that we whine about on the console are actually expected, and dealt with correctly...

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 545999 [details] [diff] [review]
Patch (v1)

Review of attachment 545999 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/document/src/nsHTMLDocument.cpp
@@ +2586,5 @@
>  
>          rv = range->SelectNode(node);
> +        if (NS_FAILED(rv)) {
> +          return;
> +        }

Why would this fail? At least add a comment.

::: editor/libeditor/html/nsHTMLEditRules.cpp
@@ +1240,5 @@
>    res = aSelection->GetIsCollapsed(&bCollapsed);
>    NS_ENSURE_SUCCESS(res, res);
> +  if (!bCollapsed) {
> +    return NS_OK;
> +  }

From the comment, it sounds like this is something we're not handling correctly.

::: editor/libeditor/html/nsHTMLEditor.cpp
@@ +4288,5 @@
>    }
>    nsCOMPtr<nsINode> node = do_QueryInterface(aNode);
> +  if (!node) {
> +    return PR_FALSE;
> +  }

This should be an assertion, it should never fail. Unless we're passing null to it, but surely that's not supposed to happen?

Comment 3

[(no longer active)]

Attachment: Patch (v2)

(In reply to comment #2)
> Comment on attachment 545999 [details] [diff] [review] [review]
> Patch (v1)
> 
> Review of attachment 545999 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/document/src/nsHTMLDocument.cpp
> @@ +2586,5 @@
> >  
> >          rv = range->SelectNode(node);
> > +        if (NS_FAILED(rv)) {
> > +          return;
> > +        }
> 
> Why would this fail? At least add a comment.

Because the node might no longer be in the document.  For example, if we have a contenteditable div, and we set the innerHTML property on a parent of it, the node gets detached from the document, and then this function is called on it to make it non-editable.  We don't need to spell check these types of nodes, so ignoring this failure is safe here.

> ::: editor/libeditor/html/nsHTMLEditRules.cpp
> @@ +1240,5 @@
> >    res = aSelection->GetIsCollapsed(&bCollapsed);
> >    NS_ENSURE_SUCCESS(res, res);
> > +  if (!bCollapsed) {
> > +    return NS_OK;
> > +  }
> 
> From the comment, it sounds like this is something we're not handling
> correctly.

Well, what this comment really means is that we just ignore non-collapsed selections here.  I modified the comment to make it clearer.

> ::: editor/libeditor/html/nsHTMLEditor.cpp
> @@ +4288,5 @@
> >    }
> >    nsCOMPtr<nsINode> node = do_QueryInterface(aNode);
> > +  if (!node) {
> > +    return PR_FALSE;
> > +  }
> 
> This should be an assertion, it should never fail. Unless we're passing null
> to it, but surely that's not supposed to happen?

Yes, it would fail if we pass null to the function.  Specifically, this can happen here: http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditRules.cpp#5768 in case the start/end node does not have any children.  I think we can allow this function to deal with null input, although it might make a bit more sense to move this change to be the first thing happening in the function.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 546165 [details] [diff] [review]
Patch (v2)

Review of attachment 546165 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 5

[Joe Drew (not getting mail)]

This (along with most things committed on Friday afternoon) was backed out of mozilla-inbound in order to clear up orange.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/bb0926cfe5f8