Closed
Bug 1088054
Opened 10 years ago
Closed 10 years ago
Reduce usage of nsISelection and nsIDOMRange; clean up CheckForEmptyBlock, GetGoodSelPointForNode, GetTopEnclosingMailCite, GetFirst/LastEditableChild, JoinNodesSmart, GetFirst/LastEditableLeaf, JoinNodeDeep, WillDeleteSelection
Categories
(Core :: DOM: Editor, enhancement)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: ayg, Assigned: ayg)
References
Details
Attachments
(10 files, 6 obsolete files)
79.88 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
8.41 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
7.06 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
9.51 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
6.85 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
14.70 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
157.96 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
18.84 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
8.43 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
45.55 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Flags: in-testsuite-
Assignee | ||
Comment 1•10 years ago
|
||
I didn't get rid of it entirely because some things get their selections directly from an nsISelectionController, and it uses an out param to return it, so it's a pain to put it in an nsRefPtr<Selection>. (I couldn't figure out a way to do it without an intermediate variable, although I figure some sort of cast would probably work.) Is there any way to add a method to nsISelectionController that returns an actual Selection? Or better yet -- could we just get rid of nsISelection entirely and merge it into Selection, and leave nsISelection as a typedef or something?
Attachment #8510342 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8510354 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8510363 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8510367 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 5•10 years ago
|
||
I'm not totally convinced ETextType actually improves code readability in this case.
Attachment #8510372 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 6•10 years ago
|
||
I changed the types of firstChild and lastChild from Element to nsIContent in AlignBlockContents, because using Element here was probably a bug introduced by <http://hg.mozilla.org/mozilla-central/rev/4439b62ad770>. The added type safety in this patch caught it (yay).
Attachment #8510449 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8510452 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8510455 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8510461 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 10•10 years ago
|
||
This is expected to be my last patch until sometime after March 22. I will be able to review your comments Sunday morning my time, so if you could finish by then it would be ideal for me, but of course I could also make time later in the week if necessary. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=767e7bafbd35
Attachment #8510478 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 11•10 years ago
|
||
I'm uploading several updated patch versions to fix small bugs that caused try failures (mostly typos).
Attachment #8510342 -
Attachment is obsolete: true
Attachment #8510342 -
Flags: review?(ehsan.akhgari)
Attachment #8511003 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8510372 -
Attachment is obsolete: true
Attachment #8510372 -
Flags: review?(ehsan.akhgari)
Attachment #8511005 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8511003 -
Attachment is obsolete: true
Attachment #8511003 -
Flags: review?(ehsan.akhgari)
Attachment #8511007 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8510449 -
Attachment is obsolete: true
Attachment #8510449 -
Flags: review?(ehsan.akhgari)
Attachment #8511008 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8510461 -
Attachment is obsolete: true
Attachment #8510461 -
Flags: review?(ehsan.akhgari)
Attachment #8511009 -
Flags: review?(ehsan.akhgari)
Assignee | ||
Comment 16•10 years ago
|
||
New try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7d9bc84b65b6
Attachment #8510478 -
Attachment is obsolete: true
Attachment #8510478 -
Flags: review?(ehsan.akhgari)
Attachment #8511010 -
Flags: review?(ehsan.akhgari)
Comment 17•10 years ago
|
||
(In reply to :Aryeh Gregor from comment #1) > Is there any way to add a method to nsISelectionController that returns an > actual Selection? Yes, you can declare natives in XPIDL. > Or better yet -- could we just get rid of nsISelection > entirely and merge it into Selection, and leave nsISelection as a typedef or > something? That probably has addon-compat issues...
Updated•10 years ago
|
Attachment #8511007 -
Flags: review?(ehsan.akhgari) → review+
Updated•10 years ago
|
Attachment #8510354 -
Flags: review?(ehsan.akhgari) → review+
Updated•10 years ago
|
Attachment #8510363 -
Flags: review?(ehsan.akhgari) → review+
Updated•10 years ago
|
Attachment #8510367 -
Flags: review?(ehsan.akhgari) → review+
Comment 18•10 years ago
|
||
Comment on attachment 8511005 [details] [diff] [review] Part 5 v2 -- Clean up nsHTMLEditRules::GetTopEnclosingMailCite Review of attachment 8511005 [details] [diff] [review]: ----------------------------------------------------------------- ::: editor/libeditor/nsHTMLEditRules.h @@ +304,5 @@ > nsIDOMNode *aNodeRight, > nsCOMPtr<nsIDOMNode> *aOutMergeParent, > int32_t *aOutMergeOffset); > + mozilla::dom::Element* GetTopEnclosingMailCite(nsINode& aNode, > + ETextType aTextType); It seems to me that in both cases, you can drop the aTextType argument completely and just check IsPlaintextEditor() inside the implementation of these functions, as we never call them with a value differnet than IsPlaintextEditor().
Attachment #8511005 -
Flags: review?(ehsan.akhgari) → review+
Updated•10 years ago
|
Attachment #8511008 -
Flags: review?(ehsan.akhgari) → review+
Comment 19•10 years ago
|
||
Comment on attachment 8510452 [details] [diff] [review] Part 7 -- Clean up nsHTMLEditRules::JoinNodesSmart Review of attachment 8510452 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the checks below fixed. ::: editor/libeditor/nsHTMLEditRules.cpp @@ +2165,5 @@ > // if so, join them! > + nsCOMPtr<nsIContent> sibling_ = do_QueryInterface(sibling); > + NS_ENSURE_STATE(sibling_ || !sibling); > + nsCOMPtr<nsIContent> startNode_ = do_QueryInterface(startNode); > + NS_ENSURE_STATE(startNode_ || !startNode); Are these checks correct? It seems like the derefs below can in theory crash if sibling or startNode are null.
Attachment #8510452 -
Flags: review?(ehsan.akhgari) → review+
Updated•10 years ago
|
Attachment #8510455 -
Flags: review?(ehsan.akhgari) → review+
Comment 20•10 years ago
|
||
Comment on attachment 8511009 [details] [diff] [review] Part 9 v2 -- Clean up nsEditor::JoinNodeDeep Review of attachment 8511009 [details] [diff] [review]: ----------------------------------------------------------------- ::: editor/libeditor/nsHTMLEditRules.cpp @@ +2428,5 @@ > { > + nsCOMPtr<nsIContent> leftParent_ = do_QueryInterface(leftParent); > + NS_ENSURE_STATE(leftParent_ || !leftParent); > + nsCOMPtr<nsIContent> rightParent_ = do_QueryInterface(rightParent); > + NS_ENSURE_STATE(rightParent_ || !rightParent); This doesn't seem correct either.
Attachment #8511009 -
Flags: review?(ehsan.akhgari) → review+
Comment 21•10 years ago
|
||
Comment on attachment 8511010 [details] [diff] [review] Part 10 v2 -- Clean up nsHTMLEditRules::WillDeleteSelection Review of attachment 8511010 [details] [diff] [review]: ----------------------------------------------------------------- ::: editor/libeditor/nsHTMLEditRules.cpp @@ +2502,5 @@ > + // selection if we'redeleting forward, because the end of the selection will > + // still be in the next block. And same thing for deleting backwards > + // (selection should collapse to the end, because the beginning will still be > + // in the first block). See Bug 507936 > + if (join ? aAction == nsIEditor::eNext : aAction == nsIEditor::ePrevious) { I'd rewrite this as: if (aAction == (join ? nsIEditor::eNext ...)) {
Attachment #8511010 -
Flags: review?(ehsan.akhgari) → review+
Assignee | ||
Comment 22•10 years ago
|
||
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO 11/3-11/21) from comment #18) > It seems to me that in both cases, you can drop the aTextType argument > completely and just check IsPlaintextEditor() inside the implementation of > these functions, as we never call them with a value differnet than > IsPlaintextEditor(). Hah! Good catch. (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO 11/3-11/21) from comment #19) > Are these checks correct? It seems like the derefs below can in theory > crash if sibling or startNode are null. sibling and startNode actually can't ever be null here, so it doesn't make a difference. IsTextNode would have returned false if they were, so we wouldn't be in the if block. When I convert nsIDOMNode to nsINode, I only return from the function if the conversion made the variable null, not if it was already null, because otherwise I'm changing behavior (and I have caused test failures before this way). But I guess I shouldn't bother if a null variable would cause an obvious crash -- I won't break anything by adding a check then.
Assignee | ||
Comment 23•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5dde7b37b24e https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=1533eef79502
Comment 24•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/311c0ed7a8d2 https://hg.mozilla.org/mozilla-central/rev/e1d2814c9e7b https://hg.mozilla.org/mozilla-central/rev/658f4c23de2a https://hg.mozilla.org/mozilla-central/rev/4c792bf9bc28 https://hg.mozilla.org/mozilla-central/rev/5c2ca13c7812 https://hg.mozilla.org/mozilla-central/rev/e94dc3c78ecb https://hg.mozilla.org/mozilla-central/rev/b162578302dd https://hg.mozilla.org/mozilla-central/rev/c3a599c2432e https://hg.mozilla.org/mozilla-central/rev/42207ac8544f https://hg.mozilla.org/mozilla-central/rev/1533eef79502
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•