Closed Bug 710135 Opened 8 years ago Closed 8 years ago

Move screen.enabled/brightness to power manager interface


(Core :: DOM: Device Interfaces, defect)

Gonk (Firefox OS)
Not set





(Reporter: kanru, Assigned: kanru)




(1 file, 2 obsolete files)

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.
Blocks: 708964
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.
Hardware: x86_64 → All
Version: unspecified → Trunk
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.
Justin, do you think this still make sense?
(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?
Assignee: nobody → kchen
Attachment #607469 - Flags: review?(justin.lebar+bug)
Comment on attachment 607469 [details] [diff] [review]
Move screen.enabled/brightness to mozPower

cjones, do you agree with this API change?
Attachment #607469 - Flags: superreview?(jones.chris.g)
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.
Attachment #607469 - Flags: review?(justin.lebar+bug) → review+
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.
Attachment #607469 - Flags: superreview?(jones.chris.g) → superreview+
Remove the the extra prefs.

Rebased on mozilla-central because bug 720794 was merged. Mounir, could you review this?
Attachment #607469 - Attachment is obsolete: true
Attachment #607847 - Flags: review?(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.
Attachment #607847 - Flags: superreview?(jonas)
Attachment #607847 - Flags: review?(mounir)
Attachment #607847 - Flags: review+
Jonas, do you want to comment on this?
Attachment #607847 - Flags: superreview?(jonas) → superreview+
Rebased on current m-c. Always return DOM_SECURITY_ERROR if the caller cannot pass the permission check.
Attachment #607847 - Attachment is obsolete: true
Keywords: checkin-needed
Component: DOM → DOM: Device Interfaces
QA Contact: general → device-interfaces
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.