Implement chrome.downloads.getFileIcon()

RESOLVED FIXED in Firefox 48

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: aswan, Assigned: mstriemer)

Tracking

(Blocks: 1 bug)

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [downloads][good first bug])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
Part of supporting chrome.downloads for WebExtensions

https://developer.chrome.com/extensions/downloads#method-getFileIcon
(Reporter)

Updated

2 years ago
Blocks: 1213445

Updated

2 years ago
Whiteboard: [downloads]
(Reporter)

Updated

2 years ago
Whiteboard: [downloads] → [downloads][good first bug]
(Assignee)

Updated

2 years ago
Assignee: nobody → mstriemer
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
Created attachment 8731196 [details]
MozReview Request: bug 1245606 - Implement chrome.downloads.getFileInfo r=kmag

Review commit: https://reviewboard.mozilla.org/r/40421/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40421/
Attachment #8731196 - Flags: review?(kmaglione+bmo)
Comment on attachment 8731196 [details]
MozReview Request: bug 1245606 - Implement chrome.downloads.getFileInfo r=kmag

https://reviewboard.mozilla.org/r/40421/#review36925

::: toolkit/components/extensions/ext-downloads.js:526
(Diff revision 1)
> +        return DownloadMap.lazyInit().then(() => {
> +          let size = options && options.size ? options.size : 32;
> +          let download = DownloadMap.fromId(downloadId).download;
> +
> +          if (!download.target.path) {
> +            // Old history downloads may not have a target path.

Can we fall back to the source URL (with query parameters stripped)?

::: toolkit/components/extensions/ext-downloads.js:534
(Diff revision 1)
> +          // When a download that was previously in progress finishes successfully, it
> +          // means that the target file now exists and we can extract its specific
> +          // icon, for example from a Windows executable. To ensure that the icon is
> +          // reloaded, however, we must change the URI used by the XUL image element,
> +          // for example by adding a query parameter. This only works if we add one of
> +          // the parameters explicitly supported by the nsIMozIconURI interface.

This comment doesn't seem relevant here.

::: toolkit/components/extensions/ext-downloads.js:536
(Diff revision 1)
> +          // icon, for example from a Windows executable. To ensure that the icon is
> +          // reloaded, however, we must change the URI used by the XUL image element,
> +          // for example by adding a query parameter. This only works if we add one of
> +          // the parameters explicitly supported by the nsIMozIconURI interface.
> +          return Promise.resolve(
> +            `moz-icon://${download.target.path}?size=${size}` +

I don't think this is right. It should work, since the service will just fall back to just using the file's extension to determine the icon, but it should only include the file's leaf name. It definitely should not include backslashes on Windows, and it shouldn't matter if the file exists.

::: toolkit/components/extensions/ext-downloads.js:537
(Diff revision 1)
> +          // reloaded, however, we must change the URI used by the XUL image element,
> +          // for example by adding a query parameter. This only works if we add one of
> +          // the parameters explicitly supported by the nsIMozIconURI interface.
> +          return Promise.resolve(
> +            `moz-icon://${download.target.path}?size=${size}` +
> +            (download.succeeded ? "&state=normal" : ""));

This shouldn't be necessary here. If this kind of mangling is necessary at all (I'm not sure it is), then we should just start with a `moz-icon:` URL with the file's leaf name, and then switch to one with a `file:` URL pointing to the file.

::: toolkit/components/extensions/schemas/downloads.json:524
(Diff revision 1)
>          "description": "Retrieve an icon for the specified download. For new downloads, file icons are available after the <a href='#event-onCreated'>onCreated</a> event has been received. The image returned by this function while a download is in progress may be different from the image returned after the download is complete. Icon retrieval is done by querying the underlying operating system or toolkit depending on the platform. The icon that is returned will therefore depend on a number of factors including state of the download, platform, registered file types and visual theme. If a file icon cannot be determined, <a href='extension.html#property-lastError'>chrome.extension.lastError</a> will contain an error message.",
>          "parameters": [
>            {
>              "description": "The identifier for the download.",
>              "name": "downloadId",
>              "type": "integer"

We should probably have a minimum value here. At least 1, but possibly something larger.
Attachment #8731196 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 3

2 years ago
https://reviewboard.mozilla.org/r/40421/#review36945

::: toolkit/components/extensions/ext-downloads.js:526
(Diff revision 1)
> +        return DownloadMap.lazyInit().then(() => {
> +          let size = options && options.size ? options.size : 32;
> +          let download = DownloadMap.fromId(downloadId).download;
> +
> +          if (!download.target.path) {
> +            // Old history downloads may not have a target path.

I think this is for legacy downloads so I'm thinking it probably isn't going to happen. `.unknown` just gives a blank white file icon on OS X, I'm assuming similar on other platforms. That doesn't seem bad to me.

::: toolkit/components/extensions/ext-downloads.js:534
(Diff revision 1)
> +          // When a download that was previously in progress finishes successfully, it
> +          // means that the target file now exists and we can extract its specific
> +          // icon, for example from a Windows executable. To ensure that the icon is
> +          // reloaded, however, we must change the URI used by the XUL image element,
> +          // for example by adding a query parameter. This only works if we add one of
> +          // the parameters explicitly supported by the nsIMozIconURI interface.

This code is actually copied from https://dxr.mozilla.org/mozilla-central/rev/7773387a9a2f1fd10e4424ea923c6185063f620b/browser/components/downloads/DownloadsViewUI.jsm#61-75. I figured it wasn't worth moving somewhere resusable since the `moz-icon://` protocal seems to handle most of this.

I could move it somewhere reuseable instead if you'd prefer. Should the comment stay if it stays here? Maybe I should just add a comment saying to look at the other file.

::: toolkit/components/extensions/ext-downloads.js:536
(Diff revision 1)
> +          // icon, for example from a Windows executable. To ensure that the icon is
> +          // reloaded, however, we must change the URI used by the XUL image element,
> +          // for example by adding a query parameter. This only works if we add one of
> +          // the parameters explicitly supported by the nsIMozIconURI interface.
> +          return Promise.resolve(
> +            `moz-icon://${download.target.path}?size=${size}` +

The comment says it will try and parse extra file-specific data from the file once it's downloaded. I'd imagine it can't do that without a full file path.

::: toolkit/components/extensions/ext-downloads.js:537
(Diff revision 1)
> +          // reloaded, however, we must change the URI used by the XUL image element,
> +          // for example by adding a query parameter. This only works if we add one of
> +          // the parameters explicitly supported by the nsIMozIconURI interface.
> +          return Promise.resolve(
> +            `moz-icon://${download.target.path}?size=${size}` +
> +            (download.succeeded ? "&state=normal" : ""));

I think it's just trying to bust the cache. We could leave it up to the developer but it seems like it's useful.

I tried putting a `file:` URL in an `<img>` and it didn't seem to work.
(Assignee)

Comment 4

2 years ago
Comment on attachment 8731196 [details]
MozReview Request: bug 1245606 - Implement chrome.downloads.getFileInfo r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40421/diff/1-2/
Attachment #8731196 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 5

2 years ago
Comment on attachment 8731196 [details]
MozReview Request: bug 1245606 - Implement chrome.downloads.getFileInfo r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40421/diff/2-3/
Comment on attachment 8731196 [details]
MozReview Request: bug 1245606 - Implement chrome.downloads.getFileInfo r=kmag

https://reviewboard.mozilla.org/r/40421/#review38311

Sorry this took so long. I just got back into the country last night.

::: toolkit/components/extensions/ext-downloads.js:594
(Diff revisions 1 - 3)
> -          if (!download.target.path) {
> -            // Old history downloads may not have a target path.
> -            return Promise.resolve(`moz-icon://.unknown?size=${size}`);
> -          }
> -          // When a download that was previously in progress finishes successfully, it
> -          // means that the target file now exists and we can extract its specific
> +          return new Promise((resolve, reject) => {
> +            let chromeWebNav = Services.appShell.createWindowlessBrowser(true);
> +            chromeWebNav
> +              .QueryInterface(Ci.nsIInterfaceRequestor)
> +              .getInterface(Ci.nsIDocShell)
> +              .createAboutBlankContentViewer(Services.scriptSecurityManager.getSystemPrincipal());

Can you add a blank line here, and a few more below, so it's easier to understand which sections of the function are related?

::: toolkit/components/extensions/ext-downloads.js:618
(Diff revisions 1 - 3)
> +              cleanup();
> +              resolve(dataURL);
> +            };
> +            handleError = (error) => {
> +              cleanup();
> +              reject(new Error("An unexpected error occurred"));

We should `Cu.reportError(error)` in this case.

::: toolkit/components/extensions/ext-downloads.js:622
(Diff revisions 1 - 3)
> +              cleanup();
> +              reject(new Error("An unexpected error occurred"));
> +            };
> +            img.addEventListener("load", handleLoad);
> +            img.addEventListener("error", handleError);
> +            img.src = `moz-icon://${path}?size=${size}`;

No `//` when `path` is a file URL, only when it's a leaf name.

::: toolkit/components/extensions/schemas/downloads.json:539
(Diff revisions 1 - 3)
>              "properties": {
>                "size": {
>                  "description": "The size of the icon.  The returned icon will be square with dimensions size * size pixels.  The default size for the icon is 32x32 pixels.",
>                  "optional": true,
> +                "minimum": 1,
> +                "maximum": 127,

Why 127?
Attachment #8731196 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 7

2 years ago
Comment on attachment 8731196 [details]
MozReview Request: bug 1245606 - Implement chrome.downloads.getFileInfo r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40421/diff/3-4/
(Assignee)

Comment 8

2 years ago
https://reviewboard.mozilla.org/r/40421/#review38311

> Why 127?

I'm not quite sure. If I navigate to moz-icon://a-pdf.pdf?size=255 it works (up to 255, 256 fails) but for some reason it will only go to 127 with this setup.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/baa84a5b4465
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.