Comment 98 for bug 434476

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

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

>+NS_IMPL_ISUPPORTS1(WakeLockListener, nsIDOMMozWakeLockListener)

Will need to remove the '1' here now.

>+ dbus_pending_call_set_notify(reply, &ReceiveInhibitReply, this, NULL);

|this| needs to stay alive until the reply is received, and I assume that may
be after the power manager service is deleted.

There are probably several ways to handle this: perhaps add a reference to the
WakeLockListener, and remove in free_user_data; or make the WakeLockListener a
singleton and check that it still exists in ReceiveInhibitReply().

>+ DBusError err;
>+ dbus_error_init(&err);

This should be freed if set, but easier is to just pass nullptr.

>+WakeLockTopic::InhibitScreensaver()
>+{
>+ NS_ASSERTION(!mShouldInhibit, "Screensaver is already inhibited");

>+WakeLockTopic::UninhibitScreensaver()
>+{
>+ if (!mShouldInhibit) {
>+ // Screensaver isn't inhibited. Nothing to do here.
>+ return NS_OK;

Could this instead assert that this path is not reached, as in
InhibitScreensaver()?

>+ nsClassHashtable<nsStringHashKey, WakeLockTopic> mTopics;

Topics are never removed from this hashtable (until shutdown). Is there a
small finite sized set of topics? Please add a comment stating this
assumption.

>+ virtual ~WakeLockListener() MOZ_FINAL;

By moving the MOZ_FINAL to the class definition, this destructor need not be
virtual.