Move screen.enabled/brightness to power manager interface

RESOLVED FIXED in mozilla14



DOM: Device Interfaces
6 years ago
5 years ago


(Reporter: kanru, Assigned: kanru)


Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)



(1 attachment, 2 obsolete attachments)



6 years ago
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.


6 years ago
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.

Comment 3

6 years ago
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?

Comment 5

6 years ago
Created attachment 607469 [details] [diff] [review]
Move screen.enabled/brightness to mozPower
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+

Comment 9

6 years ago
Created attachment 607847 [details] [diff] [review]
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?
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+

Comment 11

6 years ago
Jonas, do you want to comment on this?
Attachment #607847 - Flags: superreview?(jonas) → superreview+

Comment 12

5 years ago
Created attachment 611742 [details] [diff] [review]
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.
Attachment #607847 - Attachment is obsolete: true


5 years ago
Keywords: checkin-needed
Keywords: checkin-needed
Target Milestone: --- → mozilla14
Component: DOM → DOM: Device Interfaces
QA Contact: general → device-interfaces
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.