Closed
Bug 761806
Opened 12 years ago
Closed 12 years ago
Support webapp uninstallation on Linux
Categories
(Firefox Graveyard :: Web Apps, defect, P3)
Tracking
(firefox16 wontfix)
RESOLVED
FIXED
Firefox 17
Tracking | Status | |
---|---|---|
firefox16 | --- | wontfix |
People
(Reporter: marco, Assigned: marco)
References
Details
Attachments
(1 file, 10 obsolete files)
4.72 KB,
patch
|
myk
:
review+
lsblakk
:
approval-mozilla-aurora-
myk
:
checkin+
|
Details | Diff | Splinter Review |
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).
Updated•12 years ago
|
Summary: Support webapp uninstallation → Support webapp uninstallation on Linux
Updated•12 years ago
|
Priority: -- → P3
Comment 1•12 years ago
|
||
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])
Assignee | ||
Comment 2•12 years ago
|
||
(In reply to Pheonix from comment #1) You should open another bug for your suggestion.
Comment 3•12 years ago
|
||
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
Assignee | ||
Comment 4•12 years ago
|
||
(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.
Comment 5•12 years ago
|
||
(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.
Assignee | ||
Comment 6•12 years ago
|
||
(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).
Comment 7•12 years ago
|
||
(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.
Comment 8•12 years ago
|
||
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)
Comment 9•12 years ago
|
||
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.
Assignee | ||
Comment 10•12 years ago
|
||
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Assignee | ||
Updated•12 years ago
|
Attachment #633240 -
Flags: review?(karlt)
Comment 11•12 years ago
|
||
Curious (saw the patch) - What uninstall approach are you going with? Do you have a screenshot of what this visually does?
Assignee | ||
Comment 12•12 years ago
|
||
This is only for platforms that support desktop actions.
Comment 13•12 years ago
|
||
(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?
Assignee | ||
Comment 14•12 years ago
|
||
(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 15•12 years ago
|
||
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)
Comment 16•12 years ago
|
||
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.
Comment 17•12 years ago
|
||
I think a user on linux expects the app to be removed but not the configuration or prefs for this app.
Comment 18•12 years ago
|
||
(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.
Assignee | ||
Comment 19•12 years ago
|
||
Currently the plan is not to remove the profile, also on Win and Mac.
Comment 20•12 years ago
|
||
(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.
Comment 21•12 years ago
|
||
(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
Assignee | ||
Comment 22•12 years ago
|
||
Comment on attachment 633240 [details] [diff] [review] Patch Myk, in the meantime you could review the webapprt part.
Attachment #633240 -
Flags: review?(myk)
Comment 23•12 years ago
|
||
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-
Assignee | ||
Comment 24•12 years ago
|
||
(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 25•12 years ago
|
||
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)
Comment 26•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #637836 -
Flags: feedback?(l10n)
Assignee | ||
Comment 27•12 years ago
|
||
(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?
Comment 28•12 years ago
|
||
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.
Updated•12 years ago
|
QA Contact: jsmith
Comment 29•12 years ago
|
||
#7067
Assignee | ||
Comment 31•12 years ago
|
||
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 32•12 years ago
|
||
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+
Assignee | ||
Comment 33•12 years ago
|
||
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 34•12 years ago
|
||
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+
Assignee | ||
Comment 35•12 years ago
|
||
(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?
Assignee | ||
Comment 36•12 years ago
|
||
Carrying r+ from Myk.
Attachment #641781 -
Attachment is obsolete: true
Attachment #641781 -
Flags: review?(chrisccoulson)
Attachment #642118 -
Flags: review?(chrisccoulson)
Assignee | ||
Comment 37•12 years ago
|
||
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)
Assignee | ||
Comment 38•12 years ago
|
||
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)
Comment 39•12 years ago
|
||
(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.
Comment 40•12 years ago
|
||
(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.
Assignee | ||
Updated•12 years ago
|
Attachment #642449 -
Attachment is obsolete: true
Attachment #642449 -
Flags: review?(felipc)
Comment 41•12 years ago
|
||
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'
Assignee | ||
Comment 42•12 years ago
|
||
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 43•12 years ago
|
||
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.
Updated•12 years ago
|
status-firefox16:
--- → wontfix
Assignee | ||
Comment 44•12 years ago
|
||
(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.
Comment 45•12 years ago
|
||
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 46•12 years ago
|
||
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)
Comment 47•12 years ago
|
||
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.
Assignee | ||
Comment 48•12 years ago
|
||
(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 49•12 years ago
|
||
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.
Assignee | ||
Comment 50•12 years ago
|
||
(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 51•12 years ago
|
||
Comment on attachment 642515 [details] [diff] [review] Patch v7 I'm away on PTO.
Attachment #642515 -
Flags: feedback?(benjamin)
Assignee | ||
Comment 52•12 years ago
|
||
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 53•12 years ago
|
||
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-
Assignee | ||
Comment 54•12 years ago
|
||
(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)
Updated•12 years ago
|
Attachment #649834 -
Flags: review?(myk) → review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 55•12 years ago
|
||
Comment on attachment 649834 [details] [diff] [review] Patch v9 https://hg.mozilla.org/integration/mozilla-inbound/rev/1631dacc1066
Attachment #649834 -
Flags: checkin+
Updated•12 years ago
|
Keywords: checkin-needed
Target Milestone: --- → Firefox 17
Comment 56•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1631dacc1066
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 57•12 years ago
|
||
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 58•12 years ago
|
||
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-
Comment 59•11 years ago
|
||
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.
Comment 60•9 years ago
|
||
(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
Updated•8 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•