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)
Core
DOM: Copy & Paste and Drag & Drop
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: erahm, Assigned: erahm)
References
Details
(Keywords: addon-compat)
Attachments
(1 file, 1 obsolete file)
49.60 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
|nsISupportsArray| is deprecated. We should be able to update nsIDragService to just use nsIArray instead.
Assignee | ||
Comment 1•8 years ago
|
||
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•8 years ago
|
||
Did you check if addons are using the changed .idl methods?
Assignee | ||
Comment 3•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=800a823b6843
Comment 4•8 years ago
|
||
This would break many addons, as far as I see.
Comment 6•8 years 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)
Assignee | ||
Comment 7•8 years ago
|
||
(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
Assignee | ||
Comment 8•8 years ago
|
||
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.
Comment 9•8 years ago
|
||
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?
Assignee | ||
Comment 10•8 years ago
|
||
(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)
Comment 11•8 years ago
|
||
Just the invoke methods. There isn't an equivalent to the remaining functions currently.
Flags: needinfo?(enndeakin)
Assignee | ||
Comment 12•8 years ago
|
||
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•8 years ago
|
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 13•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8800403 -
Attachment is obsolete: true
Assignee | ||
Comment 14•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=96b3d63f937b
Comment 15•8 years 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+
Comment 16•8 years ago
|
||
Note: the invoke methods are only deprecated from JS callers.
Comment 17•8 years ago
|
||
yeah, indeed. Would be good to add some comment.
Comment 18•8 years 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.
Assignee | ||
Comment 19•8 years ago
|
||
(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.
Assignee | ||
Comment 20•8 years ago
|
||
(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.
Assignee | ||
Comment 21•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/32b590e82c8ec5251e77451f91f1e3f8b9ccb92f Bug 1309698 - Remove usage of nsISupportsArray from nsIDragService. r=smaug
Comment 22•8 years 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/32b590e82c8e
Status: ASSIGNED → RESOLVED
Closed: 8 years 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.
Description
•