deCOMtaminate nsICaret

RESOLVED FIXED in mozilla1.9.1a1

Status

()

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: roc, Assigned: johnsdaniels)

Tracking

Trunk
mozilla1.9.1a1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 2 obsolete attachments)

nsICaret should go away. We should use nsCaret.h directly, with non-virtual methods and deCOMtaminated signatures.
Whiteboard: [good first bug]
(Assignee)

Comment 1

10 years ago
I'm working on this bug now.
Assignee: nobody → johnsdaniels
(Assignee)

Comment 2

10 years ago
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.
(Assignee)

Comment 4

10 years ago
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.
Attachment #328194 - Flags: review?(roc)
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.
(Assignee)

Comment 6

10 years ago
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?
Attachment #328194 - Attachment is obsolete: true
Attachment #329394 - Flags: review?(roc)
Attachment #328194 - Flags: review?(roc)
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!!!
Attachment #329394 - Flags: superreview+
Attachment #329394 - Flags: review?(roc)
Attachment #329394 - Flags: review+
(Assignee)

Comment 8

10 years ago
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
Keywords: checkin-needed
(Assignee)

Comment 9

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) 
http://hg.mozilla.org/index.cgi/mozilla-central/rev/b9582ba12a6a
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a1

Updated

10 years ago
Blocks: 445625

Comment 12

10 years ago
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

10 years ago
What does the toolkit typeaheadfind use to turn on the caret?

Comment 14

10 years ago
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.