Closed Bug 1000959 Opened 6 years ago Closed 5 years ago

nsHTMLEditor.cpp cleanup

Categories

(Core :: Editor, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: ayg, Assigned: ayg)

References

Details

Attachments

(4 files)

No description provided.
Comment on attachment 8411924 [details] [diff] [review]
patch

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

::: editor/libeditor/html/nsHTMLEditor.cpp
@@ +832,5 @@
> +
> +  nsCOMPtr<nsINode> p = aNode->GetParentNode();
> +
> +  while (p) {
> +    if (p->IsElement() && NodeIsBlockStatic(p->AsElement())) {

You could just write this as:

   if (IsBlockNode(p))
Attachment #8411924 - Flags: review?(ehsan) → review+
A missing null check in the patch triggered a crash, happily caught by one of our crashtests.  I have to be careful about converting do_QI to AsDOMNode -- this has caught me before.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #2)
> You could just write this as:
> 
>    if (IsBlockNode(p))

No, because the method is static.
Attachment #8412472 - Flags: review?(ehsan)
Comment on attachment 8412472 [details] [diff] [review]
interdiff to fix crash

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

Oh-uh!  Sorry for not catching this!
Attachment #8412472 - Flags: review?(ehsan) → review+
It seems this was backed out, for a new assert failure that didn't show up on my try run (but which I can now reproduce locally):

https://hg.mozilla.org/integration/mozilla-inbound/rev/df459220856f
So it looks like I changed the behavior of GetBlockNodeParent().  If no block ancestor was found, previously it would just return the highest parent (typically the document).  It would set tmp to the parent of p, find that tmp was null, and break, which meant it returned p.  I think this was just a bug.  This caused one caller to break.  This patch fixes that caller, but I didn't audit all other callers.  Would you like this patch, or should I restore the old probably-unintended behavior of GetBlockNodeParent()?
Attachment #8414449 - Flags: review?(ehsan)
Comment on attachment 8414449 [details] [diff] [review]
Further patch to fix test failure

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

Let's fix all of the callers.  The following seem to be similarly affected:

http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditor.cpp#4706
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditor.cpp#4735
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditor.cpp#4794
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditor.cpp#4818
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditor.cpp#4838
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditRules.cpp#1273 and line 1275
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditRules.cpp#2384 and line 2391
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditRules.cpp#2687 and line 2692
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditRules.cpp#5090
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditRules.cpp#5286
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditRules.cpp#5384
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditRules.cpp#7931 and line 7934
Attachment #8414449 - Flags: review?(ehsan) → review-
Comment on attachment 8415328 [details] [diff] [review]
Further patch v2

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

You missed <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditor.cpp#4818> and <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditRules.cpp#5286>, please fix them similarly.
Attachment #8415328 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/5c8ccabe15ac
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1006982
You need to log in before you can comment on or make changes to this bug.