1000959 - nsHTMLEditor.cpp cleanup

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=08596671427f

Comment 2

[(no longer active)]

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))

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: interdiff to fix crash

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.

Comment 4

[(no longer active)]

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!

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/932f6530318d

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Further patch to fix test failure

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()?

Comment 8

[(no longer active)]

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

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Further patch v2

https://tbpl.mozilla.org/?tree=Try&rev=b321d9dd195e

Comment 10

[(no longer active)]

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.

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://tbpl.mozilla.org/?tree=Try&rev=af70647529d7

http://hg.mozilla.org/integration/mozilla-inbound/rev/5c8ccabe15ac

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5c8ccabe15ac