1003894 - Editing code cleanup 3

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

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

https://tbpl.mozilla.org/?tree=Try

Comment 1

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

Attachment: Patch part 1 -- Change WSPoint.mTextNode to Text

That should have been: https://tbpl.mozilla.org/?tree=Try&rev=cbb8eb0ee84f

Comment 2

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

Attachment: Part 2 -- Clean up nsWSRunObject::CheckTrailingNBSPOfRun

See XXX's.  I think what I have now is okay, but I'm not totally sure.  (I didn't check the codepaths to see how the method might fail and still return non-null, I can if you want.)

Should ::DOMPoint maybe be renamed to avoid conflict with mozilla::dom::DOMPoint?  I can't think of a better name.

I made the new GetAsciiWSBounds take references instead of pointers to the nodes/offsets.  Is this preferable, or should I go with pointers?  References don't have to be null-checked (or anyway can't be), but it looks confusing to the caller.

Also, I made it take Text, which is the most specific return type possible.  Is this sensible, or is it better to make out-params more general (nsINode) so they work with whatever variables you have handy to pass in?  Or should I use some other convention for returning the values, like std::pair<DOMPoint, DOMPoint> or something?

Comment 3

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

Attachment: Part 3 -- Clean up nsWSRunObject::InsertBreak

Here I left InsertBreak taking pointers instead of references, for no particular reason.

XXX similar to before, except here I checked the code and it looks like the only error we're potentially ignoring is if GetSelection() fails (which means the <br> was created but the selection wasn't updated).

I still haven't figured out how to get .h whitespace right for methods with long names/return types.  :(

Comment 4

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

Attachment: Part 4 -- Clean up nsWSRunObject::DeleteWSBackward

Here you have awkwardness having to convert Text to nsINode because of GetAsciiWSBounds' return param type.  Do you know why I have to use .get() there?  I would have expected it to work without it.

Comment 5

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

Attachment: Part 5 -- Clean up nsWSRunObject::ScrubBlockBoundary

I saw no reason the caller shouldn't be the one to use an nsAutoTrackDOMPoint if it wants one, instead of the callee doing it automatically.  This code became significantly simpler with refactoring!

Comment 6

[(no longer active)]

(In reply to :Aryeh Gregor from comment #2)
> Should ::DOMPoint maybe be renamed to avoid conflict with
> mozilla::dom::DOMPoint?  I can't think of a better name.

I don't care that much.

> I made the new GetAsciiWSBounds take references instead of pointers to the
> nodes/offsets.  Is this preferable, or should I go with pointers? 
> References don't have to be null-checked (or anyway can't be), but it looks
> confusing to the caller.

References prevent us from using getter_AddRefs(), which has one very nice property which is setting the pointer to null before passing its address to the callee, that means that if the callee fails, you won't end up with a stale pointer pointing to the previous object if you forget to set the out arg to null in the callee.  So please use pointers and getter_AddRefs as I've said in the review comments.

> Also, I made it take Text, which is the most specific return type possible. 
> Is this sensible, or is it better to make out-params more general (nsINode)
> so they work with whatever variables you have handy to pass in?  Or should I
> use some other convention for returning the values, like std::pair<DOMPoint,
> DOMPoint> or something?

In general, I think more specific types are quite fine, no need to be generic just for the sake of being generic!

Comment 7

[(no longer active)]

Comment on attachment 8415348 [details] [diff] [review]
Part 2 -- Clean up nsWSRunObject::CheckTrailingNBSPOfRun

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

::: editor/libeditor/base/nsEditor.cpp
@@ +2448,5 @@
> +                                              int32_t aOffset,
> +                                              bool aSuppressIME)
> +{
> +  return InsertTextIntoTextNodeImpl(aStringToInsert,
> +      static_cast<nsIDOMCharacterData*>(GetAsDOMNode(aTextNode)),

I think you can get away with just the static_cast here.

::: editor/libeditor/html/nsHTMLEditor.cpp
@@ +1130,5 @@
> +{
> +  nsCOMPtr<nsIDOMNode> parent = GetAsDOMNode(aNode);
> +  int32_t offset = aOffset;
> +  nsCOMPtr<nsIDOMNode> outBRNode;
> +  // XXX We assume that if there's an error, the return value will be null.

That is in fact not correct, see here for example: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/text/nsPlaintextEditor.cpp#501>.

The more interesting question is, what should we do here.  This happens when we create a BR node, but fail to set the selection for some reason.  If this passes our tests, I'm fine with returning non-null in that case.

::: editor/libeditor/html/nsWSRunObject.h
@@ +347,5 @@
>      WSPoint  GetCharBefore(const WSPoint &aPoint);
>      nsresult ConvertToNBSP(WSPoint aPoint,
>                             AreaRestriction aAR = eAnywhere);
> +    void     GetAsciiWSBounds(int16_t aDir, nsINode* aNode, int32_t aOffset,
> +                              nsCOMPtr<mozilla::dom::Text>& outStartNode,

Please make these mozilla::dom::Text**, and use getter_AddRefs() in the callers.

Comment 8

[(no longer active)]

Comment on attachment 8415352 [details] [diff] [review]
Part 3 -- Clean up nsWSRunObject::InsertBreak

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

::: editor/libeditor/html/nsWSRunObject.h
@@ +207,5 @@
>      // and makes any needed adjustments to ws around that point.
>      // example of fixup: normalws after {aInOutParent,aInOutOffset}
>      //                   needs to begin with nbsp.
> +    already_AddRefed<mozilla::dom::Element>
> +        InsertBreak(nsCOMPtr<nsINode>* aInOutParent, int32_t* aInOutOffset,

If you're going for regular indentation, please use 2-space and not 4-space.  Thanks!

Also, if it's not too much work, please make this nsINode**, passing nsCOMPtr*'s is an antipattern in the editor code which we should fix at some point (it basically prevents you from having the nice side effect I mentioned in comment 6, and IIRC the last time I tried to fix this pattern wholesale I found a number of bugs of that nature.)

::: editor/libeditor/text/nsPlaintextEditor.cpp
@@ +444,5 @@
> +                                EDirection aSelect)
> +{
> +  nsCOMPtr<nsIDOMNode> parent(GetAsDOMNode(*aInOutParent));
> +  nsCOMPtr<nsIDOMNode> br;
> +  // XXX This will throw away the error if GetSelection() fails

I think this is fine too if it passes our tests...

Comment 9

[(no longer active)]

(In reply to :Aryeh Gregor from comment #4)
> Here you have awkwardness having to convert Text to nsINode because of
> GetAsciiWSBounds' return param type.  Do you know why I have to use .get()
> there?  I would have expected it to work without it.

Because nsCOMPtr<T> doesn't have a constructor or assignment operator which accepts an nsCOMPtr<U>.  It does have a ctor for already_AddRefed<U>.  I forget why we don't have the former.  Nathan, do you know?

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #9)
> (In reply to :Aryeh Gregor from comment #4)
> > Here you have awkwardness having to convert Text to nsINode because of
> > GetAsciiWSBounds' return param type.  Do you know why I have to use .get()
> > there?  I would have expected it to work without it.
> 
> Because nsCOMPtr<T> doesn't have a constructor or assignment operator which
> accepts an nsCOMPtr<U>.  It does have a ctor for already_AddRefed<U>.  I
> forget why we don't have the former.  Nathan, do you know?

I don't know, but I suspect the simplest answer is that nobody has needed it yet.  (Bug 857645 didn't need the actual copying, I guess.)

I do know that in bug 980753 Trevor tried to template<typename U> nsRefPtr's constructors and ran into problems because of the specialness of the copy constructor: you need to have an explicit nsCOMPtr(const nsCOMPtr<T>&) copy constructor, just having one templated over |U| and expecting template instantiation to pick it up doesn't work.  I don't know offhand whether having:

nsCOMPtr(const nsCOMPtr<T>&) { ... }
template<typename U> nsCOMPtr(const nsCOMPtr<U>&) { ... }

would work as expected, but that's roughly what you would need to make it work.

Comment 11

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

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #7)
> I think you can get away with just the static_cast here.

Nope, doesn't compile.  Text doesn't inherit from nsIDOMCharacterData.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #8)
> Also, if it's not too much work, please make this nsINode**, passing
> nsCOMPtr*'s is an antipattern in the editor code which we should fix at some
> point (it basically prevents you from having the nice side effect I
> mentioned in comment 6, and IIRC the last time I tried to fix this pattern
> wholesale I found a number of bugs of that nature.)

This particular case is an in-out param, so getter_AddRefs doesn't work and this is the right way to do it, correct?


https://tbpl.mozilla.org/?tree=Try&rev=af70647529d7

http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=209d6ee29737

Comment 12

[(no longer active)]

Yeah, I guess so..  Thanks!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e33360172721
https://hg.mozilla.org/mozilla-central/rev/789fa974738e
https://hg.mozilla.org/mozilla-central/rev/f0ff0e4ed341
https://hg.mozilla.org/mozilla-central/rev/a096d53fac90
https://hg.mozilla.org/mozilla-central/rev/209d6ee29737

Comment 14

[[e-mail]]

Comment on attachment 8415340 [details] [diff] [review]
Patch part 1 -- Change WSPoint.mTextNode to Text

>-      nsCOMPtr<nsIContent> mTextNode;
>+      nsCOMPtr<mozilla::dom::Text> mTextNode;
>       uint32_t mOffset;
>       char16_t mChar;
> 
>       WSPoint() : mTextNode(0),mOffset(0),mChar(0) {}
>       WSPoint(nsINode* aNode, int32_t aOffset, char16_t aChar) :
>                      mTextNode(do_QueryInterface(aNode)),mOffset(aOffset),mChar(aChar)
mozilla::dom::Text isn't an interface, so you shouldn't be able to do_QueryInterface to it. The fact you can is bug 514280, and this new regression just blocked my landing of it, so this is now my problem instead of yours. Except I'm going to file a bug for you to fix it, and add a workaround and a comment pointing to the bug that I file.

Comment 15

[(no longer active)]

(In reply to comment #14)
> Comment on attachment 8415340 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=8415340
> Patch part 1 -- Change WSPoint.mTextNode to Text
> 
> >-      nsCOMPtr<nsIContent> mTextNode;
> >+      nsCOMPtr<mozilla::dom::Text> mTextNode;
> >       uint32_t mOffset;
> >       char16_t mChar;
> > 
> >       WSPoint() : mTextNode(0),mOffset(0),mChar(0) {}
> >       WSPoint(nsINode* aNode, int32_t aOffset, char16_t aChar) :
> >                      mTextNode(do_QueryInterface(aNode)),mOffset(aOffset),mChar(aChar)
> mozilla::dom::Text isn't an interface, so you shouldn't be able to
> do_QueryInterface to it. The fact you can is bug 514280, and this new
> regression just blocked my landing of it, so this is now my problem instead of
> yours. Except I'm going to file a bug for you to fix it, and add a workaround
> and a comment pointing to the bug that I file.

Can't we give Text an IID?

Comment 16

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

I wouldn't have a problem with using IsNodeOfType() and static_cast here, although I'd like IsText()/AsText() helpers like for Element.  IsText/AsText is much clearer code than do_QI anyway, IMO.  (What about IsContent/AsContent, for that matter?  Seems like a lot of methods, but I guess dynamic_cast doesn't actually work in real life?)

Comment 17

[[e-mail]]

(In reply to comment #14)
> add a workaround

Except you put this in a header file, so I can't just copy and paste my usual workaround...

Comment 18

[(no longer active)]

We have RTTI turned off so no dynamic_cast for you, but the other alternative in comment 16 sounds good too.

Comment 19

[[e-mail]]

Attachment: Alternate workaround

Does this work for you?

Comment 20

[(no longer active)]

Comment on attachment 8420382 [details] [diff] [review]
Alternate workaround

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

Yes, thanks!

Comment 21

[[e-mail]]

Attachment: Alternate workaround

Turns out the previous patch missed a few more abuses of do_QueryInterface that I could only find when I had combined it with my do_QueryInterface abuse finding patch from bug 514280.

tl;dr: extra Text to nsIContent changes.

Comment 22

[[e-mail]]

https://tbpl.mozilla.org/?tree=Try&rev=8874abfbcbc0
https://hg.mozilla.org/integration/mozilla-inbound/rev/443b60f30115

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/443b60f30115