Closed Bug 781060 Opened 8 years ago Closed 8 years ago

All apps should use the same webapp database

Categories

(Firefox for Android :: Web Apps (PWAs), defect, P1)

defect

Tracking

()

VERIFIED FIXED
Tracking Status
firefox16 --- verified
firefox17 --- verified

People

(Reporter: wesj, Unassigned)

References

Details

(Whiteboard: [blocking-webrtandroid1+])

Attachments

(1 file)

Webapps on desktop all use the same database. We should do the same. i.e. right now if the marketplace app installs a webapp, there is no way to uninstall it (unless the market has an uninstall button I can't see?)

The webapprt on desktop implements their own directory provider:
http://mxr.mozilla.org/mozilla-central/source/webapprt/DirectoryProvider.js#33
Which checks a config file written for the app (at install?)
http://mxr.mozilla.org/mozilla-central/source/webapprt/WebappRT.jsm#43
And forces to refer to this via also some constants in WebApps.jsm:
http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#46

i.e. none of this will work well for us. We have plumbing to set some default prefs on install. nsIFile prefs don't quite work yet, but I think we should look into moving this there. i.e. set a pref at install, if its set use it in Webapps.jsm.
Whiteboard: [blocking-webrtandroid1?]
Sounds like we could add a webapp-config.js file in the root "profile" folder. We should be able to access that file from any profile. That file would have a regsitryDir pointing to the main Firefox profile that holds the real webapp json file. Right?
Attached patch Patch v1Splinter Review
This makes us use the same string and B2G and implements it for our directory service. It will point to the parent of the current apps profile directory. If you're using a crazy profile, that might be wrong, but that seems acceptable to me?
Attachment #650399 - Flags: review?(mark.finkle)
Comment on attachment 650399 [details] [diff] [review]
Patch v1

This means that even two different Firefox profiles will use the same webapps DB. I'm not sure we want that, and we know desktop does not do it that way.

I need to noodle on this...
So as an alternative, we could set a pref and use this same code to read it in our dirprovider. The pref would point to the app parent.

We'll need a few more underpinnings for that. Because of the way the WebAppAllocator works, we can't currently have two installs of the Marketplace App for instance, so if two users install it... things will get funky. We may have to hash the app url with the current profile name (and store the parent profile with installed apps) to make things like that work.
(In reply to Wesley Johnston (:wesj) from comment #4)

> We'll need a few more underpinnings for that. Because of the way the
> WebAppAllocator works, we can't currently have two installs of the
> Marketplace App for instance, so if two users install it... things will get
> funky. We may have to hash the app url with the current profile name (and
> store the parent profile with installed apps) to make things like that work.

These restrictions might mean that a global webapp registry for all Firefox profiles is the right thing, on Android. Since we ship Firefox with a limited number built-in WebApp entries in AndroidManifest.xml

If I had two Firefox profiles and we _did_ store the webapp registry in the profile, WebApp1 could be associated with two different webapps, which would be bad.
If we use the same webapp database on Android, could we run into problems that are brought up in bug 765865?
(In reply to Jason Smith [:jsmith] from comment #6)
> If we use the same webapp database on Android, could we run into problems
> that are brought up in bug 765865?

This bug actually fixes bug 765865, if I understand things correctly.
Nope. 765865 is about concurrent access to the db. Since its basically a JSON file that problem is a bit tougher.

On phones I expect concurrent access to be a pretty rare situation anyway.
(In reply to Mark Finkle (:mfinkle) from comment #5)
> These restrictions might mean that a global webapp registry for all Firefox
> profiles is the right thing, on Android. Since we ship Firefox with a
> limited number built-in WebApp entries in AndroidManifest.xml

I'm not familiar with the user-facing use cases for multiple profiles in Fennec, but my uninformed perspective is that there aren't any that we currently support nor any that we plan to support in the future.  And in that case it makes sense to have a global webapp registry for all profiles.

Note that Firefox OS has exactly such a global registry (it's /data/local):

http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#40
http://mxr.mozilla.org/mozilla-central/source/b2g/components/DirectoryProvider.js#13

(I've actually considered doing something similar for desktop.  It's a harder call there, where there are user-facing use cases for profiles that we ostensibly support.  But desktop OSes associate native apps with the OS account that installed them, and I think we're going to trip over the incongruity between our profile-specificity and the OS's account-specificity sooner than later.  But that's another bug.)


> If I had two Firefox profiles and we _did_ store the webapp registry in the
> profile, WebApp1 could be associated with two different webapps, which would
> be bad.

Indeed.
(In reply to Mark Finkle (:mfinkle) from comment #5)
> (In reply to Wesley Johnston (:wesj) from comment #4)
> 
> > We'll need a few more underpinnings for that. Because of the way the
> > WebAppAllocator works, we can't currently have two installs of the
> > Marketplace App for instance, so if two users install it... things will get
> > funky. We may have to hash the app url with the current profile name (and
> > store the parent profile with installed apps) to make things like that work.
> 
> These restrictions might mean that a global webapp registry for all Firefox
> profiles is the right thing, on Android. Since we ship Firefox with a
> limited number built-in WebApp entries in AndroidManifest.xml
> 
> If I had two Firefox profiles and we _did_ store the webapp registry in the
> profile, WebApp1 could be associated with two different webapps, which would
> be bad.

This isn't quite right. There are actually two databases we're juggling here. The one used by Gecko and the one used by Java.  Gecko just keeps track of the manifests themselves. That database is (currently) not shared by users

Java keeps track of things like which webapp is WebApp10. It IS currently shared by all users, and is based off a single key, the app's domain. i.e. if User 1 installs the Market app it will get assigned:

Webapp0 = marketplace.mozilla.org

Now if user 2 comes along, they can also install the same marketplace webapp (because they aren't sharing a gecko webapp db). Gecko will download and store the manifest again, but Java will check and see that this domain is already registered to WebApp0 and not assign a new one for it.

That means that when user 2 launches the marketplace, they'll start the same webapp as user 1 profile and all. If multi-user support is something we want, we'll want/need to change that so that the two different users launch the marketplace with different profiles. Might be better to plan for it now than to wish we had planned for it later?

To support this, I'd probably just alter those keys so that instead we had something like:

WebApp0 = profile1:marketplace.mozilla.org (or maybe a hash of the two?)

but since we'd still want a shared app database for everything a single "user" did. we'll have to pass the path to the original database to each app a user installs, and then return that from the dirProvider. I'll write that up too.
Desktop shortcuts will also make this kinda suck.... If two users install the same app, shortcuts to both will appear on the desktop because the two apps will have slightly different intents (different Webapp numbers), but they'll be identical.

Maybe that's just the tradeoff for this sort of support.

I know mfinkle really wants multi-user support for Fennec on Tablets, so I'll move forward trying to make sure it kinda sorta works unless he says no.
(In reply to Wesley Johnston (:wesj) from comment #11)

> I know mfinkle really wants multi-user support for Fennec on Tablets, so
> I'll move forward trying to make sure it kinda sorta works unless he says no.

I don't think we care about multi-user (meaning Firefox profiles) needing to have isolated webapps. Native apps don't work that way on Android (yet).

If the DOM webapp registry _and_ the Java webapp lookup are both singletons (the DOM webapp registry is not yet, I know) then two users can't install the same app... the app is already considered "installed" after the first user installs, and two shortcuts can't appear anyway, since we don't allow duplicates. The Webapp numbers are the same too.

I think we just go ahead and make the DOM webapp registry become global.
Priority: -- → P1
Whiteboard: [blocking-webrtandroid1?] → [blocking-webrtandroid1+]
Comment on attachment 650399 [details] [diff] [review]
Patch v1

This is good, but I wonder if we should make a subfolder for the webapps, to keep things a little cleaner: "profileroot/webapps"  ?

The downside is that you'll need to test for the existence and create it if needed.
Attachment #650399 - Flags: review?(mark.finkle) → feedback+
(In reply to Mark Finkle (:mfinkle) from comment #13)
> Comment on attachment 650399 [details] [diff] [review]
> Patch v1
> 
> This is good, but I wonder if we should make a subfolder for the webapps, to
> keep things a little cleaner: "profileroot/webapps"  ?

Webapps.jsm already does this:

http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#69
(In reply to Myk Melez [:myk] [@mykmelez] from comment #14)
> (In reply to Mark Finkle (:mfinkle) from comment #13)
> > Comment on attachment 650399 [details] [diff] [review]
> > Patch v1
> > 
> > This is good, but I wonder if we should make a subfolder for the webapps, to
> > keep things a little cleaner: "profileroot/webapps"  ?
> 
> Webapps.jsm already does this:
> 
> http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#69

I was referring to letting "webapps.json" live in profile_root folder, or whether it should be in a subfolder too. I can live with it in profile_root folder.
(In reply to Mark Finkle (:mfinkle) from comment #15)
> I was referring to letting "webapps.json" live in profile_root folder, or
> whether it should be in a subfolder too. I can live with it in profile_root
> folder.

Erm, perhaps I misunderstand what you are saying, and I certainly don't have any objection to moving webapps.json up one level.

I just wanted to note that, with the current patch, webapps.json will live in the webapps/ subfolder of the profile_root folder, as that's where Webapps.jsm will put it (because it automatically puts it into a webapps/ subdirectory of whatever directory it is given, which in this case is profile_root).  So the directory structure will look something like this:

profile_root/
  webapps/
    webapps.json
    {UUID}/
      manifest.webapp
    {UUID}/
      manifest.webapp
    ...
(In reply to Myk Melez [:myk] [@mykmelez] from comment #16)
> (In reply to Mark Finkle (:mfinkle) from comment #15)
> > I was referring to letting "webapps.json" live in profile_root folder, or
> > whether it should be in a subfolder too. I can live with it in profile_root
> > folder.
> 
> Erm, perhaps I misunderstand what you are saying, and I certainly don't have
> any objection to moving webapps.json up one level.
> 
> I just wanted to note that, with the current patch, webapps.json will live
> in the webapps/ subfolder of the profile_root folder, as that's where
> Webapps.jsm will put it (because it automatically puts it into a webapps/
> subdirectory of whatever directory it is given, which in this case is
> profile_root).  So the directory structure will look something like this:

I totally failed at looking at the docs for FileUtils.getFile(...) which works exactly as you say. This patch is golden. Thanks, Myk.
Attachment #650399 - Flags: feedback+ → review+
I accidentally landed this with bug 650399 in the commit message:

http://hg.mozilla.org/integration/mozilla-inbound/rev/2f0808dd3a11
https://hg.mozilla.org/mozilla-central/rev/2f0808dd3a11
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Comment on attachment 650399 [details] [diff] [review]
Patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Webapps
User impact if declined: Strange behavior with marketplace WEB app (and any webapp that wants to know what's installed)
Testing completed (on m-c, etc.): landed on 1 week ago.
Risk to taking this patch (and alternatives if risky): Low risk. Just moves the database location. Similar behaviour used by desktop and b2g so most callers are already aware of what's going on here. Just us catching up.
String or UUID changes made by this patch: None.
Attachment #650399 - Flags: approval-mozilla-aurora?
Comment on attachment 650399 [details] [diff] [review]
Patch v1

Verified on central, approving for Aurora due to need for wider testing.
Attachment #650399 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.