Create a Battery.jsm module that supports battery spoofing

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: manishearth, Assigned: manishearth, Mentored)

Tracking

(Blocks 1 bug)

Trunk
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [battery])

Attachments

(1 attachment, 5 obsolete attachments)

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

Updated

5 years ago
Blocks: 506975
Assignee

Updated

5 years ago
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Assignee

Updated

5 years ago
Whiteboard: [battery]
Assignee

Updated

5 years ago
Component: Session Restore → General
OS: Linux → All
Hardware: x86_64 → All
Product: Firefox → Toolkit
Assignee

Updated

5 years ago
See Also: → 446418
Posted patch Basic structure (obsolete) — Splinter Review
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)
Attachment #8388067 - Flags: feedback?(ttaubert)
Tim, feedback ping.
Flags: needinfo?(ttaubert)
Attachment #8388067 - Flags: feedback?(ttaubert) → feedback?(dteller)
Flags: needinfo?(ttaubert)
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`.
Attachment #8388067 - Flags: feedback?(dteller) → feedback+
Posted patch Module + tests (obsolete) — Splinter Review
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.
Attachment #8388067 - Attachment is obsolete: true
Attachment #8447137 - Flags: review?(dteller)
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.
Attachment #8447137 - Flags: review?(dteller) → feedback+
Posted patch Module + tests (obsolete) — Splinter Review
Ah, forgot to hg add.
Attachment #8447137 - Attachment is obsolete: true
Attachment #8447153 - Flags: review?(dteller)
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 `===` ?
Attachment #8447153 - Flags: review?(dteller) → feedback+
Posted patch Patch with documentation (obsolete) — Splinter Review
> @@ +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.
Attachment #8447153 - Attachment is obsolete: true
Attachment #8447754 - Flags: review?(dteller)
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.
Attachment #8447754 - Flags: review?(dteller) → review+
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.
Oops, my bad. You are absolutely right.
Posted patch Final patch (obsolete) — Splinter Review
Fixed the second nit.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=a8f4441af311
Attachment #8447754 - Attachment is obsolete: true
Attachment #8448694 - Flags: review?(dteller)
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`
Attachment #8448694 - Flags: review?(dteller)
Posted patch Final patchSplinter Review
Wow, that was stupid of me. And wow, that was a fast review ;)

Trypush cancelled, repushed: https://tbpl.mozilla.org/?tree=Try&rev=f46b0223d95a
Attachment #8448694 - Attachment is obsolete: true
Attachment #8448696 - Flags: review?(dteller)
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.
Attachment #8448696 - Flags: review?(dteller) → review+
Everything green (https://tbpl.mozilla.org/?tree=Try&rev=f46b0223d95a), marking for checkin. I fixed the commit message before uploading above.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3e013301d93a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1351078
You need to log in before you can comment on or make changes to this bug.