1088054 - Reduce usage of nsISelection and nsIDOMRange; clean up CheckForEmptyBlock, GetGoodSelPointForNode, GetTopEnclosingMailCite, GetFirst/LastEditableChild, JoinNodesSmart, GetFirst/LastEditableLeaf, JoinNodeDeep, WillDeleteSelection

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Comment 1

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

Attachment: Part 1 -- Reduce nsISelection use in editor/

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?

Comment 2

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

Attachment: Part 2 -- Reduce nsIDOMRange use in editor/

Comment 3

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

Attachment: Part 3 -- Clean up nsHTMLEditRules::CheckForEmptyBlock

Comment 4

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

Attachment: Part 4 -- Clean up nsHTMLEditRules::GetGoodSelPointForNode

Comment 5

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

Attachment: Part 5 -- Clean up nsHTMLEditRules::GetTopEnclosingMailCite

I'm not totally convinced ETextType actually improves code readability in this case.

Comment 6

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

Attachment: Part 6 -- Clean up nsHTMLEditor::GetFirst/LastEditableChild

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

Comment 7

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

Attachment: Part 7 -- Clean up nsHTMLEditRules::JoinNodesSmart

Comment 8

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

Attachment: Part 8 -- Clean up nsHTMLEditor::GetFirst/LastEditableLeaf

Comment 9

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

Attachment: Part 9 -- Clean up nsEditor::JoinNodeDeep

Comment 10

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

Attachment: Part 10 -- Clean up nsHTMLEditRules::WillDeleteSelection

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

Comment 11

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

Attachment: Part 1 v2 -- Reduce nsISelection use in editor/

I'm uploading several updated patch versions to fix small bugs that caused try failures (mostly typos).

Comment 12

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

Attachment: Part 5 v2 -- Clean up nsHTMLEditRules::GetTopEnclosingMailCite

Comment 13

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

Attachment: Part 1 v2 -- Reduce nsISelection use in editor/

Comment 14

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

Attachment: Part 6 v2 -- Clean up nsHTMLEditor::GetFirst/LastEditableChild

Comment 15

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

Attachment: Part 9 v2 -- Clean up nsEditor::JoinNodeDeep

Comment 16

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

Attachment: Part 10 v2 -- Clean up nsHTMLEditRules::WillDeleteSelection

New try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7d9bc84b65b6

Comment 17

[(no longer active)]

(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...

Comment 18

[(no longer active)]

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

Comment 19

[(no longer active)]

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.

Comment 20

[(no longer active)]

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.

Comment 21

[(no longer active)]

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

Comment 22

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

(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.

Comment 23

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

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5dde7b37b24e
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=1533eef79502

Comment 24

[Phil Ringnalda (:philor)]

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