1050749 - Expose BatteryManager via getBattery() returning a Promise instead of a synchronous accessor (navigator.battery).

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Félix B. (:NinjaTrappeur)]

We need to create a new getBattery() method into the navigator interface in order to support the new W3C battery manager API draft:
https://dvcs.w3.org/hg/dap/raw-file/tip/battery/Overview.html#the-navigator-interface

Comment 1

[f.iovine]

Is anyone working on this? I started looking into it at the FirefoxOS Bug Squash Party in London last weekend, and I would like to implement the new getBattery() method.

Comment 2

[Félix B. (:NinjaTrappeur)]

No, you can go ahead.

Comment 3

[f.iovine]

Awesome, thanks Felix.

Comment 4

[Alexandre BM (:rednaks)]

still working on it ?

Comment 5

[f.iovine]

Hi Alexandre, I'm still working on it, but quite new to Mozilla Central development and working in spare time, so it might take a very long while actually (~2015 at a first glance)

Considering that Chrome and Opera has already implemented navigator.getBattery()

http://caniuse.com/#search=BatteryManager

I would happily reassign this bug to someone who has already experience in developing for Mozilla Central and can apply the patch faster than me. I can focus on taking care of the MDN documentation:

https://www.mail-archive.com/[e-mail]/msg11944.html

Comment 6

[Alexandre BM (:rednaks)]

Please feel free to ask if you need help :) 
otherwise please tell me I can take it.

Comment 7

[f.iovine]

Thanks! :cwiiis and :Paolo Amadini have helped me a lot, I'm dealing with building issues at the moment, so feel free to take it over. I can start working on another bug as soon as I got above issues fixed.

Check this mail by :ferjm for useful information about adding the getBattery() method to navigator:

https://www.mail-archive.com/[e-mail]/msg11877.html

Comment 8

[Alexandre BM (:rednaks)]

Attachment: Part 1 : exposing BatteryManager via navigator.getBattery() returing a promise

Comment 9

[Alexandre BM (:rednaks)]

I'm rewriting the tests too, I was able to run the basic test but not for the marionette, I don't have access to the try server actually.

Comment 10

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8529752 [details] [diff] [review]
Part 1 : exposing BatteryManager via navigator.getBattery() returing a promise

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

Good start! The algorithm used for creating the BatteryManager and the spec are a bit different. The spec says:

"The getBattery() method, when invoked, MUST run the following steps:

    * If battery promise is not null, return battery promise and abort these steps.
    * Otherwise, set battery promise to a newly created Promise.
    * Return battery promise and continue asynchronously.
    * Create a new BatteryManager object, and let battery be that object.
    * Resolve battery promise with battery.
"

So we shouldn't recreate the promise every time getBattery() is called. Instead we should return the same promise object.

This patch needs a DOM peer's review.

::: dom/base/Navigator.cpp
@@ +1350,5 @@
>      if (!mWindow) {
>        aRv.Throw(NS_ERROR_UNEXPECTED);
>        return nullptr;
>      }
>      NS_ENSURE_TRUE(mWindow->GetDocShell(), nullptr);

Can't simply return nullptr here since the return value should be a promise. You will need to throw.

::: dom/webidl/Navigator.webidl
@@ +123,5 @@
>  interface NavigatorBattery {
>      // XXXbz Per spec this should be non-nullable, but we return null in
>      // torn-down windows.  See bug 884925.
>      [Throws, Pref="dom.battery.enabled"]
> +    Promise<BatteryManager> getBattery();

The comment above could be removed

Comment 11

[Alexandre BM (:rednaks)]

So before attaching a new version of my patch I have some questions :

(In reply to Kan-Ru Chen [:kanru] from comment #10)
> "The getBattery() method, when invoked, MUST run the following steps:
> 
>     * If battery promise is not null, return battery promise and abort these
> steps.
>     * Otherwise, set battery promise to a newly created Promise.
>     * Return battery promise and continue asynchronously.
>     * Create a new BatteryManager object, and let battery be that object.

Does that mean we have to keep |mBatteryManager| ?  I think we need it when |Navigator::Invalidate()| is called to shutdown the BatteryManager.

>     * Resolve battery promise with battery.
> "
> 
> So we shouldn't recreate the promise every time getBattery() is called.
> Instead we should return the same promise object.
> 
> This patch needs a DOM peer's review.
> 
> ::: dom/base/Navigator.cpp
> @@ +1350,5 @@
> >      if (!mWindow) {
> >        aRv.Throw(NS_ERROR_UNEXPECTED);
> >        return nullptr;
> >      }
> >      NS_ENSURE_TRUE(mWindow->GetDocShell(), nullptr);
> 
> Can't simply return nullptr here since the return value should be a promise.
> You will need to throw.

What should we throw ?

Comment 12

[Alexandre BM (:rednaks)]

Attachment: WIP : Part 1 : exposing BatteryManager via navigator.getBattery() returing a promise

Comment 13

[Alexandre BM (:rednaks)]

Any feedback ? :)

Comment 14

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8531657 [details] [diff] [review]
WIP : Part 1 : exposing BatteryManager via navigator.getBattery() returing a promise

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

Sorry for the late reply. This patch looks good!

::: dom/base/Navigator.cpp
@@ +1359,5 @@
> +  if (!mWindow) {
> +    aRv.Throw(NS_ERROR_UNEXPECTED);
> +    return nullptr;
> +  }
> +  NS_ENSURE_TRUE(mWindow->GetDocShell(), nullptr);

We could throw NS_ERROR_UNEXPECTED as above.

Comment 15

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: 0001-expose-BatteryManager-via-getBattery.patch

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8640331 [details] [diff] [review]
0001-expose-BatteryManager-via-getBattery.patch

>+  if (mBatteryPromise) {
>+    mBatteryPromise = nullptr;

Skip the null-check; just null it out.

>+  nsRefPtr<Promise> pBatteryPromise = mBatteryPromise;

Why the "p" prefix?  Just "batteryPromise".

>+  pBatteryPromise = Promise::Create(go, rv);
>+  if (rv.Failed()) {
>+    aRv.Throw(NS_ERROR_UNEXPECTED);
>+    return nullptr;

Why not:

  batteryPromise = Promise::Create(go, aRv);
  if (aRv.Failed()) {
    return nullptr;
  }

No need for the extra ErrorResult.

>+  nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction([pThis]()->void {

Why do you need this?  If you had to get the batter manager async, that would make sense to do off a runnable, but you're just getting it synchronously.  It's not clear why the spec uses a promise here, by the way; is the expectation that creating the battery manager object might need to be done async but the actual battery status can be queried sync???

In any case, just set up mBatteryManager, resolve the promise, and return it.  No need for all this complexity.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Also, don't you need to update existing in-tree code that uses navigator.battery?  Both in mozilla-central and in gaia....

Comment 18

[Boris Zbarsky [:bzbarsky]]

So I posted a mail to the list, but there is an open spec issue here too: <http://www.w3.org/2009/dap/track/issues/167>.  It's been open for a year (!).

I think we should go back to the working group saying that we won't be implementing the API until this longstanding issue is resolved.  The API as currently specified just makes no sense.

Comment 19

[Boris Zbarsky [:bzbarsky]]

OK, so per the mailing list the API does make sense once you realize that the BatteryManager is supposed to return stale information (cached, updated via tasks, not whatever is current).  Is that what our implementation does?

Comment 20

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Boris Zbarsky [:bz] from comment #19)
> OK, so per the mailing list the API does make sense once you realize that
> the BatteryManager is supposed to return stale information (cached, updated
> via tasks, not whatever is current).  Is that what our implementation does?

Yes, we use cached value in the BatteryManager and it's receiving updates from
lower layer periodically.

Comment 21

[Kan-Ru Chen [:kanru] (UTC+9)]

We currently ship navigator.battery and google-chrome ships navigator.getBattery(). What should we do to the old interface? A few apps on our marketplace uses the battery api but I'm not sure if they are chrome compatible. I'm in favor of just replace the old interface with the new one since 1) it will be several gecko releases before we actually ship it. 2) google-chrome already ships the new interface.

PS. in order to ship both interface we have to overload the C++ Navigator::GetBattery interface since webidl uses same name mapping.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, I think this would be fine to break compat with the existing API. I doubt that the battery API is used enough that this would really be a problem.

Comment 23

[Boris Zbarsky [:bzbarsky]]

I'm pretty happy to just break compat and ship getBattery.  Just need to land the corresponding changes to consumers in mozilla-central and gaia at the same time as the patch lands.

> in order to ship both interface we have to overload the C++ Navigator::GetBattery
> interface since webidl uses same name mapping.

You could binaryName your way out of that, but let's not do that.

Comment 24

[Zibi Braniecki [:zbraniecki][:gandalf]]

If you're looking for people to help with that, I'll be happy to take the gaia updates.

Comment 25

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: 0001-expose-BatteryManager-via-getBattery.patch

Fixed problems, and updated related jsm.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #24)
> If you're looking for people to help with that, I'll be happy to take the
> gaia updates.

Thank you!

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8641602 [details] [diff] [review]
0001-expose-BatteryManager-via-getBattery.patch

>+  NS_ENSURE_TRUE(mWindow->GetDocShell(), nullptr);

I missed this the first time... this is not OK and will lead to a crash.  You need to throw on aRv before returning null.

Also, you're doing this _before_ you null-check mWindow.  That's a bit backwards.  The null-check of mWindow needs to move to before this line.

I don't see changes to dom/battery/test in here.  Have you done a try run for this patch?

r=me with the null issues above fixed and the remaining tests also modified.

Comment 27

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: 0001-expose-BatteryManager-via-getBattery.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5d0be54b235

Comment 28

[Zibi Braniecki [:zbraniecki][:gandalf]]

For the gaia fix (bug 1190243) I'd prefer to get a window of transition where both - navigator.battery and navigator.getBattery work.

That would allow me to ensure that all the patches and tests work.

I guess that really a two week window would be optimal if we can afford that.

Comment 29

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

I'll working on that patch, to add the old API and tests back.

Comment 30

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: 0001-expose-BatteryManager-via-getBattery.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45ca41a52ecf

Changes from the last patch:

1. Add the old battery API back.
2. Add the old battery test cases back.

Comment 31

[Andrea Marchesini [:baku]]

Comment on attachment 8649632 [details] [diff] [review]
0001-expose-BatteryManager-via-getBattery.patch

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

::: dom/base/Navigator.cpp
@@ +1457,5 @@
>  //*****************************************************************************
>  //    Navigator::nsINavigatorBattery
>  //*****************************************************************************
>  
> +already_AddRefed<Promise>

Promise*

@@ +1460,5 @@
>  
> +already_AddRefed<Promise>
> +Navigator::GetBattery(ErrorResult& aRv)
> +{
> +  nsRefPtr<Promise> batteryPromise = mBatteryPromise;

if (mBatteryPromise) {
  return mBatteryPromise;
}

@@ +1471,5 @@
> +    return nullptr;
> +  }
> +
> +  nsCOMPtr<nsIGlobalObject> go = do_QueryInterface(mWindow);
> +  batteryPromise = Promise::Create(go, aRv);

nsRefPtr<Promise> batteryPromise = ...

@@ +1472,5 @@
> +  }
> +
> +  nsCOMPtr<nsIGlobalObject> go = do_QueryInterface(mWindow);
> +  batteryPromise = Promise::Create(go, aRv);
> +  if (aRv.Failed()) {

if (NS_WARN_IF(aRv.Failed()))

@@ +1484,5 @@
> +  }
> +
> +  mBatteryPromise->MaybeResolve(mBatteryManager);
> +
> +  return batteryPromise.forget();

return mBatteryPromise;

::: dom/battery/BatteryManager.cpp
@@ +10,5 @@
>  #include "mozilla/DOMEventTargetHelper.h"
>  #include "mozilla/Hal.h"
>  #include "mozilla/dom/BatteryManagerBinding.h"
>  #include "nsIDOMClassInfo.h"
> +#include "mozilla/Preferences.h"

alphabetic order. Move it to line 13.

@@ +58,5 @@
>  }
>  
> +bool
> +BatteryManager::Charging() const
> +{

MOZ_ASSERT(NS_IsMainThread());

@@ +77,3 @@
>  double
>  BatteryManager::DischargingTime() const
>  {

MOZ_ASSERT(NS_IsMainThread());

@@ +93,5 @@
>  }
>  
>  double
>  BatteryManager::ChargingTime() const
>  {

MOZ_ASSERT(NS_IsMainThread());

@@ +111,5 @@
>  }
>  
> +double
> +BatteryManager::Level() const
> +{

MOZ_ASSERT(NS_IsMainThread());

Comment 32

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: 0001-expose-BatteryManager-via-getBattery.patch

Fixed above problems.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84bb763190f8

Comment 33

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6f579ae99a6

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6f579ae99a6

Comment 35

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

This broke web platform tests like https://treeherder.mozilla.org/logviewer.html#?job_id=13111919&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5392300058b6

Comment 36

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

I've tried to comply the spec in web platform tests, but I've some questions:

1. According to the test, getBattery() should throw TypeError if *this* does not implement Navigator interface.
But in my implementation, the exception will reject by the returned promise. Is there a way in Gecko to check this before returning a Promise?

2. Some errors are related to EventTarget, so I think they should be fixed in other bugs.

Comment 37

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: 0001-expose-BatteryManager-via-getBattery.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d4634a1a163

Changed from the last patch:
Update testing/web-platform/meta/battery-status/*.ini

BTW, the interface test for navigator.getBattery() does not make sense at all, maybe we should fire a bug to W3C.

Comment 38

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Wei-Cheng Pan [:wcp] [:legnaleurc] from comment #36)
> 1. According to the test, getBattery() should throw TypeError if *this* does
> not implement Navigator interface.
> But in my implementation, the exception will reject by the returned promise.
> Is there a way in Gecko to check this before returning a Promise?

I think it depends on whether a Promise returning function should throw when "this" does not implement the interface or reject the Promise. Looks like our WebIDL wrapper rejected it automatically.

(In reply to Wei-Cheng Pan [:wcp] [:legnaleurc] from comment #37)
> BTW, the interface test for navigator.getBattery() does not make sense at
> all, maybe we should fire a bug to W3C.

Looks like the W3C interface test harness does not support function returning a Promise.

Comment 39

[James Graham [:jgraham]]

Comment on attachment 8652135 [details] [diff] [review]
0001-expose-BatteryManager-via-getBattery.patch

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

r+ for the three ini file changes assuming they pass try. I didn't review anything else ofc.

Please do file an upstream issue if the interfaces tests aren't working well with promise-returning interfaces; the test generation code predates promises by some time so that likely needs to be updated.

Comment 40

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: 0001-expose-BatteryManager-via-getBattery.patch

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a4340bbb7a3

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5a4340bbb7a3

Comment 43

[Anssi Kostiainen]

> (In reply to Wei-Cheng Pan [:wcp] [:legnaleurc] from comment #37)
> > BTW, the interface test for navigator.getBattery() does not make sense at
> > all, maybe we should fire a bug to W3C.
> 
> Looks like the W3C interface test harness does not support function
> returning a Promise.

I informed the W3C group and the spec Test Facilitator Zhiqiang:

https://lists.w3.org/Archives/Public/public-device-apis/2015Aug/0013.html

Please feel free to chime in on the W3C list with follow-ups re the test suite.

Comment 44

[John Dai[:jdai]]

Hi :wcp, I got a marionette-webapi error, after I dig into the error message, I found out manifest.ini has defined test_deprecated_battery_status_unknown.js, but m-c only has test_deprecated_battery_unknown.js file. Could you confirm of this? Thank you.

Comment 45

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Yes, the file name should be test_deprecated_battery_status_unknown.js .
I apologize for this.
Should I create another patch, or you are already working on this?

Comment 46

[John Dai[:jdai]]

(In reply to Wei-Cheng Pan [:wcp] [:legnaleurc] from comment #45)
> Yes, the file name should be test_deprecated_battery_status_unknown.js .
> I apologize for this.
> Should I create another patch, or you are already working on this?

Yes, please create another patch for this. thank you.

Comment 47

[Kohei Yoshino]

Added the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/navigator-battery-has-been-deprecated-in-favor-of-async-getbattery-method/

Comment 48

[f.iovine]

Tested the API on Firefox Nightly 43.0a1 (2015-09-13) using http://franciov.github.io/low-energy-messenger/

Updated the compatibility info on MDN: https://developer.mozilla.org/en-US/docs/Web/API/Battery_Status_API

Updated the Firefox 43 release note: https://developer.mozilla.org/en-US/Firefox/Releases/43

Updated the MozHacks article about this bug: https://hacks.mozilla.org/2015/05/lets-get-charged-updates-to-the-battery-status-api/

Comment 49

[Chris Mills [:cmills]]

Documentation complete - thanks franciov!