Closed
Bug 783765
Opened 12 years ago
Closed 12 years ago
Use libnotify as a dynamic library
Categories
(Toolkit :: General, defect)
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: marco, Assigned: marco)
Details
Attachments
(1 file, 3 obsolete files)
14.89 KB,
patch
|
glandium
:
checkin+
|
Details | Diff | Splinter Review |
This is a simple fix for the libnotify problem, while we wait for bug 404738/bug 750141.
Assignee | ||
Updated•12 years ago
|
Attachment #653047 -
Attachment is patch: true
Attachment #653047 -
Flags: review?(mh+mozilla)
Comment 1•12 years ago
|
||
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)
Updated•12 years ago
|
Assignee: nobody → mar.castelluccio
Comment 2•12 years ago
|
||
Actually, libnotify is enabled by default, and the configure option is to disable it. So forget about my previous comment, just rip that out.
Updated•12 years ago
|
Attachment #653047 -
Flags: feedback?(karlt)
Comment 3•12 years ago
|
||
Looks good, thanks very much Marco.
Assignee | ||
Comment 4•12 years ago
|
||
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 5•12 years ago
|
||
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-
Assignee | ||
Comment 6•12 years ago
|
||
> 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 7•12 years ago
|
||
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+
Assignee | ||
Comment 8•12 years ago
|
||
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)
Comment 9•12 years ago
|
||
Comment on attachment 653505 [details] [diff] [review] Patch https://hg.mozilla.org/integration/mozilla-inbound/rev/d7fb23a822dd
Attachment #653505 -
Flags: checkin?(mh+mozilla) → checkin+
Comment 10•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d7fb23a822dd
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•