419532 - deCOMtaminate nsICaret

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsICaret should go away. We should use nsCaret.h directly, with non-virtual methods and deCOMtaminated signatures.

Comment 1

[John Daniels]

I'm working on this bug now.

Comment 2

[John Daniels]

Since the functions are no longer "defined" in the header file, they need to be linked to the other components. What's the best way to do that? Right now I'm having trouble figuring out how to link the accessibility module to the layout module. It doesn't help that I'm unfamiliar with the build system. 

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Urgh, what functions does accessibility use? We don't really want to link it into layout. If it can include nsCaret.h successfully then you can just make the functions it needs virtual to make linking problems go away.

Comment 4

[John Daniels]

Attachment: makes nsICaret.h unnessesary, makes signatures cleaner.

Ok, so nsCaret.h is left with a couple virtual functions, and it currently lists itself as implementing itself as an interface, which may be a little odd, but I had to do it or else it just segfaults. Obviously it still uses XPCom, but the signatures are nicer and there are only 2 virtual functions. I'm building and running after deleting nsICaret.h, though I'm not sure what other tests need to be run, or what else should be done.

Also, I had to change something unrelated to this bug to get it to build. Someone with a username of john put some stuff in an #ifdef DEBUG-john that broke the build when DEBUG-john was defined, as it is by default for anyone with a username of john. Unfortunately, my username is also john, so I just decided to make a change to the code ( I moved one function definition higher) so that it didn't break the build, although you think whoever set that would have realized that it was breaking things. Oh well.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks great!

That "john" probably disappeared years ago. If any DEBUG_john stuff gives you any more trouble, just remove it.

+  return;
 }

Just remove trailing "return"s

+PRBool nsCaret::SetCaretDOMSelection(nsISelection *aDOMSel)

I wouldn't bother converting nsresults to PRBools. Seems a bit more informative to just leave them as nsresult.

+  else
     return nsnull;

Don't do else after return. Just put return nsnull as the last statement.

+    void    GetCaretDOMSelection(nsISelection **outDOMSel);

This can switch to returning nsISelection* directly. I don't think callers hold on to it long enough to need to addref it, so their pointers can change from nsCOMPtr to just plain nsISelection*.

You can change all the nsCOMPtr<nsCaret> to nsRefPtr<nsCaret> --- that might let you get rid of the IID on nsCaret, since nsCOMPtrs need IIDs but nsRefPtrs don't.

Comment 6

[John Daniels]

Attachment: Gets rid of nsCaret IID

This still leaves nsCaret with QueryInterface(), since it implements nsISelectionListener. I changed all the instances of nsCOMPtr<nsCaret> to nsRefPtr<nsCaret>, and got rid of any instances where the pointer was created indirectly using XPCOM.

This isn't relevant for nsCaret, since it currently still implements QueryInterface, but is there any Macro that implements AddRef() and Release() only for the purpose of RefPtrs?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 329394 [details] [diff] [review]
Gets rid of nsCaret IID

+  nsISelection* domSelection;
+  
+  domSelection = caret->GetCaretDOMSelection();

Fuse this to one line, "nsISelection* domSelection = ...;"

+class nsCaret : 
                 public nsISelectionListener

Put those on one line.

Otherwise looks good to me.

I don't think we have a macro for inline AddRef/Release. Well, Thebes has one ... THEBES_INLINE_DECL_REFCOUNTING.

Post a new patch with those changes and mark this bug checkin-needed. Thanks!!!

Comment 8

[John Daniels]

Attachment: last few changes requested by roc

That should be everything, and you're welcome.

Comment 9

[John Daniels]

Thanks for setting checkin-needed. Sorry about that.

Comment 10

[Matthias Versen [:Matti]]

BTW: The John is John Keiser (jkeiser) which is long gone (added in bug 34297) 

Comment 11

[Dão Gottwald [:dao]]

http://hg.mozilla.org/index.cgi/mozilla-central/rev/b9582ba12a6a

Comment 12

[[e-mail]]

I don't suppose there's a public API to force the caret to appear any more?
(If not, we'll just have to remove the code that we used to use, I guess.)

Comment 13

[Philip Chee]

What does the toolkit typeaheadfind use to turn on the caret?

Comment 14

[[e-mail]]

It doesn't, since it moves the focus to the find bar.