Last Comment Bug 710135 - Move screen.enabled/brightness to power manager interface
: Move screen.enabled/brightness to power manager interface
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All Gonk (Firefox OS)
: -- normal (vote)
: mozilla14
Assigned To: Kan-Ru Chen [:kanru] (UTC+8)
:
Mentors:
Depends on:
Blocks: 708964
  Show dependency treegraph
 
Reported: 2011-12-12 23:35 PST by Kan-Ru Chen [:kanru] (UTC+8)
Modified: 2012-04-04 04:47 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Move screen.enabled/brightness to mozPower (14.58 KB, patch)
2012-03-20 00:07 PDT, Kan-Ru Chen [:kanru] (UTC+8)
justin.lebar+bug: review+
cjones.bugs: superreview+
Details | Diff | Review
Move screen.enabled/brightness to mozPower, v2 (14.50 KB, patch)
2012-03-20 21:13 PDT, Kan-Ru Chen [:kanru] (UTC+8)
mounir: review+
jonas: superreview+
Details | Diff | Review
Move screen.enabled/brightness to mozPower, v2.1 (18.08 KB, patch)
2012-04-03 01:51 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review

Description Kan-Ru Chen [:kanru] (UTC+8) 2011-12-12 23:35:08 PST
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) 2011-12-13 08:39:58 PST
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) 2011-12-13 10:21:30 PST
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+8) 2012-02-05 18:41:15 PST
Justin, do you think this still make sense?
Comment 4 Justin Lebar (not reading bugmail) 2012-02-05 18:45:20 PST
(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+8) 2012-03-20 00:07:18 PDT
Created attachment 607469 [details] [diff] [review]
Move screen.enabled/brightness to mozPower
Comment 6 Justin Lebar (not reading bugmail) 2012-03-20 08:07:46 PDT
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) 2012-03-20 08:18:11 PDT
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 2012-03-20 10:33:22 PDT
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+8) 2012-03-20 21:13:11 PDT
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?
Comment 10 Mounir Lamouri (:mounir) 2012-03-21 02:32:12 PDT
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+8) 2012-03-25 19:02:27 PDT
Jonas, do you want to comment on this?
Comment 12 Kan-Ru Chen [:kanru] (UTC+8) 2012-04-03 01:51:22 PDT
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.
Comment 13 Justin Lebar (not reading bugmail) 2012-04-03 08:45:44 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f5925b56c85
Comment 14 Marco Bonardo [::mak] 2012-04-04 04:47:16 PDT
https://hg.mozilla.org/mozilla-central/rev/9f5925b56c85

Note You need to log in before you can comment on or make changes to this bug.