Remove nsISupportsArray usage from nsIDragService. r=smaug

RESOLVED FIXED in Firefox 52

Status

()

Core
Drag and Drop
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: erahm, Assigned: erahm)

Tracking

({addon-compat})

unspecified
mozilla52
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

|nsISupportsArray| is deprecated. We should be able to update nsIDragService to just use nsIArray instead.
Created attachment 8800403 [details] [diff] [review]
Remove usage of nsISupportsArray from nsIDragService

This switches over to using nsIArray instead of nsISupportsArray. Unfortunately it spread out to quite a few files, but they're all mostly cosmetic changes. Olli, it looks like you're the best reviewer for this, but feel free to redirect/add another reviewer.
Attachment #8800403 - Flags: review?(bugs)

Comment 2

10 months ago
Did you check if addons are using the changed .idl methods?
https://treeherder.mozilla.org/#/jobs?repo=try&revision=800a823b6843

Comment 4

10 months ago
This would break many addons, as far as I see.

Comment 5

10 months ago
https://dxr.mozilla.org/addons/search?q=invokeDragSession&redirect=true

Comment 6

10 months ago
Comment on attachment 8800403 [details] [diff] [review]
Remove usage of nsISupportsArray from nsIDragService

So I don't think we can do this easily.

Could we make nsSupportsArray to implement also nsIArray and then make the .idl methods to take nsISupports and QI to nsIArray in the method implementations?
Attachment #8800403 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #4)
> This would break many addons, as far as I see.

Good point, I neglected to search this one first. Lets see what the add-ons folks think. Of particular interest to me is how many of these add-ons support the latest release, it looks like a lot of instances of the same add-ons (adblock) and/or copy and pasted code.

Kris, what I'm looking at here is transitioning from |nsISupportsArray| to |nsIArray|. |nsISupportsArray| has been deprecated since 2002.
Flags: needinfo?(kmaglione+bmo)
Keywords: addon-compat
My calculations put it at 114 add-ons (supported or not) reference this method. A naive approach of downloading the 'install.rdf' and grepping for max version came out with a smaller list that indicate they support the latest release, 49, or all versions:

> ./486302-install.rdf - Mouse Gestures Suite - 50
> ./649888-install.rdf - Gmail Manager-community - 50
> ./654084-install.rdf - Outwit Docs - *
> ./672534-install.rdf - Gmail Manager-Community - 50
> ./681516-install.rdf - Gestão de Conteúdos - 50
> ./691264-install.rdf - PasswordMaker - *
> ./701973-install.rdf - DOM Inspector - 49 49
> ./701974-install.rdf - DOM Inspector - 49 49
> ./701976-install.rdf - DOM Inspector - 49 49 49
> ./712774-install.rdf - Planetromeo-Extension vom Basar der Sklaven-Gazette - * *
> ./718669-install.rdf - DOM Inspector - 50
> ./720315-install.rdf - AutoProxy - *
> ./721650-install.rdf - autoproxy Customization - *
> ./723849-install.rdf - AutoProxyTemp - 50
> ./723850-install.rdf - AutoProxyTemp - 50
> ./727671-install.rdf - AutoProxy - *
> ./731119-install.rdf - AutoProxy - 50
> ./736403-install.rdf - DOM Inspector - 51
> ./737398-install.rdf - Gestão de Conteúdos - 51

Now we can see many of those are duplicates in name at least, if we pare that down:

> ./486302-install.rdf - Mouse Gestures Suite - 50
> ./654084-install.rdf - Outwit Docs - *
> ./672534-install.rdf - Gmail Manager-Community - 50
> ./691264-install.rdf - PasswordMaker - *
> ./712774-install.rdf - Planetromeo-Extension vom Basar der Sklaven-Gazette - * *
> ./721650-install.rdf - autoproxy Customization - *
> ./723849-install.rdf - AutoProxyTemp - 50
> ./731119-install.rdf - AutoProxy - 50
> ./736403-install.rdf - DOM Inspector - 51
> ./737398-install.rdf - Gestão de Conteúdos - 51

And now lets see which ones actually show up in searches on addons.mozilla.org:

> "Mouse Gestures Suite": https://addons.mozilla.org/en-US/firefox/addon/mouse-gestures-suite/?src=search
> "Outwit Docs": https://addons.mozilla.org/en-US/firefox/addon/outwit-docs/?src=search
> "Gmail Manager-Community": https://addons.mozilla.org/en-US/firefox/addon/gmail-manager-community/?src=search
> "PasswordMaker": https://addons.mozilla.org/en-US/firefox/addon/passwordmaker/?src=search
> "AutoProxy": https://addons.mozilla.org/en-US/firefox/addon/autoproxy/?src=search
> "DOM Inspector": https://addons.mozilla.org/en-US/firefox/addon/dom-inspector-6622/?src=search

"Gestão de Conteúdos" doesn't show up for me, but seems like it should? Maybe it's a localization thing.

So by my naive estimation 6 installable from addons.mozilla.org add-ons would be affected.
We should be stronger about encouraging addon authors to use the standard dnd api instead. Using the invoke methods directly from js has been unofficially deprecated since 2008. Can you perhaps mark this as such in nsIDragService?
(In reply to Neil Deakin from comment #9)
> We should be stronger about encouraging addon authors to use the standard
> dnd api instead. Using the invoke methods directly from js has been
> unofficially deprecated since 2008. Can you perhaps mark this as such in
> nsIDragService?

Sure, is the whole service deprecated or just the invoke methods?
Flags: needinfo?(enndeakin)
Just the invoke methods. There isn't an equivalent to the remaining functions currently.
Flags: needinfo?(enndeakin)
Discussed with Kris, the plan is to make nsISupportsArray implement nsIArray for a transition period of one release cycle giving add-on devs time to update. In the meantime we can transition our interfaces to nsIArray.

Updated

10 months ago
Flags: needinfo?(kmaglione+bmo)
Depends on: 1310040
Created attachment 8801943 [details] [diff] [review]
Remove usage of nsISupportsArray from nsIDragService

This a rebased version of the original patch that also deprecates the
invokeDrag functions. Once bug 1310040 lands concerns about breaking addons
should be a non-issue.

Olli, do you mind taking another look? Of course I'll hold off on landing
until bug 1310040 lands.
Attachment #8801943 - Flags: review?(bugs)
Attachment #8800403 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=96b3d63f937b

Comment 15

10 months ago
Comment on attachment 8801943 [details] [diff] [review]
Remove usage of nsISupportsArray from nsIDragService

>@@ -43,18 +43,18 @@ interface nsIDragService : nsISupports
>     * @param  aTransferables - an array of transferables to be dragged
>     * @param  aRegion - a region containing rectangles for cursor feedback, 
>     *            in window coordinates.
>     * @param  aActionType - specified which of copy/move/link are allowed
>     * @param  aContentPolicyType - the contentPolicyType that will be
>     *           passed to the loadInfo when creating a new channel
>     *           (defaults to TYPE_OTHER)
>     */
>-  void invokeDragSession (in nsIDOMNode aDOMNode,
>-                          in nsISupportsArray aTransferables, 
>+  [deprecated] void invokeDragSession (in nsIDOMNode aDOMNode,
>+                          in nsIArray aTransferables, 
So does this still let JS to pass the same nsISupportsArray (which just happens to support also nsIArray) without any extra QI on JS side?
Please make sure to test it works. I think xpconnect does the QI implicitly, but better to test.
Attachment #8801943 - Flags: review?(bugs) → review+
Note: the invoke methods are only deprecated from JS callers.

Comment 17

10 months ago
yeah, indeed. Would be good to add some comment.

Comment 18

10 months ago
(In reply to Olli Pettay [:smaug] from comment #15)
> So does this still let JS to pass the same nsISupportsArray (which just
> happens to support also nsIArray) without any extra QI on JS side?
> Please make sure to test it works. I think xpconnect does the QI implicitly,
> but better to test.

It does implicitly QI, yes. There's a lot of existing code that depends on this.
(In reply to Kris Maglione [:kmag] from comment #18)
> (In reply to Olli Pettay [:smaug] from comment #15)
> > So does this still let JS to pass the same nsISupportsArray (which just
> > happens to support also nsIArray) without any extra QI on JS side?
> > Please make sure to test it works. I think xpconnect does the QI implicitly,
> > but better to test.
> 
> It does implicitly QI, yes. There's a lot of existing code that depends on
> this.

And there's also a test in bug 1310040 making sure that a nsSupportsArray instance can be QI'd to a nsIArray, so we're probably good.
(In reply to Neil Deakin from comment #16)
> Note: the invoke methods are only deprecated from JS callers.

Okay I'll switch to just adding a comment, explicitly marking as deprecated will cause warnings on the native side.
https://hg.mozilla.org/integration/mozilla-inbound/rev/32b590e82c8ec5251e77451f91f1e3f8b9ccb92f
Bug 1309698 - Remove usage of nsISupportsArray from nsIDragService. r=smaug

Comment 22

10 months ago
Pushed by erahm@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32b590e82c8e
Remove usage of nsISupportsArray from nsIDragService. r=smaug

Comment 23

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/32b590e82c8e
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.