1245606 - Implement chrome.downloads.getFileIcon()

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Andrew Swan [:aswan]]

Part of supporting chrome.downloads for WebExtensions

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

Comment 1

[Mark Striemer [:mstriemer]]

Attachment: 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/

Comment 2

[Kris Maglione [:kmag]]

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.

Comment 3

[Mark Striemer [:mstriemer]]

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.

Comment 4

[Mark Striemer [:mstriemer]]

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/

Comment 5

[Mark Striemer [:mstriemer]]

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 6

[Kris Maglione [:kmag]]

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?

Comment 7

[Mark Striemer [:mstriemer]]

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/

Comment 8

[Mark Striemer [:mstriemer]]

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.

Comment 9

[Pulsebot]

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

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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