Closed Bug 671672 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: DOM: Editor, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

Details

Attachments

(1 file, 1 obsolete file)

A bunch of stuff that we whine about on the console are actually expected, and dealt with correctly...
Attached patch Patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #545999 - Flags: review?(roc)
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?
Attached patch Patch (v2)Splinter Review
(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.
Attachment #545999 - Attachment is obsolete: true
Attachment #546165 - Flags: review?(roc)
Attachment #545999 - Flags: review?(roc)
Comment on attachment 546165 [details] [diff] [review]
Patch (v2)

Review of attachment 546165 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #546165 - Flags: review?(roc) → review+
This (along with most things committed on Friday afternoon) was backed out of mozilla-inbound in order to clear up orange.
http://hg.mozilla.org/mozilla-central/rev/bb0926cfe5f8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.