Closed Bug 1050749 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: NinjaTrappeur, Assigned: wcpan, Mentored)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 8 obsolete files)

49.39 KB, patch
wcpan
: review+
Details | Diff | Splinter Review
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
Blocks: 1047119
OS: Linux → All
Hardware: x86_64 → All
Assignee: nobody → baylac.felix
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.
Assignee: baylac.felix → nobody
No, you can go ahead.
Awesome, thanks Felix.
Assignee: nobody → f.iovine
still working on it ?
Flags: needinfo?(f.iovine)
Status: UNCONFIRMED → NEW
Ever confirmed: true
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/dev-b2g@lists.mozilla.org/msg11944.html
Flags: needinfo?(f.iovine)
Please feel free to ask if you need help :) 
otherwise please tell me I can take it.
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/dev-b2g@lists.mozilla.org/msg11877.html
Assignee: f.iovine → s
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.
Status: NEW → ASSIGNED
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
Attachment #8529752 - Flags: review?(kchen) → feedback+
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 ?
Any feedback ? :)
Flags: needinfo?(kchen)
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.
Attachment #8531657 - Flags: feedback?(kchen) → feedback+
Flags: needinfo?(kchen)
Assignee: s → wpan
Attachment #8531657 - Attachment is obsolete: true
Attachment #8640331 - Flags: review?(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.
Attachment #8640331 - Flags: review?(bzbarsky) → review-
Also, don't you need to update existing in-tree code that uses navigator.battery?  Both in mozilla-central and in gaia....
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.
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?
Flags: needinfo?(wpan)
(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.
Flags: needinfo?(wpan)
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.
Flags: needinfo?(jonas)
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(jonas)
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.
Flags: needinfo?(bzbarsky)
If you're looking for people to help with that, I'll be happy to take the gaia updates.
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!
Attachment #8640331 - Attachment is obsolete: true
Attachment #8641602 - Flags: review?(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.
Attachment #8641602 - Flags: review?(bzbarsky) → review+
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.
I'll working on that patch, to add the old API and tests back.
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.
Attachment #8642232 - Attachment is obsolete: true
Attachment #8649632 - Flags: review?(amarchesini)
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());
Attachment #8649632 - Flags: review?(amarchesini) → review+
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.
Flags: needinfo?(wpan)
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.
Attachment #8650393 - Attachment is obsolete: true
Attachment #8652135 - Flags: review?(james)
(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 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.
Attachment #8652135 - Flags: review?(james) → review+
https://hg.mozilla.org/mozilla-central/rev/5a4340bbb7a3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
> (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.
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.
Flags: needinfo?(wpan)
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?
Flags: needinfo?(wpan)
(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.
Depends on: 1200960
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/
Documentation complete - thanks franciov!
Duplicate of this bug: 1050746
You need to log in before you can comment on or make changes to this bug.