Comment 92 for bug 434476

Revision history for this message
In , Karlt (karlt) wrote :

Comment on attachment 8404230
Implement WakeLockListener on Linux to disable screensaver while video is playing

The main thing missing here is appropriate handling of this situation:
After a InhibitScreenSaver() call, WakeLockListener::Callback() is called with
a non-"locked-foreground" state before WakeLockListenerStatusNotify() receives
the cookie for mInhibitRequest. The Uninhibit message can't be sent until the
cookie is received in the reply from Inhibit.

The other things below should be easier to resolve.

>+CFLAGS += $(MOZ_DBUS_GLIB_CFLAGS)

This line can be dropped because only cpp files are involved here.

>+using namespace mozilla;
>
> using mozilla::unused;

I assume the first line here makes the other unnecessary.

>+ char const *const sReason = "Media Playback";

This is generic code inhibiting the screen saver on any "topic" in
locked-foreground state, so mention of "Media Playback" seems strange. It
seems that either Callback() should listen only for the "Playing_media" topic
or should pass on each topic to the screen saver control (the cookie means
that multiple inhibit/unhibit pairs will be handled appropriately).

>+ mDesktopEnvironment++;

Re a postfix ++ expression, N3242 says "The type of the operand shall be an
arithmetic type or a pointer to a complete object type."

gcc 4.7.3 says:

> /home/karl/moz/dev/widget/gtk/nsAppShell.cpp: In member function 'void WakeLockListener::InhibitFailed()':
> /home/karl/moz/dev/widget/gtk/nsAppShell.cpp:156:32: error: no 'operator++(int)' declared for postfix '++' [-fpermissive]

>+ void Init(void)
>+ {
>+ if (mConnection) {
>+ dbus_connection_setup_with_g_main(mConnection, nullptr);
>+ }
>+ }

The dbus_connection_set_exit_on_disconnect() call seems to have been lost.
If there is no lazy initialization, then there is no need to have a separate
Init() function and moving the code into the constructor would make it clearer
that this can only happen once.

>+ GNOME,
>+ FreeDesktop,
>+ Unsupported,

This is now a freedesktop spec and implemented on GNOME (comment 9) so try the
FreeDesktop interface first and fall back to GNOME if not available.
I assume the GNOME interface will be deprecated at some point.
http://specs.freedesktop.org/idle-inhibit-spec/latest/re01.html

>+ DBusConnection* mConnection;

This needs a dbus_connection_unref(mConnection).

>+ // Keep track of all the topics that have requested a wake lock. When the
>+ // number of topics in the hashtable reaches zero, we can uninhibit the
>+ // screensaver again.
>+ nsTHashtable<nsStringHashKey> mLockedTopics;

If only one topic is handled, then this hash table should no longer be
necessary.
If multiple topics are handled then this could hold the cookies.

>+ nsCOMPtr<nsIPowerManagerService> powerManagerService;
>+
> // make the pipe nonblocking

>+ powerManagerService = do_GetService(POWERMANAGERSERVICE_CONTRACTID);

Please declare at the initialization.