1305663 - Support downloadItem.byExtensionId

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P3)

Description

[Mostafa Elsaie]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.79 Safari/537.36 Edge/14.14393

Steps to reproduce:

1) Initiate a download from an extension using the downloads.download().
2) Try to analyse the dowloadItem in the downloads.onCreated event.
3) Look for the byExtensionId & byExtensionName attributes of the downloadItem.


Actual results:

Both attributes are always "undefined"


Expected results:

Both attributes should contain the extensions ID & Name respectively.

Comment 1

[Rob Wu [:robwu]]

Attachment: downloads-byExtensionId.zip

Confirmed in Firefox 55.0b5.

1. Install the attached add-on (via about:debugging for example).
2. Click on the extension button in the toolbar that appears after installing the extension.
3. Open the global browser console and look at the printed messages.

Result:
onCreated at 7/9/2017, 10:42:31 PM  background.js:2:5
id = 8  background.js:4:9
... (cut) ...
byExtensionId = undefined  background.js:4:9
byExtensionName = undefined  background.js:4:9
downloads.download callback with ID: 8  background.js:12:9

Note hat byExtensionId and byExtensionName are both undefined, while they should have a sensible value.

The only place where the extension is set for a DownloadItem is for the callback of downloads.download [1]. However, the downloads.onCreated event fires before that [2] and causes a DownloadItem to be created without extension. The result is cached too, so when the download.download callback is invoked, the DownloadItem is not cached (nor updated with the extension).


Even if the above bug were to be fixed, the whole implementation would only work for downloads created in the current session; if the browser is restarted, the byExtensionId/byExtensionName fields would be cleared. Though if the browser is restarted, the downloads list would be empty anyway, because of bug 1255507.

And, if an extension is uninstalled, the byExtensionId and byExtensionName fields are cleared too (contrary to Chrome, which shows the ID and the last known name if the extension has been uninstalled).

[1] https://searchfox.org/mozilla-central/rev/4aaa08513911c3cfe750611a6c087b438bb1f88e/toolkit/components/extensions/ext-downloads.js#551
[2] https://searchfox.org/mozilla-central/rev/4aaa08513911c3cfe750611a6c087b438bb1f88e/toolkit/components/extensions/ext-downloads.js#163-165

Comment 2

[Rob Wu [:robwu]]

(updating bug flags based on previous comment)

Comment 3

[Rob Wu [:robwu]]

This is an interoperability issue too, so blocking bug 1213445.

Comment 4

[Antoine Turmel [:GeekShadow]]

I'm working on an WebExt and I'm getting both undefined for byExtensionId and byExtensionName.
Is it normal that it's marked as supported on MDN Browser compatibility table ?
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/downloads/DownloadItem#Browser_compatibility

Comment 5

[Martin Matous]

Attachment: add test for download's extension properties

Comment 6

[Martin Matous]

Attachment: save extension info to DownloadMap properly

Comment 7

[Rob Wu [:robwu]]

Hi Martin, thanks for your patch! It looks very clean.

Could you upload it to our Phabricator (documentation here), so that it can be reviewed and landed?

If you are not able to (e.g. because you cannot add a 2-factor method to your Bugzilla account, which is required to log in to Phabricator), then the review can also be done here. This is a bit inconvenient, but better than not taking the patches at all.

Comment 8

[Martin Matous]

Attachment: Bug 1305663 - add test for download's extension properties

Comment 9

[Martin Matous]

Attachment: Bug 1305663 - save extension info to DownloadMap properly.

Previous implementation created new DownloadItem with a null as an indirect result of list.add()

Depends on D30591

Comment 10

[Martin Matous]

(In reply to Rob Wu [:robwu] from comment #7)

Hi Martin, thanks for your patch! It looks very clean.

Could you upload it to our Phabricator (documentation here), so that it can be reviewed and landed?

If you are not able to (e.g. because you cannot add a 2-factor method to your Bugzilla account, which is required to log in to Phabricator), then the review can also be done here. This is a bit inconvenient, but better than not taking the patches at all.

Done. I did not feel like reading yet more documentation and configuring yet more tools that day, but it turned to be pretty easy.

Comment 11

[Martin Matous]

Attachment: Bug 1305663 - save extension info to DownloadMap properly, r=aswan

Previous implementation created new DownloadItem with a null as an indirect result of list.add()

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9850cc6f4b7a
save extension info to DownloadMap properly, r=aswan

Comment 13

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/9850cc6f4b7a

Comment 14

[Caitlin Neiman [:caitmuenster]]

Thanks so much for the patch, Martin! 🎉 Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you!

Comment 15

[Martin Matous]

Thanks. I created an account. But... reCAPTCHA? Really? I expected better.

Comment 16

[Madalin Cotetiu]

Verified as fixed using the steps from comment 1 using FF69.0b3 and FF70.0a1(20190708182549) in Win7x64 and Ubuntu 14.04