Closed Bug 761806 Opened 12 years ago Closed 12 years ago

Support webapp uninstallation on Linux

Categories

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

All
Linux
defect

Tracking

(firefox16 wontfix)

RESOLVED FIXED
Firefox 17
Tracking Status
firefox16 --- wontfix

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(1 file, 10 obsolete files)

Support uninstallation both with a desktop action (http://standards.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#extra-actions) and through a Firefox specific UI (maybe about:apps).
Summary: Support webapp uninstallation → Support webapp uninstallation on Linux
Priority: -- → P3
One of the beauty of Mozilla is its platform independent
MAC_OS/WINDOWS/*NIX same profile will work everywhere(just copy the profile folder if you dual boot or have many devices, please keep it that way)


But implementation of web apps seems to be breaking this
So i would request to have an options to disable this because i prefer cross platform profile usability over a web app, i don't want to sync profiles(using internet) to test stuff under same profile on various platforms


My advis would be to install all files in the profile directory called Web Apps/appname
so it is cross platform(can be launched from inside firefox if profile is shared cross platform[similar to addons])
(In reply to Pheonix from comment #1)

You should open another bug for your suggestion.
Marco: I assume you've considered the option of an implementation which creates either a .rpm or a .deb behind the scenes and then triggers the platform's package manager silently to do the install or uninstall?

That doesn't mean you couldn't have a Firefox-specific UI too. You just need to track which packages you have created and installed, and ask the package manager if they are still installed when you show the UI.

I think this would have significant advantages in terms of providing integration with platform uninstall mechanisms. It would also mean that, if you create the deb/rpm right, all the data files etc. would be installed in the correct places for the platform (one hidden directory per app in the user's root is not a great solution IMO).

Gerv
(In reply to Gervase Markham [:gerv] from comment #3)
> Marco: I assume you've considered the option of an implementation which
> creates either a .rpm or a .deb behind the scenes and then triggers the
> platform's package manager silently to do the install or uninstall?
> 
> That doesn't mean you couldn't have a Firefox-specific UI too. You just need
> to track which packages you have created and installed, and ask the package
> manager if they are still installed when you show the UI.

There are some problems with this approach, this is why we decided not to start with it.
Users would need root privileges to run package managers, even if we touch only user directories (because the package managers need to lock and use a protected database).
It's difficult to install to the home directory (but it's possible).
We can't support every package format (but we can support deb and rpm that are the most widespread).

> I think this would have significant advantages in terms of providing
> integration with platform uninstall mechanisms. It would also mean that, if
> you create the deb/rpm right, all the data files etc. would be installed in
> the correct places for the platform (one hidden directory per app in the
> user's root is not a great solution IMO).

We are installing the applications to the home directory so that every user has his own set of applications. So even with a .deb or .rpm, we would install to the home directory.
Anyway, the plan is to move the installation to XDG_CONFIG_HOME (~/.config).

I think the best solution for better integration is to create a skeleton addon for distributions that they can adapt to their own package system.
(In reply to Marco Castelluccio from comment #4)
> Users would need root privileges to run package managers, even if we touch
> only user directories (because the package managers need to lock and use a
> protected database).

There are APIs to do that without privileges (and the user is then asked to provide a root password, etc.). These APIs usually rely on dbus. The main problem is that, TTBOMK, there is no cross-distro API.
 
> So even with a .deb or .rpm, we would install to the home directory.

That is really not recommended to use deb or rpms for that.
(In reply to Mike Hommey [:glandium] from comment #5)
> There are APIs to do that without privileges (and the user is then asked to
> provide a root password, etc.). These APIs usually rely on dbus. The main
> problem is that, TTBOMK, there is no cross-distro API.

We could use PackageKit, I think it's supported by most distributions. (And if we make Firefox depend on PackageKit, I think every distribution will want to support it).
The problem is that users would however need to have root privileges. I know this is how currently apps installation on Linux works, but it's something that, in my opinion, needs to change to make Linux more user friendly.

> That is really not recommended to use deb or rpms for that.

Yes, it's one of the problems that I considered. It's difficult (and ugly) but feasible (for example, with DEBs, through the postinst script).
(In reply to Marco Castelluccio from comment #6)
> Yes, it's one of the problems that I considered. It's difficult (and ugly)
> but feasible (for example, with DEBs, through the postinst script).

More than ugly, it's perverting the use of packages. Don't do it. Ever.
I'm a big fan of package management systems but here I'm against using them:
- they are "sysadmin" controlled and system wide (and meant to be)
- they must never ever touch home directories IMHO (and for distributions I even think it's a rule; at least for openSUSE it is)
Yeah, I also think that packages here are not helpful since we are not installing apps system-wide.

For example, Firefox extension are not systemwide installed and the are store on user home too.
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Attachment #633240 - Flags: review?(karlt)
Curious (saw the patch) - What uninstall approach are you going with? Do you have a screenshot of what this visually does?
Attached image Screenshot (obsolete) —
This is only for platforms that support desktop actions.
(In reply to Marco Castelluccio from comment #12)
> Created attachment 633267 [details]
> Screenshot
> 
> This is only for platforms that support desktop actions.

Okay cool. What platforms do we know this approach will be supported? What platforms do we know this approach won't be supported? Why won't it be supported?
(In reply to Jason Smith [:jsmith] from comment #13)
> Okay cool. What platforms do we know this approach will be supported? What
> platforms do we know this approach won't be supported? Why won't it be
> supported?

AFAIK, only Unity supports them.
GNOME 3 not yet: https://bugzilla.gnome.org/show_bug.cgi?id=669603 and https://bugzilla.gnome.org/show_bug.cgi?id=664444

I think this will not be supported on "classic" DEs (KDE, XFCE, LXDE, etc.) because these desktop actions are used to build quicklists, that don't make much sense in a "classic" menu.
Comment on attachment 633240 [details] [diff] [review]
Patch

If this is for Unity only, then Chris would be a better reviewer.
Attachment #633240 - Flags: review?(karlt) → review?(chrisccoulson)
What is intended to happen to the profile directory inside this dir? Shouldn’t the user have an expectation that this will delete _everything_ installed from this webapp.
I think a user on linux expects the app to be removed but not the configuration or prefs for this app.
(In reply to Rubén Martín [:Nukeador] from comment #17)
> I think a user on linux expects the app to be removed but not the
> configuration or prefs for this app.

It would make sense to ask.
Currently the plan is not to remove the profile, also on Win and Mac.
(In reply to Rubén Martín [:Nukeador] from comment #17)
> I think a user on linux expects the app to be removed but not the
> configuration or prefs for this app.

OK, then I hope uninstallation could remove more, below is a list of what will be left behind.

removed
john@joran ~/.http;www.sandglaz.com > du -h profiles.ini webapp*
4.0K	profiles.ini
4.0K	webapp.ini
4.0K	webapp.json
160K	webapprt-stub

not removed
john@joran ~/.http;www.sandglaz.com > du -h --max-depth 2
76K	./22gut5tp.default/startupCache
1.3M	./22gut5tp.default/Cache
260K	./22gut5tp.default/OfflineCache
13M	./22gut5tp.default
13M	.

This seems to be a default profile size, as a few more examples I have are 

john@joran ~ > du -h --max-depth=1 .http\;jsucks7689.testmanifest.com/
12M	.http;jsucks7689.testmanifest.com/zp0lz89m.default
12M	.http;jsucks7689.testmanifest.com/

john@joran ~ > du -h --max-depth=1 .https\;marketplace-dev.allizom.org/
13M	.https;marketplace-dev.allizom.org/aude9rlm.default
14M	.https;marketplace-dev.allizom.org/

john@joran ~ > du -h --max-depth=1 .http\;www.sandglaz.com/
14M	.http;www.sandglaz.com/22gut5tp.default
14M	.http;www.sandglaz.com/

It’s mostly places.sqlite at ~10MB, is that a default init size? Could/should I file a bug to have this as a smaller default for webapps?

Anyhow, its a bit concerning to me that I could install and remove a collection of webapps and have all this wastage sat around.
(In reply to John Drinkwater (:beta) from comment #20)
> It’s mostly places.sqlite at ~10MB, is that a default init size?
> Could/should I file a bug to have this as a smaller default for webapps?

That's bug 762083, I'll try to get traction on that again
Comment on attachment 633240 [details] [diff] [review]
Patch

Myk, in the meantime you could review the webapprt part.
Attachment #633240 - Flags: review?(myk)
Comment on attachment 633240 [details] [diff] [review]
Patch

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

Note the many lines with whitespace issues:

06-26 15:52 > git apply ~/remove_webapps_linux 
git app/Users/myk/remove_webapps_linux:41: trailing whitespace.
const char kWEBAPP_JSON[] = "webapp.json";
/Users/myk/remove_webapps_linux:59: trailing whitespace.
void RemoveApplication(const char* curExeDir, const char* profile)  {
/Users/myk/remove_webapps_linux:60: trailing whitespace.
  // Remove the desktop entry file.
/Users/myk/remove_webapps_linux:61: trailing whitespace.
  char desktopEntryFilePath[MAXPATHLEN];
/Users/myk/remove_webapps_linux:62: trailing whitespace.

warning: squelched 41 whitespace errors
warning: 46 lines add whitespace errors.

Probably most of those are due to the line endings in webapprt.cpp being \r\n instead of merely \n.  I'm not sure why they're like that, as they shouldn't be, and that should've been caught in the initial review.  In any case, we should fix that, but we can do so in a separate bug.

::: browser/modules/WebappsInstaller.jsm
@@ +753,5 @@
>      writer.setString("Desktop Entry", "Icon", this.iconFile.path);
>      writer.setString("Desktop Entry", "Type", "Application");
>      writer.setString("Desktop Entry", "Terminal", "false");
> +    writer.setString("Desktop Entry", "Actions", "Remove");
> +    writer.setString("Desktop Action Remove", "Name", "Remove the application");

This needs to be localizable, i.e. stored in a .properties file (like browser/locales/en-US/chrome/browser/browser.properties) and accessed via Services.strings.

Also, does Linux call native apps "applications"?  We call them "apps" in the runtime itself, and I'm inclined to say that we should be consistent across platforms in this regard, although I'm open to the alternative.

::: webapprt/gtk2/webapprt.cpp
@@ +253,5 @@
> +  unlink(iconPath);
> +
> +  gtk_init(pargc, pargv);
> +
> +  GtkWidget* dialog = gtk_message_dialog_new(NULL, GTK_DIALOG_MODAL, GTK_MESSAGE_INFO, GTK_BUTTONS_OK, "Application correctly removed");

Is this what happens when a native app is removed?  What does the alert say in that case?  Displaying an alert seems ok, although a notification seems better, and the text could use wordsmithing.

At first glance, it should be something like: "[NAME] has been removed from your computer." But I'd really like to know what Linux says when you remove a native app, as that may influence it.
Attachment #633240 - Flags: review?(myk) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
(In reply to Myk Melez [:myk] [@mykmelez] from comment #23)> 
> Also, does Linux call native apps "applications"?  We call them "apps" in
> the runtime itself, and I'm inclined to say that we should be consistent
> across platforms in this regard, although I'm open to the alternative.

AFAIK there isn't a common way to call them across distributions.
However, there is a GNOME application to configure startup applications that is called "Startup Applications". So I think we can use the name "Applications".

> Is this what happens when a native app is removed?  What does the alert say
> in that case?  Displaying an alert seems ok, although a notification seems
> better, and the text could use wordsmithing.
> 
> At first glance, it should be something like: "[NAME] has been removed from
> your computer." But I'd really like to know what Linux says when you remove
> a native app, as that may influence it.

On Linux distributions, you usually don't have a notification after an uninstallation.
Attachment #633240 - Attachment is obsolete: true
Attachment #633240 - Flags: review?(chrisccoulson)
Attachment #637836 - Flags: review?(myk)
Attachment #637836 - Flags: review?(chrisccoulson)
Comment on attachment 637836 [details] [diff] [review]
Patch v2

Close!

The only thing left is to make the uninstall notification string localizable (which I missed in my first review, sorry!), as it is a normal part of the app experience and should be localized just as app installation and usage strings are localized.

(I know we don't localize the error messages in the stub.  Firefox's stub contains some unlocalized error messages too.  But in this case the string is part of the normal user experience, not an exceptional case, and thus warrants localization.  And we'll tackle those error messages too in time.)

The Windows uninstaller uses a separate .properties file:

  http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/webapp-uninstaller/webapp-uninstaller.properties

whose strings are then written to an NSIS configuration file:

  http://mxr.mozilla.org/mozilla-central/source/webapprt/win/webapp-uninstaller.nsi.in

from which the Windows uninstaller executable reads them.

We could do something similar, but a simpler approach that seems sufficient (especially given that you're reusing the stub to do uninstallation instead of creating a separate executable, as on Windows) is to add the string to the existing browser/locales/en-US/chrome/browser/browser.properties file, alongside the webapp installation strings, and write it (after replacing %S in it with the name of the app) to the webapp.ini file during installation.  Then the stub can retrieve it from that file when called with -remove.

Requesting feedback from Pike on this approach.
Attachment #637836 - Flags: review?(myk)
Attachment #637836 - Flags: review-
Attachment #637836 - Flags: feedback?(l10n)
We actually have a bunch of plain ini files in the l10n sources, http://mxr.mozilla.org/mozilla-central/find?string=\.ini%24&tree=mozilla-central&hint=locales%2Fen-US.

So you could just go for plain ini files, if that helps.
Attachment #637836 - Flags: feedback?(l10n)
(In reply to Axel Hecht [:Pike] from comment #26)
> We actually have a bunch of plain ini files in the l10n sources,
> http://mxr.mozilla.org/mozilla-central/find?string=\.ini%24&tree=mozilla-
> central&hint=locales%2Fen-US.
> 
> So you could just go for plain ini files, if that helps.

What would you prefer, this or the solution suggested by Myk?
I don't have a strong preference, but it's usually a good idea to have the source format match the destination format as close as possible, so that we can't trigger bugs between the original source format, and source-based checks on those, and the final deliverable, by a mismatch between what the tools on the source side assume, and what the compiler between source and target is able to port.

So in the end, it mostly depends on what you're intending to be reading at runtime. If we have that format supported by the l10n infrastructure right now, use that. Also, try to re-use as much parsing code as we currently using, as we have creative bugs in our existing parsers that we occasionally declare features. Example, we had a bug in our properties parser that it handled .properties files as utf-8 instead of mac roman whatnot-java. And at some point, we just declared that to be a feature, so no our gecko parser is incompatible with what other tools expect .properties to be.
QA Contact: jsmith
Depends on: 771405
#7067
This has nothing to do with bug 771405.
No longer depends on: 771405
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #633267 - Attachment is obsolete: true
Attachment #637836 - Attachment is obsolete: true
Attachment #637836 - Flags: review?(chrisccoulson)
Attachment #639654 - Flags: review?(myk)
Attachment #639654 - Flags: review?(chrisccoulson)
Comment on attachment 639654 [details] [diff] [review]
Patch v3

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

Looks great, modulo a few nits. r=myk

Note: I can't help thinking that it'd be even better to write a separate uninstaller executable to the app dir, like we do on Windows.  But that shouldn't stop us from implementing this good solution.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +356,5 @@
>  webapps.install = Install
>  webapps.install.accesskey = I
>  #LOCALIZATION NOTE (webapps.requestInstall) %1$S is the web app name, %2$S is the site from which the web app is installed
>  webapps.requestInstall = Do you want to install "%1$S" from this site (%2$S)?
> +webapps.uninstall = %S has been removed from your computer.

Nit: this could use a localization note, i.e.:

# LOCALIZATION NOTE (webapps.uninstall): %S will be replaced with the name
# of the webapp being uninstalled.

Also, it isn't clear why this confirmation text is called webapps.uninstall while the button label is called webapps.remove.  I would call these something like webapps.uninstall.confirm and webapps.uninstall.button, respectively.

::: browser/modules/WebappsInstaller.jsm
@@ +780,3 @@
>      writer.setString("Webapp", "Name", this.appName);
>      writer.setString("Webapp", "Profile", this.uniqueName);
> +    writer.setString("WebappRT", "RemoveMsg", bundle.formatStringFromName("webapps.uninstall", [this.appName], 1));

Nit: since the message is specific to the app, it would make more sense in the [Webapp] section of the INI file.

@@ +785,5 @@
>  
>      // $XDG_DATA_HOME/applications/owa-<webappuniquename>.desktop
>      this.desktopINI.create(Ci.nsIFile.NORMAL_FILE_TYPE, 0755);
>  
> +    bundle = Services.strings.createBundle("chrome://webapprt/locale/webapp.properties");

Nit: reassigning a variable like this is prone to cause problems down the road when someone unfamiliar with the code tries to modify it with noticing the reassignment.  I'd assign each bundle to its own variable (f.e. browserBundle and webappBundle).
Attachment #639654 - Flags: review?(myk) → review+
Attached patch Patch v4 (obsolete) — Splinter Review
I've called the quicklist label "webapps.uninstall.label" and the notification text "webapps.uninstall.notification".
Attachment #639654 - Attachment is obsolete: true
Attachment #639654 - Flags: review?(chrisccoulson)
Attachment #641781 - Flags: review?(myk)
Attachment #641781 - Flags: review?(chrisccoulson)
Comment on attachment 641781 [details] [diff] [review]
Patch v4

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

Looks good, r=myk!

::: webapprt/gtk2/webapprt.cpp
@@ +304,5 @@
>      return 255;
>    }
>  
> +  // Check if the application has to be removed.
> +  for (int i=1; i<argc; i++)

Nit: add space around "=" and "<", i.e.:

  for (int i = 1; i < argc; i++)
Attachment #641781 - Flags: review?(myk) → review+
(In reply to Myk Melez [:myk] [@mykmelez] from comment #34)
> Nit: add space around "=" and "<", i.e.:
> 
>   for (int i = 1; i < argc; i++)

I always forget the spaces around operators in for cycles :D

Chris, do you have time to review this before Firefox 16 becomes Aurora?
Attached patch Patch v5 (obsolete) — Splinter Review
Carrying r+ from Myk.
Attachment #641781 - Attachment is obsolete: true
Attachment #641781 - Flags: review?(chrisccoulson)
Attachment #642118 - Flags: review?(chrisccoulson)
Attached patch Patch v6 (obsolete) — Splinter Review
I had to modify a bit the patch (just the for cycle to check for the "-remove" argument), because of the landing of bug 770772.

Myk, if your review for the code changes in webapprt.cpp is complete, we can land this.
I can deal with Chris's feedback for WebappsInstaller.jsm with follow-ups (as the patch contains string changes, it would be difficult to uplift to Aurora).
Attachment #642118 - Attachment is obsolete: true
Attachment #642118 - Flags: review?(chrisccoulson)
Attachment #642448 - Flags: review?(myk)
It's a really simple change. We will probably move the code to WebappOSUtils.jsm as soon as it will be present.
Attachment #642449 - Flags: review?(felipc)
(In reply to Marco Castelluccio from comment #38)
> Created attachment 642449 [details] [diff] [review]
> Support native removal from about:apps
> 
> It's a really simple change. We will probably move the code to
> WebappOSUtils.jsm as soon as it will be present.

Suggestion - I'd file a new bug for this and put that patch there, as this sounds more appropriate in a followup bug IMO.
(In reply to Jason Smith [:jsmith] from comment #39)
> (In reply to Marco Castelluccio from comment #38)
> > Created attachment 642449 [details] [diff] [review]
> > Support native removal from about:apps
> > 
> > It's a really simple change. We will probably move the code to
> > WebappOSUtils.jsm as soon as it will be present.
> 
> Suggestion - I'd file a new bug for this and put that patch there, as this
> sounds more appropriate in a followup bug IMO.

Also - for clarification for wording in the bug:

Should be "Allow for native uninstall of an app through uninstall() in the mozapps API." The about:apps reference just so happens to occur because that's what about:apps is using, but the patch actually isn't directly changing what about:apps does. The actual change is at the mozapps API level.
Blocks: 774142
Attachment #642449 - Attachment is obsolete: true
Attachment #642449 - Flags: review?(felipc)
Marco, I pushed the patch to tryserver but it failed with an error:  (click unwrap^ to make lines readable)

make -C webapprt libs
make[5]: Entering directory `/builds/slave/try-lnx-dbg/build/obj-firefox/webapprt'
make -C gtk2 libs
/builds/slave/try-lnx-dbg/build/obj-firefox/config/nsinstall -D ../dist/bin/webapprt/defaults/preferences
mkdir -p ../dist/bin/webapprt/chrome/
/builds/slave/try-lnx-dbg/build/obj-firefox/_virtualenv/bin/python /builds/slave/try-lnx-dbg/build/config/Preprocessor.py -DMOZ_GLUE_IN_PROGRAM -DMOZ_DEBUG=1 -DMOZILLA_OFFICIAL -DGRE_MILESTONE=16.0a1 -DGRE_BUILDID=20120715230847 -DMOZ_APP_BASENAME=Firefox  /builds/slave/try-lnx-dbg/build/webapprt/application.ini.in > webapprt.ini
make[6]: Entering directory `/builds/slave/try-lnx-dbg/build/obj-firefox/webapprt/gtk2'
webapprt.cpp
/usr/bin/ccache /tools/gcc-4.5-0moz3/bin/g++ -o webapprt.o -c  -I../../dist/system_wrappers -include /builds/slave/try-lnx-dbg/build/config/gcc_hidden.h -DXPCOM_GLUE -DMOZ_GLUE_IN_PROGRAM -DGRE_BUILDID=20120715230847 -I/builds/slave/try-lnx-dbg/build/toolkit/xre -I/builds/slave/try-lnx-dbg/build/xpcom/base -I/builds/slave/try-lnx-dbg/build/xpcom/build -I../../build -I/builds/slave/try-lnx-dbg/build/webapprt/gtk2 -I. -I../../dist/include  -I/builds/slave/try-lnx-dbg/build/obj-firefox/dist/include/nspr -I/builds/slave/try-lnx-dbg/build/obj-firefox/dist/include/nss      -fPIC  -pedantic -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wcast-align -Wno-long-long -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -std=gnu++0x -pthread -pipe  -DDEBUG -D_DEBUG -DTRACING -g -Os -freorder-blocks -finline-limit=50 -fno-omit-frame-pointer  -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/gtk-unix-print-2.0   -I/usr/include/gtk-2.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/dbus-1.0 -I/usr/lib/dbus-1.0/include -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/freetype2 -I/usr/include/libpng12      -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MF .deps/webapprt.o.pp /builds/slave/try-lnx-dbg/build/webapprt/gtk2/webapprt.cpp
../../../webapprt/gtk2/webapprt.cpp: In function 'void RemoveApplication(nsINIParser&, const char*, const char*)':
../../../webapprt/gtk2/webapprt.cpp:270:98: error: too few arguments to function 'NotifyNotification* notify_notification_new(const gchar*, const gchar*, const gchar*, GtkWidget*)'
/usr/include/libnotify/notification.h:86:21: note: declared here
make[6]: *** [webapprt.o] Error 1
make[6]: Leaving directory `/builds/slave/try-lnx-dbg/build/obj-firefox/webapprt/gtk2'
Attached patch Patch v7 (obsolete) — Splinter Review
Looks like we use an older version of libnotify. This should fix the problem.
https://tbpl.mozilla.org/?tree=Try&rev=284debda507d
Attachment #642448 - Attachment is obsolete: true
Attachment #642448 - Flags: review?(myk)
Attachment #642515 - Flags: review?(myk)
Comment on attachment 642515 [details] [diff] [review]
Patch v7

>diff --git a/webapprt/gtk2/Makefile.in b/webapprt/gtk2/Makefile.in
>--- a/webapprt/gtk2/Makefile.in
>+++ b/webapprt/gtk2/Makefile.in
>@@ -20,20 +20,22 @@ LOCAL_INCLUDES += -I$(topsrcdir)/xpcom/b
> LOCAL_INCLUDES += -I$(DEPTH)/build
> 
> DEFINES += -DXPCOM_GLUE
> STL_FLAGS=
> 
> LIBS = \
>   $(XPCOM_STANDALONE_GLUE_LDOPTS) \
>   $(MOZ_GTK2_LIBS) \
>+  $(MOZ_LIBNOTIFY_LIBS) \

This will mean that webapprt-stub will not run if it does not find the same version of libnotify as it is built against.
libnotify does not have stable API and so is not suitable for binary distributions.

Would it be practical to check for the nsIAlertsService XPCOM service?
At least the program would run then, even if the notification does not show (until bug 750141 is fixed).

Alternatives are dlopen or dbus, but it would seem preferable to do this only once instead of having to do it again for nsIAlertsService.

Perhaps we should disable libnotify support by default (for all of Gecko).  Then distros can turn it on and Mozilla builds at least won't be harmed by it.
(In reply to Karl Tomlinson (:karlt) from comment #43)
> Would it be practical to check for the nsIAlertsService XPCOM service?
> At least the program would run then, even if the notification does not show
> (until bug 750141 is fixed).

I'm trying to figure out if the XPCOM service is usable before calling XRE_main, no positive results so far.
I'm using this code, after loading XPCOM:
> nsCOMPtr<nsIAlertsService> sysAlerts(do_GetService("@mozilla.org/system-alerts-service;1"));
But the service isn't created.
I expect XPCOM needs to be initialized, but that may not require XRE_main.

xpcshell might be a simpler xpcom user for reference.
Much of the code there is used only for specific option arguments.
http://hg.mozilla.org/mozilla-central/annotate/d78729026fb9/js/xpconnect/shell/xpcshell.cpp#l1666

(I don't really know the best solution here.  Just trying to think of ideas.)
Comment on attachment 642515 [details] [diff] [review]
Patch v7

(In reply to Marco Castelluccio from comment #44)
> (In reply to Karl Tomlinson (:karlt) from comment #43)
> > Would it be practical to check for the nsIAlertsService XPCOM service?
> > At least the program would run then, even if the notification does not show
> > (until bug 750141 is fixed).
> 
> I'm trying to figure out if the XPCOM service is usable before calling
> XRE_main, no positive results so far.
> I'm using this code, after loading XPCOM:
> > nsCOMPtr<nsIAlertsService> sysAlerts(do_GetService("@mozilla.org/system-alerts-service;1"));
> But the service isn't created.

I'm far from the expert here, but AFAICT you can't get the alerts service in webapprt.cpp because the component manager isn't available yet.

Perhaps bsmedberg has some insight; cc:ing him.

bsmedberg: this patch adds a command-line option to the Linux webapprt stub executable for uninstalling the app.  Upon uninstallation, we intend for the stub to notify the user that the app has been uninstalled via a notification, and the question at hand is how to enable the stub to display that notification.
Attachment #642515 - Flags: feedback?(benjamin)
Who would be calling this command line flag? Could the caller provide the UI?

In order to show localized UI, you'd need to start XPCOM (probably without a profile in this case) in order to load the strings and then show the dialog. You could also write native GTK UI code for this, but you'd still need to deal with the localization somehow.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #47)
> Who would be calling this command line flag? Could the caller provide the UI?
> 
> In order to show localized UI, you'd need to start XPCOM (probably without a
> profile in this case) in order to load the strings and then show the dialog.
> You could also write native GTK UI code for this, but you'd still need to
> deal with the localization somehow.

I'm reading the strings from an INI file, I needed XPCOM to use nsIAlertsService.
I'm going to try to use NS_InitXPCOM2 like in http://mxr.mozilla.org/mozilla-central/source/xpcom/sample/program/nsTestSample.cpp
Comment on attachment 642515 [details] [diff] [review]
Patch v7

As for this patch, the macro NOTIFY_CHECK_VERSION usage is not acceptable: we need this to build and work against whatever binary version of libnotify we require at runtime, and so we should require the correct version.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #49)
> Comment on attachment 642515 [details] [diff] [review]
> Patch v7
> 
> As for this patch, the macro NOTIFY_CHECK_VERSION usage is not acceptable:
> we need this to build and work against whatever binary version of libnotify
> we require at runtime, and so we should require the correct version.

This is why we wanted to use nsIAlertsService (see comment 43). Do you think it's feasible?
Comment on attachment 642515 [details] [diff] [review]
Patch v7

I'm away on PTO.
Attachment #642515 - Flags: feedback?(benjamin)
Blocks: 780528
Blocks: 780530
Attached patch Patch v8 (obsolete) — Splinter Review
I decided to open two new bugs about the desktop action and the notification, so this patch is simpler, doesn't contain new strings and so could be backported to Aurora.
Attachment #642515 - Attachment is obsolete: true
Attachment #642515 - Flags: review?(myk)
Attachment #649156 - Flags: review?(myk)
Comment on attachment 649156 [details] [diff] [review]
Patch v8

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

(In reply to Marco Castelluccio from comment #52)
> Created attachment 649156 [details] [diff] [review]
> Patch v8
> 
> I decided to open two new bugs about the desktop action and the
> notification, so this patch is simpler, doesn't contain new strings and so
> could be backported to Aurora.

Removing the strings does make it easier to backport, and it's fine to split the work across several bugs, but it isn't clear what value this change provides without the desktop action.  The command-line option it adds is internal, not a user-facing feature, so it doesn't provide a way to actually uninstall an app.

With that in mind, the code looks pretty good, just a few minor issues.

::: webapprt/gtk2/webapprt.cpp
@@ +226,5 @@
>  
>    ErrorDialog("Couldn't execute the new webapprt-stub executable");
>  }
>  
> +void RemoveApplication(nsINIParser& parser, const char* curExeDir, const char* profile)  {

RemoveApplication no longer uses `parser`, so that parameter should be removed from its list.

@@ +238,5 @@
> +    else {
> +      char* home = getenv("HOME");
> +      snprintf(desktopEntryFilePath, MAXPATHLEN, "%s/.local/share/applications/owa-%s.desktop", home, profile);
> +    }
> +  }

Nit: simple "if/else" conditionals like this one are typically easier to read when the positive assertion comes first (i.e. it starts with |if (isProfileOverridden)|).

@@ +240,5 @@
> +      snprintf(desktopEntryFilePath, MAXPATHLEN, "%s/.local/share/applications/owa-%s.desktop", home, profile);
> +    }
> +  }
> +
> +  unlink(desktopEntryFilePath);

If the profile is not overridden, but XDG_DATA_HOME is not defined, then desktopEntryFilePath will not be initialized.  This code should take that into account.
Attachment #649156 - Flags: review?(myk) → review-
Attached patch Patch v9Splinter Review
(In reply to Myk Melez [:myk] [@mykmelez] from comment #53)
> Removing the strings does make it easier to backport, and it's fine to split
> the work across several bugs, but it isn't clear what value this change
> provides without the desktop action.  The command-line option it adds is
> internal, not a user-facing feature, so it doesn't provide a way to actually
> uninstall an app.

If we backport also the patch in bug 774142, users will have the opportunity to remove the applications through mozApps (and so about:apps or a web dashboard). The desktop action is only for Unity (it's the only desktop environment currently supporting them).
Attachment #649156 - Attachment is obsolete: true
Attachment #649834 - Flags: review?(myk)
Attachment #649834 - Flags: review?(myk) → review+
Keywords: checkin-needed
Keywords: checkin-needed
Target Milestone: --- → Firefox 17
https://hg.mozilla.org/mozilla-central/rev/1631dacc1066
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 649834 [details] [diff] [review]
Patch v9

[Approval Request Comment]
User impact if declined: Users won't be able to automatically uninstall applications.
Testing completed (on m-c, etc.): On central for some days.
Risk to taking this patch (and alternatives if risky): No risk.
String or UUID changes made by this patch: None.

We need also the patch in bug 774142.
Attachment #649834 - Flags: approval-mozilla-aurora?
Comment on attachment 649834 [details] [diff] [review]
Patch v9

This is already wontfixed for 16. It can ride the trains as desktop webapps aren't a focus for Firefox 16 release.
Attachment #649834 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Is there any progress? In trying to uninstall on Linux, I'm unable to find the files installed to remove them. (looking in "/home/user/.mozilla/firefox/ff_version/webapps" only shows two ".json" files, neither linking to any folders with data. I'm also unable to find where the icon and ".desktop" files would be.
(In reply to donrhummy from comment #59)
> Is there any progress? In trying to uninstall on Linux, I'm unable to find
> the files installed to remove them. (looking in
> "/home/user/.mozilla/firefox/ff_version/webapps" only shows two ".json"
> files, neither linking to any folders with data. I'm also unable to find
> where the icon and ".desktop" files would be.

If anyone is looking for an answer to this.
The desktop entries are in ~./local/share/applications. They will be like owa-appname-hash.desktop.
the webapps can be uninstalled by following the command given in the last line of desktop entry. 
It will be something like ~/.appname-hash/webapprt-stub -remove
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.