Comment 95 for bug 434476

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

All the DBus callbacks should be happening on the main thread unless someone
calls dbus_connection_setup_with_g_main() with an off-main-thread
context, which would be scary. This means that the ReentrantMonitor can be removed, and there are also some other simplifications possible due to things that won't happen.

There is still a problem if shouldLock changes to true, then false, then true,
before any replies are received. Two inhibit requests would end up in flight
and then only one of the reply cookies would be handled. I think this
requires state to be tracked a little differently. Perhaps use one state
variable to record what state has been requested of the WakeLockListener and
another for what state has been requested through DBus interfaces.

>+#include "WakeLockListener.h"
>+
>+#ifdef MOZ_ENABLE_DBUS

WakeLockListener.h includes dbus headers unconditionally, so something needs
change to avoid that when MOZ_ENABLE_DBUS is not set.

Providing a forward declaration for class DBusConnection I think will mean that WakeLockListener.h no longer needs to include any dbus headers.

WakeLockTopic and DesktopEnvironment are used only by the WakeLockListener
implementation and so their declarations can move to the .cpp file to make
this clearer.

>+static char const* const sApplication = "Mozilla Firefox";

g_get_prgname() is a simple way to get something reasonable here.
I'd probably use that unless there is a good reason why
chrome://branding/locale/brand.properties is necessary.
The "reverse domain" suggestion seems to indicate this doesn't need to be human
readable, just an identifier. I wouldn't bother constructing any reverse
domain hierarchy - we can have a tld.

>+bool
>+WakeLockTopic::SendInhibit()
>+{
>+ NS_ASSERTION(!mInhibitRequest, "Screensaver has already been inhibited");
>+
>+ switch (mDesktopEnvironment)
>+ {
>+ case Unknown:
>+ mDesktopEnvironment = FreeDesktop;
>+ case FreeDesktop:
>+ if (SendFreeDesktopInhibitMessage()) {
>+ return true;
>+ } else {
>+ mDesktopEnvironment = GNOME;
>+ }
>+ case GNOME:
>+ if (SendGNOMEInhibitMessage()) {
>+ return true;
>+ } else {
>+ mDesktopEnvironment = Unsupported;
>+ }
>+ case Unsupported:
>+ return false;
>+ }
>+}

dbus_connection_send_with_reply() may set the pending_return parameter to NULL
on OOM or if the connection is disconnected, but there is no check to see
whether the interface is available. Therefore Send*() method failure is not
an indication to try a fallback interface. The FreeDesktop and GNOME cases
can just return the result of the Send*() methods.

>+ NS_ASSERTION(mInhibitRequest || mDesktopEnvironment == Unsupported,
>+ "Screensaver wasn't inhibited");

I liked having these kind of assertions due to the assumptions that
WakeLockListener::Callback() would only be called when the state changed.
However, the documentation for the interfaces doesn't specify any particular
values for the cookie, so a valid cookie could be zero. A rethink of the
state variables, as discussed at the top of this comment, may make this
assertion easier to make or unnecessary.

>+WakeLockTopic::InhibitFailed()

>+ if (!mWaitingForInhibit) {
>+ // We were interrupted by UninhibitScreensaver() before we could find the
>+ // correct desktop environment.
>+ return;
>+ }

Even if the WakeLock state has changed, this failure indicates that the
interface is not supported and so this test can be moved to after
mDesktopEnvironment is updated.

>+ if (mDesktopEnvironment < Unsupported) {
>+ switch (mDesktopEnvironment) {
>+ case Unknown:
>+ mDesktopEnvironment = FreeDesktop;
>+ break;
>+ case FreeDesktop:
>+ mDesktopEnvironment = GNOME;
>+ break;
>+ case GNOME:
>+ mDesktopEnvironment = Unsupported;
>+ return;
>+ default:
>+ NS_WARNING("Unknown desktop environment");
>+ return;
>+ }

The warning can be an assertion because mDesktopEnvironment is controlled by
this code and so the default case should not happen.

The Unknown case should also not happen because a messages has been sent to
some interface. The Unknown enumerator is not really used, so I think it
would be simplest to just remove it and initialize mDesktopEnvironment to
FreeDesktop.

If SendInhibit() no longer modifies mDesktopEnvironment, and we have only one method call in flight at a time, then mDesktopEnvironment will never be Unsupported here. I would just have a single case (perhaps FreeDesktop) and
default, with an assert in the default case that the mDesktopEnvironment is
the expected value (GNOME).

>+WakeLockTopic::InhibitSucceeded(uint32_t aInhibitRequest)
>+{
>+ ReentrantMonitorAutoEnter mon(mMonitor);
>+
>+ NS_ASSERTION(!mInhibitRequest || mDesktopEnvironment == Unsupported,
>+ "Inhibit request handle is already set.");

I expect there will also be no way to get here when mDesktopEnvironment == Unsupported.

>+//#include "nsTHashtable.h"

Can remove this line now.

>+class WakeLockListener : public nsIDOMMozWakeLockListener
>+{
>+public:
>+ NS_DECL_ISUPPORTS;
>+
>+ WakeLockListener();
>+ virtual ~WakeLockListener();

This is a reference counted class, so please make the destructor private.
The (virtual) Release() method is implemented on this class and there are no
derived classes, so the destructor need not be virtual. Adding MOZ_FINAL
would clarify this.

>-nsAppShell::EventProcessorCallback(GIOChannel *source,
>+nsAppShell::EventProcessorCallback(GIOChannel *source,

No unrelated whitespace changes please, even if it is removing unnecessary
whitespace.