Closed Bug 1312901 Opened 8 years ago Closed 8 years ago

Use nsIArray for URL window argument

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

Details

(Keywords: addon-compat)

Attachments

(2 files)

nsISupportsArray is deprecated. We can just swap in nsIArray for nsISupports array in browser.js and optionally update callers to use nsIArray.
nsISupportsArrays now implement the nsIArray interface so this can be swapped out.

Mike, I think you're the right person to review this (Gijs is out), feel free to redirect of course.

MozReview-Commit-ID: 8DAa7pYdt1b
Attachment #8804511 - Flags: review?(mconley)
This updates the URL argument to use an nsIMutableArray instead of an nsIArray.

This is fine for the core of Firefox as we updated browser.js to expect an
nsIArray but has potential to affect addons. I'll add a follow-up analysis of
that and request feedback from the addons team.

Jared, it looks like you're the right person to review this, but feel free to
redirect.

MozReview-Commit-ID: 45kvKCBYNK6
Attachment #8804514 - Flags: review?(jaws)
8 addons appear to use the URL params (searching by "instanceof nsISupportsArray" and auditing the usage):

> 1122-install.rdf - Tab Mix Plus
> 399862-install.rdf - Search Fun and Easy Sidebar
> 399938-install.rdf - Netscape Communicator 4.x Account Settings Migrator
> 565634-install.rdf - BackTrack Tab History
> 647194-install.rdf - Addons Manager
> 681099-install.rdf - Tab Mix Plus
> 722520-install.rdf - Tab Mix Plus
> 724821-install.rdf - Tab Mix Plus

Lest filter out the dupes:
> 1122-install.rdf - Tab Mix Plus
> 399862-install.rdf - Search Fun and Easy Sidebar
> 399938-install.rdf - Netscape Communicator 4.x Account Settings Migrator
> 565634-install.rdf - BackTrack Tab History
> 647194-install.rdf - Addons Manager

And then lets see if I can find them on addons.mozilla.org:
> Tab Mix Plus
> BackTrack Tab History

To make these (backward)compatible they'd have to add support for nsIArray, something like the following for Tab Mix:

>    else if (uriToLoad instanceof Ci.nsIArray) {
>      let count = uriToLoad.length;
>      for (let i = 0; i < count; i++) {
>        let uriString = uriToLoad.queryElementAt(i, Ci.nsISupportsString);
>        urls.push(uriString.data);
>      }
>    }

and for BackTrack:

> - if (customDataReferrerUri instanceof Ci.nsISupportsArray) {
> + if (customDataReferrerUri instanceof Ci.nsISupportsArray ||
> +      customDataReferrerUri instanceof Ci.nsIArray) {

Jorge, what do you think of my assessment? Does it seem reasonable to push this change and just do outreach to the addon devs?
Flags: needinfo?(jorge)
Keywords: addon-compat
Comment on attachment 8804511 [details] [diff] [review]
Part 1: Update browser.js URL argument check to use nsIArray

Review of attachment 8804511 [details] [diff] [review]:
-----------------------------------------------------------------

AIUI this has no add-on impact because it only alters consumers, which will still work even if we keep sending nsISupportsArray, no?
Attachment #8804511 - Flags: review?(mconley) → review+
(In reply to Eric Rahm [:erahm] from comment #3)
> And then lets see if I can find them on addons.mozilla.org:
> > Tab Mix Plus
> > BackTrack Tab History
> 
> To make these (backward)compatible they'd have to add support for nsIArray,
> something like the following for Tab Mix:
> 
> >    else if (uriToLoad instanceof Ci.nsIArray) {
> >      let count = uriToLoad.length;
> >      for (let i = 0; i < count; i++) {
> >        let uriString = uriToLoad.queryElementAt(i, Ci.nsISupportsString);
> >        urls.push(uriString.data);
> >      }
> >    }
> 
> and for BackTrack:
> 
> > - if (customDataReferrerUri instanceof Ci.nsISupportsArray) {
> > + if (customDataReferrerUri instanceof Ci.nsISupportsArray ||
> > +      customDataReferrerUri instanceof Ci.nsIArray) {
> 
> Jorge, what do you think of my assessment? Does it seem reasonable to push
> this change and just do outreach to the addon devs?

Both of these devs are on bugzilla, so I'm ni'ing them.
Flags: needinfo?(tabmix.onemen)
Flags: needinfo?(bugzilla)
Comment on attachment 8804514 [details] [diff] [review]
Part 2: Use nsIArray for URL argument when opening a window

Review of attachment 8804514 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: browser/components/extensions/ext-windows.js
@@ +124,2 @@
>              for (let url of createData.url) {
> +              array.appendElement(mkstr(url), /* weak = */ false);

hrmpf. Can you file a bug to make the 'weak' argument optional and default to false? From a JS perspective it feels dumb to have to specify this.
Attachment #8804514 - Flags: review?(jaws) → review+
Thank you for the info, I will update Tab mix code according to comment 3
Flags: needinfo?(tabmix.onemen)
Having reviewed the code, I do not believe this change will impact BackTrack Tab History.

(As you have drawn my attention to nsISupportsArray being deprecated, though, I will change to using nsIMutableArray instead for the next version anyway - thanks)
Flags: needinfo?(bugzilla)
It looks like a reasonable change, yes. Echoing what Gijs said, I didn't see any changes that would break add-ons right away. Is it that we want to warn developers in advance that this usage is deprecated?
Flags: needinfo?(jorge)
(In reply to :Gijs Kruitbosch from comment #4)
> Comment on attachment 8804511 [details] [diff] [review]
> Part 1: Update browser.js URL argument check to use nsIArray
> 
> Review of attachment 8804511 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> AIUI this has no add-on impact because it only alters consumers, which will
> still work even if we keep sending nsISupportsArray, no?

Correct.

(In reply to :Gijs Kruitbosch from comment #6)
> Comment on attachment 8804514 [details] [diff] [review]
> Part 2: Use nsIArray for URL argument when opening a window
> 
> Review of attachment 8804514 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me
> 
> ::: browser/components/extensions/ext-windows.js
> @@ +124,2 @@
> >              for (let url of createData.url) {
> > +              array.appendElement(mkstr(url), /* weak = */ false);
> 
> hrmpf. Can you file a bug to make the 'weak' argument optional and default
> to false? From a JS perspective it feels dumb to have to specify this.

Agreed, it's been pretty annoying. Filed bug 1313150 for this.
(In reply to Jorge Villalobos [:jorgev] from comment #9)
> It looks like a reasonable change, yes. Echoing what Gijs said, I didn't see
> any changes that would break add-ons right away. Is it that we want to warn
> developers in advance that this usage is deprecated?

I was mainly concerned about whether or not my analysis of the add-ons that use the |window.arguments[0]| param was correct and that it would be okay to break them. But the add-on devs have chimed in so I think we're good!
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f6c4c5eb7035be37363cd651aefebd2b8d1c957
Bug 1312901 - Part 1: Update browser.js URL argument check to use nsIArray. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0f100627ed89dfaede75f4ae63be078f35d8e0c
Bug 1312901 - Part 2: Use nsIArray for URL argument when opening a window. r=Gijs
(In reply to Jorge Villalobos [:jorgev] from comment #9)
> It looks like a reasonable change, yes. Echoing what Gijs said, I didn't see
> any changes that would break add-ons right away. Is it that we want to warn
> developers in advance that this usage is deprecated?

At this point, we should warn them that any use of nsISupportsArray/"@mozilla.org/supports-array;1" is deprecated.
https://hg.mozilla.org/mozilla-central/rev/8f6c4c5eb703
https://hg.mozilla.org/mozilla-central/rev/d0f100627ed8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.