Use libnotify as a dynamic library

RESOLVED FIXED in mozilla17

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: marco, Assigned: marco)

Tracking

Trunk
mozilla17
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Posted patch Patch (obsolete) — Splinter Review
This is a simple fix for the libnotify problem, while we wait for bug 404738/bug 750141.
Attachment #653047 - Attachment is patch: true
Attachment #653047 - Flags: review?(mh+mozilla)
Comment on attachment 653047 [details] [diff] [review]
Patch

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

r-ing just because I want to see next iteration.

::: configure.in
@@ +4950,4 @@
>      then
>          if test "$MOZ_ENABLE_LIBNOTIFY"
>          then
>              PKG_CHECK_MODULES(MOZ_LIBNOTIFY, libnotify >= $LIBNOTIFY_VERSION)

Please also get rid of the PKG_CHECK_MODULES. I'm tempted to say also drop the configure option, which would always enable libnotify, but I'm not sure we want to go there just yet. Karl, what do you think?

::: toolkit/system/gnome/nsAlertsIconListener.cpp
@@ +63,5 @@
> +  notify_notification_set_icon_from_pixbuf = (notify_notification_set_icon_from_pixbuf_t)dlsym(libNotifyHandle, "notify_notification_set_icon_from_pixbuf");
> +  notify_notification_add_action = (notify_notification_add_action_t)dlsym(libNotifyHandle, "notify_notification_add_action");
> +  if (!notify_is_initted || !notify_init || !notify_get_server_caps || !notify_notification_new || !notify_notification_show || !notify_notification_set_icon_from_pixbuf || !notify_notification_add_action) {
> +    dlclose(libNotifyHandle);
> +    libNotifyHandle = 0;

nullptr instead of 0

@@ +199,5 @@
>  
>  nsresult
>  nsAlertsIconListener::ShowAlert(GdkPixbuf* aPixbuf)
>  {
> +  mNotification = notify_notification_new(mAlertTitle.get(), mAlertText.get(), NULL, NULL);

You should add a guard on libNotifyHandle.

::: toolkit/system/gnome/nsAlertsIconListener.h
@@ +29,5 @@
> +typedef GList* (*notify_get_server_caps_t)(void);
> +typedef void*  (*notify_notification_new_t)(const char*, const char*, const char*, const char*);
> +typedef bool   (*notify_notification_show_t)(void*, char*);
> +typedef void   (*notify_notification_set_icon_from_pixbuf_t)(void*, GdkPixbuf*);
> +typedef void   (*notify_notification_add_action_t)(void*, const char*, const char*, NotifyActionCallback, gpointer, GFreeFunc);

Might as well make these typedefs part of the class definition, in the protected part.
Attachment #653047 - Flags: review?(mh+mozilla)
Attachment #653047 - Flags: review-
Attachment #653047 - Flags: feedback?(karlt)
Assignee: nobody → mar.castelluccio
Actually, libnotify is enabled by default, and the configure option is to disable it. So forget about my previous comment, just rip that out.
Attachment #653047 - Flags: feedback?(karlt)
Looks good, thanks very much Marco.
Posted patch Patch v2 (obsolete) — Splinter Review
I've had to modify the patch, because libnotify uses atexit.

I've declared all the libnotify related variables as static, to avoid reloading the library for each notification. I've added a variable called "libNotifyAvail" to avoid re-trying to load the library if a previous load failed.

(In reply to Mike Hommey [:glandium] from comment #1)
> You should add a guard on libNotifyHandle.

There's already a guard in InitAlertAsync, that is the function that starts the request. ShowAlert is only called by the request callbacks, so a guard there shouldn't be needed.
Attachment #653047 - Attachment is obsolete: true
Attachment #653238 - Flags: review?(mh+mozilla)
Comment on attachment 653238 [details] [diff] [review]
Patch v2

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

::: configure.in
@@ +4939,5 @@
>      MOZ_ENABLE_LIBNOTIFY=1)
>  
>      if test "$MOZ_ENABLE_LIBNOTIFY"
>      then
>          AC_DEFINE(MOZ_ENABLE_LIBNOTIFY)

Please remove this configure option too, as said in comment 2.

::: toolkit/system/gnome/nsAlertsIconListener.cpp
@@ +19,5 @@
>  static bool gHasActions = false;
>  static bool gHasCaps = false;
>  
> +void* nsAlertsIconListener::libNotifyHandle = nullptr;
> +bool nsAlertsIconListener::libNotifyAvail = true;

Please invert the meaning of this variable and initialize it to false. That avoids storing the value in .data instead of .bss.

::: toolkit/system/gnome/nsAlertsIconListener.h
@@ +16,4 @@
>  
>  class imgIRequest;
>  
> +typedef void NotifyNotification;

struct NotifyNotification;
Attachment #653238 - Flags: review?(mh+mozilla) → review-
Posted patch Patch v3 (obsolete) — Splinter Review
> Please remove this configure option too, as said in comment 2.

Sorry, I misunderstood comment 2.

I've sent the patch to try: https://tbpl.mozilla.org/?tree=Try&rev=17312557ad7a
Attachment #653238 - Attachment is obsolete: true
Attachment #653373 - Flags: review?(mh+mozilla)
Comment on attachment 653373 [details] [diff] [review]
Patch v3

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

::: toolkit/system/gnome/Makefile.in
@@ +23,1 @@
>  	

Can you remove the spurious whitespaces while you're here?
Attachment #653373 - Flags: review?(mh+mozilla) → review+
Posted patch PatchSplinter Review
The try run is green, except for a crash that is due to another patch I have in the queue.
Attachment #653373 - Attachment is obsolete: true
Attachment #653505 - Flags: checkin?(mh+mozilla)
https://hg.mozilla.org/mozilla-central/rev/d7fb23a822dd
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.