1086349 - Clean up JoinNodeTxn, SplitNodeTxn, InsertTextImpl, DeleteSelectionAndCreateElement, SplitNodeImpl, GetNext/PriorNode, GetRight/LeftmostChild, (Tag)CanContain(Tag), BeginningOfDocument

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Comment 1

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

Attachment: Patch part 1 -- Clean up JoinNodeTxn

Comment 2

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

Attachment: Patch part 2 -- Clean up SplitNodeTxn

Comment 3

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

Attachment: Patch part 3 -- Clean up nsEditor::InsertTextImpl

Comment 4

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

Attachment: Patch part 4 -- Clean up nsEditor::DeleteSelectionAndCreateElement

Comment 5

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

Attachment: Patch part 5 -- Clean up nsEditor::SplitNodeImpl

Comment 6

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

Attachment: Patch part 6 -- Remove old nsEditor::GetNextNode/GetPriorNode overloads

Three of these were not even called, which is why I like porting callers to nsINode* instead of keeping an old nsIDOMNode* shim around.

Comment 7

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

Attachment: Patch part 7 -- Remove old nsEditor::GetRight/LeftmostChild overloads

Comment 8

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

Attachment: Patch part 8 -- Clean up nsEditor::(Tag)CanContain(Tag)

These functions are pretty broken, and return false for lots of things that they should return true from.  But probably it's not a good idea to try fixing.

Comment 9

[Mats Palmgren (inactive)]

(In reply to :Aryeh Gregor from comment #1)
> Created attachment 8508642 [details] [diff] [review]
> Patch part 1 -- Clean up JoinNodeTxn

-nsEditor::JoinNodes(nsINode* aNodeToKeep, nsIContent* aNodeToMove)
+nsEditor::JoinNodes(nsINode& aLeftNode, nsINode& aRightNode)

Do we really want to start using references for well-known objects
like nsINode?  I feel like it isn't worth it, it just introduces
friction with the rest of the code base which is using pointers.

A few examples of what I mean by friction:
I have a "nsCOMPtr<nsINode> node" and want to call the method above,
now I have to use "*node".  To compare nodes "&aLeftNode == node",
or call methods in the rest the platform "parent->IndexOf(&aLeftNode)".
I suspect that there will frequently be a mix of nsCOMPtr<nsINode>
(or nsINode*) and nsINode& in the same method which is confusing
to work with (should I use node. or node-> here?)
This patch have examples of all of these.

I would prefer if we keep using nsINode* here for consistency with
the rest of the code base.  Because it's just easier to work with
when everyone is using the same conventions.

Unless, of course, we're planning to change the rest of the code base
to use references as well, but I haven't heard of such plans.

Comment 10

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

Attachment: Patch part 9 -- Clean up nsHTMLEditor::BeginningOfDocument

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

Comment 11

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

(In reply to Mats Palmgren (:mats) from comment #9)
> Do we really want to start using references for well-known objects
> like nsINode?

I'm somewhat ambivalent.  I first saw the pattern in nsINode::InsertBefore and friends, which at some point started using it.  I asked Ehsan if I should use it when refactoring, and he said try it and we'd see how we like it, or something like that.

Ehsan, what do you think at this point?  I'm ambivalent.  On the one hand, I like a lot the fact that it removes the tedious boilerplate MOZ_ASSERT() for parameters and replaces it with dereferencing the pointer at the call site, which is a very clear indicator to the caller that the parameter had better not be null, as well as a guaranteed crash.  I also like that you use . instead of -> to call methods, because it removes the need for a mental "is this null?" check on every method invocation.  Having to add & every time you call another function is useless, but not really harmful.

Overall I'm inclined to say I like it, although it's a shame that it doesn't work for nsCOMPtrs that we know cannot be null.  But it isn't consistent with the rest of our codebase, so I don't mind giving it up for new code if we don't like it.  If we do like it, it would be interesting to have a decision about the broader Mozilla codebase switching to this style, specifically for types like nsINode or nsIAtom where there's no confusion about whether the callee can modify the object.

(In reply to Mats Palmgren (:mats) from comment #9)
> A few examples of what I mean by friction:
> I have a "nsCOMPtr<nsINode> node" and want to call the method above,
> now I have to use "*node".

This I specifically think is a good thing!  It means the caller realizes that the node had better not be null.

Comment 12

[Mats Palmgren (inactive)]

Don't get me wrong - I have nothing against reference types per se.
It's just that different coding conventions in different parts of
the code for a common type like nsINode seems like a bad idea.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Bindings use reference types for known non-null args, which is where the InsertBefore bit is coming from.

You _can_ always use an OwningNonNull instead of an nsCOMPtr if you know it can't be null, btw.

My take on this is that explicit indications of non-nullness expectations are good.  But then again, I was involved in the decision to make bindings use reference types for known-not-null, so I'm kinda biased.  ;)

Comment 14

[(no longer active)]

In the abstract, I think there is a lot of value in being able to look at a type and assume that it _can_ be null if it's a pointer, or that it's guaranteed to be non-null if it's a reference (let's ignore the fact that C++ actually allows null references. ;-)

I think the biggest downside of this, as Mats has suggested, is the inconsistency.  It's also obviously a very long term project, especially that a boil-the-ocean solution for a one time conversion is probably impractical.  However, my hope is that the inconsistency is not bad enough to make the whole effort pointless (as I believe the bindings code has demonstrated), which is why I told Aryeh that he should feel free to do this in editor/.  Doing this in editor/ has the advantage that it's quite disentangled from the rest of Gecko, so we won't be inflicting the pain on other code by doing this.  My hope is that if down the road we agree that the upsides here are more than the downsides, we can try to socialize the idea to the broader code base.

FWIW, I'd say that OwningNonNull should also be considered usable in editor/.

Comment 15

[(no longer active)]

Comment on attachment 8508642 [details] [diff] [review]
Patch part 1 -- Clean up JoinNodeTxn

Review of attachment 8508642 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/JoinNodeTxn.cpp
@@ +56,4 @@
>    NS_ENSURE_TRUE(leftParent, NS_ERROR_NULL_POINTER);
>  
> +  // Verify that mLeftNode and mRightNode have the same parent
> +  NS_ENSURE_TRUE(mRightNode->GetParentNode(), NS_ERROR_NULL_POINTER);

We can eliminate this condition based on the if statement below, right?

::: editor/libeditor/JoinNodeTxn.h
@@ +35,3 @@
>  
> +  /* Call this after constructing to ensure the inputs are correct */
> +  NS_IMETHOD Init();

I hate it when constructors do part of the work and expect you to call another function to finish it.  How about we make the constructor just take an nsEditor&, and make Init() take the other two args?

Comment 16

[(no longer active)]

(I'll try to finish reviewing the rest of the patches tomorrow, sorry for the delay.)

Comment 17

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

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #15)
> I hate it when constructors do part of the work and expect you to call
> another function to finish it.  How about we make the constructor just take
> an nsEditor&, and make Init() take the other two args?

What about renaming the method something like CheckValidity(), to reflect what it actually does?  That seems better to me than splitting initialization across two methods.  This is a trivial object, so it doesn't make a big difference, but in general it's nice to have types that can't actually be uninitialized.

Comment 18

[(no longer active)]

(In reply to :Aryeh Gregor from comment #17)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #15)
> > I hate it when constructors do part of the work and expect you to call
> > another function to finish it.  How about we make the constructor just take
> > an nsEditor&, and make Init() take the other two args?
> 
> What about renaming the method something like CheckValidity(), to reflect
> what it actually does?  That seems better to me than splitting
> initialization across two methods.  This is a trivial object, so it doesn't
> make a big difference, but in general it's nice to have types that can't
> actually be uninitialized.

That sounds fine to me.

Comment 19

[(no longer active)]

Comment on attachment 8508683 [details] [diff] [review]
Patch part 5 -- Clean up nsEditor::SplitNodeImpl

Review of attachment 8508683 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsEditor.cpp
@@ +2647,5 @@
> +  } else {
> +    // Otherwise it's an interior node, so shuffle around the children. Go
> +    // through list backwards so deletes don't interfere with the iteration.
> +    nsCOMPtr<nsINodeList> childNodes = aExistingRightNode.ChildNodes();
> +    for (int32_t i = aOffset - 1; 0 <= i; i--) {

Nit: i >=0

Comment 20

[(no longer active)]

Comment on attachment 8508689 [details] [diff] [review]
Patch part 7 -- Remove old nsEditor::GetRight/LeftmostChild overloads

Review of attachment 8508689 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsHTMLEditRules.cpp
@@ +6890,5 @@
>    NS_ENSURE_SUCCESS(res, res);
>  
>    // selection to beginning of right hand para;
>    // look inside any containers that are up front.
> +  nsCOMPtr<nsINode> rightPara_ = do_QueryInterface(rightPara);

Perhaps call this rightParaNode, or some such?

::: editor/libeditor/nsHTMLEditorStyle.cpp
@@ +683,5 @@
>        NS_ENSURE_SUCCESS(res, res);
>      }
>    }
>    if (rightNode) {
> +    nsCOMPtr<nsINode> rightNode_ = do_QueryInterface(rightNode);

Ditto.  (Coming up with good names is hard!  If you can't think of something better, this is fine I guess)

@@ +707,5 @@
>                                 aOffset, aProperty, aAttribute,
>                                 address_of(leftNode), address_of(rightNode));
>      NS_ENSURE_SUCCESS(res, res);
>      // should be impossible to not get a new leftnode here
> +    nsCOMPtr<nsINode> leftNode_ = do_QueryInterface(leftNode);

Here too.

Comment 21

[(no longer active)]

Comment on attachment 8508697 [details] [diff] [review]
Patch part 8 -- Clean up nsEditor::(Tag)CanContain(Tag)

Review of attachment 8508697 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsHTMLEditRules.cpp
@@ -8023,5 @@
> -  res = mHTMLEditor->GetPriorHTMLNode(selNode, selOffset, address_of(nearNode), true);
> -  NS_ENSURE_SUCCESS(res, res);
> -  if (nearNode && (nsTextEditUtils::IsBreak(nearNode)
> -                   || nsEditor::IsTextNode(nearNode)
> -                   || nsHTMLEditUtils::IsImage(nearNode)

We probably want to keep nsHTMLEditUtils::IsImage around, and make it handle the picture element too.  So please keep calling IsImage here.

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 #20)
> ::: editor/libeditor/nsHTMLEditorStyle.cpp
> @@ +683,5 @@
> >        NS_ENSURE_SUCCESS(res, res);
> >      }
> >    }
> >    if (rightNode) {
> > +    nsCOMPtr<nsINode> rightNode_ = do_QueryInterface(rightNode);
> 
> Ditto.  (Coming up with good names is hard!  If you can't think of something
> better, this is fine I guess)
> 
> @@ +707,5 @@
> >                                 aOffset, aProperty, aAttribute,
> >                                 address_of(leftNode), address_of(rightNode));
> >      NS_ENSURE_SUCCESS(res, res);
> >      // should be impossible to not get a new leftnode here
> > +    nsCOMPtr<nsINode> leftNode_ = do_QueryInterface(leftNode);
> 
> Here too.

I haven't come up with any good naming scheme for an nsINode variable that comes from an nsIDOMNode (except for a parameter, where I drop the "a" prefix).  For the other way around, I prefix "dom".  I considered suffixing "node", but a lot of the variable names already end with "node".  So I settled on suffixing "_" for lack of any better ideas.

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/936687e4d443
https://hg.mozilla.org/mozilla-central/rev/36d01e61f60e
https://hg.mozilla.org/mozilla-central/rev/60d33b0a0ba0
https://hg.mozilla.org/mozilla-central/rev/5bf80a177e22
https://hg.mozilla.org/mozilla-central/rev/df69fb203da8
https://hg.mozilla.org/mozilla-central/rev/47da3b3b465f
https://hg.mozilla.org/mozilla-central/rev/d86b232a8910
https://hg.mozilla.org/mozilla-central/rev/2970a427f4c6
https://hg.mozilla.org/mozilla-central/rev/14e58f7fcb22