Closed
Bug 1000959
Opened 10 years ago
Closed 10 years ago
nsHTMLEditor.cpp cleanup
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: ayg, Assigned: ayg)
References
Details
Attachments
(4 files)
12.92 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
995 bytes,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
1.03 KB,
patch
|
ehsan.akhgari
:
review-
|
Details | Diff | Splinter Review |
11.08 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=08596671427f
Attachment #8411924 -
Flags: review?(ehsan)
Comment 2•10 years ago
|
||
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+
Assignee | ||
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/932f6530318d
Flags: in-testsuite-
Assignee | ||
Comment 6•10 years ago
|
||
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
Assignee | ||
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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-
Assignee | ||
Comment 9•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b321d9dd195e
Attachment #8415328 -
Flags: review?(ehsan)
Comment 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=af70647529d7 http://hg.mozilla.org/integration/mozilla-inbound/rev/5c8ccabe15ac
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5c8ccabe15ac
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•