Notification after webapp uninstallation

RESOLVED FIXED in Firefox 17

Status

Firefox Graveyard
Webapp Runtime
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: marco, Assigned: marco)

Tracking

Trunk
Firefox 17
All
Linux
Dependency tree / graph
Bug Flags:
in-testsuite -

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 652635 [details] [diff] [review]
Patch

http://upstream-tracker.org/versions/libnotify.html
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Attachment #652635 - Flags: review?(mh+mozilla)
Comment on attachment 652635 [details] [diff] [review]
Patch

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

::: webapprt/gtk2/webapprt.cpp
@@ +272,5 @@
> +    typedef void  (*notify_init_t)(char*);
> +    typedef void* (*notify_notification_new_t)(char*, char*, char*, char*);
> +    typedef void  (*notify_notification_show_t)(void*, char*);
> +
> +    void *handle= dlopen("libnotify.so", RTLD_LAZY);

You need to open libnotify.so.4, and libnotify.so.1 if that fails.

@@ +288,5 @@
> +    nn_init(appName);
> +
> +    void* n = nn_new(removeMsg, NULL, "dialog-information", NULL);
> +
> +    nn_show(n, NULL);

dlclose(handle);
Attachment #652635 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 3

5 years ago
Created attachment 652768 [details] [diff] [review]
Patch v2
Attachment #652635 - Attachment is obsolete: true
Attachment #652768 - Flags: review?(mh+mozilla)
Comment on attachment 652768 [details] [diff] [review]
Patch v2

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

Please add a comment somewhere about the fact that the only difference between libnotify.so.4 and libnotify.so.1 for the symbols we use is in the definition of notify_notification_new, which takes 3 char* in one and 4 in the other (I don't remember which is which, but it would be good to tell), and that always passing 4 with the 4th being NULL is binary compatible with the one that takes 3.

Please also get review from someone else for the string addition. No idea who would be suitable, though.

::: webapprt/gtk2/webapprt.cpp
@@ +267,5 @@
> +    strcpy(appName, profile);
> +  }
> +
> +  char uninstallMsg[MAXPATHLEN];
> +  if (!NS_FAILED(parser.GetString("Webapp", "UninstallMsg", uninstallMsg, MAXPATHLEN))) {

NS_SUCCEEDED()

@@ +272,5 @@
> +    typedef void  (*notify_init_t)(char*);
> +    typedef void* (*notify_notification_new_t)(char*, char*, char*, char*);
> +    typedef void  (*notify_notification_show_t)(void*, char*);
> +
> +    void *handle= dlopen("libnotify.so.4", RTLD_LAZY);

whitespace before the =.
Attachment #652768 - Flags: review?(mh+mozilla)
Attachment #652768 - Flags: review?
Attachment #652768 - Flags: review+
(Assignee)

Comment 5

5 years ago
Created attachment 652808 [details] [diff] [review]
Patch v2 with comment

I've already got a review from Myk in another bug for the string addition.
Attachment #652768 - Attachment is obsolete: true
Attachment #652768 - Flags: review?
Attachment #652808 - Flags: review?(mh+mozilla)
Comment on attachment 652808 [details] [diff] [review]
Patch v2 with comment

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

You didn't really need another review from me :)

::: webapprt/gtk2/webapprt.cpp
@@ +267,5 @@
> +  char uninstallMsg[MAXPATHLEN];
> +  if (NS_SUCCEEDED(parser.GetString("Webapp", "UninstallMsg", uninstallMsg, MAXPATHLEN))) {
> +    /**
> +     * The only difference between libnotify.so.4 and libnotify.so.1 for these symbols
> +     * is that notify_notification_new takes three arguments in libnotify.so.4 and 

trailing whitespace here.
Attachment #652808 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 652819 [details] [diff] [review]
For landing
Attachment #652808 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Green on Try.
https://tbpl.mozilla.org/?tree=Try&rev=e01235322203

https://hg.mozilla.org/integration/mozilla-inbound/rev/50dd132b8440
Flags: in-testsuite-
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/50dd132b8440
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Depends on: 783847

Updated

5 years ago
Depends on: 787971
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.