Marco Castelluccio [:marco] Assignee
	Description • 13 years ago

I'm working on this, I'd need GIO support if possible.

	Marco Castelluccio [:marco] Assignee
	Comment 1 • 13 years ago

I've got something working, but I want to improve the code before posting it.

	Jason Smith [:jsmith]
	Comment 2 • 13 years ago

FYI - More test cases for testing are here - https://apps.mozillalabs.com/appdir/?db=db/apps-preview.json

	Benjamin Smedberg
	Comment 4 • 13 years ago

Comment on attachment 622500 [details] [diff] [review]
Patch

>diff --git a/webapprt/Makefile.in b/webapprt/Makefile.in
>--- a/webapprt/Makefile.in
>+++ b/webapprt/Makefile.in
>@@ -18,16 +18,20 @@ include $(topsrcdir)/config/config.mk
> # into a WebappRT-specific subdirectory, so we redefine it here.
> FINAL_TARGET = $(DIST)/bin/webapprt
> 
> ifneq (,$(filter WINNT,$(OS_ARCH)))
> DIRS += win
> else
> ifeq ($(OS_ARCH),Darwin)
> DIRS += mac
>+else
>+ifeq ($(OS_ARCH),Linux)

Is this the right test? In particular, we also compile Linux-Qt, but I'm kinda presuming that the code here is tied to GTK-style desktops and not Qt-style desktops, correct? If that's the case, shouldn't this be based on the toolkit and as much the OS?

>diff --git a/webapprt/linux/webapprt.cpp b/webapprt/linux/webapprt.cpp

>+// Function to open a dialog box displaying the message provided.
>+void ErrorDialog(const char* message) {

For this function and many others below: please put the opening brace in column 0 of the next line.

>+bool GRELoadAndLaunch(const char* firefoxDir, const char* profile) {
>+  char* xpcomDllPath = g_build_filename(firefoxDir, XPCOM_DLL, NULL);

I personally have no experience with these g_ functions, so this will need review by someone who does.

>+  // NOTE: The GRE has successfully loaded, so we can use XPCOM now
>+  NS_LogInit();

Please use ScopedLogging for this, otherwise the various error cases here will apparently leak (not a big deal, but we should avoid it if we can).

>+bool CopyAndRelaunch(const char* firefoxDir, const char* curExeDir) {

>+  const char *argv[] = {kAPP_RT, NULL};
>+  if (!g_spawn_async(curExeDir, (char**)argv, NULL, (GSpawnFlags)0, NULL, NULL, NULL, &err)) {

Why is this launching a new process instead of exec'ing into the existing process? I can't think of a reason we'd need a new process here.

>+int main(int argc, char *argv[]) {
>+  g_type_init();
>+  gtk_init(&argc, &argv);
>+
>+  pargc = &argc;
>+  pargv = &argv;
>+
>+  // Current executable path
>+  char curExe[MAXPATHLEN];
>+  if (readlink("/proc/self/exe", curExe, MAXPATHLEN) == -1) {
>+    ErrorDialog("Couldn't read current executable path");
>+    return 255;
>+  }
>+
>+  char* curExeDir = g_path_get_dirname(curExe);
>+
>+  // Set up webAppIniPath with path to webapp.ini.
>+  char* webAppIniPath = g_build_filename(curExeDir, kWEBAPP_INI, NULL);
>+
>+  // Open webapp.ini as an INI file
>+  nsINIParser parser;
>+  if (NS_FAILED(parser.Init(webAppIniPath))) {
>+    ErrorDialog("Couldn't open webapp.ini");
>+    g_free(webAppIniPath);
>+    g_free(curExeDir);

There's a lot of this (g_free in failure cases)... if we really care about freeing memory in this case (and I'm not sure it matters), we should use a scoped C++ class to hold these instead of manually freeing them. The codepaths will be a lot easier to read. You could try to use Scoped.h to create an autofree class for g_things, but you could also just write a simple one here.

>+  if (NS_FAILED(parser.GetString("WebappRT", "InstallDir", firefoxDir, MAXPATHLEN))) {
>+    // TODO: We could save in GSettings the Firefox installation directory, so we have another way to find its directory other than InstallDir in webapp.ini
>+    ErrorDialog("This app requires that Firefox version 15 or above is installed. Firefox 15+ hasn't been detected.");

This is a strange error message for this case, since it doesn't seem to actually check the Firefox version here...

In general this looks fine, but should be reviewed by somebody who knows the g_ functions.

	Marco Castelluccio [:marco] Assignee
	Comment 5 • 13 years ago

> Is this the right test? In particular, we also compile Linux-Qt, but I'm
> kinda presuming that the code here is tied to GTK-style desktops and not
> Qt-style desktops, correct? If that's the case, shouldn't this be based on
> the toolkit and as much the OS?

I've updated the patch to compile on Linux only for the gtk2 version.

> I personally have no experience with these g_ functions, so this will need
> review by someone who does.

I've decided to avoid using glib. It's only a useless dependency. Do you agree?

I've also addressed your other comments :)

	:Felipe Gomes (needinfo for replies!)
	Comment 6 • 12 years ago

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

Hi Karl, would you be able to review this for us? Benjamin already give feedback+ on the approach and said he is fine with someone else doing the final review, but he will be unable to get to it any time soon.


What this patch does is to implement the webapp runtime, which is a small binary that is copied into the webapps' folders, and when ran it locates the original Firefox folder and loads libxul from there.

After libxul is loaded it reads these prefs to load its xul window:
http://mxr.mozilla.org/mozilla-central/source/webapprt/prefs.js#5

For reference, the Win/Mac counterparts were implemented in bug 725408 and live in
http://mxr.mozilla.org/mozilla-central/source/webapprt/win/webapprt.cpp
http://mxr.mozilla.org/mozilla-central/source/webapprt/mac/webapprt.mm

	Mike Hommey [:glandium]
	Comment 7 • 12 years ago

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

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

::: webapprt/linux/Makefile.in
@@ +15,5 @@
> +CPPSRCS = webapprt.cpp
> +
> +# Don't create a dependency on mozglue, which is impossible (difficult?)
> +# to dynamically link into our executable, as we copy it to arbitrary locations.
> +MOZ_GLUE_LDFLAGS =

You don't want to do that. mozglue is not dynamically linked, on linux, it's statically linked, so it's not a problem. Not linking mozglue actually leaves out jemalloc, which is likely to break storage and about:memory (while the latter is probably not going to be used in webapps, the former is another story). If it doesn't break them now, it will certainly do when switching to jemalloc3.

	Johnny Stenback (:jst)
	Comment 8 • 12 years ago

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

- In CopyFile(const char* inputFile, const char* outputFile):

+  if (bytesRead < 0)
+    return false;
+
+  return true;

Maybe simply make those 3 lines be "return bytesRead >= 0"?

- In CopyAndRelaunch(const char* firefoxDir, const char* curExePath):

+  execv(curExePath, *pargv);
+  return true;

This method returns false on all failures, except at the end of the method, which we'll hit only if execv() fails. Seems that last return should be false too. That of course means this method always return false, which argues for not even returning a value, IOW this could be a void method, and if it ever returns (i.e. all cases except when execv() succeeds), then we hit an error. Up to you whether you want to make the return type change there or not, I can deal either way.

- In main(int argc, char *argv[]):

+  if (strcmp(buildid, NS_STRINGIFY(GRE_BUILDID))) {
+    if (CopyAndRelaunch(firefoxDir, curExePath))
+      return 0;

We'll never get to that return, per above comment.

r=jst with that fixed, this looks good to me. I'd love to have karlt and/or someone from the webapps team give this a look too though, if they have cycles.

	Johnny Stenback (:jst)
	Comment 9 • 12 years ago

And we need Mike Hommey's comment dealt with here too before landing this, of course.

	Myk Melez [:myk] [@mykmelez]
	Comment 10 • 12 years ago

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

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

I'll be that reviewer "from the webapps team." :-)

Overall this looks great, very close.  I only have a couple of small issues...

::: webapprt/linux/webapprt.cpp
@@ +151,5 @@
> +      ErrorDialog("Couldn't load the runtime INI");
> +      return false;
> +    }
> +
> +    if (!rtINI)

!rtINI.exists() is what you want, I think.

@@ +152,5 @@
> +      return false;
> +    }
> +
> +    if (!rtINI)
> +      return false;

This error should get an error dialog too.

@@ +164,5 @@
> +    SetAllocatedString(webShellAppData->profile, profile);
> +
> +    nsCOMPtr<nsILocalFile> directory;
> +    if (NS_FAILED(XRE_GetFileFromPath(rtPath, getter_AddRefs(directory)))) {
> +      ErrorDialog("Couldn't open app dir");

Nit: app dir -> runtime dir

@@ +178,5 @@
> +    xreDir.forget(&webShellAppData->xreDirectory);
> +    NS_IF_RELEASE(webShellAppData->directory);
> +    directory.forget(&webShellAppData->directory);
> +
> +    XRE_main(*pargc, *pargv, webShellAppData);

The build fails at this point because the call is missing an argument for XRE_main's fourth parameter, aFlags <http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#3936>.  The Windows and Mac stubs both pass 0.

@@ +275,5 @@
> +  if (strcmp(buildid, NS_STRINGIFY(GRE_BUILDID))) {
> +    if (CopyAndRelaunch(firefoxDir, curExePath))
> +      return 0;
> +  }
> +  // Else if WebAppRT version >= Firefox version, load XUL and execute the application

The build IDs have to match exactly, so this should be "WebappRT version == Firefox version".

@@ +279,5 @@
> +  // Else if WebAppRT version >= Firefox version, load XUL and execute the application
> +  else if (GRELoadAndLaunch(firefoxDir, profile))
> +    return 0;
> +
> +  ErrorDialog("This app requires that Firefox version 15 or above is installed. Firefox 15+ hasn't been detected."); 

If I understand this code correctly, most errors in GRELoadAndLaunch (except for NS_FAILED(XPCOMGlueStartup(xpcomDllPath)) and NS_FAILED(XPCOMGlueLoadXULFunctions(kXULFuncs))) are going to cause two dialogs to appear, one describing the specific problem, and this one saying that the app can't find a compatible Firefox.

But this message will be incorrect in many cases (since it appears no matter what the failure in GRELoadAndLaunch), and two error dialogs in a row is at least one too many!  The Mac and Windows stubs only report that Firefox is incompatible when they have reason to believe that it is.  And even so, they don't actually check Firefox's version, so referencing a specific version in the error message, which the Windows stub does, is error-prone.

Thus drop this error dialog, but add one each to the first two failures in GRELoadAndLaunch, so all failure conditions in that function result in an error message that is specific to the failure being encountered.

(Note to self: find a better way to report such problems across all three OSes.)

	Johnny Stenback (:jst)
	Comment 12 • 12 years ago

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

r=jst, this looks great! I verified that all the feedback on this patch so far has been incorporated, and as far as I can tell it's all been dealt with. I think we're good to land this here, I don't think myk needs to re-review this (unless he wants to). And while I welcome karlt's feedback here, I don't know that it's necessary in order to land this (leaving his request in place in case he has cycles real soon to look here).

	Mike Hommey [:glandium]
	Comment 13 • 12 years ago

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

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

::: webapprt/linux/Makefile.in
@@ +16,5 @@
> +
> +# mozglue uses mfbt's Assertions.cpp, which provides MOZ_ASSERT, which lots
> +# of code in libxpcom uses, so we have to do the same.
> +VPATH += $(topsrcdir)/mfbt
> +CPPSRCS += Assertions.cpp

This shouldn't be needed (in fact, I'm surprised it builds at all)

	Mike Hommey [:glandium]
	Comment 14 • 12 years ago

(In reply to Mike Hommey [:glandium] from comment #13)
> Comment on attachment 629421 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 629421 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: webapprt/linux/Makefile.in
> @@ +16,5 @@
> > +
> > +# mozglue uses mfbt's Assertions.cpp, which provides MOZ_ASSERT, which lots
> > +# of code in libxpcom uses, so we have to do the same.
> > +VPATH += $(topsrcdir)/mfbt
> > +CPPSRCS += Assertions.cpp
> 
> This shouldn't be needed (in fact, I'm surprised it builds at all)

Let me detail why that is: mozglue includes mfbt, so linking with mozglue (which building a linux binary does) will already add all mfbt.

	Marco Castelluccio [:marco] Assignee
	Comment 15 • 12 years ago

Thanks Johnny and Mike!

	:Felipe Gomes (needinfo for replies!)
	Comment 16 • 12 years ago

The change to configure.in had a problem with the Linux Qt build because it had an extra "test" in the line. I fixed it and added parens around the Linux+gtk condition but I couldn't get the parens for the bash's test command.. so I changed the line a little bit to use MOZ_WIDGET_TOOLKIT only instead of a combination of it + OS_TARGET.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ae2ca987cf9

	:Felipe Gomes (needinfo for replies!)
	Comment 17 • 12 years ago

Comment on attachment 629634 [details] [diff] [review]
Patch v4

Karl: I landed the patch with Johnny's review, but you're welcome to take a look over if you want and suggest follow-ups.

	(no longer active)
	Comment 18 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/0ae2ca987cf9

	Jason Smith [:jsmith]
	Comment 19 • 12 years ago

For testing - I'll do a quick check for verifying that this has landed for this bug and bug 745018 by doing proof of concept testing for Ubuntu 11. Then, I'll throw up some blog posts and notifications to try to crowd source some of the testing here. For formalized test cases, see the following doc for test cases currently under development:

https://docs.google.com/spreadsheet/ccc?key=0AiZoGR-iOAlUdDdfMHo5aTI2WjVIeWxnNWxvV0ZVOWc

And formalized ones are here if you filter by apps and Desktop Firefox 15:

https://moztrap.mozilla.org/manage/cases/

Feel free to enhance these test cases with your own ideas for general testing areas and test cases specific to the linux implementation. Ping me if you would like to help testing this feature and I'll get you access to the doc and moztrap. Marco - Do you need access to that doc and MozTrap? Anybody else interested in helping the testing here?

	Jason Smith [:jsmith]
	Comment 20 • 12 years ago

Verified that this indeed has landed on inbound. I did very bare bones testing to ensure that the feature has indeed landed (e.g. install apps, launch apps, use apps) using smoke tests from comment 50. At the smoke test level, the only major issue identified was bug 761496, although we can track this in a separate bug (I usually mark these verified if I confirm the implementation has landed and a proof of concept works).

When the nightly build containing the patch is ready, I'll throw up some blog posts and advertising to encourage testers to start hammering this build. Note that the testing done here is bare bones, not extensive, so there's still more testing to be done here.

An open question I think that needs to be addressed from a UX perspective - How would a linux user know how to remove their web apps from their machine? I didn't manage to find the location of where the app was installed after doing some searching, although I might be looking in the wrong spot. I'm fine with just relying on the user to do a direct deletion of the app, but we need to make sure that the user can easily find and remove the app if they want it removed. Another follow-up thing to think about is integration with existing services - such as the Ubuntu Software Center. Is is something to consider?

These are just ideas - Feel free to think about this more. But overall, nice job on this.

	Marco Castelluccio [:marco] Assignee
	Comment 21 • 12 years ago

(In reply to Felipe Gomes (:felipe) from comment #16)
> The change to configure.in had a problem with the Linux Qt build because it
> had an extra "test" in the line. I fixed it and added parens around the
> Linux+gtk condition but I couldn't get the parens for the bash's test
> command.. so I changed the line a little bit to use MOZ_WIDGET_TOOLKIT only
> instead of a combination of it + OS_TARGET.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0ae2ca987cf9

I think this caused bug 761516, we should set MOZ_WEBAPPS_RUNTIME to 1 only for Linux.

	Marco Castelluccio [:marco] Assignee
	Comment 22 • 12 years ago

(In reply to Jason Smith [:jsmith] from comment #20)
> Verified that this indeed has landed on inbound. I did very bare bones
> testing to ensure that the feature has indeed landed (e.g. install apps,
> launch apps, use apps) using smoke tests from comment 50. At the smoke test
> level, the only major issue identified was bug 761496, although we can track
> this in a separate bug (I usually mark these verified if I confirm the
> implementation has landed and a proof of concept works).

I think this is a Unity issue, I'll investigate as soon as possible.
(On my PC, something similar happens for Firefox custom builds and the installed version)

> When the nightly build containing the patch is ready, I'll throw up some
> blog posts and advertising to encourage testers to start hammering this
> build. Note that the testing done here is bare bones, not extensive, so
> there's still more testing to be done here.
> 
> An open question I think that needs to be addressed from a UX perspective -
> How would a linux user know how to remove their web apps from their machine?
> I didn't manage to find the location of where the app was installed after
> doing some searching, although I might be looking in the wrong spot. I'm
> fine with just relying on the user to do a direct deletion of the app, but
> we need to make sure that the user can easily find and remove the app if
> they want it removed.

The uninstallation isn't implemented yet. However to remove the application from the system you can remove the directory under ~/ and the desktop entry file under ~/.local/share/applications. The name of the dir and the file is: https://mxr.mozilla.org/mozilla-central/source/browser/modules/WebappsInstaller.jsm#655

> Another follow-up thing to think about is integration
> with existing services - such as the Ubuntu Software Center. Is is something
> to consider?
> 
> These are just ideas - Feel free to think about this more. But overall, nice
> job on this.

I don't think we can integrate with the Ubuntu Software Centre or other services like this. As far as I know, there isn't any standard way to do this.
The most important thing to do to integrate better the applications is categorization (bug 760748).
And another thing we could do is to add the installed applications to the Unity Launcher and the GNOME Shell Dash (https://live.gnome.org/GnomeShell/Design#Dash), so that they are better discoverable. But there isn't a standard way to do this. (Maybe we could add this feature to the specific addons that the distributions ship by default with Firefox).

	Jason Smith [:jsmith]
	Comment 23 • 12 years ago

(In reply to Marco Castelluccio from comment #22)
> > When the nightly build containing the patch is ready, I'll throw up some
> > blog posts and advertising to encourage testers to start hammering this
> > build. Note that the testing done here is bare bones, not extensive, so
> > there's still more testing to be done here.
> > 
> > An open question I think that needs to be addressed from a UX perspective -
> > How would a linux user know how to remove their web apps from their machine?
> > I didn't manage to find the location of where the app was installed after
> > doing some searching, although I might be looking in the wrong spot. I'm
> > fine with just relying on the user to do a direct deletion of the app, but
> > we need to make sure that the user can easily find and remove the app if
> > they want it removed.
> 
> The uninstallation isn't implemented yet. However to remove the application
> from the system you can remove the directory under ~/ and the desktop entry
> file under ~/.local/share/applications. The name of the dir and the file is:
> https://mxr.mozilla.org/mozilla-central/source/browser/modules/
> WebappsInstaller.jsm#655

Can you document this somewhere for now? Maybe on your wiki?

Also, could you get a bug on file for the uninstall implementation?

> 
> > Another follow-up thing to think about is integration
> > with existing services - such as the Ubuntu Software Center. Is is something
> > to consider?
> > 
> > These are just ideas - Feel free to think about this more. But overall, nice
> > job on this.
> 
> I don't think we can integrate with the Ubuntu Software Centre or other
> services like this. As far as I know, there isn't any standard way to do
> this.
> The most important thing to do to integrate better the applications is
> categorization (bug 760748).
> And another thing we could do is to add the installed applications to the
> Unity Launcher and the GNOME Shell Dash
> (https://live.gnome.org/GnomeShell/Design#Dash), so that they are better
> discoverable. But there isn't a standard way to do this. (Maybe we could add
> this feature to the specific addons that the distributions ship by default
> with Firefox).

Would this only help with the install flow? Or would this also help the user discover how to uninstall an app manually from their machine? From my perspective, the install flow is in alright shape at the bare bones level, although improvements never hurt. The uninstall flow is the piece I think needs work initially.

	Marco Castelluccio [:marco] Assignee
	Comment 24 • 12 years ago

(In reply to Jason Smith [:jsmith] from comment #23)
> Can you document this somewhere for now? Maybe on your wiki?
> 
> Also, could you get a bug on file for the uninstall implementation?

I'll document this in my next weekly report.
I filed bug 761806.

> Would this only help with the install flow? Or would this also help the user
> discover how to uninstall an app manually from their machine? From my
> perspective, the install flow is in alright shape at the bare bones level,
> although improvements never hurt. The uninstall flow is the piece I think
> needs work initially.

Yes, this would help only with the install flow. Sadly I don't see a way to improve the uninstall flow.

	Jason Smith [:jsmith]
	Comment 25 • 12 years ago

We need test cases in our manual test suite for Linux. Flagging as such.

	Marco Castelluccio [:marco] Assignee
	Comment 26 • 12 years ago

(In reply to Jason Smith [:jsmith] from comment #25)
> We need test cases in our manual test suite for Linux. Flagging as such.

I've added Linux in the "Launch a Free Native Application from shortcut" test case.

	Thomasy
	Comment 27 • 12 years ago

I did find application at ~/.local/share/applications/ (and can execute ) but not in  Applications menu for Lubuntu 12.04 .