Move screen.enabled/brightness to power manager interface

RESOLVED FIXED in mozilla14

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: kanru, Assigned: kanru)

Tracking

Trunk
mozilla14
All
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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.
(Assignee)

Updated

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.
(Assignee)

Comment 3

5 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?
(Assignee)

Comment 5

5 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+
(Assignee)

Comment 9

5 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+
(Assignee)

Comment 11

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

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
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f5925b56c85
Keywords: checkin-needed
Target Milestone: --- → mozilla14
Component: DOM → DOM: Device Interfaces
QA Contact: general → device-interfaces
https://hg.mozilla.org/mozilla-central/rev/9f5925b56c85
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.