+++ 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

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

Is this a mozbrowser API or something else?

I guess it's a mozApps API?

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?

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

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.

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.

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

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.

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

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.

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.

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.

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

(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.

Fixed the rebased version.

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 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.

> 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 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.

(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 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 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 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.

(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)

Oh. Sorry for the confusion. Thanks, guys!

Rebased.

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

Fixed check-in comment.

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

Fabrice: Comment #9.

(In reply to Ted Clancy [:tedders1] from comment #34) > Fabrice: Comment #9. That was feedback+, not r+

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() ?

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

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 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.

Cool. Thanks for letting me know.

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 on attachment 8538738 [details] [diff] [review] bug-1000305-fix-part1.patch r=me on the WebIDL change.

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

Needs rebasing.

Rebased (again).

Rebased (again).

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

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

\o/

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

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.

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

(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.

not in 2.2, flag correct

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.)

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

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

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

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

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

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

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

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

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