1000305 - Implement new API function for fetching app icons

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect, P2)

Description

[Ted Clancy [:tedders1]]

+++ This bug was initially created as a clone of Bug #899994 +++

From Bug 899994, Comment 17:

Add a new API: DOMRequest<Blob> getAppIcon(app, size). The 'size' argument is the size of the image that the homescreen app intends to display. Note that icon syntax is getting pretty hairy, so it's good if we don't need to require every homescreen app to implement this.

http://w3c.github.io/manifest/#icons-member

Comment 1

[[:fabrice] Fabrice Desré]

Note, the new hotness is to use Promise<Blob> and not DOMRequest

Comment 2

[(no longer active)]

Is this a mozbrowser API or something else?

Comment 3

[(no longer active)]

I guess it's a mozApps API?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This API is part of the Apps API. It'll be navigator.mozApps.mgmt.getAppIcon(app, size);

Marcos, do we need to pass in both a width and a height?

Comment 5

[John Hu [:johnhu][:johu][:醬糊小弟]]

I don't think we need to pass both width and height because our icon list is indexed with only one size and the image supposed to be a square image, like:

https://github.com/mozilla-b2g/gaia/blob/b64555b9ed961923b923239e96f87d6ba83cf951/apps/homescreen/manifest.webapp#L40-L45

Comment 6

[Jason Smith [:jsmith]]

This shouldn't block - this is a feature, which is something we won't block on unless we're past FL & we're planning to still land the feature.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, we should add this API to the App objects themselves, not to mozApps.mgmt.

Anyone that can get a reference to an App object should also be able to render that App's icon.

Comment 8

[Ted Clancy [:tedders1]]

Attachment: bug-1000305.patch

This patch should look familiar to you, Fabrice. It's largely based on your work.

Comment 9

[[:fabrice] Fabrice Desré]

Comment on attachment 8461666 [details] [diff] [review]
bug-1000305.patch

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

We need to send back a different error code depending on why we failed to fetch the icon:
- we're offline. In this case, should we register an online/offline observer and retry on the gecko side?
- bad url

We also need to use the content-type from the request if there's one instead of the file extension. That should give us the right mime type for a.jpg and a.jpeg.

And please add tests, both with hosted and packaged apps.

::: dom/apps/src/Webapps.js
@@ +740,5 @@
>                          {
>                            messages: ["Webapps:Install:Return:OK",
>                                       "Webapps:Uninstall:Return:OK",
> +                                     "Webapps:Uninstall:Broadcast:Return:OK",
> +                                     "Webapps:GetIcon:Return"]

you don't need to register this message

@@ +771,5 @@
>      cpmm.sendAsyncMessage("Webapps:UnregisterForMessages",
>                            ["Webapps:Install:Return:OK",
>                             "Webapps:Uninstall:Return:OK",
> +                           "Webapps:Uninstall:Broadcast:Return:OK",
> +                           "Webapps:GetIcon:Return"]);

same here

@@ +907,5 @@
>          Services.DOMRequest.fireError(req, "NOT_INSTALLED");
>          break;
> +      case "Webapps:GetIcon:Return":
> +        if (msg.blob) {
> +          let blob = new this._window.Blob([msg.blob], { type: msg.type });

is that better than blob = Cu.cloneInto(msg.blob, this._window) ?

::: dom/apps/src/Webapps.jsm
@@ +3819,5 @@
> +      // Set up an xhr to download a blob.
> +      let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +                  .createInstance(Ci.nsIXMLHttpRequest);
> +      xhr.mozBackgroundRequest = true;
> +      let mimeType = "image/" + aUrl.split(".").reverse()[0];

that will fail if the url has no "." in the filename.

Comment 10

[Kai-Chih Hu [:khu]]

Use feature-b2g:2.2? rather than just 2.2.

Comment 11

[Ted Clancy [:tedders1]]

To ease landing of this patch, I'm splitting some of it into a new Bugzilla ticket. That's because some of it needs to land before related changes to the homescreen app, and some of it needs to land after.

Comment 12

[Ted Clancy [:tedders1]]

Attachment: new-bug-1000305-fix.patch

Rebased & Updated! Some of the code has been moved to 1092726.

I still have to write tests. I'll mark this checkin-needed as soon as I have tests. It's my top priority.

Comment 13

[Ted Clancy [:tedders1]]

Attachment: new-bug-1000305-fix.patch

Comment 14

[Ted Clancy [:tedders1]]

Okay, after doing some ad hoc testing, I've found this code doesn't work anymore, and I can't figure out why. It used to work.

Unfortunately I have to set it aside for a few days to work on something else. Feel free to have at it.

Comment 15

[Kevin Grandon :kgrandon]

Ted, Gregor - Is this something we can prioritize? I don't think we can have replaceable home screens until this is done.

Comment 16

[Gregor Wagner [:gwagner]]

(In reply to Kevin Grandon :kgrandon from comment #15)
> Ted, Gregor - Is this something we can prioritize? I don't think we can have
> replaceable home screens until this is done.

Ted is working in it right now.

Comment 17

[Ted Clancy [:tedders1]]

Attachment: new-bug-1000305-fix.patch

Fixed the rebased version.

Comment 18

[Ted Clancy [:tedders1]]

Attachment: new-bug-1000305-test-packaged-app

I've added jduell to this review to review the changes in netwerk/test/httpserver/httpd.js.

I've added ferjm to review the changes in dom/apps/tests/.

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8535131 [details] [diff] [review]
new-bug-1000305-test-packaged-app

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

Thanks Ted. Looks good! It seems that we can add more tests for the entry point cases. Could you handle that in this patch as well, please? Thank you!

::: dom/apps/tests/file_packaged_app.template.webapp
@@ +12,5 @@
>       "downloads": {}
>     },
>    "launch_path": "tests/dom/apps/tests/file_packaged_app.sjs",
> +  "icons": {
> +    "15": "icon.png"

I didn't look at the implementation in detail, but here is a dumb question: what happen with icon redeclarations? Do we control that in any way (ignore, error, warning)? I guess we should throw an INVALID_MANIFEST error but I am not sure if we are currently doing that for duplicated entries.

::: dom/apps/tests/test_packaged_app_install.html
@@ +254,5 @@
> +      }
> +
> +      reader.readAsBinaryString(blob);
> +    }, (err) => {
> +      info("Can't get an icon");

I think we should fail and finish here.

ok(false, "Can't get an icon " + error);
PackagedTestHelper.finish();

@@ +257,5 @@
> +    }, (err) => {
> +      info("Can't get an icon");
> +    });
> +  },
> +  function() {

Could you also add a test for the "NoSuchIcon" case, please? BTW, Webapps.js error strings are written in CAPITAL_SNAKE_CASE, so NoSuchIcon should be NO_SUCH_ICON for consistency.

Comment 20

[Ted Clancy [:tedders1]]

Attachment: bug-1000305-fix-part1.patch

Comment 21

[Ted Clancy [:tedders1]]

Attachment: bug-1000305-fix-part2.patch

Comment 22

[Ted Clancy [:tedders1]]

> what happen with icon redeclarations? Do we control that in any way (ignore, error, warning)?

I experimented with that, and it looks like the second "icons" declaration overrides the first "icons" declaration.

Yeah, we probably should throw an error. However, we're using JSON.parse() to parse the manifest file, and JSON.parse() doesn't report any error. It just silently uses the second declaration and ignores the first one. So we have no way to detect an error unless we modify JSON.parse().

Comment 23

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8536717 [details] [diff] [review]
bug-1000305-fix-part2.patch

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

LGTM! r=me for the tests part.

In an ideal world, we will be removing packaged apps support at some point, so I would add the same cases to both hosted and packaged apps tests. But feel free to ignore me about this.

Thank you Ted.

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Ted Clancy [:tedders1] from comment #22)
> > what happen with icon redeclarations? Do we control that in any way (ignore, error, warning)?
> 
> I experimented with that, and it looks like the second "icons" declaration
> overrides the first "icons" declaration.
> 
> Yeah, we probably should throw an error. However, we're using JSON.parse()
> to parse the manifest file, and JSON.parse() doesn't report any error. It
> just silently uses the second declaration and ignores the first one. So we
> have no way to detect an error unless we modify JSON.parse().

We could check this at [1] at install/update time keeping the fields in memory and freeing it after the check. This is probably out of the scope of this bug though. Feel free to address it or file another bug for it.

Also, I don't know if duplicated fields is something that the web manifest spec takes into account.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/apps/AppsUtils.jsm#396

Comment 25

[Jason Duell]

Comment on attachment 8536717 [details] [diff] [review]
bug-1000305-fix-part2.patch

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

I'm not really a good reviewer for any of this.  Guessing that :Waldo might be (or would know who else could take it)

Comment 26

[Ted Clancy [:tedders1]]

Comment on attachment 8536717 [details] [diff] [review]
bug-1000305-fix-part2.patch

:waldo isn't a peer for the netwerk/test directory, so I'm reassigning the review to :mcmanus.

Patrick, I just need you to review the two line change to netwerk/test/httpserver/httpd.js. The rest has been reviewed by :ferjm.

Comment 27

[Jeff Walden [:Waldo]]

Comment on attachment 8536717 [details] [diff] [review]
bug-1000305-fix-part2.patch

I'm a peer for httpd.js, tho, regardless that it resides in netwerk/test.

I mildly dislike adding random functions into SJS globals this way, versus some sort of system for letting SJS scripts pick and choose what they want, or something.  But not enough to complain, and not when we're just talking atob/btoa.

Comment 28

[Patrick McManus [:mcmanus]]

(In reply to Ted Clancy [:tedders1] from comment #26)
> Comment on attachment 8536717 [details] [diff] [review]
> bug-1000305-fix-part2.patch
> 
> :waldo isn't a peer for the netwerk/test directory, so I'm reassigning the
> review to :mcmanus.
> 

waldo has my standing endorsement to review/write any part of httpd.js he likes :)

(but I'll do this)

Comment 29

[Ted Clancy [:tedders1]]

Oh. Sorry for the confusion. Thanks, guys!

Comment 30

[Ted Clancy [:tedders1]]

Attachment: bug-1000305-fix-part2.patch

Rebased.

Comment 31

[Ted Clancy [:tedders1]]

Green TBPL run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=73f89ec517e2

Comment 32

[Ted Clancy [:tedders1]]

Attachment: bug-1000305-fix-part2.patch

Fixed check-in comment.

Comment 33

[[:fabrice] Fabrice Desré]

Was part 1 ever review+ ? I don't see it in this bug?

Comment 34

[Ted Clancy [:tedders1]]

Fabrice: Comment #9.

Comment 35

[[:fabrice] Fabrice Desré]

(In reply to Ted Clancy [:tedders1] from comment #34)
> Fabrice: Comment #9.

That was feedback+, not r+

Comment 36

[[:fabrice] Fabrice Desré]

Comment on attachment 8536716 [details] [diff] [review]
bug-1000305-fix-part1.patch

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

A few things to fix, and I'd like to know the answer to the last question.

::: dom/apps/Webapps.jsm
@@ +4171,5 @@
> +    let app = this.getAppByManifestURL(aData.manifestURL);
> +    if (!app) {
> +      debug("NO_APP");
> +      aData.error = "NO_APP";
> +      aMm.sendAsyncMessage("Webapps:GetIcon:Return", aData);

Please refactor all the:
aData.error = XXX
aMm.sendAsyncMessage("Webapps:GetIcon:Return", aData);

into a local helper method eg. sendError("NO_APP");

@@ +4186,5 @@
> +      xhr.open("GET", aUrl, true);
> +      xhr.responseType = "blob";
> +      xhr.addEventListener("load", function() {
> +        debug("Got http status=" + xhr.status + " for " + aUrl);
> +        let payload;

just do a |let payload = {..| in the status == 200 branch.

@@ +4195,5 @@
> +          payload = {
> +            "oid": aData.oid,
> +            "requestID": aData.requestID,
> +            "blob": blob,
> +            "type": xhr.getResponseHeader('Content-Type') || fallbackMimeType

nit: double quotes for strings in this file.

@@ +4198,5 @@
> +            "blob": blob,
> +            "type": xhr.getResponseHeader('Content-Type') || fallbackMimeType
> +          };
> +          aMm.sendAsyncMessage("Webapps:GetIcon:Return", payload);
> +        } else if (status === 0) {

That needs to be xhr.status === 0

@@ +4236,5 @@
> +        // No icons smaller (or equal) to requested size.
> +        aData.error = "NO_SUCH_ICON";
> +        aMm.sendAsyncMessage("Webapps:GetIcon:Return", aData);
> +        return;
> +      }

Why not just use iconURLForSize() ?

Comment 37

[Kai-Chih Hu [:khu]]

Candice, can you follow up this topic? Please either feature-b2g + or - it. 
Thanks.

Comment 38

[Ted Clancy [:tedders1]]

Attachment: bug-1000305-fix-part1.patch

Okay, I guess I should use iconURLForSize(). 

I was trying to preserve the logic that formerly existed in the homescreen app. (It differs from iconURLForSize() in that it chooses the largest icon which is less-than-or-equal-to than the requested size; not the icon which is closest-but-possibly-bigger than the requested size.) 

But it's probably better to use iconURLForSize(). I mean, that's what it's for.

Comment 39

[Ted Clancy [:tedders1]]

Attachment: bug-1000305-fix-part2.patch

Comment 40

[[:fabrice] Fabrice Desré]

Comment on attachment 8538672 [details] [diff] [review]
bug-1000305-fix-part1.patch

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

Note that you need a r+ from a DOM peer to land that since there's a commit hook that watches changes in dom/webidl against the list of DOM peers.

Comment 41

[Ted Clancy [:tedders1]]

Cool. Thanks for letting me know.

Comment 42

[Ted Clancy [:tedders1]]

Attachment: bug-1000305-fix-part1.patch

Adding r=ehsan.

I just need Ehsan to review the change to dom/webidl/Apps.webidl. The rest has already been reviewed by :fabrice.

Comment 43

[(no longer active)]

Comment on attachment 8538738 [details] [diff] [review]
bug-1000305-fix-part1.patch

r=me on the WebIDL change.

Comment 44

[Ted Clancy [:tedders1]]

Green TBPL run: https://tbpl.mozilla.org/?tree=Try&rev=05b6f70fcaf7

Comment 45

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 46

[Ted Clancy [:tedders1]]

Attachment: bug-1000305-fix-part1.patch

Rebased (again).

Comment 47

[Ted Clancy [:tedders1]]

Attachment: bug-1000305-fix-part2.patch

Rebased (again).

Comment 48

[Ted Clancy [:tedders1]]

Green TBPL run: https://tbpl.mozilla.org/?tree=Try&rev=f77ab212829e

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/45940d63b0c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/2996cc51cb0d

Comment 50

[Ted Clancy [:tedders1]]

\o/

Comment 51

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures on Android like https://treeherder.mozilla.org/logviewer.html#?job_id=4800543&repo=mozilla-inbound

Comment 52

[Ted Clancy [:tedders1]]

Alrighty, the tests on Android are dying when they encounter the atob() function. And I think I know why.

As mentioned above, for these tests to work, I had to modify httpd.js to add the atob() and btoa() functions. However, according to what Joel Maher (jmaher) tells me, the Android tests on the test server are run using an old build of xpcshell which might have its own copy of httpd.js (without my changes). So I think that's likely to be the problem.

Joel happens to be in the process of updating the test server's version of xpcshell right now, so I'm going to split my changes to httpd.js into its own patch file and land it now, so Joel can put the new version on the test server.

I'll land the rest of this after Joel updates xpcshell.

Comment 53

[Ted Clancy [:tedders1]]

Attachment: bug-1000305-fix-part2.patch

Comment 54

[Ted Clancy [:tedders1]]

Attachment: bug-1000305-fix-part3.patch

Comment 55

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8450d74f782e

Comment 56

[Carsten Book [:Tomcat]]

backed out but turned out this change was innocent so relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/2815e5bcfef5

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2815e5bcfef5

Comment 58

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/cypress/rev/af0a3c1a42e2

Comment 59

[Marcos Caceres [:marcosc]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #24)
> Also, I don't know if duplicated fields is something that the web manifest
> spec takes into account.

Web manifest also depends on JSON.parse() - so it's expected that the last duplicate always wins.

Comment 60

[Candice Serran (:cserran)]

not in 2.2, flag correct

Comment 61

[Ted Clancy [:tedders1]]

Attachment: bug-1000305-fix-part2.patch

Due to problems with btoa/atob on our Android ICS platform, I've removed the base-64 encoded data from the tests, and replaced them with raw strings. (I should have done that a month ago.)

Comment 62

[Ted Clancy [:tedders1]]

Treeherder run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d165201ae92c

Comment 63

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfa7ca09a3c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6ef2ee2020f

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bfa7ca09a3c7
https://hg.mozilla.org/mozilla-central/rev/c6ef2ee2020f

Comment 65

[Kevin Grandon :kgrandon]

Hmm, should we add dev-doc-needed to get documentation for this? Not really sure how this works.

Comment 66

[leandro]

can any caritative soul point me to a doc page where consult how to use this?

i did a lot of homescreens, they work well as certified. In order to send them to the marketplace i want to do them privileged, but in i can't access mgmt as privileged. And i can't find how to use this new development.

thanks in (gameboy) advance

Comment 67

[Adrian Crespo]

Hello,

Despite I have set the "homescreen-webapps-manage" permission, I can't access the scope of navigator.mozApps.mgmt.getAppIcon() and navigator.mozApps.mgmt.getAll():
http://imgur.com/5EPZDT2.png

This is the manifest:
https://github.com/novia713/smokingtedscreen/blob/master/manifest.webapp#L13

This is the function:
https://github.com/novia713/smokingtedscreen/blob/master/js/apps.js#L115

This is the error shown in logcat (which, by the way, it's not too much self-explanatory)
I/GeckoDump(  208): DeveloperHUD: [app://763cd424-fd12-4b4e-ac65-2538da91e231/manifest.webapp] Error (content javascript): "NS_ERROR_UNEXPECTED: " in app://763cd424-fd12-4b4e-ac65-2538da91e231/js/apps.js:115:0

Comment 68

[[:fabrice] Fabrice Desré]

Is your app the current homescreen? This needs to be the case for these apis to be accessible.

Comment 69

[Adrian Crespo]

Thanks fabrice, it was because of what you said :)

Comment 70

[Adrian Crespo]

Related: https://bugzilla.mozilla.org/show_bug.cgi?id=1241189