710135 - Move screen.enabled/brightness to power manager interface

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

DOM API for turning screen on/off and adjusting the screen's brightness was introduced in bug 702256, but they are closer to the power manager interface so after the power manager landed we should move them.

Comment 1

[Mounir Lamouri (:mounir)]

That doesn't seem that bad to have them in the screen interface. What do you think Justin?

I don't think we should put all power-related stuff to the power manager interface in that case enabling/disabling wifi should be done there as of bluetooth or CPU frequence scaling. That seems quite a few different things. Stuff like hibernating, shutting down, rebooting or maybe CPU freq scaling would make sense here but I doubt anything very specific to a feature should appear in the power manager interface.

Comment 2

[Justin Lebar (not reading bugmail)]

The reason I thought it made sense to move these off screen is because mozPower is a super-privileged API which only makes sense on devices where we're managing the whole device ourselves, and only can be called from the power-management code.  So it's similar to reboot/shutdown/CPU scaling, in that sense.

Comment 3

[Kan-Ru Chen [:kanru] (UTC+9)]

Justin, do you think this still make sense?

Comment 4

[Justin Lebar (not reading bugmail)]

(In reply to Kan-Ru Chen [:kanru] from comment #3)
> Justin, do you think this still make sense?

Yes; unless there's some new information which isn't present in this bug?

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Move screen.enabled/brightness to mozPower

Comment 6

[Justin Lebar (not reading bugmail)]

Comment on attachment 607469 [details] [diff] [review]
Move screen.enabled/brightness to mozPower

cjones, do you agree with this API change?

Comment 7

[Justin Lebar (not reading bugmail)]

I don't think you need the extra prefs anymore.  At least, I don't see a reason to disable screenEnabled but allow whitelisted sites to reboot the device.  :)

I'd also make setScreenEnabled and setScreenBrightness throw exceptions when you have insufficient permission, so they match the other methods on mozPower.  Returning true and 1 for getScreen{Enabled,Brightness} when you don't have permission sounds good to me.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 607469 [details] [diff] [review]
Move screen.enabled/brightness to mozPower

I agree with not mixing privileged and unprivileged APIs.  I think this is a good change.

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Move screen.enabled/brightness to mozPower, v2

Remove the the extra prefs.

Rebased on mozilla-central because bug 720794 was merged. Mounir, could you review this?

Comment 10

[Mounir Lamouri (:mounir)]

Comment on attachment 607847 [details] [diff] [review]
Move screen.enabled/brightness to mozPower, v2

Review of attachment 607847 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments applied.

Also, I wonder if we couldn't have navigator.power.screen.{enabled,brightness} instead of navigator.power.screen{Enabled,Brightness}. We would have to create another object under power but I kind of prefer that.
Jonas, what do you think?

::: dom/base/nsScreen.cpp
@@ -35,5 @@
>   * the terms of any one of the MPL, the GPL or the LGPL.
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
> -#include "mozilla/Hal.h"

Don't remove hal.h, it's still required for screen orientation.

::: dom/power/PowerManager.cpp
@@ +88,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
>  PowerManager::CheckPermission()

I wonder if that wouldn't be better to return a boolean and if |false| is returned, we always return a DOM_SECURITY_ERROR when it's in a Set* method. That way, we prevent throwing random errors to the content.

Comment 11

[Kan-Ru Chen [:kanru] (UTC+9)]

Jonas, do you want to comment on this?

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Move screen.enabled/brightness to mozPower, v2.1

Rebased on current m-c. Always return DOM_SECURITY_ERROR if the caller cannot pass the permission check.

Comment 13

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f5925b56c85

Comment 14

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/9f5925b56c85