Merge nsISelection* interfaces

RESOLVED FIXED in mozilla8

Status

()

Core
Selection
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

({dev-doc-complete})

Trunk
mozilla8
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 546809 [details]
Patch
(Assignee)

Updated

6 years ago
Summary: Combine nsISelection, nsISelection2, nsISelection3, nsIPrivateSelection → Merge nsISelection* interfaces
(Assignee)

Comment 1

6 years ago
Created attachment 550521 [details] [diff] [review]
Combine nsISelection3 and nsISelection
Assignee: nobody → dzbarsky
Attachment #546809 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #550521 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 2

6 years ago
Created attachment 550528 [details] [diff] [review]
Combine nsISelection2 and nsISelectionPrivate
Attachment #550521 - Attachment is obsolete: true
Attachment #550528 - Flags: review?(Olli.Pettay)
Attachment #550521 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 3

6 years ago
Created attachment 550529 [details] [diff] [review]
Combine nsISelection3 and nsISelection
Attachment #550529 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 4

6 years ago
Created attachment 550705 [details] [diff] [review]
Combine nsISelection2 and nsISelectionPrivate

Gah, previous patch didn't compile.
Attachment #550528 - Attachment is obsolete: true
Attachment #550705 - Flags: review?(Olli.Pettay)
Attachment #550528 - Flags: review?(Olli.Pettay)

Comment 5

6 years ago
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.
Attachment #550529 - Flags: review?(Olli.Pettay) → review+

Comment 6

6 years ago
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.
Attachment #550705 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 7

6 years ago
https://addons.mozilla.org/en-US/firefox/addon/lucifox/ has one use of nsISelection2, there are no uses of nsISelection3
(Assignee)

Comment 8

6 years ago
Created attachment 551359 [details] [diff] [review]
Combine nsISelection2 and nsISelectionPrivate r=smaug
Attachment #550705 - Attachment is obsolete: true
(Assignee)

Comment 9

6 years ago
Created attachment 551360 [details] [diff] [review]
Combine nsISelection3 and nsISelection r=smaug
Attachment #550529 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
Attachment #551359 - Flags: checkin?
(Assignee)

Updated

6 years ago
Attachment #551360 - Flags: checkin?
(Assignee)

Updated

6 years ago
Keywords: dev-doc-needed
(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?
Keywords: checkin-needed
(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
Whiteboard: [inbound]
Attachment #551359 - Flags: checkin? → checkin+
Attachment #551360 - Flags: checkin? → checkin+
(Assignee)

Comment 12

6 years ago
Yeah, I should have specified the order to apply patches in.  Thanks!
http://hg.mozilla.org/mozilla-central/rev/bd2459fe814c
http://hg.mozilla.org/mozilla-central/rev/6a508e9802db
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Component: General → Selection
QA Contact: general → selection
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8

Updated

6 years ago
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.