783765 - Use libnotify as a dynamic library

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Marco Castelluccio [:marco]]

Attachment: Patch

This is a simple fix for the libnotify problem, while we wait for bug 404738/bug 750141.

Comment 1

[Mike Hommey [:glandium]]

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.

Comment 2

[Mike Hommey [:glandium]]

Actually, libnotify is enabled by default, and the configure option is to disable it. So forget about my previous comment, just rip that out.

Comment 3

[Karl Tomlinson (:karlt back Feb 10)]

Looks good, thanks very much Marco.

Comment 4

[Marco Castelluccio [:marco]]

Attachment: Patch v2

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.

Comment 5

[Mike Hommey [:glandium]]

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;

Comment 6

[Marco Castelluccio [:marco]]

Attachment: Patch v3

> 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

Comment 7

[Mike Hommey [:glandium]]

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?

Comment 8

[Marco Castelluccio [:marco]]

Attachment: Patch

The try run is green, except for a crash that is due to another patch I have in the queue.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 653505 [details] [diff] [review]
Patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7fb23a822dd

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d7fb23a822dd