Last Comment Bug 738832 - Allow desktop runtime to support webRT installs (mozApps.install API)
: Allow desktop runtime to support webRT installs (mozApps.install API)
Status: VERIFIED FIXED
[qa!]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Webapp Runtime (show other bugs)
: unspecified
: All All
: P2 enhancement
: Firefox 16
Assigned To: Ed Lee :Mardak
: Jason Smith [:jsmith]
Mentors:
: 751438 (view as bug list)
Depends on: 746213 751438 765865
Blocks: 741488 738816
  Show dependency treegraph
 
Reported: 2012-03-23 15:53 PDT by Jason Smith [:jsmith]
Modified: 2016-03-21 12:39 PDT (History)
14 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (10.09 KB, patch)
2012-06-13 15:02 PDT, Ed Lee :Mardak
myk: review-
Details | Diff | Splinter Review
screenshot v1 (444.02 KB, image/png)
2012-06-13 15:03 PDT, Ed Lee :Mardak
no flags Details
v2 (9.67 KB, patch)
2012-06-18 12:15 PDT, Ed Lee :Mardak
myk: review+
Details | Diff | Splinter Review
screenshot v2 (420.58 KB, image/png)
2012-06-18 12:16 PDT, Ed Lee :Mardak
no flags Details
v3 (10.09 KB, patch)
2012-06-20 22:28 PDT, Ed Lee :Mardak
felipc: review+
Details | Diff | Splinter Review
screenshot v3 (221.29 KB, image/png)
2012-06-20 22:28 PDT, Ed Lee :Mardak
no flags Details
examples of prompts (195.51 KB, image/png)
2012-06-21 11:42 PDT, Myk Melez [:myk] [@mykmelez]
no flags Details

Description Jason Smith [:jsmith] 2012-03-23 15:53:41 PDT
An idea thrown around in connection to bug 738816 and other "market" apps - We should allow the desktop runtime to allow for a special "marketplace mode" to enable operations against the mozApps API to install open web apps. That way, we could have bug 738816 be successful to allow installing of a marketplace application on our platform on desktop. Additionally, other groups could easily create their own "market" apps to fit their specific needs (e.g. Atlantic Records Web App Store).
Comment 1 Jason Smith [:jsmith] 2012-03-27 14:23:49 PDT
Myk - Knowing that the proof of concept for Mozilla Marketplace App using WebRT desktop is on the table now, should this be marked as a blocker for marketplace beta? How should we change the state of this bug?
Comment 2 Myk Melez [:myk] [@mykmelez] 2012-03-29 09:55:40 PDT
Jason: I've heard conflicting accounts from folks regarding the priority of a Marketplace App for WebRT Desktop.  cc:ing the PM folks for their input.
Comment 3 Bill Walker [:bwalker] [@wfwalker] 2012-03-29 11:48:41 PDT
(In reply to Myk Melez [:myk] [@mykmelez] from comment #2)
> Jason: I've heard conflicting accounts from folks regarding the priority of
> a Marketplace App for WebRT Desktop.  cc:ing the PM folks for their input.

I suspect I was the cause of those conflicting accounts. I don't think this is a blocker for April.
Comment 4 Ed Lee :Mardak 2012-04-02 09:56:39 PDT
Related to bug 740922 (which is for Firefox), we should be able to have a marketplace/dashboard app to launch installed apps as well. So potentially the logic for launching apps in bug 740922 and here would be the same (given the same goal in Firefox and runtime to launch a separate native app).
Comment 5 Jason Smith [:jsmith] 2012-04-02 09:58:45 PDT
(In reply to Edward Lee :Mardak from comment #4)
> Related to bug 740922 (which is for Firefox), we should be able to have a
> marketplace/dashboard app to launch installed apps as well. So potentially
> the logic for launching apps in bug 740922 and here would be the same (given
> the same goal in Firefox and runtime to launch a separate native app).

Sounds like this would require making the HTML 5 dashboard a web app. Could you file a feature request in Web Apps --> Dashboard for this feature? This is a good idea.
Comment 6 Ed Lee :Mardak 2012-05-04 13:51:59 PDT
*** Bug 751438 has been marked as a duplicate of this bug. ***
Comment 7 Jason Smith [:jsmith] 2012-05-07 21:14:56 PDT
Nominating for k9o, as this relates to the Marketplace App on Desktop in the https://wiki.mozilla.org/Kilimanjaro/ProductDraft#Mozilla_will_deliver_a_compelling_device.2Fsystem_un-boxing_experience user story. This needs to be implemented in order to allow marketplace-based interactions such as installing apps.
Comment 8 Sheila Mooney 2012-05-15 11:45:05 PDT
This is a P2 for k9o so not blocking.
Comment 9 Ed Lee :Mardak 2012-05-22 15:08:34 PDT
Has there been any mockups of what native-looking dialogs should look and behave in the runtime for various prompts like geolocation? It could probably be reused here for app installation but probably not so much for navigator.id.
Comment 10 Jason Smith [:jsmith] 2012-05-22 15:10:14 PDT
(In reply to Edward Lee :Mardak from comment #9)
> Has there been any mockups of what native-looking dialogs should look and
> behave in the runtime for various prompts like geolocation? It could
> probably be reused here for app installation but probably not so much for
> navigator.id.

Good point. Flagging uiwanted. I believe Dils was thinking of going with a native confirmation prompt (not a doorhanger) for geolocation. We could probably do the same for installing a web app inside the runtime.
Comment 11 Ed Lee :Mardak 2012-06-12 17:49:01 PDT
Notes from a WIP...

Need to update browser/modules/WebappsInstaller.jsm to use GreD runtime instead of processFolder:
+const WEBAPP_RUNTIME = Services.appinfo.ID == "webapprt@mozilla.org";
-    this.processFolder = Services.dirsvc.get("CurProcD", Ci.nsIFile);
+    this.runtimeFolder = Services.dirsvc.get("GreD", Ci.nsIFile);

These imports work fine from the runtime:
+Cu.import("resource://gre/modules/Webapps.jsm");
+Cu.import("resource://gre/modules/WebappsInstaller.jsm");

Can install and update registry from "webapps-ask-install":
+        if (WebappsInstaller.install(data)) {
+          DOMApplicationRegistry.confirmInstall(data);
Comment 12 Ed Lee :Mardak 2012-06-12 17:55:49 PDT
One tricky part of all this is keeping the registry correct across Firefox/AITC and multiple runtimes.

With my WIP, I can install an app from a native Marketplace app, but if Firefox is currently open, it won't see the registry changes. If I restart Firefox, it'll show up fine. This is beause DOMApplicationRegistry.webapps has an in-memory reference that it references/updates and explicitly saveApps.

The really bad problem is if a user installs from the native app and also installs from Firefox when both are open at the same time because there'll be data loss.

This also ties into AITC because I would assume only Firefox is syncing and not the native apps. So an app install from an app won't be handled by AITC until Firefox is opened.
Comment 13 Ed Lee :Mardak 2012-06-13 14:57:28 PDT
Changing bug title as uninstall doesn't actually exist as mozApps.uninstall (it's part of the app record).
Comment 14 Ed Lee :Mardak 2012-06-13 15:02:02 PDT
Created attachment 632898 [details] [diff] [review]
v1

Uses the same type of dialog as the geolocation prompt with similar wording.
Comment 15 Ed Lee :Mardak 2012-06-13 15:03:32 PDT
Created attachment 632899 [details]
screenshot v1

This is probably a Marketplace issue.. but "Campfire" is the name of the app while the listing in Marketplace allows arbitrary names "fire firy"?
Comment 16 Jason Smith [:jsmith] 2012-06-13 15:11:11 PDT
(In reply to Edward Lee :Mardak from comment #15)
> Created attachment 632899 [details]
> screenshot v1
> 
> This is probably a Marketplace issue.. but "Campfire" is the name of the app
> while the listing in Marketplace allows arbitrary names "fire firy"?

Right, probably just someone doing testing and flipping names around (this is the dev server).

A couple of nits on the UI:

- Should the default highlight be install instead? I don't think the privacy concerns brought up in geolocation apply here, although I could be mistaken
- I'd reduce the wording of "Install Web App" to "Install" - the dialog already provides context that a web app is being installed
- I'd suggest following the same wording the doorhanger does - Use "Not Now" instead of "Don't install"
Comment 17 Ed Lee :Mardak 2012-06-13 15:14:32 PDT
(In reply to Jason Smith [:jsmith] from comment #16)
> - Should the default highlight be install instead? I don't think the privacy
> concerns brought up in geolocation apply here, although I could be mistaken
The confirm action is creating files on the device, but I suppose downloads are default enter to download.

> - I'd reduce the wording of "Install Web App" to "Install" - the dialog
> already provides context that a web app is being installed
Then should the geolocation prompt not say "Share Location" on the button and just "Share"?

> - I'd suggest following the same wording the doorhanger does - Use "Not Now"
> instead of "Don't install"
The doorhanger dismiss actually means Not Now because the user can get back to it. Canceling the dialog actually cancels the install.
Comment 18 Jason Smith [:jsmith] 2012-06-13 15:41:28 PDT
(In reply to Edward Lee :Mardak from comment #17)
> (In reply to Jason Smith [:jsmith] from comment #16)
> > - I'd reduce the wording of "Install Web App" to "Install" - the dialog
> > already provides context that a web app is being installed
> Then should the geolocation prompt not say "Share Location" on the button
> and just "Share"?

Hmm...I guess either way works when I think about the connection to geolocation.

> 
> > - I'd suggest following the same wording the doorhanger does - Use "Not Now"
> > instead of "Don't install"
> The doorhanger dismiss actually means Not Now because the user can get back
> to it. Canceling the dialog actually cancels the install.

Makes sense.
Comment 19 Myk Melez [:myk] [@mykmelez] 2012-06-14 09:35:04 PDT
(In reply to Jason Smith [:jsmith] from comment #18)
> (In reply to Edward Lee :Mardak from comment #17)
> > Then should the geolocation prompt not say "Share Location" on the button
> > and just "Share"?
> 
> Hmm...I guess either way works when I think about the connection to
> geolocation.

Dils proposed "Share Location" on Mac and "Share" on Windows in bug 748214 comment 21.  In the end, Ed implemented "Share Location" on both platforms, and I think that's preferable to "Share", not because the additional word reinforces the action the user is taking.

(As an engineer, I'm inclined to prefer "Share", because DRY.  But humans aren't computers; they benefit from repetition.)
Comment 20 Myk Melez [:myk] [@mykmelez] 2012-06-15 15:59:37 PDT
Comment on attachment 632898 [details] [diff] [review]
v1

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

Looking good!  A few issues...


(In reply to Edward Lee :Mardak from comment #12)
> One tricky part of all this is keeping the registry correct across
> Firefox/AITC and multiple runtimes.

Indeed, this is going to be a significant pain point that we'll need to address.

The original Apps prototype used an SQLite database, which can be accessed safely by multiple processes via existing XPCOM APIs (perhaps with some slight modifications). It is also probably performant enough that its data doesn't need to be cached in memory; and it supports triggers, which we could use to update a memory cache when data is changed by another process.

The mozilla-central implementation of the webapps registry uses JSON files, however, in order to enable B2G to provision a new profile with a default set of apps.  And those files have none of the multi-process safety of SQLite.

We need to make it possible for multiple processes to safely share a single registry while continuing to make it possible for B2G to provision new profiles.  I'm not sure exactly what that'll look like yet, but it might involve an SQLite datastore plus a set of flat files from which that registry is initialized on firstrun.

But that can be a different bug.

::: browser/locales/en-US/webapprt/webapp.properties
@@ +28,5 @@
> +# of the webapp.
> +webapps.install.title=%S - Install Web App
> +webapps.install.description=Do you want to install this web app?
> +webapps.install.install=Install Web App
> +webapps.install.dontinstall=Don't Install

Marketplace has presumably been through a naming exercise, and it refers to them as "apps" rather than "web apps", so we should do the same.

But note that Firefox's UI doesn't refer to them by either term, because it uses a more direct prompt that references the app by name:

  Do you want to install "[APP NAME]" from this site ([MARKETPLACE DOMAIN])?

With the options:

    Install
    Not Now

Unless we have a particular reason for doing it differently, we should copy Firefox's copy, for both the message and the title ("Install [APP NAME]").

::: webapprt/CommandLineHandler.js
@@ +25,5 @@
>                             []);
> +
> +    // Initialize window-independent handling of webapps- notifications
> +    Cu.import("resource://webapprt/modules/WebappsHandler.jsm");
> +    WebappsHandler.init();

Is there a particular reason why the caller initializes the WebappsHandler object?  Would a caller ever access the WebappsHandler.jsm module without initializing that object?  Or initialize it lazily?  It seems like the module should initialize the object itself, based on the current calling pattern.

::: webapprt/Makefile.in
@@ +39,5 @@
>    $(NULL)
>  
>  EXTRA_JS_MODULES = \
>    WebappRT.jsm \
> +  WebappsHandler.jsm \

Hmm, I wonder whether it would make more sense to simply add WebappsHandler.jsm's functionality to the existing (and somewhat generically-named) WebappRT.jsm module.

::: webapprt/WebappsHandler.jsm
@@ +20,5 @@
> +  
> +  uninit: function() {
> +    Services.obs.removeObserver(this, "webapps-ask-install");
> +    Services.obs.removeObserver(this, "webapps-launch");
> +  },

I don't see uninit() being called anywhere.  If it needs to happen, we need to call it from somewhere; otherwise, we should remove it.
Comment 21 Ed Lee :Mardak 2012-06-18 12:15:37 PDT
Created attachment 634135 [details] [diff] [review]
v2

(In reply to Myk Melez [:myk] [@mykmelez] from comment #20)
> We need to make it possible for multiple processes to safely share a single
> registry while continuing to make it possible for B2G to provision new
> profiles.
Filed bug 765865.

> ::: browser/locales/en-US/webapprt/webapp.properties
> But note that Firefox's UI doesn't refer to them by either term
Oh, I was referencing the Firefox UI strings file when making the strings, but I think I glanced too quickly and the "web app" reference was only for the localizer.

>   Do you want to install "[APP NAME]" from this site ([MARKETPLACE DOMAIN])?
Should this explicitly include the domain or is the situation different being in an app context where urls/domains are less prominent?

> both the message and the title ("Install [APP NAME]").
Right now the app name appears 3 times in this patch: title, description, button.

> add WebappsHandler.jsm's functionality to the existing WebappRT.jsm
Moved it inside, but eagerly importing some modules caused some oddness that I didn't fully track down. It happens when importing Webapps.jsm at the top level, immediately.

JS Component Loader: ERROR resource://gre/modules/FileUtils.jsm:62
                     NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get]
Comment 22 Ed Lee :Mardak 2012-06-18 12:16:49 PDT
Created attachment 634137 [details]
screenshot v2
Comment 23 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-18 12:53:25 PDT
That's probably caused by a module being instantiated during xpcom component registration (because it's imported by a JS component like the command line handler?), and trying to retrieve something from the directory service. Modules should generally avoid having code that does work on import().
Comment 24 Myk Melez [:myk] [@mykmelez] 2012-06-20 16:30:51 PDT
Comment on attachment 634135 [details] [diff] [review]
v2

(In reply to Edward Lee :Mardak from comment #21)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #20)
> >   Do you want to install "[APP NAME]" from this site ([MARKETPLACE DOMAIN])?
> Should this explicitly include the domain or is the situation different
> being in an app context where urls/domains are less prominent?

The situation is different, as users expect to be interacting with an app installed locally, not a website hosted on a domain; so this message should not explicitly include the domain of the app.


> > both the message and the title ("Install [APP NAME]").
> Right now the app name appears 3 times in this patch: title, description,
> button.

The title and description appearances are great!  But the one on the button is problematical, because buttons are click targets, and users benefit from consistent positioning of such targets.  Buttons are also less accommodating of long app names than the other two affordances (which are elided and wrapped, respectively).

So remove the name from the button while leaving it in the other two places.


> > add WebappsHandler.jsm's functionality to the existing WebappRT.jsm
> Moved it inside, but eagerly importing some modules caused some oddness that
> I didn't fully track down. It happens when importing Webapps.jsm at the top
> level, immediately.
> 
> JS Component Loader: ERROR resource://gre/modules/FileUtils.jsm:62
>                      NS_ERROR_FAILURE: Component returned failure code:
> 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get]

Erm, sorry, I gave you bad advice!

Making WebappRT.jsm import Webapps.jsm actually creates a circular dependency, because Webapps.jsm queries the directory service for the location of WebappRegD; the directory service accesses the runtime's directory provider (webapprt/DirectoryProvider.js) to resolve that key; and the provider imports WebappRT.jsm to get the path from its config.registryDir property.

Importing Webapps.jsm lazily works around the problem, but it still seems failure-prone to leave that dependency around, so let's go back to the original implementation, in which the handler is in a separate module.

(Note also some startup cleanup work in bug 766753, although I don't think it affects what we do here.)


Otherwise this looks great, r=myk with these two changes.
Comment 25 Ed Lee :Mardak 2012-06-20 22:28:10 PDT
Created attachment 635189 [details] [diff] [review]
v3

Additional r?felipe for the browser/modules/WebappsInstaller.jsm changes

Also, myk, any preference on the two buttons for install/don't install? Right now I have it as "Install App" and "Don't Install" partially because just "Install" would have a relatively small button compared to "Don't Install" which is highlighted by default.

Should it be "Install App" and "Cancel" to keep the positive action text larger?
Comment 26 Ed Lee :Mardak 2012-06-20 22:28:32 PDT
Created attachment 635190 [details]
screenshot v3
Comment 27 Myk Melez [:myk] [@mykmelez] 2012-06-21 11:42:46 PDT
Created attachment 635394 [details]
examples of prompts

(In reply to Edward Lee :Mardak from comment #25)
> Also, myk, any preference on the two buttons for install/don't install?
> Right now I have it as "Install App" and "Don't Install" partially because
> just "Install" would have a relatively small button compared to "Don't
> Install" which is highlighted by default.

Good question!  I took a look at several prompts--Install App in Firefox, Install Extension in Safari, and Allow Geolocation in both Firefox and Safari--for guidance.

Here's a combined screenshot showing those prompts, plus a prompt that appeared in my image editor when I tried to close one of the individual screenshots after making changes to it.

The Firefox prompts are not particularly instructive, as they are doorhangers that expose the negative action via a dropdown menu item.  But the Safari ones are modal dialog boxes that expose it via a button, and in both cases the negative-action button is larger than the positive-action one, regardless of which is the default button.

So I wouldn't worry about the relative sizes of these buttons.  Although I do see some value in making the positive-action button be a bit of a larger click target, so I'm fine with adding the word App to it.


> Should it be "Install App" and "Cancel" to keep the positive action text
> larger?

That image editor prompt reminded me that "Cancel" is often used as a third action when the prompt is about a side-effect of another action, f.e. you try to close a document that hasn't been saved, and the app prompts you to save the document before closing it, where Cancel means "don't close it after all."  So I would avoid using "Cancel" in this case.
Comment 28 Jason Smith [:jsmith] 2012-06-21 11:44:43 PDT
Myk - Could you upload that attachment in an image format? I can't read it from the browser.
Comment 29 Ed Lee :Mardak 2012-06-21 11:45:13 PDT
Comment on attachment 635394 [details]
examples of prompts

So used to clicking that patch checkbox! ;)
Comment 30 :Felipe Gomes (needinfo me!) 2012-06-21 12:00:14 PDT
Comment on attachment 635189 [details] [diff] [review]
v3

r=felipe for changes in WebappsInstaller.jsm
Comment 32 Ed Morley [:emorley] 2012-06-22 03:45:02 PDT
https://hg.mozilla.org/mozilla-central/rev/67326ea1c697
Comment 33 Jason Smith [:jsmith] 2012-06-26 17:29:26 PDT
Works on linux.
Comment 34 Jason Smith [:jsmith] 2012-06-26 17:38:52 PDT
Also works on Windows.
Comment 35 Jason Smith [:jsmith] 2012-06-26 17:43:43 PDT
And works on OS X. Verified!

Note You need to log in before you can comment on or make changes to this bug.