Disable setting of origin in metadata.json in external apps

RESOLVED FIXED

Status

Firefox OS
Gaia
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jsmith, Assigned: albert)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [systemsfe])

Attachments

(2 attachments)

(Reporter)

Description

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

Updated

4 years ago
Depends on: 929600
(Reporter)

Updated

4 years ago
Whiteboard: [systemsfe]
We should let them set an origin if they are privileged and declare this origin in the manifest.
(Reporter)

Comment 2

4 years ago
(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.
(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 :).
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!)
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?
(Assignee)

Updated

4 years ago
Assignee: nobody → acperez
Flags: needinfo?(yurenju.mozilla)
(Assignee)

Updated

4 years ago
Depends on: 932223
(Assignee)

Comment 6

4 years ago
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..
(Assignee)

Comment 7

4 years ago
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..
let's talk on bug 897325.
Flags: needinfo?(yurenju.mozilla)
(Assignee)

Comment 9

4 years ago
Created attachment 828880 [details]
Patch
Attachment #828880 - Flags: feedback?(amac)
(Assignee)

Comment 10

4 years ago
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?
Flags: needinfo?(yurenju.mozilla)
Created attachment 829169 [details]
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.
Flags: needinfo?(yurenju.mozilla)
Comment on attachment 828880 [details]
Patch

Comments on the PR.
Attachment #828880 - Flags: feedback?(amac) → feedback-
(Assignee)

Updated

4 years ago
Attachment #828880 - Flags: feedback- → feedback?(amac)
Comments on the PR, Albert.
Attachment #828880 - Flags: feedback?(amac) → feedback-
(Assignee)

Updated

4 years ago
Attachment #828880 - Attachment description: Patch (WIP) → Patch
Attachment #828880 - Flags: feedback- → feedback?(amac)
Comment on attachment 828880 [details]
Patch

This looks good to me, Albert. Thanks!
Attachment #828880 - Flags: feedback?(amac) → feedback+
(Assignee)

Updated

4 years ago
Attachment #828880 - Flags: review?(yurenju.mozilla)
Albert, I'm pretty busy several days, I'll review this patch on next week.
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.
Attachment #828880 - Flags: review?(yurenju.mozilla)
(Assignee)

Updated

4 years ago
Attachment #828880 - Flags: review?(yurenju.mozilla)
Comment on attachment 828880 [details]
Patch

Makes sense with your comments, r=yurenju and I notice travis-ci state is red, please check it.
Attachment #828880 - Flags: review?(yurenju.mozilla) → review+
(Assignee)

Updated

4 years ago
Depends on: 945684
(Assignee)

Comment 18

4 years ago
Comment on attachment 828880 [details]
Patch

Rebased and added patch for bug 929600.
Attachment #828880 - Flags: review+ → review?(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 :)
Attachment #828880 - Flags: review?(amac) → review+
(Assignee)

Comment 20

4 years ago
Master: https://github.com/mozilla-b2g/gaia/commit/2fab27987174744f4f615e403a9de602a36edb07
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Duplicate of this bug: 945684
(Assignee)

Updated

4 years ago
Duplicate of this bug: 929600
(Reporter)

Updated

4 years ago
Depends on: 950136
(Reporter)

Comment 23

4 years ago
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?
Flags: needinfo?(acperez)
The Bug 952922 fixed this issue.
The Buri pvt m-c 20140101040201 build works fine now.
Flags: needinfo?(acperez)
You need to log in before you can comment on or make changes to this bug.