Last Comment Bug 702256 - [Gonk] Add DOM API for turning screen on/off and adjusting the screen's brightness
: [Gonk] Add DOM API for turning screen on/off and adjusting the screen's brigh...
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla12
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on: 707626
Blocks: webapi 707589 717155
  Show dependency treegraph
 
Reported: 2011-11-14 06:09 PST by Justin Lebar (not reading bugmail)
Modified: 2012-09-30 03:16 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (17.67 KB, patch)
2011-11-17 14:15 PST, Justin Lebar (not reading bugmail)
mounir: review+
cjones.bugs: review+
gal: feedback+
Details | Diff | Review
Patch v2 (24.29 KB, patch)
2011-12-05 01:00 PST, Justin Lebar (not reading bugmail)
no flags Details | Diff | Review
Patch v2.1 (24.07 KB, patch)
2011-12-05 01:07 PST, Justin Lebar (not reading bugmail)
no flags Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2011-11-14 06:09:45 PST
I'm thinking we can have

  bool attribute navigator.mozScreen.enabled

I'd also want to put screen brightness in here.  I guess if it's just brightness and enabled/disabled, we could put them straight on the navigator object, but maybe there are other things we want to expose?  (We could, for example, expose details about the screen, such as its subpixel layout.)

We don't have a permissions API yet, but once we do, we probably want to hide mozScreen entirely; unprivileged pages can use the visibility API to tell whether they're visible.
Comment 1 Kyle Machulis [:kmachulis] [:qdot] 2011-11-15 12:45:05 PST
This needs to tie into the RIL backend too, as there's a RIL_REQUEST_SCREEN_STATE variable we need to update when the screen is turned on/off.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-15 12:48:51 PST
What does that do in rild?
Comment 3 Kyle Machulis [:kmachulis] [:qdot] 2011-11-15 13:11:08 PST
Mostly it's a power saving mechanism. From the ril.h header:

 Indicates the current state of the screen.  When the screen is off, the
 RIL should notify the baseband to suppress certain notifications (eg,
 signal strength and changes in LAC/CID or BID/SID/NID/latitude/longitude)
 in an effort to conserve power.  These notifications should resume when the
 screen is on.
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-15 13:14:20 PST
But that doesn't apply to when the screen is turned off based on the proximity sensor while a call is in progress, right?  We'll probably need some hackery dackery doo to know when to notify rild that the screen is really for srs off.
Comment 5 Kyle Machulis [:kmachulis] [:qdot] 2011-11-15 13:17:24 PST
I haven't actually tested to see whether that happens or not, but I wouldn't be surprised if it still does. We don't care about the listed changes when on a call with the screen off, I don't think it'll effect the call status.
Comment 6 Justin Lebar (not reading bugmail) 2011-11-15 22:54:04 PST
I have a lot of code written for this, but I'm currently putting out some fires.  I'll get back to this in a few days.
Comment 7 Justin Lebar (not reading bugmail) 2011-11-17 14:15:03 PST
Created attachment 575294 [details] [diff] [review]
Patch v1

This adds

  bool attribute window.screen.mozEnabled
  double attribute window.screen.mozBrightness

and hooks screen.mozEnabled up to the power button.

We don't currently fire the correct visibility change events when we turn off the screen, but I figure we can do that in a follow-up.

I tested screen.mozBrightness by setting it to random values before turning on the screen, but I've taken that out in the final patch.
Comment 8 Mounir Lamouri (:mounir) 2011-11-18 14:39:49 PST
Comment on attachment 575294 [details] [diff] [review]
Patch v1

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

I don't think I have enough background to review changes in those files:
b2g/chrome/content/shell.js
hal/gonk/GonkHal.cpp
widget/src/gonk/nsAppShell.cpp

r=me for the other files (with the comments addressed).

::: dom/base/nsScreen.cpp
@@ +54,5 @@
> +
> +/* static */ void
> +nsScreen::Initialize()
> +{
> +  sInitialized = true;

Add a NS_ASSERTION checking that sInitialized isn't true.

@@ +58,5 @@
> +  sInitialized = true;
> +  Preferences::AddBoolVarCache(&sAllowScreenEnabledProperty,
> +                               "dom.screenEnabledProperty.enabled");
> +  Preferences::AddBoolVarCache(&sAllowScreenBrightnessProperty,
> +                               "dom.screenBrightnessProperty.enabled");

Is it really useful to have a VarCache? We could just check the pref everytime. I don't know the costs of using VarCache but if there is one, it seems unworthy.

@@ +261,5 @@
> +nsresult
> +nsScreen::GetMozEnabled(bool *aEnabled)
> +{
> +  NS_ENSURE_TRUE(sAllowScreenEnabledProperty, NS_ERROR_NOT_AVAILABLE);
> +  NS_ENSURE_ARG_POINTER(aEnabled);

I think if sAllowScreen... isn't true, we could just return NS_OK and a default value. Throwing an exception seems a bad idea to me.

Also, you don't need NS_ENSURE_ARG_POINTER.

@@ +269,5 @@
> +
> +nsresult
> +nsScreen::SetMozEnabled(bool aEnabled)
> +{
> +  NS_ENSURE_TRUE(sAllowScreenEnabledProperty, NS_ERROR_NOT_AVAILABLE);

Ditto for the exception.

@@ +272,5 @@
> +{
> +  NS_ENSURE_TRUE(sAllowScreenEnabledProperty, NS_ERROR_NOT_AVAILABLE);
> +
> +  // TODO: When the screen's state changes, all visible windows should fire a
> +  // visibility change event.

File a bug and add the bug number before landing please :)

@@ +282,5 @@
> +nsresult
> +nsScreen::GetMozBrightness(double *aBrightness)
> +{
> +  NS_ENSURE_TRUE(sAllowScreenBrightnessProperty, NS_ERROR_NOT_AVAILABLE);
> +  NS_ENSURE_ARG_POINTER(aBrightness);

Ditto for the exception and the NS_ENSURE_ARG_POINTER.

@@ +290,5 @@
> +
> +nsresult
> +nsScreen::SetMozBrightness(double aBrightness)
> +{
> +  NS_ENSURE_TRUE(sAllowScreenBrightnessProperty, NS_ERROR_NOT_AVAILABLE);

Ditto for the exception.

::: hal/Hal.cpp
@@ +405,5 @@
> +
> +  if (brightness < 0)
> +    brightness = 0;
> +  if (brightness > 1)
> +    brightness = 1;

You could use |clamped| from Algorithm.h. That would make this more readable and you wouldn't violate the coding style ;)
Comment 9 Justin Lebar (not reading bugmail) 2011-11-18 15:51:06 PST
Comment on attachment 575294 [details] [diff] [review]
Patch v1

Chris, would you mind looking at the remaining files?
Comment 10 Justin Lebar (not reading bugmail) 2011-11-18 15:56:20 PST
> Is it really useful to have a VarCache? We could just check the pref everytime.

I really have no idea when it's appropriate to use a cache or not, so I've been using caches everywhere.  I have similar code in the navigator for whether the vibrator is enabled.

Anyone have guidance on when we should prefer caches versus going to the pref service?  I suspect it doesn't matter much either way.

> I think if sAllowScreen... isn't true, we could just return NS_OK and a default value. Throwing an 
> exception seems a bad idea to me.

I'm concerned about lying to the page if we don't have to.  A page might, for example, try to determine whether Firefox is in the background by examining |screen.enabled && !document.mozVisible|.

If we don't throw an exception, do we just drop writes to the properties?  The page does screen.enabled = false, but the next time we read screen.enabled, it's true?

I guess throwing an exception is also lame.  I'd rather the properties just not be there if we don't support them.
Comment 11 Justin Lebar (not reading bugmail) 2011-11-18 15:57:15 PST
Comment on attachment 575294 [details] [diff] [review]
Patch v1

Jonas, what do you think we should do about the API when turning the screen on/off and modifying brightness isn't available?
Comment 12 Jonas Sicking (:sicking) 2011-11-18 17:39:46 PST
Is the plan to allow any page to modify the on-state and brightness? That seems a bit scary to me. Especially since there is no provision for resetting these modifications if the user wants the screen back (like pressing the escape key, or touching the screen on a touch device).

I could easily see this leading to dataloss if the user thinks that the device has crashed and does a force-reboot.

Or is the idea that this will only be available to privileged apps?
Comment 13 Justin Lebar (not reading bugmail) 2011-11-18 21:46:27 PST
> Or is the idea that this will only be available to privileged apps?

Yes; I should have made that clear.  I don't think either property should be available to anything except the most privileged app.

We don't have the privilege API done yet, though.
Comment 14 Jonas Sicking (:sicking) 2011-11-20 19:14:14 PST
Ok, please call into the privilege manager, or add a pref which controls which sites are able to use this feature. Other than that it looks ok.
Comment 15 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-11-21 01:13:51 PST
Comment on attachment 575294 [details] [diff] [review]
Patch v1

>diff --git a/b2g/app/mobile.js b/b2g/app/mobile.js
> // deep within the bowels of the widgetry system.  Remove me when GL
> // compositing isn't default disabled in widget/src/android.
> pref("layers.acceleration.force-enabled", true);
>+
>+pref("dom.screenEnabledProperty.enabled", true);
>+pref("dom.screenBrightnessProperty.enabled", true);

Nit: People usually add a small comment line to separate block of preferences. For this I guess '// Screen WebAPI' should be enough.


>diff --git a/b2g/chrome/content/shell.js b/b2g/chrome/content/shell.js
>     if (e.keyCode == e.DOM_VK_HOME) {
>       shell.doCommand("cmd_close");
>     }
>+    else if (e.keyCode == e.DOM_VK_SLEEP) {
>+      screen.mozEnabled = !screen.mozEnabled;
>+    }
>   }
> };

Nit: } else if {
Comment 16 Justin Lebar (not reading bugmail) 2011-11-21 06:43:19 PST
> Ok, please call into the privilege manager

Does this code exist in the tree atm?  I can't find it, if so.

> or add a pref which controls which sites are able to use this feature. Other than that it looks ok.

Is a pref disabling it everywhere except B2G sufficient?
Comment 17 Mounir Lamouri (:mounir) 2011-11-21 06:47:34 PST
(In reply to Justin Lebar [:jlebar] from comment #16)
> > or add a pref which controls which sites are able to use this feature. Other than that it looks ok.
> 
> Is a pref disabling it everywhere except B2G sufficient?

There is an implementation for something else than Gonk?
Comment 18 Justin Lebar (not reading bugmail) 2011-11-21 06:58:33 PST
> There is an implementation for something else than Gonk?

No.  So the API is twice-disabled outside gonk; once by a pref, and the other by there being no code.  :)
Comment 19 Mounir Lamouri (:mounir) 2011-11-21 07:02:26 PST
Then, except if we want to implement a system for B2G we could just not handle that for the moment in my opinion. Given that B2G can hardly run non-pre-installed content, it seems to be quite safe to have it available. We could also add a whitelist preference like I did for WebSMS.
Comment 20 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-21 23:45:07 PST
Comment on attachment 575294 [details] [diff] [review]
Patch v1

>diff --git a/b2g/app/mobile.js b/b2g/app/mobile.js

>+pref("dom.screenEnabledProperty.enabled", true);
>+pref("dom.screenBrightnessProperty.enabled", true);

I assume there's another mechanism preventing general web content from
seeing this API?  If not, we need that :).

>diff --git a/hal/Hal.cpp b/hal/Hal.cpp

>+#define PROXY_IF_SANDBOXED_RV(_call)              \
>+  do {                                            \
>+    if (InSandbox()) {                            \
>+      return hal_sandbox::_call;                  \
>+    } else {                                      \
>+      return hal_impl::_call;                     \
>+    }                                             \
>+  } while (0)
>+

Yuck :).  My kingdom for gcc expression-statements ... Anywho, please
rename this to RETURN_PROXY_IF_SANDBOXED() so the effect on control
flow is a bit clearer.

>+void SetScreenBrightness(double brightness)
>+{
>+  AssertMainThread();
>+
>+  if (brightness < 0)
>+    brightness = 0;
>+  if (brightness > 1)

else if

>diff --git a/hal/gonk/GonkHal.cpp b/hal/gonk/GonkHal.cpp

>+class AutoFd

See ScopedClose in xpcom/glue/FileUtils.h

>+const char *screenEnabledFilename = "/sys/power/state";
>+const char *screenBrightnessFilename = "/sys/class/backlight/pwm-backlight/brightness";
>+
>+template<ssize_t n>

size_t

>+// We can write to screenEnabledFilename to enable/disable the screen, but when
>+// we read, we always get "mem"!  So we have to keep track ourselves whether
>+// the screen is on or not.

Is that what android does?

>+double
>+GetScreenBrightness()
>+{
>+  char buf[32];
>+  ReadFromFile(screenBrightnessFilename, buf);
>+
>+  unsigned int val;
>+  if (sscanf(buf, "%u", &val) != 1) {

strtoul() please.  sscanf() is massive overkill.  Yeah doesn't really
matter here, but good habit to be in ...

>+  return val / 256.0;

Shouldn't this be / 255.0?

>+}
>+
>+void
>+SetScreenBrightness(double brightness)
>+{
>+  // Don't use De Morgan's law to push the ! into this expression; we want to
>+  // catch NaN too.
>+  if (!(brightness >= 0 && brightness <= 1)) {

Nit: I like the inequalities to point the same direction for easier scanning, so

  0 <= brightness && brightness <= 1

>+  // Convert the value in [0, 1] to an int between 0 and 255, then write to a
>+  // string.
>+  int val = static_cast<int>(round(brightness * 255));

No need to round, the float->int conversion will do that incidentally.

>+  char str[4];
>+  if (snprintf(str, sizeof(str), "%d", val) > static_cast<int>(sizeof(str))) {

This should be an assertion.

  char str[4];
  DebugOnly<int> bytesWritten = snprintf(str, sizeof(str), "%d", val);
  MOZ_ASSERT(bytesWritten < sizeof(str));

This is just nit-y stuff, r=me with those fixed.
Comment 21 Justin Lebar (not reading bugmail) 2011-11-22 05:49:07 PST
> I assume there's another mechanism preventing general web content from
> seeing this API?  If not, we need that :).

You mean unprivileged pages shouldn't be aware that screen.mozEnabled exists?  We don't have a mechanism for this that I'm aware of...

>> +template<ssize_t n>
> size_t

This is ssize_t because we compare it with a signed number later on:

> ssize_t numRead = read(...);
> +  buf[PR_MIN(numRead, n - 1)] = '\0';

> No need to round, the float->int conversion will do that incidentally.

Really?  http://codepad.org/SHSVR262

int main() {
  float f = 1.9;
  printf("%d\n", static_cast<int>(f));
}

outputs 1.

> >+// We can write to screenEnabledFilename to enable/disable the screen, but when
> >+// we read, we always get "mem"!  So we have to keep track ourselves whether
> >+// the screen is on or not.

> Is that what android does?

I didn't find where Android keeps track of whether the screen is on or off, but as far as I can tell, Android never reads from this file.
Comment 22 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-22 18:26:08 PST
(In reply to Justin Lebar [:jlebar] from comment #21)
> > I assume there's another mechanism preventing general web content from
> > seeing this API?  If not, we need that :).
> 
> You mean unprivileged pages shouldn't be aware that screen.mozEnabled
> exists?  We don't have a mechanism for this that I'm aware of...
> 

My understanding is that we make the sms and telephony APIs disappear when there's no available backend.  I don't know how that's done.

What happens if an unprivileged page frobs the screen brightness, e.g.?  You have an explicit security check?

> > No need to round, the float->int conversion will do that incidentally.
> 
> Really?  http://codepad.org/SHSVR262
> 
> int main() {
>   float f = 1.9;
>   printf("%d\n", static_cast<int>(f));
> }
> 
> outputs 1.
> 

That's rounding for you ;).  But yeah you're right, best to keep that extra bit of precision.

> > >+// We can write to screenEnabledFilename to enable/disable the screen, but when
> > >+// we read, we always get "mem"!  So we have to keep track ourselves whether
> > >+// the screen is on or not.
> 
> > Is that what android does?
> 
> I didn't find where Android keeps track of whether the screen is on or off,
> but as far as I can tell, Android never reads from this file.

I would be interested to know what android does.  I'm not against tracking that state in gecko a priori, but if something else already tracks it, closer to the HW, I would prefer to use that.  We can do that in a followup though.
Comment 23 Justin Lebar (not reading bugmail) 2011-11-22 18:36:27 PST
> What happens if an unprivileged page frobs the screen brightness, e.g.?  You have an explicit 
> security check?

Yes, that's how it works right now.

> I would be interested to know what android does.

I just had another look.  PowerManagerService.java keeps track of mPowerState itself.  It calls into the Power JNI wrapper class to turn off the screen; that file doesn't provide any way to read the screen state.

So it looks like Android does basically what we're doing in this patch.
Comment 24 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-22 18:42:24 PST
Sounds good.  Thanks for checking.
Comment 25 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-22 18:46:59 PST
BTW, on the b2g call today, there were some questions about this API raised by mwu.  As far as I understood, his points were
 - do away with enabled/disabled in favor of brightness==0 -> disabled.  I think the counter-objection is that it's convenient to turn off the display while having the backend remember the preferred brightness, so that the "screen-off" code doesn't have to remember that itself.
 - there might already something exposed on |window| on which this new interface could have been stuck.

The idea of making the screen brightness a CSS property was also mooted, so that it could be animated by CSS animations etc., but I'm not sure if there's any precedent for this kind of CSS property.
Comment 26 Justin Lebar (not reading bugmail) 2011-11-22 19:07:37 PST
I'm sorry I wasn't on the call to talk about this.

I'm wary of overloading an existing DOM property to control the screen's brightness or on/off state, for three reasons:

  1. New DOM properties are cheap compared to the cost of fishy API.
  2. If we overload an existing property, then content has to detect which of the two overloads it's calling.
  3. window.screen already has relevant properties like the screen's height and width, and we'll probably add others, such as the screen's subpixel layout.  It feels like a natural place for the knobs to adjust the screen's properties.

But I defer to Jonas and the WebAPI folks on this issue.  There may well be a nook or cranny that this API fits well into.

I don't think we should make backlight == 0 mean "turn the screen off".  Again, three points:

 a. On the SGII, we can't dim the backlight smoothly down to black.  hw-backlight==255 is very bright, and hw-backlight==0 is dim but most definitely not "no backlight".  So if in the DOM backlight == 0 means "screen off", you'd be able to dim the screen smoothly from very bright to kind of dim, and then there'd be a discontinuity when the screen finally turned off at backlight == 0.

 b. Even if this weren't an issue (on traditional rather than organic LEDs, you can certainly turn off the backlight while leaving the screen on), there's a big difference between "backlight off" and "screen off".  In the former case, the screen still receives touch events.  It's perfectly reasonable to ask for the backlight to be off with the screen still on, so you can "wake" the phone by touching it.

 c. Last, the backlight responds very quickly when frobbed, but turning the screen on and off has a visible lag.  So the screen will respond very quickly as you move from backlight == 1 to backlight == 0 + epsilon, but then there will be about a second's delay before the screen goes to black.  We could hack this in Hal (black the screen immediately, but pretend that it's off) but that hack doesn't translate to non-OLED screens, where a black screen isn't the same as backlight off.
Comment 27 Michael Wu [:mwu] 2011-11-22 19:13:42 PST
I like the idea of making screen brightness a CSS property a lot. The samsung galaxy s2 firmware turns down the screen brightness before turning it off, so making screen brightness a css property and animating it would fit that case perfectly.

On the other hand, it is in fact weird. It would only affect the window where as most of the css properties I know also affect the children, and children overriding them usually have a meaning.

Combining brightness and enabled/disabled is another option though I don't feel particularly strong one way or another on combining or keeping separate.
Comment 28 Michael Wu [:mwu] 2011-11-22 19:36:24 PST
As for borrowing existing dom properties, one option I was looking at was:

https://developer.mozilla.org/en/DOM/window.minimize
https://developer.mozilla.org/en/DOM/window.restore

since in B2G, the window is the screen, and trying to hide your screen should mean we stop displaying things/turn off the screen. I don't remember anything in the dom or css that would make sense for brightness though.
Comment 29 Justin Lebar (not reading bugmail) 2011-11-22 19:45:54 PST
> I like the idea of making screen brightness a CSS property a lot.

It'd be cute to use a CSS animation here, but CSS is all about applying your effects to just some elements on the page.  The backlight is a global property.

It's not hard to animate the backlight using JS.

> As for borrowing existing dom properties, one option I was looking at was:

What happens when content (say, a web app) calls window.minimize?  Does it want to turn off the screen, or does it want to minimize itself and return to the home screen?  Using window.minimize/restore for task switching seems much more reasonable...

Unless you'd want window.minimize to do something different for apps and the top-level window?  This feels like a hack to me.
Comment 30 Michael Wu [:mwu] 2011-11-22 20:09:15 PST
(In reply to Justin Lebar [:jlebar] from comment #29)
> > I like the idea of making screen brightness a CSS property a lot.
> 
> It'd be cute to use a CSS animation here, but CSS is all about applying your
> effects to just some elements on the page.  The backlight is a global
> property.
> 
> It's not hard to animate the backlight using JS.
> 

Well, you could say that of css animation in general.

Also I already raised the same objections in my own comments regarding specific elements vs. global.

> > As for borrowing existing dom properties, one option I was looking at was:
> 
> What happens when content (say, a web app) calls window.minimize?  Does it
> want to turn off the screen, or does it want to minimize itself and return
> to the home screen?  Using window.minimize/restore for task switching seems
> much more reasonable...
> 
> Unless you'd want window.minimize to do something different for apps and the
> top-level window?  This feels like a hack to me.

Why? Minimize/restore only applies to the top level "chrome" html, and content html would have minimize/restore mean another thing, as the policy by the content html dictates. window.minimize is a sufficiently nasty api for content that it shouldn't even be allowed in most cases, really.
Comment 31 Justin Lebar (not reading bugmail) 2011-11-22 20:22:16 PST
> Why [is it a hack]?

 1. A window.minimize function which does two things, one of which isn't "minimize", is confusing.
 2. If we overload an this property, then pages have to be aware of which context they're running in.
 3. New DOM properties are cheap compared to the cost of fishy API.
 4. It harms the abstraction (illusion?) that the only difference between the B2G homescreen and any other app is privilege.  Now the B2G homescreen is a privileged app running *in a special context* which modifies the definition of "minimize" and "restore".
Comment 32 Michael Wu [:mwu] 2011-11-22 20:38:26 PST
(In reply to Justin Lebar [:jlebar] from comment #31)
> > Why [is it a hack]?
> 
>  1. A window.minimize function which does two things, one of which isn't
> "minimize", is confusing.
>  2. If we overload an this property, then pages have to be aware of which
> context they're running in.
>  3. New DOM properties are cheap compared to the cost of fishy API.
>  4. It harms the abstraction (illusion?) that the only difference between
> the B2G homescreen and any other app is privilege.  Now the B2G homescreen
> is a privileged app running *in a special context* which modifies the
> definition of "minimize" and "restore".

Very well. Carry on then.
Comment 33 Mounir Lamouri (:mounir) 2011-11-23 02:42:26 PST
(In reply to Chris Jones [:cjones] [:warhammer] from comment #22)
> (In reply to Justin Lebar [:jlebar] from comment #21)
> > > I assume there's another mechanism preventing general web content from
> > > seeing this API?  If not, we need that :).
> > 
> > You mean unprivileged pages shouldn't be aware that screen.mozEnabled
> > exists?  We don't have a mechanism for this that I'm aware of...
> > 
> 
> My understanding is that we make the sms and telephony APIs disappear when
> there's no available backend.  I don't know how that's done.

navigator.mozSms is null if the security constraints are not fulfilled or when the current platform doesn't handle SMS's. mozSms never disappears from the navigator object.
However, this would be doable by conditionally adding nsIDOMNavigotarWhateverPartialInterface to the navigator object in nsDOMClassInfo. It's not done for WebSMS because the temporary security model consists of mozSms being null if the domain isn't whitelisted.

If you want to do that for screen.mozEnabled and screen.mozBrightness you could probably create a new interface (nsIDOMMozScreenSomething that have those attributes) and conditionally add it to Screen class info.
Comment 34 Justin Lebar (not reading bugmail) 2011-11-26 09:45:08 PST
Chris / Jonas: Are we good to go here with the current API and current API security measures, or would you like some changes?
Comment 35 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-26 17:12:12 PST
The API is ok with me.  No better ideas.

It seems suboptimal that the incantation needed to enable the screen properties is different than the one for sms etc., but the check here is also fine with me.
Comment 36 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-26 17:13:56 PST
(In reply to Chris Jones [:cjones] [:warhammer] from comment #35)
> It seems suboptimal that the incantation needed to enable the screen
> properties is different than the one for sms etc., but the check here is
> also fine with me.

Actually wait, no, this is not suboptimal but bad: when the pref is flipped, *all* web content, privileged or not, will be able to set these properties, right?  How about we copy the SMS check, where there's an on/off switch and then prefs to enable access for particular domains?  Should be able to copy/paste the code, or turn it into common helpers.
Comment 37 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-26 19:20:29 PST
But I would also be ok with fixing that up in a followup bug.
Comment 38 Justin Lebar (not reading bugmail) 2011-11-27 10:18:04 PST
> when the pref is flipped, *all* web content, privileged or not, will be able to set these 
> properties, right?

Yes, all web content running on Gonk, privileged or not, would be able to set the properties, once the pref is flipped.  And when the patch is checked in, all web content running under Gecko will be able to see that the properties exist, regardless of what the pref is set to.

> How about we copy the SMS check, where there's an on/off switch and then prefs to enable access for 
> particular domains?  Should be able to copy/paste the code, or turn it into common helpers.

My understanding was that a privilege API was being worked on, which would subsume this.  The SMS whitelist is a hack, right?  I can add the same hack, but like Mounir said in comment 19:

> Then, except if we want to implement a system for B2G we could just not handle that for the moment 
> in my opinion. Given that B2G can hardly run non-pre-installed content, it seems to be quite safe 
> to have it available.
Comment 39 Justin Lebar (not reading bugmail) 2011-11-27 10:22:02 PST
Jonas, outstanding questions for you:

1.

>> Is it really useful to have a VarCache? We could just check the pref everytime.

> I really have no idea when it's appropriate to use a cache or not, so I've been using caches 
> everywhere.  I have similar code in the navigator for whether the vibrator is enabled.

2.

>> I think if sAllowScreen... isn't true, we could just return NS_OK and a default value. Throwing an 
>> exception seems a bad idea to me.

> I'm concerned about lying to the page if we don't have to.  A page might, for example, try to 
> determine whether Firefox is in the background by examining |screen.enabled && 
> !document.mozVisible|.

> If we don't throw an exception, do we just drop writes to the properties?  The page does 
> screen.enabled = false, but the next time we read screen.enabled, it's true?  I guess throwing an 
> exception is also lame.

3.

>> Ok, please call into the privilege manager
> Does this code exist in the tree atm?  I can't find it, if so.
Comment 40 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-28 06:57:52 PST
(In reply to Justin Lebar [:jlebar] from comment #38)
> > How about we copy the SMS check, where there's an on/off switch and then prefs to enable access for 
> > particular domains?  Should be able to copy/paste the code, or turn it into common helpers.
> 
> My understanding was that a privilege API was being worked on, which would
> subsume this.  The SMS whitelist is a hack, right?  I can add the same hack,
> but like Mounir said in comment 19:
> 
> > Then, except if we want to implement a system for B2G we could just not handle that for the moment 
> > in my opinion. Given that B2G can hardly run non-pre-installed content, it seems to be quite safe 
> > to have it available.

B2G can load arbitrary web content.

But like I said, I'm fine with this landing as-is and us sorting it out in a followup, soon-ish.  (Which might be the full permissions impl.)
Comment 41 Justin Lebar (not reading bugmail) 2011-11-30 11:54:59 PST
Jonas, ping re comment 39?
Comment 42 Andreas Gal :gal 2011-12-02 02:27:03 PST
Comment on attachment 575294 [details] [diff] [review]
Patch v1

Looks good to me. Stealing from jonas.
Comment 43 Jonas Sicking (:sicking) 2011-12-02 11:48:18 PST
(In reply to Justin Lebar [:jlebar] from comment #39)
> Jonas, outstanding questions for you:
> 
> 1.
> 
> >> Is it really useful to have a VarCache? We could just check the pref everytime.
> 
> > I really have no idea when it's appropriate to use a cache or not, so I've been using caches 
> > everywhere.  I have similar code in the navigator for whether the vibrator is enabled.


Anywhere perf sensitive should definitely have them, but this doesn't look like it'll get called a lot. I don't really think it matters either way, it doesn't take a lot of code and I doubt having the pref observer there costs us a lot (if it does we need to fix that anyway).

> 2.
> 
> >> I think if sAllowScreen... isn't true, we could just return NS_OK and a default value. Throwing an 
> >> exception seems a bad idea to me.
> 
> > I'm concerned about lying to the page if we don't have to.  A page might, for example, try to 
> > determine whether Firefox is in the background by examining |screen.enabled && 
> > !document.mozVisible|.
> 
> > If we don't throw an exception, do we just drop writes to the properties?  The page does 
> > screen.enabled = false, but the next time we read screen.enabled, it's true?  I guess throwing an 
> > exception is also lame.

Exceptions are kind'a lame. document.mozVisible should return false if the screen is off so there should be no need to check screen.enabled.

> 3.
> 
> >> Ok, please call into the privilege manager
> > Does this code exist in the tree atm?  I can't find it, if so.

It does. See:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#2479
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#5643
Comment 44 Justin Lebar (not reading bugmail) 2011-12-02 12:15:21 PST
> document.mozVisible should return false if the screen is off so there should be no need to check 
> screen.enabled.

My concern is about naive code which is written expecting screen.enabled to be truthful and observing things which don't make much sense.

What do you think screen.enabled and screen.brightness should return when the page doesn't have permissions?  [true, 1], [true, -1]?
Comment 45 Justin Lebar (not reading bugmail) 2011-12-04 17:13:42 PST
I'm just going to default enabled to true and brightness to 1.  We can change this later if need be.
Comment 46 Mounir Lamouri (:mounir) 2011-12-04 18:43:30 PST
(In reply to Justin Lebar [:jlebar] from comment #45)
> I'm just going to default enabled to true and brightness to 1.  We can
> change this later if need be.

FWIW, that seems reasonable to me.
Comment 47 Justin Lebar (not reading bugmail) 2011-12-04 20:15:10 PST
WRT the permission manager:

Right now, the screen state is frobbed from Chrome:

  chrome://browser/content/shell.xul

I can allow chrome to frob the property and check the permission manager for content.  That sound OK?
Comment 48 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-04 22:18:06 PST
I heard we're not supposed to touch the permission manager, it's dying.

But just enabling the API for chrome code for now, and filing a followup to enable for content that blocks on The New Permissions Model, sounds fine to me.
Comment 49 Justin Lebar (not reading bugmail) 2011-12-05 01:00:34 PST
Created attachment 579010 [details] [diff] [review]
Patch v2
Comment 50 Justin Lebar (not reading bugmail) 2011-12-05 01:07:56 PST
Created attachment 579011 [details] [diff] [review]
Patch v2.1

rm some debugging code.
Comment 51 Justin Lebar (not reading bugmail) 2011-12-05 03:10:11 PST
Pushed to m-i.  Also need to push gonk parts to b2g repo.
Comment 52 Justin Lebar (not reading bugmail) 2011-12-05 03:10:40 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/407fba8cbd5a
Comment 53 Marco Bonardo [::mak] 2011-12-05 04:50:24 PST
backed out for build failures on windows

Hal.obj : error LNK2019: unresolved external symbol "bool __cdecl mozilla::hal_impl::GetScreenEnabled(void)" (?GetScreenEnabled@hal_impl@mozilla@@YA_NXZ) referenced in function "bool __cdecl mozilla::hal::GetScreenEnabled(void)" (?GetScreenEnabled@hal@mozilla@@YA_NXZ)
Hal.obj : error LNK2019: unresolved external symbol "void __cdecl mozilla::hal_impl::SetScreenEnabled(bool)" (?SetScreenEnabled@hal_impl@mozilla@@YAX_N@Z) referenced in function "void __cdecl mozilla::hal::SetScreenEnabled(bool)" (?SetScreenEnabled@hal@mozilla@@YAX_N@Z)
Hal.obj : error LNK2019: unresolved external symbol "double __cdecl mozilla::hal_impl::GetScreenBrightness(void)" (?GetScreenBrightness@hal_impl@mozilla@@YANXZ) referenced in function "double __cdecl mozilla::hal::GetScreenBrightness(void)" (?GetScreenBrightness@hal@mozilla@@YANXZ)
Hal.obj : error LNK2019: unresolved external symbol "void __cdecl mozilla::hal_impl::SetScreenBrightness(double)" (?SetScreenBrightness@hal_impl@mozilla@@YAXN@Z) referenced in function "void __cdecl mozilla::hal::SetScreenBrightness(double)" (?SetScreenBrightness@hal@mozilla@@YAXN@Z)
xul.dll : fatal error LNK1120: 4 unresolved externals
Comment 54 Justin Lebar (not reading bugmail) 2011-12-05 06:47:43 PST
Ah, there's Windows hal now.  That slipped by me.
Comment 55 Mounir Lamouri (:mounir) 2011-12-05 08:27:11 PST
(In reply to Justin Lebar [:jlebar] from comment #54)
> Ah, there's Windows hal now.  That slipped by me.

It has been created a few hours ago so you just ran out of luck ;)
Comment 56 Justin Lebar (not reading bugmail) 2011-12-05 22:15:52 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/da074baa9f05

I'll land on the B2G clone once this sticks on m-i.  (There's gonk-only code I can't land on m-c.)
Comment 57 Justin Lebar (not reading bugmail) 2011-12-06 00:02:03 PST
Landed in B2G:

https://github.com/cgjones/mozilla-central/commit/fe06ae146b7df4d1d339cea2848f8f3d25ab5328
https://github.com/andreasgal/B2G/commit/62af95c50cf40d5a2d4a2ff4b1f85a9e71267dca

I landed the whole patch v2.1, which is what I landed on m-c minus the WindowsHal stubs (because WindowsHal isn't in B2G yet.)
Comment 58 Ed Morley [:emorley] 2011-12-06 10:05:03 PST
https://hg.mozilla.org/mozilla-central/rev/da074baa9f05
Comment 59 Michael Wu [:mwu] 2011-12-20 11:40:11 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e861cc550d4 for the gonkhal bits
Comment 60 Ed Morley [:emorley] 2011-12-21 04:33:43 PST
https://hg.mozilla.org/mozilla-central/rev/3e861cc550d4

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