672536 - Merge nsISelection* interfaces

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Combine nsISelection3 and nsISelection

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: Combine nsISelection2 and nsISelectionPrivate

Comment 3

[David Zbarsky (:dzbarsky)]

Attachment: Combine nsISelection3 and nsISelection

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: Combine nsISelection2 and nsISelectionPrivate

Gah, previous patch didn't compile.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

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)]

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)]

Attachment: Combine nsISelection2 and nsISelectionPrivate r=smaug

Comment 9

[David Zbarsky (:dzbarsky)]

Attachment: Combine nsISelection3 and nsISelection r=smaug

Comment 10

[Justin Wood (:Callek)]

(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)]

(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)]

Yeah, I should have specified the order to apply patches in.  Thanks!

Comment 13

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/bd2459fe814c
http://hg.mozilla.org/mozilla-central/rev/6a508e9802db