Closed
Bug 671672
Opened 14 years ago
Closed 14 years ago
Reduce a bunch of console spam in debug builds caused by the HTML editor
Categories
(Core :: DOM: Editor, defect)
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
Details
Attachments
(1 file, 1 obsolete file)
19.09 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
A bunch of stuff that we whine about on the console are actually expected, and dealt with correctly...
Assignee | ||
Comment 1•14 years ago
|
||
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?
Assignee | ||
Comment 3•14 years ago
|
||
(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+
Comment 5•14 years ago
|
||
This (along with most things committed on Friday afternoon) was backed out of mozilla-inbound in order to clear up orange.
Comment 6•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in
before you can comment on or make changes to this bug.
Description
•