Closed Bug 1114624 Opened 8 years ago Closed 7 years ago

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

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch The patchSplinter Review
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.
Attachment #8540183 - Flags: review?(margaret.leibovic)
Attachment #8540183 - Flags: review?(mak77)
Attachment #8540183 - Flags: review?(fabrice)
Attachment #8540183 - Flags: review?(mar.castelluccio)
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 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
Attachment #8540183 - Flags: review?(fabrice) → review-
(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?
Flags: needinfo?(fabrice)
Sorry for the slow review. I tried applying this patch to test on Android, but it failed to apply.
Don't worry. You may need to apply the patch from bug 1114614, or ignore the Desktop conflicts.
Comment on attachment 8540183 [details] [diff] [review]
The patch

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

/mobile changes look fine.
Attachment #8540183 - Flags: review?(margaret.leibovic) → review+
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.
Attachment #8540183 - Flags: review?(mak77) → review+
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.
Attachment #8540183 - Flags: review- → review?(fabrice)
Full try build to check for build issues:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=014c97bafa11
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.
Flags: needinfo?(mar.castelluccio)
(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.
Flags: needinfo?(myk)
Comment on attachment 8540183 [details] [diff] [review]
The patch

Thanks! Adding as a possible reviewer.
Attachment #8540183 - Flags: review?(myk)
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?
Attachment #8540183 - Flags: review?(myk)
Attachment #8540183 - Flags: review?(mar.castelluccio)
Attachment #8540183 - Flags: review+
Flags: needinfo?(mar.castelluccio)
(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?
(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.
Flags: needinfo?(myk)
(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.
I can test the patch on my system, but there are some conflicts when I try to apply it.
Attached patch On latest trunk (obsolete) — Splinter Review
This should apply cleanly.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Tests are passing!
(In reply to Marco Castelluccio [:marco] from comment #19)
> Tests are passing!

Thanks for checking!
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?
Attachment #8549689 - Flags: review?(aus)
:Paolo, I'll try and give it spin over the weekend.
(In reply to Ghislain Aus Lacroix [:aus] from comment #22)
> :Paolo, I'll try and give it spin over the weekend.

Thank you!
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.
Flags: needinfo?(fabrice)
Attachment #8540183 - Flags: review?(fabrice) → review?(francisco)
Attachment #8549689 - Flags: review?(aus)
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 ;)
Attachment #8540183 - Flags: review?(francisco) → review?(fabrice)
Comment on attachment 8540183 [details] [diff] [review]
The patch

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

Tested on device, works fine.
Attachment #8540183 - Flags: review?(fabrice) → review+
Attachment #8549689 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/afbba6c11b19
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1130488
You need to log in before you can comment on or make changes to this bug.