1259335 - Remove deprecated navigator.battery API

Status: VERIFIED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

As per bug 1190243 we don't need this API anymore.

Comment 1

[Chris Peterson [:cpeterson]]

Support for navigator.getBattery(), the standard Promise-based API, landed in Firefox 43 (bug 1050749), so we can remove the deprecated navigator.battery API. No other browser supports navigator.battery. They all either support navigator.getBattery() or don't support the battery status API:

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

Here is the Firefox Site Compatibility page warning about the deprecation of navigator.battery:

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

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: part-1-deprecate-navigator-battery.patch

Part 1: Log deprecation warning for navigator.battery.

I'd like to uplift this deprecation warning to Aurora 48 (or even Beta 47) and then remove the API in Firefox 49 or 50. navigator.getBattery(), the standard Promise-based API replacing navigator.battery, landed in Firefox 43 (bug 1050749).

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: part-2-remove-navigator-battery.patch

Part 2: Remove deprecated navigator.battery API and tests.

Comment 4

[Chris Peterson [:cpeterson]]

Comment on attachment 8755292 [details] [diff] [review]
part-2-remove-navigator-battery.patch

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

::: dom/battery/test/marionette/test_battery_status_full.js
@@ +6,3 @@
>  MARIONETTE_TIMEOUT = 10000;
>  
> +var battery = null;

btw, `battery` is initialized below (line 14) when the getBattery() promise is resolved. Initializing `battery = window.navigator.battery` is old test code from before this test was ported to the Promised-based navigator.getBattery() API.

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8755292 [details] [diff] [review]
part-2-remove-navigator-battery.patch

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

Looks good to me. Any reason why you want to uplift the first patch to aurora instead waiting for another cycle?

::: dom/battery/test/marionette/test_battery_status_full.js
@@ +6,3 @@
>  MARIONETTE_TIMEOUT = 10000;
>  
> +var battery = null;

remove '= null'; by default a variable is set to undefined.

::: dom/battery/test/marionette/test_battery_status_not_charging.js
@@ +6,3 @@
>  MARIONETTE_TIMEOUT = 10000;
>  
> +var battery = null;

same here.

::: dom/battery/test/marionette/test_battery_status_unknown.js
@@ +6,3 @@
>  MARIONETTE_TIMEOUT = 10000;
>  
> +var battery = null;

here.

Comment 6

[Chris Peterson [:cpeterson]]

(In reply to Andrea Marchesini (:baku) from comment #5)
> Comment on attachment 8755292 [details] [diff] [review]
> part-2-remove-navigator-battery.patch
> 
> Review of attachment 8755292 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me. Any reason why you want to uplift the first patch to
> aurora instead waiting for another cycle?

It would be convenient to land the removal patch now (in Nightly 49) so I don't need to remember to land it in Nightly 50 two weeks from now, possibly needing to rebase it on top of any other changes. <:) But I can wait until Nightly 50 to remove the API, if you prefer.

Uplifting the warning patch to Aurora would at least give one release cycle with the warning before the API was removed.


> ::: dom/battery/test/marionette/test_battery_status_full.js
> @@ +6,3 @@
> >  MARIONETTE_TIMEOUT = 10000;
> >  
> > +var battery = null;
> 
> remove '= null'; by default a variable is set to undefined.

Thanks. I'll fix these.

Comment 7

[Pulsebot]

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

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Chris Peterson [:cpeterson]]

Attachment: part-1.5-collect-battery-telemetry.patch

Collect use count telemetry for Battery Status API. Before removing navigator.battery in Nightly 50, I'd like to collect telemetry on the usage of the navigator.battery and navigator.getBattery() APIs to:

1. compare the usage of the APIs before and after the deprecation warning.
2. and watch whether navigator.battery callers move to navigator.getBattery().

Comment 10

[Andrea Marchesini [:baku]]

Comment on attachment 8756201 [details] [diff] [review]
part-1.5-collect-battery-telemetry.patch

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

::: dom/base/Navigator.cpp
@@ +416,5 @@
> +
> +  // Did this page access navigator.battery?
> +  if (mReportBatteryTelemetry) {
> +    Telemetry::Accumulate(Telemetry::BATTERY_STATUS_COUNT, 0);
> +    mReportBatteryTelemetry = false;

Why here in invalidate?

@@ +1606,5 @@
>    }
>  
>    mBatteryPromise->MaybeResolve(mBatteryManager);
>  
> +  mReportGetBatteryTelemetry = true;

What about if here you do:

if (!mTelemetryGetBatteryReported) {
  Telemetry::Accumulate(Telemetry::BATTERY_STATUS_COUNT, 1);
  mTelemetryGetBatteryReported = true;
}

@@ +1630,5 @@
>    if (doc) {
>      doc->WarnOnceAbout(nsIDocument::eNavigatorBattery);
>    }
>  
> +  mReportBatteryTelemetry = true;

same here.

Comment 11

[Chris Peterson [:cpeterson]]

(In reply to Andrea Marchesini (:baku) from comment #10)
> What about if here you do:
> 
> if (!mTelemetryGetBatteryReported) {
>   Telemetry::Accumulate(Telemetry::BATTERY_STATUS_COUNT, 1);
>   mTelemetryGetBatteryReported = true;
> }

Thanks. That approach looks clearer and more localized. I will post a new telemetry patch.

Comment 12

[Chris Peterson [:cpeterson]]

Comment on attachment 8755289 [details] [diff] [review]
part-1-deprecate-navigator-battery.patch

Approval Request Comment
[Feature/regressing bug #]: This patch logs a deprecation warning to the web console for a feature that was replaced by the new battery API from bug 1050749.
[User impact if declined]: Web developers have just one release cycle (FF 49) instead of two (FF 48 and 49) before this deprecated API is removed.
[Describe test coverage new/current, TreeHerder]: No automated test coverage.
[Risks and why]: 
[String/UUID change made/needed]: A new console warning string was added: NavigatorBatteryWarning="navigator.battery is deprecated. Use navigator.getBattery() instead." This warning will only be seen by web developers and the string be removed in FF 50 when I remove the deprecated API.

Comment 13

[Chris Peterson [:cpeterson]]

Attachment: part-1.5-collect-battery-telemetry-v2.patch

V2 of my telemetry patch simplifies the reporting of navigator.battery and getBattery() usage.

We no longer need a mGetBatteryTelemetryReported member variable flag because we can piggyback on the mBatteryPromise member variable. It is initialized on the first call to navigator.getBattery() so we can use it as our flag to know whether we already reported getBattery() telemetry.

We still need a mBatteryTelemetryReported member variable flag for navigator.battery.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11f6b550511f
Part 1.5: Collect use count telemetry for Battery Status API. r=baku

Comment 15

[Chris Peterson [:cpeterson]]

Comment on attachment 8759026 [details] [diff] [review]
part-1.5-collect-battery-telemetry-v2.patch

Approval Request Comment
[Feature/regressing bug #]: None
[User impact if declined]: No user impact, but we will have to wait longer before we know how much the navigator.battery API is used in the field.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Low risk because we're just adding some telemetry probes for usage of the navigator.battery API.
[String/UUID change made/needed]: No string changes.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8755289 [details] [diff] [review]
part-1-deprecate-navigator-battery.patch

Should have a too big impact, and will be help to get the message sooner, taking it.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3134d875a771
Part 1.5b: Report telemetry for navigator.battery use. r=baku

Comment 18

[Chris Peterson [:cpeterson]]

(In reply to Pulsebot from comment #17)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3134d875a771
> Part 1.5b: Report telemetry for navigator.battery use. r=baku

When I landed telemetry patch part 1.5 in comment 7, I forgot to include this bit of code. It was part of the reviewed patch attached to this bug. The code just got lost when I rebased my patch.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11f6b550511f

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8755289 [details] [diff] [review]
part-1-deprecate-navigator-battery.patch

Moving the approval to beta so that this can land for 48 beta 1 or beta 2, since it didn't land on aurora 48 before the merge.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69c00649c977
Part 2: Remove deprecated navigator.battery API and tests. r=baku

Comment 22

[Chris Peterson [:cpeterson]]

The console warning about navigator.battery landed in 49 and the navigator.battery API has been removed in 50.

Comment 23

[Kohei Yoshino [:kohei]]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/navigator-battery-has-been-removed-in-favour-of-async-getbattery-method/

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3134d875a771

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/69c00649c977

Comment 26

[Carsten Book [:Tomcat]]

is part 2 also planned/expected to uplift.

in this case it need l10n approval since i got:

* File used for localization (dom/locales/en-US/chrome/dom/dom.properties) altered in this changeset *
remote: This repository is string frozen. Please request explicit permission from
remote: release managers to break string freeze in your bug.
remote: If you have that explicit permission, denote that by including in
remote: your commit message l10n=.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Yes, I think that's ok. Flod, do you have any objection to this string? Chris points out  in comment 12 that it will be only a console warning for developers, and will be removed in 50.

Comment 28

[Chris Peterson [:cpeterson]]

(In reply to Carsten Book [:Tomcat] from comment #26)
> is part 2 also planned/expected to uplift.

It would have been nice to uplift the warning, but it is not a big deal. I uplifted the telemetry changes to 48:

https://hg.mozilla.org/releases/mozilla-beta/rev/a23a6103b9be

The current roll-out of this feature removal is:

* 48 = Collect telemetry on navigator.battery and navigator.getBattery() API usage.
* 49 = Report deprecation warning for navigator.battery API usage.
* 50 = Remove navigator.battery API.

Comment 29

[Francesco Lodolo [:flod]]

If the patch (I'm looking at part 2) is only removing the string, it would be great to have a version of it that doesn't remove it, and let the removal rides the train.

If that's not possible given the size of the patch, let's land it as it is (the sooner the better).

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8755289 [details] [diff] [review]
part-1-deprecate-navigator-battery.patch

Clearning flags as per CPeterson's request.

Comment 31

[Eric Shepherd [:sheppy]]

Updated:

https://developer.mozilla.org/en-US/docs/Web/API/Battery_Status_API
https://developer.mozilla.org/en-US/docs/Web/API/Navigator/battery
https://developer.mozilla.org/en-US/Firefox/Releases/50

This should be done, documentation-wise.