Closed Bug 1000959 Opened 8 years ago Closed 8 years ago

nsHTMLEditor.cpp cleanup


(Core :: DOM: Editor, defect)

Not set





(Reporter: ayg, Assigned: ayg)




(4 files)

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

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):
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: and line 1275 and line 2391 and line 2692 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 <> and <>, please fix them similarly.
Attachment #8415328 - Flags: review?(ehsan) → review+
Closed: 8 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.