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

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/

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

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

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

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

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

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

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.

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

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

Pushed by cpeterson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3134d875a771 Part 1.5b: Report telemetry for navigator.battery use. r=baku

(In reply to Pulsebot from comment #17) > Pushed by cpeterson@mozilla.com: > 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 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.

Pushed by cpeterson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/69c00649c977 Part 2: Remove deprecated navigator.battery API and tests. r=baku

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

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

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

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.

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

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 on attachment 8755289 [details] [diff] [review] part-1-deprecate-navigator-battery.patch Clearning flags as per CPeterson's request.

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.