Closed Bug 1309698 Opened 8 years ago Closed 8 years ago

Remove nsISupportsArray usage from nsIDragService. r=smaug

Categories

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

Details

(Keywords: addon-compat)

Attachments

(1 file, 1 obsolete file)

|nsISupportsArray| is deprecated. We should be able to update nsIDragService to just use nsIArray instead.
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)
Did you check if addons are using the changed .idl methods?
This would break many addons, as far as I see.
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.
Flags: needinfo?(kmaglione+bmo)
Depends on: 1310040
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
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.
yeah, indeed. Would be good to add some comment.
(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
Pushed by erahm@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32b590e82c8e
Remove usage of nsISupportsArray from nsIDragService. r=smaug
https://hg.mozilla.org/mozilla-central/rev/32b590e82c8e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.