929602 - Disable setting of origin in metadata.json in external apps

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Jason Smith [:jsmith]]

Context - bug 927967. We're allowing setting of the origin in external apps in the old external apps directory, but we shouldn't be.

Comment 1

[[:fabrice] Fabrice Desré]

We should let them set an origin if they are privileged and declare this origin in the manifest.

Comment 2

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #1)
> We should let them set an origin if they are privileged and declare this
> origin in the manifest.

Well right, but I think Antonio's point was that it should be set via the webapp manifest, not metadata.json. Otherwise, we run into the problems Antonio cited in bug 927967.

Comment 3

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jason Smith [:jsmith] from comment #2)
> (In reply to Fabrice Desré [:fabrice] from comment #1)
> > We should let them set an origin if they are privileged and declare this
> > origin in the manifest.
> 
> Well right, but I think Antonio's point was that it should be set via the
> webapp manifest, not metadata.json. Otherwise, we run into the problems
> Antonio cited in bug 927967.

I think you two and me are saying the same thing. There's support on the code *already* to define an origin on the manifest for privileged apps. It was added on bug 852720. So unless this is just to remove the origin set at build time using metadata, and to let the marketplace know they should support/allow that I don't understand this at all :).

Comment 4

[[:fabrice] Fabrice Desré]

Antonio, I just want to make sure that the build system does the right thing:
- reject metadata.json files that have invalid data (and not silently ignore them). julienw cleaned up a lot around that, maybe there's more to do.
- make use of the origin field in the manifest if any even at build time (I don't think we do, but I'd be happy to be wrong!)

Comment 5

[Antonio Manuel Amaya Calvo (:amac)]

So we should do the following, I think
* Check that the operator apps build process is building the metadata.json based on the manifest URL. I think it does but I'm not sure, and stuck at this traffic jam (gotta move rainy days) cannot check it now. 
* Fix the external apps build process so I gets the origin for webapps.json for packaged apps from the app manifest and not from the metadata.json. I think this is working incorrectly now. The operator apps though are working correctly since they're actually installed at run time and not at build time. 

And besides this, somebody should reach the marketplace people so they're aware that they can allow people to specify origins for privileged apps, and that they should do it so on the marketplace app if they want it to have an specific origin. 

Can you take this, Albert?

Comment 6

[Albert [:albert]]

While bug 932223 is fixed, do you know if it's possible to know the amount of apps per homescreen at build time? You'll have to face the same problem when implementing your tool to configure singlevariant..

Comment 7

[Albert [:albert]]

This comment is for yuren..

(In reply to Albert [:albert] from comment #6)
> While bug 932223 is fixed, do you know if it's possible to know the amount
> of apps per homescreen at build time? You'll have to face the same problem
> when implementing your tool to configure singlevariant..

Comment 8

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

let's talk on bug 897325.

Comment 9

[Albert [:albert]]

Attachment: Patch

Comment 10

[Albert [:albert]]

There is a problem because icon links are generated and saved in apps/homescreen/js/init.json during applications-data.js execution, so if webapp-manifests.js changes the application folder name to a generated random uuid, then 'homescreen' won't find icons. Moreover, this bug assumes that non privileged packaged apps doesn't have the origin in metadata and it will depend on the generated uuid, so it will be a problem for applications-data.js also, dealing with origin to compose icon descriptor.

To fix both problems I think the best approach is to move homescreen customization from applications-data.js to webapp-manifests.js, where the uuid is known and icon descriptor can be composed easily. What do you think?

Comment 11

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Attachment: diagram(4).png

how about remove calling webapp-manifest.js in Makefile and call webapp-manifest as a module in applications-data?

then you can get manifests object which can use in homescreen customization.

Comment 12

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 828880 [details]
Patch

Comments on the PR.

Comment 13

[Antonio Manuel Amaya Calvo (:amac)]

Comments on the PR, Albert.

Comment 14

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 828880 [details]
Patch

This looks good to me, Albert. Thanks!

Comment 15

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Albert, I'm pretty busy several days, I'll review this patch on next week.

Comment 16

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 828880 [details]
Patch

Overall looks good, I'll give r+ if you set |manifests[webappTargetDirName].origin| instead of |webappsJson| and set |readZipManifest(webapp.sourceDirectoryFile).origin| instead of all |pckManifest| object.

Comment 17

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 828880 [details]
Patch

Makes sense with your comments, r=yurenju and I notice travis-ci state is red, please check it.

Comment 18

[Albert [:albert]]

Comment on attachment 828880 [details]
Patch

Rebased and added patch for bug 929600.

Comment 19

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 828880 [details]
Patch

This looks good to me. Please clear up with :cvan that he's ok with merging his patch into this one before landing :)

Comment 20

[Albert [:albert]]

Master: https://github.com/mozilla-b2g/gaia/commit/2fab27987174744f4f615e403a9de602a36edb07

Comment 23

[Jason Smith [:jsmith]]

Albert - this is confirmed to cause bug 950136, which breaks a smoketest. We need this back this out to get the smoketest passing. Can you back out?

Comment 24

[Askeing Yen[:askeing][:fyen]]

The Bug 952922 fixed this issue.
The Buri pvt m-c 20140101040201 build works fine now.