Last Comment Bug 672536 - Merge nsISelection* interfaces
: Merge nsISelection* interfaces
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Selection (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: David Zbarsky (:dzbarsky)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-19 09:41 PDT by David Zbarsky (:dzbarsky)
Modified: 2011-08-12 21:04 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (167.04 KB, text/plain)
2011-07-19 09:41 PDT, David Zbarsky (:dzbarsky)
no flags Details
Combine nsISelection3 and nsISelection (9.86 KB, patch)
2011-08-03 14:48 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Combine nsISelection2 and nsISelectionPrivate (38.32 KB, patch)
2011-08-03 15:13 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Combine nsISelection3 and nsISelection (9.86 KB, patch)
2011-08-03 15:14 PDT, David Zbarsky (:dzbarsky)
bugs: review+
Details | Diff | Splinter Review
Combine nsISelection2 and nsISelectionPrivate (38.35 KB, patch)
2011-08-04 08:52 PDT, David Zbarsky (:dzbarsky)
bugs: review+
Details | Diff | Splinter Review
Combine nsISelection2 and nsISelectionPrivate r=smaug (38.17 KB, patch)
2011-08-07 16:17 PDT, David Zbarsky (:dzbarsky)
bugspam.Callek: checkin+
Details | Diff | Splinter Review
Combine nsISelection3 and nsISelection r=smaug (9.87 KB, patch)
2011-08-07 16:18 PDT, David Zbarsky (:dzbarsky)
bugspam.Callek: checkin+
Details | Diff | Splinter Review

Description David Zbarsky (:dzbarsky) 2011-07-19 09:41:46 PDT
Created attachment 546809 [details]
Patch
Comment 1 David Zbarsky (:dzbarsky) 2011-08-03 14:48:29 PDT
Created attachment 550521 [details] [diff] [review]
Combine nsISelection3 and nsISelection
Comment 2 David Zbarsky (:dzbarsky) 2011-08-03 15:13:25 PDT
Created attachment 550528 [details] [diff] [review]
Combine nsISelection2 and nsISelectionPrivate
Comment 3 David Zbarsky (:dzbarsky) 2011-08-03 15:14:03 PDT
Created attachment 550529 [details] [diff] [review]
Combine nsISelection3 and nsISelection
Comment 4 David Zbarsky (:dzbarsky) 2011-08-04 08:52:13 PDT
Created attachment 550705 [details] [diff] [review]
Combine nsISelection2 and nsISelectionPrivate

Gah, previous patch didn't compile.
Comment 5 Olli Pettay [:smaug] 2011-08-05 07:07:46 PDT
Comment on attachment 550529 [details] [diff] [review]
Combine nsISelection3 and nsISelection

Please ask someone to check if addons use nsISelection3 often,
since if they do, we may need to keep a dummy
nsISelection3 : nsISelection {}; interface and perhaps warn about using it
before removing the interface.
Comment 6 Olli Pettay [:smaug] 2011-08-05 07:15:33 PDT
Comment on attachment 550705 [details] [diff] [review]
Combine nsISelection2 and nsISelectionPrivate


>-  nsCOMPtr<nsISelection2> sel2(do_QueryInterface(aSelection));
>+  nsCOMPtr<nsISelectionPrivate> privSel(do_QueryInterface(aSelection));
>+  NS_ASSERTION(privSel, "Selection should implement nsISelectionPrivate");
Useless assertion. We crash anyway right after this.


> nsCaretAccessible::ProcessSelectionChanged(nsISelection* aSelection)
> {
>-  nsCOMPtr<nsISelection2> sel2(do_QueryInterface(aSelection));
>+  nsCOMPtr<nsISelectionPrivate> privSel(do_QueryInterface(aSelection));
>+  NS_ASSERTION(privSel, "Selection should implement nsISelectionPrivate");
ditto, and also elsewhere


>-[scriptable, uuid(98552206-ad7a-4d2d-8ce3-b6fa2389298b)]
>+[scriptable, uuid(1820a940-6203-4e27-bc94-fa81131722a4)]
> interface nsISelectionPrivate : nsISupports
Could nsISelectionPrivate extend nsISelection


Please ask someone, maybe Mossop?, to check if nsISelection2 is used in addons often.
We may need to add a dummy interface for it temporarily.
Comment 7 David Zbarsky (:dzbarsky) 2011-08-05 10:57:02 PDT
https://addons.mozilla.org/en-US/firefox/addon/lucifox/ has one use of nsISelection2, there are no uses of nsISelection3
Comment 8 David Zbarsky (:dzbarsky) 2011-08-07 16:17:50 PDT
Created attachment 551359 [details] [diff] [review]
Combine nsISelection2 and nsISelectionPrivate r=smaug
Comment 9 David Zbarsky (:dzbarsky) 2011-08-07 16:18:20 PDT
Created attachment 551360 [details] [diff] [review]
Combine nsISelection3 and nsISelection r=smaug
Comment 10 Justin Wood (:Callek) 2011-08-09 03:15:51 PDT
(In reply to David Zbarsky from comment #8)
> Created attachment 551359 [details] [diff] [review] [diff] [details] [review]
> Combine nsISelection2 and nsISelectionPrivate r=smaug

At least this patch failed to apply, (on inbound) can you please refresh and attach a new patch?
Comment 11 Justin Wood (:Callek) 2011-08-09 03:40:00 PDT
(In reply to Justin Wood (:Callek) from comment #10)
> (In reply to David Zbarsky from comment #8)
> > Created attachment 551359 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > Combine nsISelection2 and nsISelectionPrivate r=smaug
> 
> At least this patch failed to apply, (on inbound) can you please refresh and
> attach a new patch?

Actually didn't catch that the order of patches was important here (at least opposit order than they are attached)

Also STILL had a reject in nsISelection2.idl, which apparantly was removed from the build anyway (above) so I took a gamble and dropped it from what I am checking in. (if that is wrong, I'll backout)

http://hg.mozilla.org/integration/mozilla-inbound/rev/bd2459fe814c
http://hg.mozilla.org/integration/mozilla-inbound/rev/6a508e9802db
Comment 12 David Zbarsky (:dzbarsky) 2011-08-09 11:39:51 PDT
Yeah, I should have specified the order to apply patches in.  Thanks!

Note You need to log in before you can comment on or make changes to this bug.