978857 - Create a Battery.jsm module that supports battery spoofing

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Manish Goregaokar [:manishearth]]

Currently, components using the battery (eg bug 506975) directly call navigator.battery.

We can provide a js module that wraps around the BatteryManager and encapsulates the functions, but also has a "debug" mode where the values can be spoofed.

This would help in writing mochitests, currently there is no way of spoofing the battery.

Comment 1

[Manish Goregaokar [:manishearth]]

Attachment: Basic structure

I haven't been able to test this out yet, but this is what the Battery.jsm probably would look like. Any feedback? Would this work?


I assume that when you run Cu.import it imports the *same* object (so that the faked preferences are consistent across files that import it)

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Tim, feedback ping.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8388067 [details] [diff] [review]
Basic structure

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

Good base.
Could you add a few tests, too?

::: toolkit/modules/Battery.jsm
@@ +14,5 @@
> +
> +
> +this.Battery =  {
> +  _battery: null,
> +  fake: false,

All of these fields should be private to the module. To enter/exit fake mode, I would use a preference. Say "testing.battery.fake".

@@ +20,5 @@
> +  _chargingTime: 0,
> +  _dischargingTime: Infinity,
> +  _level: 1,
> +  init: function(){
> +    this.Battery._battery = Services.appShell.hiddenDOMWindow.navigator.battery;

This method doesn't look very useful.

@@ +29,5 @@
> +  Object.defineProperty(this.Battery, i, {
> +    get: function() {
> +            return this.Battery.fake?this.Battery["_"+i]:this.Battery._battery[i];
> +         },
> +    set: function(fakeSetting) { this.Battery["_"+i]=fakeSetting; }

This should throw if `fake` is `false`.

Comment 4

[Manish Goregaokar [:manishearth]]

Attachment: Module + tests

I improved the module and added a test. 

I tried xpcshell, but xpcshell doesn't seem to be able to access the `navigator` APIs through `hiddenDOMWindow` (I suspect the xpcshell harness doesn't load these parts), so I ended up using mochitest-browser.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8447137 [details] [diff] [review]
Module + tests

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

I don't see the test file. Did you forget to add it?

::: toolkit/modules/Battery.jsm
@@ +1,5 @@
> +// -*- Mode: javascript; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> +// This Source Code Form is subject to the terms of the Mozilla Public
> +// License, v. 2.0. If a copy of the MPL was not distributed with this
> +// file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

"use strict";

@@ +3,5 @@
> +// License, v. 2.0. If a copy of the MPL was not distributed with this
> +// file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +// This module wraps around navigator.battery (https://developer.mozilla.org/en-US/docs/Web/API/Navigator.battery)
> +// and provides a framework for spoofing battery values in test code

Nit: /** */ for these comments.

@@ +11,5 @@
> +const Ci = Components.interfaces;
> +const Cc = Components.classes;
> +const Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

Please add `this` as a second argument. This is necessary for use in B2G.

@@ +15,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +                                  "resource://gre/modules/Services.jsm");
> +

Everything needs documentation.

@@ +16,5 @@
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +                                  "resource://gre/modules/Services.jsm");
> +
> +_FakeBattery = {

gFakeBattery. Also, please declare it.

@@ +32,5 @@
> +for (let i of ["charging", "chargingTime", "dischargingTime", "level"]) {
> +  addProperty(i);
> +}
> +
> +function addProperty(prop) {

I believe that you should inline this in the loop above.

@@ +34,5 @@
> +}
> +
> +function addProperty(prop) {
> +  Object.defineProperty(this.Battery, prop, {
> +    get: function() {

Nit: Please use two spaces for indentation, everywhere in this file.

@@ +35,5 @@
> +
> +function addProperty(prop) {
> +  Object.defineProperty(this.Battery, prop, {
> +    get: function() {
> +            return Debugging.fake ? _FakeBattery["_" + prop] : Services.appShell.hiddenDOMWindow.navigator.battery[prop];

That line is too long for my tastes :) Can you make this a `if`?

@@ +39,5 @@
> +            return Debugging.fake ? _FakeBattery["_" + prop] : Services.appShell.hiddenDOMWindow.navigator.battery[prop];
> +         },
> +    set: function(fakeSetting) {
> +        if (!Debugging.fake) {
> +            throw "Tried to set fake battery value when battery spoofing was disabled";

Please throw an Error rather than a string. Otherwise, we can't get stack traces.

Comment 6

[Manish Goregaokar [:manishearth]]

Attachment: Module + tests

Ah, forgot to hg add.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8447153 [details] [diff] [review]
Module + tests

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

We're getting there.

::: toolkit/modules/Battery.jsm
@@ +6,5 @@
> +"use strict";
> +
> +/** This module wraps around navigator.battery (https://developer.mozilla.org/en-US/docs/Web/API/Navigator.battery)
> +  * and provides a framework for spoofing battery values in test code
> +  */

You should explain how to spoof the battery.

Nit:
/**
 * This module ...
 */

Oh, and a dot at the end of the sentence would be nice :)

@@ +17,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);
> +
> +// Load Services, for the BatteryManager API
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +                                  "resource://gre/modules/Services.jsm");

Usually, we only put 2 whitespaces here.

@@ +19,5 @@
> +// Load Services, for the BatteryManager API
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +                                  "resource://gre/modules/Services.jsm");
> +
> +// Store all faked values here

// Values for the fake battery. See the documentation of Navigator.battery for the meaning of each field.

@@ +29,5 @@
> +}
> +
> +// BackendPass-exported object for toggling spoofing
> +this.Debugging = {
> +    fake: false

/**
 * If `false`, use the DOM Battery implementation.
 * Set it to `true` if you need to fake battery values
 * for testing or debugging purposes.
 */
Also, please use consistent indentation (two spaces).

@@ +34,5 @@
> +}
> +
> +this.Battery = {};
> +
> +for (let i of ["charging", "chargingTime", "dischargingTime", "level"]) {

Nit: since it's a key and not an index, I would prefer `k` instead of `i`.

@@ +37,5 @@
> +
> +for (let i of ["charging", "chargingTime", "dischargingTime", "level"]) {
> +  let prop = i;
> +  Object.defineProperty(this.Battery, prop, {
> +    get: function() {

Two spaces for indentation.

@@ +40,5 @@
> +  Object.defineProperty(this.Battery, prop, {
> +    get: function() {
> +            // Return fake value if spoofing is enabled, otherwise fetch the real value from the BatteryManager API
> +            if (Debugging.fake) {
> +              return gFakeBattery["_" + prop];

The "_" looks inconsistent with `gFakeBattery`.

::: toolkit/modules/tests/browser/browser_Battery.js
@@ +8,5 @@
> +function test() {
> +  is(imported.Debugging.fake, false, "Battery spoofing is initially false")
> +
> +  for (let i of ["charging", "chargingTime", "dischargingTime", "level"]) {
> +    try { 

Nit: whitespace.

@@ +9,5 @@
> +  is(imported.Debugging.fake, false, "Battery spoofing is initially false")
> +
> +  for (let i of ["charging", "chargingTime", "dischargingTime", "level"]) {
> +    try { 
> +      Battery[i] = 10;

This value is incorrect for both `charging` and `level`.

@@ +11,5 @@
> +  for (let i of ["charging", "chargingTime", "dischargingTime", "level"]) {
> +    try { 
> +      Battery[i] = 10;
> +      do_throw("Setting battery " + i + "preference without spoofing enabled should throw");
> +    } catch (ex) {}

This do_throw messes things up. You should rather use

 Assert.throws(() => /* Your operation */, /* Message */)

@@ +12,5 @@
> +    try { 
> +      Battery[i] = 10;
> +      do_throw("Setting battery " + i + "preference without spoofing enabled should throw");
> +    } catch (ex) {}
> +    ok(Battery[i] === Services.appShell.hiddenDOMWindow.navigator.battery[i], "Battery "+ i + " is correctly set");

Why `===` ?

Comment 8

[Manish Goregaokar [:manishearth]]

Attachment: Patch with documentation

> @@ +9,5 @@
> > +  is(imported.Debugging.fake, false, "Battery spoofing is initially false")
> > +
> > +  for (let i of ["charging", "chargingTime", "dischargingTime", "level"]) {
> > +    try { 
> > +      Battery[i] = 10;
> 
> This value is incorrect for both `charging` and `level`.

Doesn't matter, we're only checking if it throws.

As for the ===, I kept it there to avoid missing undefined properties and similar, but there's nothing on the RHS that coerces to undefined, so we're okay.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8447754 [details] [diff] [review]
Patch with documentation

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

This looks good, with the trivial nit below.

::: toolkit/modules/Battery.jsm
@@ +42,5 @@
> +
> +this.Battery = {};
> +
> +for (let k of ["charging", "chargingTime", "dischargingTime", "level"]) {
> +  let prop = k;

That alias is not useful. Please get rid of either `k` or `prop`.

::: toolkit/modules/tests/browser/browser_Battery.js
@@ +7,5 @@
> +
> +function test() {
> +  is(imported.Debugging.fake, false, "Battery spoofing is initially false")
> +
> +  for (let i of ["charging", "chargingTime", "dischargingTime", "level"]) {

Nit: either `k` or `prop`, but not `i`, which should generally be reserved for indices.

Comment 10

[Manish Goregaokar [:manishearth]]

The alias copies it as a local variable.

There's a getter/setter being created there, and `k` gets captured in its scope. k changes through the loop, and thee changes are reflected when you call the setter, so in the end the getters and setters will return on the "level" variable. Hence the `prop`, which copies `k` by value into an iteration-local scope.


I'll fix the other bit and upload when I'm back on Linux.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Oops, my bad. You are absolutely right.

Comment 12

[Manish Goregaokar [:manishearth]]

Attachment: Final patch

Fixed the second nit.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=a8f4441af311

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8448694 [details] [diff] [review]
Final patch

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

Not entirely :)

::: toolkit/modules/tests/browser/browser_Battery.js
@@ +8,5 @@
> +function test() {
> +  is(imported.Debugging.fake, false, "Battery spoofing is initially false")
> +
> +  for (let k of ["charging", "chargingTime", "dischargingTime", "level"]) {
> +    Assert.throws(() => Battery[i] = 10, "Setting battery " + i + "preference without spoofing enabled should throw");

`k`

@@ +9,5 @@
> +  is(imported.Debugging.fake, false, "Battery spoofing is initially false")
> +
> +  for (let k of ["charging", "chargingTime", "dischargingTime", "level"]) {
> +    Assert.throws(() => Battery[i] = 10, "Setting battery " + i + "preference without spoofing enabled should throw");
> +    ok(Battery[i] == Services.appShell.hiddenDOMWindow.navigator.battery[i], "Battery "+ i + " is correctly set");

`k`

Comment 14

[Manish Goregaokar [:manishearth]]

Attachment: Final patch

Wow, that was stupid of me. And wow, that was a fast review ;)

Trypush cancelled, repushed: https://tbpl.mozilla.org/?tree=Try&rev=f46b0223d95a

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8448696 [details] [diff] [review]
Final patch

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

Looks good.
We can land this if it passes tests.

Comment 16

[Manish Goregaokar [:manishearth]]

Everything green (https://tbpl.mozilla.org/?tree=Try&rev=f46b0223d95a), marking for checkin. I fixed the commit message before uploading above.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e013301d93a

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3e013301d93a