763786 - Allow launching of natively installed webapps from launch() method in mozapps API - Windows

Status: RESOLVED DUPLICATE of bug 772600

(Firefox Graveyard :: Web Apps, defect, P1)

Description

[Jason Smith [:jsmith]]

Similar to bug 763740, but for windows. This should allow users to launch apps from the launch() method, so that dashboards (such as the new tab up and coming) can launch the native web runtime shell.

Comment 1

[Jason Smith [:jsmith]]

Nominating for k9o - prerequsite bug needed to be implemented to support launching of native web apps on windows.

Comment 2

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v1

This patch is totally untested (might erase your hard drive).

The idea is:
  Add a "handled" flag that can be set to true by any OS-specific logic that attempts to launch a natively-installed webapp
  On Windows, do the following:
    Check for an uninstall reg key for the specified app
    Use the "InstallLocation" and "AppFilename" values to build up a path to the EXE
    Launch with `nsIProcess`
    Set "handled" flag to `true`
    If any of the above fail, catch the exception and give up on native launch
  The original codepath will execute if and only if the "handled" flag is `false` after OS-specific logic runs

Comment 3

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v2

Turns out there was very little needed to get the v1 patch working.  Patch v1 used an undeclared variable `id`.  Replaced that with `msg.origin` which is what was intended.

Comment 4

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Dan, Marco: Since you guys are writing the Mac and Linux patches, respectively, how do you feel about using the `handled` flag to indicate whether the original (non-OS-specific) codepath should run?

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 637315 [details] [diff] [review]
Patch v2

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

I'm starting to really dislike what Webapps.jsm is looking like, with huge #ifdef sections. What about moving windows/mac/linux specific code into .jsms and load them lazily when needed?

That could look like:
XPCOMUtils.defineLazyGetter(this, "webappsUtils", function() {
#ifdef XP_WIN
  Cu.import("resource://gre/modules/webappsWinUtils.jsm");
  return webappsUtils;
#endif
#ifdef XP_MACOSX
  Cu.import("resource://gre/modules/webappsMacUtils.jsm");
  return webappsUtils;
#endif
  webappsUtils = { ... }; // dummy implementation for non-native platforms
  return webappsUtils;
});

and then we only have to call one lazy loading function.

::: dom/apps/src/Webapps.jsm
@@ +121,5 @@
>          break;
>        case "Webapps:Launch":
> +        let handled = false;
> +#ifdef XP_WIN
> +        let uninstallKey;

Nit: rename this, we're not uninstalling anything there

@@ +150,5 @@
> +          }
> +        }
> +#endif
> +        if (!handled) {
> +          Services.obs.notifyObservers(this, "webapps-launch", JSON.stringify(msg));

So, if something fails in the try{} block, you will notify observers even on Windows. Is it what we want?

Comment 6

[Ed Lee :Mardak]

(In reply to Fabrice Desré [:fabrice] from comment #5)
> I'm starting to really dislike what Webapps.jsm is looking like, with huge
> #ifdef sections.
Yeah, there are similar comments in bug 763789 comment 2 and 3. Another unclean part is that these dom/..Webapps changes are depending on browser/..Installer implementation, so the code should probably be consolidated into core.

Comment 7

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v3

(In reply to Fabrice Desré [:fabrice] from comment #5)
> Comment on attachment 637315 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 637315 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm starting to really dislike what Webapps.jsm is looking like, with huge
> #ifdef sections. What about moving windows/mac/linux specific code into
> .jsms and load them lazily when needed?
> 
> That could look like:
> XPCOMUtils.defineLazyGetter(this, "webappsUtils", function() {
> #ifdef XP_WIN
>   Cu.import("resource://gre/modules/webappsWinUtils.jsm");
>   return webappsUtils;
> #endif
> #ifdef XP_MACOSX
>   Cu.import("resource://gre/modules/webappsMacUtils.jsm");
>   return webappsUtils;
> #endif
>   webappsUtils = { ... }; // dummy implementation for non-native platforms
>   return webappsUtils;
> });
> 
> and then we only have to call one lazy loading function.

Do you mind if we do this cleanup as a followup?  Maxim Zhilyaev has also expressed interest in removing the `#ifdef`s (see the 3rd comment in bug 763740 and the 2nd comment in bug 763789), but doing so now requires changing three patches while doing the cleanup as a followup requires only one patch and gets the needed functionality landed sooner.

> ::: dom/apps/src/Webapps.jsm
> @@ +121,5 @@
> >          break;
> >        case "Webapps:Launch":
> > +        let handled = false;
> > +#ifdef XP_WIN
> > +        let uninstallKey;
> 
> Nit: rename this, we're not uninstalling anything there

Renamed to `appRegKey`. Let me know if this is acceptable.

> @@ +150,5 @@
> > +          }
> > +        }
> > +#endif
> > +        if (!handled) {
> > +          Services.obs.notifyObservers(this, "webapps-launch", JSON.stringify(msg));
> 
> So, if something fails in the try{} block, you will notify observers even on
> Windows. Is it what we want?

My understanding is that, if the app fails to launch natively, we should notify observers so that the app launches in a pinned tab.  Is this accurate?

Comment 8

[Dan Walkowski]

I had always intended to move the #ifdefs out of there, but was waiting for the other patches to be complete.  Feel free to clean it up however you prefer, Tim, or let me know, and I can do it.

Comment 9

[Marco Castelluccio [:marco]]

(In reply to Tim Abraldes from comment #7)
> My understanding is that, if the app fails to launch natively, we should
> notify observers so that the app launches in a pinned tab.  Is this accurate?

If we want this behaviour I agree with you about using the 'handled' flag.
Otherwise we should run that code only on unsupported platforms, and so the flag becomes useless.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Marco Castelluccio from comment #9)
> (In reply to Tim Abraldes from comment #7)
> > My understanding is that, if the app fails to launch natively, we should
> > notify observers so that the app launches in a pinned tab.  Is this accurate?
> 
> If we want this behaviour I agree with you about using the 'handled' flag.
> Otherwise we should run that code only on unsupported platforms, and so the
> flag becomes useless.

Do we really want to launch a pinned tab if we fail to launch? Shouldn't we just report an error back? AITC I think may deal with apps in this situation (the acquired apps not natively installed in on a machine).

Comment 11

[Dan Walkowski]

I don't think we want to launch to a pinned tab (ever, in Firefox), OR report an error.
Apps are going to be launched by using native methods (ie: double click it on the desktop), or by using a sanctioned dashboard in the Firefox, which will not make the 'mistake' of trying to launch an uninstalled app.  It will have two piles, displayed however it likes:  'installed' apps, and 'uninstalled' apps.  Clicking an uninstalled app does not launch it, it -installs- it. (likely with user confirmation)

Comment 12

[Jason Smith [:jsmith]]

(In reply to Dan Walkowski from comment #11)
> I don't think we want to launch to a pinned tab (ever, in Firefox), OR
> report an error.
> Apps are going to be launched by using native methods (ie: double click it
> on the desktop), or by using a sanctioned dashboard in the Firefox, which
> will not make the 'mistake' of trying to launch an uninstalled app.  It will
> have two piles, displayed however it likes:  'installed' apps, and
> 'uninstalled' apps.  Clicking an uninstalled app does not launch it, it
> -installs- it. (likely with user confirmation)

Right, that makes sense. That sounds like a use case for the "acquired" apps but not installed we've described in previous bugs.

Comment 13

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 637563 [details] [diff] [review]
Patch v3

This was discussed in IRC, and the outcomes of the discussion are:
  1) We should never launch apps in pinned tabs on platforms that support native installation
  2) If `mozApps.launch()` detects that the natively-installed app has failed to launch, it should somehow notify its consumers that a failure has occurred

I'll update the patch to fulfill these requirements.

Comment 14

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v4

This patch no longer launches apps in pinned tabs on Windows.  Also, Webapps.jsm has been split into 3 files:
  WebappsWin.jsm: Contains Windows-specific logic. Loads WebappsBase.jsm and augments it with Windows-only behavior.
  WebappsBase.jsm: Contains the base implementation of the mozApps API. Should be loaded by platform-specific JSMs or by the browser on platforms where native installation is not supported.
  Webapps.jsm: Selects the appropriate platform-specific implementation to load.

Fabrice:
  Do you approve of the organization of OS-specific code presented in this patch?
  What mechanism should we use to notify consumers when a failure occurs while trying to launch? (Should we create a "Webapps:Launch:Return:KO" message or similar?)

Comment 15

[:Felipe Gomes (needinfo for replies!)]

I think the best way to split the code is to simply have a helper module in toolkit/content that will have OS-specific app behavior, similar to nsMacWebUtils but written in JS. And then Webapps.jsm would just call that function, e.g.:

in Webapps.jsm:

 Cu.import("resource://gre/modules/WebAppOSUtils.jsm");

 launch: function(args) {
   WebAppOSUtils.launch(args);
 }

and thus the platform-specific logic would be encased in functions instead of overriding a base implementation. What do you think?


WebAppOSUtils.jsm could be just one file so you always include it and the ifdef parts per-platform are inside it (instead of choosing which file to include via ifdefs)

Comment 16

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Felipe Gomes (:felipe) from comment #15)
> I think the best way to split the code is to simply have a helper module in
> toolkit/content that will have OS-specific app behavior, similar to
> nsMacWebUtils but written in JS. And then Webapps.jsm would just call that
> function, e.g.:
> 
> in Webapps.jsm:
> 
>  Cu.import("resource://gre/modules/WebAppOSUtils.jsm");
> 
>  launch: function(args) {
>    WebAppOSUtils.launch(args);
>  }
> 
> and thus the platform-specific logic would be encased in functions instead
> of overriding a base implementation. What do you think?
> 
> 
> WebAppOSUtils.jsm could be just one file so you always include it and the
> ifdef parts per-platform are inside it (instead of choosing which file to
> include via ifdefs)

I feel like we would just be moving the big blocks of `#ifdef`s from one file (Webapps.jsm) to another (WebappOSUtils.jsm).  Is this really better than leaving the `#ifdef`s where they are?

Additionally, Webapps.jsm would start to look a little funny; some functions would be implemented in Webapps.jsm while others would be implemented as `WebappOSUtils.actualImplementation(args)`.

Comment 17

[Marco Castelluccio [:marco]]

We should also consider that there could be some common code between different platform implementations (just like in the installer and in the runtime).

Comment 18

[:Felipe Gomes (needinfo for replies!)]

(In reply to Tim Abraldes from comment #16)
> Additionally, Webapps.jsm would start to look a little funny; some functions
> would be implemented in Webapps.jsm while others would be implemented as
> `WebappOSUtils.actualImplementation(args)`.

But note that this would not be an arbitrary split. Usually code inside dom/ should not have any platform-specific code, as that folder is meant for the platform-independent bits related to web content. Note how there's APIs definition in dom/ like sms but the actual implementation goes into hal/. (or other similar examples like widget/)

So the functions that would be implemented elsewhere are the ones that are clearly platform related, like launching or finding an app. And code that is related to the app registry itself stays there

Comment 19

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch for merging into 772600

Instead of writing 3 separate patches for the 3 separate OSes, we're going to put all the platform-specific logic into a patch in bug 772600.

The attached patch will be used to write the Windows part of the launch implementation in bug 772600.

Comment 20

[:Felipe Gomes (needinfo for replies!)]

Fixed at bug 772600