Last Comment Bug 780530 - Notification after webapp uninstallation
: Notification after webapp uninstallation
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Webapp Runtime (show other bugs)
: Trunk
: All Linux
: -- normal
: Firefox 17
Assigned To: Marco Castelluccio [:marco] (PTO until August 24/25)
: Jason Smith [:jsmith]
Mentors:
Depends on: 761806 783847 787971
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-05 17:33 PDT by Marco Castelluccio [:marco] (PTO until August 24/25)
Modified: 2016-03-21 12:39 PDT (History)
2 users (show)
ryanvm: in‑testsuite-
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (6.27 KB, patch)
2012-08-16 17:47 PDT, Marco Castelluccio [:marco] (PTO until August 24/25)
mh+mozilla: review-
Details | Diff | Splinter Review
Patch v2 (6.40 KB, patch)
2012-08-17 08:04 PDT, Marco Castelluccio [:marco] (PTO until August 24/25)
mh+mozilla: review+
Details | Diff | Splinter Review
Patch v2 with comment (6.69 KB, patch)
2012-08-17 10:08 PDT, Marco Castelluccio [:marco] (PTO until August 24/25)
mh+mozilla: review+
Details | Diff | Splinter Review
For landing (6.69 KB, patch)
2012-08-17 10:27 PDT, Marco Castelluccio [:marco] (PTO until August 24/25)
no flags Details | Diff | Splinter Review

Description Marco Castelluccio [:marco] (PTO until August 24/25) 2012-08-05 17:33:18 PDT

    
Comment 1 Marco Castelluccio [:marco] (PTO until August 24/25) 2012-08-16 17:47:02 PDT
Created attachment 652635 [details] [diff] [review]
Patch

http://upstream-tracker.org/versions/libnotify.html
Comment 2 Mike Hommey [:glandium] 2012-08-16 22:25:13 PDT
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);
Comment 3 Marco Castelluccio [:marco] (PTO until August 24/25) 2012-08-17 08:04:10 PDT
Created attachment 652768 [details] [diff] [review]
Patch v2
Comment 4 Mike Hommey [:glandium] 2012-08-17 08:19:37 PDT
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 =.
Comment 5 Marco Castelluccio [:marco] (PTO until August 24/25) 2012-08-17 10:08:59 PDT
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.
Comment 6 Mike Hommey [:glandium] 2012-08-17 10:22:37 PDT
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.
Comment 7 Marco Castelluccio [:marco] (PTO until August 24/25) 2012-08-17 10:27:31 PDT
Created attachment 652819 [details] [diff] [review]
For landing
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-08-18 16:19:39 PDT
https://hg.mozilla.org/mozilla-central/rev/50dd132b8440

Note You need to log in before you can comment on or make changes to this bug.