Closed Bug 1086349 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: DOM: Editor, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: ayg, Assigned: ayg)

References

Details

Attachments

(9 files)

28.25 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
25.43 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
21.82 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
10.60 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
11.82 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
10.35 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
11.52 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
43.28 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
4.97 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
No description provided.
Flags: in-testsuite-
Three of these were not even called, which is why I like porting callers to nsINode* instead of keeping an old nsIDOMNode* shim around.
Attachment #8508687 - Flags: review?(ehsan.akhgari)
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.
Attachment #8508697 - Flags: review?(ehsan.akhgari)
(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.
(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.
Flags: needinfo?(ehsan.akhgari)
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.
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.  ;)
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/.
Flags: needinfo?(ehsan.akhgari)
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?
Attachment #8508642 - Flags: review?(ehsan.akhgari) → review+
Attachment #8508671 - Flags: review?(ehsan.akhgari) → review+
(I'll try to finish reviewing the rest of the patches tomorrow, sorry for the delay.)
(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.
Flags: needinfo?(ehsan.akhgari)
(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.
Flags: needinfo?(ehsan.akhgari)
Attachment #8508677 - Flags: review?(ehsan.akhgari) → review+
Attachment #8508680 - Flags: review?(ehsan.akhgari) → review+
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
Attachment #8508683 - Flags: review?(ehsan.akhgari) → review+
Attachment #8508687 - Flags: review?(ehsan.akhgari) → review+
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.
Attachment #8508689 - Flags: review?(ehsan.akhgari) → review+
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.
Attachment #8508697 - Flags: review?(ehsan.akhgari) → review+
Attachment #8508711 - Flags: review?(ehsan.akhgari) → review+
(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.
Depends on: 1130651
No longer depends on: 1130651
Depends on: 1134545
You need to log in before you can comment on or make changes to this bug.