nsICaret should go away. We should use nsCaret.h directly, with non-virtual methods and deCOMtaminated signatures.
11 years ago
Whiteboard: [good first bug]
I'm working on this bug now.
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.
Status: NEW → ASSIGNED
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.
Created attachment 328194 [details] [diff] [review] 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.
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.
Created attachment 329394 [details] [diff] [review] 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 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!!!
Created attachment 329608 [details] [diff] [review] last few changes requested by roc That should be everything, and you're welcome.
Attachment #329394 - Attachment is obsolete: true
10 years ago
Thanks for setting checkin-needed. Sorry about that.
BTW: The John is John Keiser (jkeiser) which is long gone (added in bug 34297)
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a1
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.)
What does the toolkit typeaheadfind use to turn on the caret?
It doesn't, since it moves the focus to the find bar.
You need to log in before you can comment on or make changes to this bug.