Closed Bug 763786 Opened 8 years ago Closed 8 years ago

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

Categories

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

All
Windows 7
defect

Tracking

(blocking-kilimanjaro:+)

RESOLVED DUPLICATE of bug 772600
Firefox 16
blocking-kilimanjaro +

People

(Reporter: jsmith, Assigned: TimAbraldes)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Depends on: 756306
Blocks: 745924
blocking-kilimanjaro: --- → ?
Nominating for k9o - prerequsite bug needed to be implemented to support launching of native web apps on windows.
blocking-kilimanjaro: ? → +
Priority: -- → P1
Assignee: nobody → tabraldes
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
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
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #637304 - Attachment is obsolete: true
Attachment #637315 - Flags: review?(felipc)
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 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?
(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.
Attached patch Patch v3 (obsolete) — Splinter Review
(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?
Attachment #637315 - Attachment is obsolete: true
Attachment #637315 - Flags: review?(felipc)
Attachment #637563 - Flags: review?(fabrice)
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.
(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.
(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).
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)
(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 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.
Attachment #637563 - Flags: review?(fabrice)
Attached patch Patch v4Splinter Review
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?)
Attachment #637563 - Attachment is obsolete: true
Attachment #638518 - Flags: feedback?(fabrice)
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)
(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)`.
We should also consider that there could be some common code between different platform implementations (just like in the installer and in the runtime).
QA Contact: jsmith
(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
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.
Attachment #638518 - Flags: feedback?(fabrice)
Fixed at bug 772600
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Resolution: FIXED → DUPLICATE
Duplicate of bug: 772600
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.