1114624 - Don't register the legacy nsIDownloadManager implementation of nsITransfer by default anymore

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

Attachment: The patch

Now that all the core applications have migrated to Downloads.jsm, we should actively remove uses of nsIDownloadManager. The first step is to register the JavaScript implementation of nsITransfer, avoiding to register the legacy nsIDownloadManager implementation and letting external applications do that if needed.

The override required in applications that still use nsIDownloadManager would be implemented at startup using a technique similar to the one removed by the patch.

The legacy CID and the contract ID are defined as NS_DOWNLOAD_CID and NS_TRANSFER_CONTRACTID, and they are "{b02be33b-d47c-4bd3-afd9-402a942426b0}" and "@mozilla.org/transfer;1" respectively.

Comment 1

[:Paolo Amadini]

Review requested for Firefox, Firefox OS, Firefox for Android, and the Webapp Runtime.

Firefox for Windows 8 Metro is already unsupported, and will need to be updated to Downloads.jsm for downloads to work. This patch does not deal with Metro code.

We should not wait to review and test this, but we should wait some time before landing, since people might want to land corresponding patches in Thunderbird and SeaMonkey to implement the override. If they're not packaging the "jsdownloads" folder, they might be able to just add a line in a manifest file.

Comment 2

[[:fabrice] Fabrice Desré]

Comment on attachment 8540183 [details] [diff] [review]
The patch

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

Fails on b2g when downloading:

Timestamp: 12/22/2014 10:25:52 AM
Error: [Exception... "Method not implemented'Method not implemented' when calling method: [nsIFilePicker::init]"  nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame :: file:///home/fabrice/dev/builds/obj-b2g-desktop-b2g-inbound/dist/bin/components/nsHelperAppDlg.js :: nsUnknownContentTypeDialog.prototype.promptForSaveToFileAsync/< :: line 259"  data: no]
Source File: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js
Line: 873

Comment 3

[:Paolo Amadini]

(In reply to Fabrice Desré [:fabrice] from comment #2)
> Fails on b2g when downloading:
> 
> Timestamp: 12/22/2014 10:25:52 AM
> Error: [Exception... "Method not implemented'Method not implemented' when
> calling method: [nsIFilePicker::init]"  nsresult: "0x80004001
> (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame ::
> file:///home/fabrice/dev/builds/obj-b2g-desktop-b2g-inbound/dist/bin/
> components/nsHelperAppDlg.js ::
> nsUnknownContentTypeDialog.prototype.promptForSaveToFileAsync/< :: line 259"
> data: no]
> Source File: resource://gre/modules/Promise.jsm ->
> resource://gre/modules/Promise-backend.js
> Line: 873

That's curious, it looks like we're calling the Desktop implementation of nsIHelperAppLauncherDialog.

The registration of this interface should not be affected by this patch, maybe there is something involved indirectly. Fabrice, do you have any idea of what might be going on?

Comment 4

[:Margaret Leibovic]

Sorry for the slow review. I tried applying this patch to test on Android, but it failed to apply.

Comment 5

[:Paolo Amadini]

Don't worry. You may need to apply the patch from bug 1114614, or ignore the Desktop conflicts.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8540183 [details] [diff] [review]
The patch

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

/mobile changes look fine.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8540183 [details] [diff] [review]
The patch

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

please ensure to get a green Try run out of this.

Comment 8

[:Paolo Amadini]

Comment on attachment 8540183 [details] [diff] [review]
The patch

Fabrice, can you please confirm that the error in comment 2 is not an artifact of a partial build? Not sure if Firefox OS can do clobber builds, but would be worth trying.

Unfortunately I don't think this code path is covered by regression tests.

Comment 9

[:Paolo Amadini]

Full try build to check for build issues:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=014c97bafa11

Comment 10

[:Paolo Amadini]

Hi Marco, do you have time to look at this for the Webapp Runtime?

We should try to land when we have all the reviews.

Comment 11

[:Margaret Leibovic]

(In reply to :Paolo Amadini from comment #10)
> Hi Marco, do you have time to look at this for the Webapp Runtime?
> 
> We should try to land when we have all the reviews.

Myk is also qualified to review this if Marco isn't around.

Comment 12

[:Paolo Amadini]

Comment on attachment 8540183 [details] [diff] [review]
The patch

Thanks! Adding as a possible reviewer.

Comment 13

[Marco Castelluccio [:marco]]

Comment on attachment 8540183 [details] [diff] [review]
The patch

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

I wanted to fix the webapprt download tests before this. Looks good to me, but
could you run "./mach webapprt-test-chrome" to make sure there's no regressions
in the downloads tests?

Comment 14

[:Paolo Amadini]

(In reply to Marco Castelluccio [:marco] from comment #13)
> I wanted to fix the webapprt download tests before this. Looks good to me,
> but could you run "./mach webapprt-test-chrome" to make sure there's no
> regressions in the downloads tests?

Running this command on Mac OS X without the patch after a clobber build results in the error "Unable to parse environment files for application startup". It seems it's trying to use the installed Firefox instead of Nightly. I see there were bugs related to the new bundle structure, but these are expected to be solved by now, so this may be a different issue. How can I work around it?

Comment 15

[Myk Melez [:myk] [@mykmelez]]

(In reply to :Paolo Amadini from comment #14)
> Running this command on Mac OS X without the patch after a clobber build
> results in the error "Unable to parse environment files for application
> startup". It seems it's trying to use the installed Firefox instead of
> Nightly.

Yes, we have an issue with the way we look for the Firefox installation that can cause the tests to use a different Firefox installation than the one you built.  And that could trigger the message you saw, which is bug 1087362.  For details, see bug 914754.


> I see there were bugs related to the new bundle structure, but
> these are expected to be solved by now, so this may be a different issue.
> How can I work around it?

You can temporarily move your other Firefox installations to the Trash.  I typically select them in Finder, press Cmd+Backspace to move them to the Trash, run the tests, and then press Cmd+Z to undo the move.

Comment 16

[:Paolo Amadini]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #15)
> You can temporarily move your other Firefox installations to the Trash.  I
> typically select them in Finder, press Cmd+Backspace to move them to the
> Trash, run the tests, and then press Cmd+Z to undo the move.

This doesn't work for me, simply no runtime is found. Do you think you can test the patch on your system while waiting for bug 914754 to be fixed?

With regard to bug 1087362, which I found previously but seemed to describe a different situation, let me know there if I can help by providing more information, but I wouldn't block landing this bug on fixing the webapprt tests.

Comment 17

[Marco Castelluccio [:marco]]

I can test the patch on my system, but there are some conflicts when I try to apply it.

Comment 18

[:Paolo Amadini]

Attachment: On latest trunk

This should apply cleanly.

Comment 19

[Marco Castelluccio [:marco]]

Tests are passing!

Comment 20

[:Paolo Amadini]

(In reply to Marco Castelluccio [:marco] from comment #19)
> Tests are passing!

Thanks for checking!

Comment 21

[:Paolo Amadini]

Comment on attachment 8549689 [details] [diff] [review]
On latest trunk

Hi aus, Fabrice found an error when testing this patch in comment 2 but I'm not sure whether that can be due to an issue with his local build. Do you have some spare cycles to check whether this is a real issue on your system?

Comment 22

[Aus Lacroix [:aus]]

:Paolo, I'll try and give it spin over the weekend.

Comment 23

[:Paolo Amadini]

(In reply to Ghislain Aus Lacroix [:aus] from comment #22)
> :Paolo, I'll try and give it spin over the weekend.

Thank you!

Comment 24

[:Paolo Amadini]

Comment on attachment 8540183 [details] [diff] [review]
The patch

Upon suggestion from Francisco I tried this manually on a Simulator build on Windows and I don't see any Console error during downloads.

When the download finishes, the Simulator says that the file cannot be found on the device, but this is not a regression from this patch as the same issue happens on a Nightly build.

Comment 25

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8540183 [details] [diff] [review]
The patch

Hei Fabrice,

I think you are better than me to review this, looks simple change though ;)

Comment 26

[[:fabrice] Fabrice Desré]

Comment on attachment 8540183 [details] [diff] [review]
The patch

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

Tested on device, works fine.

Comment 27

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/afbba6c11b19

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/afbba6c11b19