1441976 - Restrict BatteryManager to chrome script

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, task, P3)

Description

[Jonathan Kingston [:jkt] he/him]

In Bug 1313580 navigator.getBattery was restricted to parent process however the BatteryManager interface itself is still exposed.

This just looks like an oversight and removing it might help with web-compat issues expecting the API.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I think you mean making the BatteryManager restricted to chrome script?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Anyway, I think this should be as simple as adding [ChromeOnly] to BatterManager interface.

Comment 3

[Jonathan Kingston [:jkt] he/him]

Ran it on try to see what happens.

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

I wasn't sure if setting "xbl:true" on the interfaces test file or just ripping it out was the right direction here.

Comment 4

[Jonathan Kingston [:jkt] he/him]

:qdot asked why we were keeping this around at all instead of just restricting to Chrome. I suspect there isn't a reason to keep this for chrome anymore either.

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

ni?'ing dhylands, since he still heads up HAL and might know if this is used anywhere that I wasn't seeing. I'll take the bug too.

There were some mentions of it being used in addons in Bug 1313580, but that's pre-web-extensions.

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, so this bug may conflict with bug 1314076, where there's work happening to expose battery to webextensions. Should probably get that sorted. I'm NI'ing :aswan (and removing :dhylands) to get things sync'd up here.

Comment 7

[Andrew Swan [:aswan]]

If I understand correctly, the low-level battery stuff in the platform needs to stay (https://bugzilla.mozilla.org/show_bug.cgi?id=1314076#c2) so this is just about whether it is exposed to javascript (and if it is, exactly how it is exposed).  But we should make a decision one way or the other, either remove the binding altogether or update it to just be exposed to extensions.

So the question is if we want to support this for extensions at all, redirect to mconca for a decision on that.  There was a flicker of interest earlier on bug 1314076 but not much recent activity.

Comment 8

[Mike Conca [:mconca]]

(In reply to Andrew Swan [:aswan] from comment #7)
> If I understand correctly, the low-level battery stuff in the platform needs
> to stay (https://bugzilla.mozilla.org/show_bug.cgi?id=1314076#c2) so this is
> just about whether it is exposed to javascript (and if it is, exactly how it
> is exposed).  But we should make a decision one way or the other, either
> remove the binding altogether or update it to just be exposed to extensions.
> 
> So the question is if we want to support this for extensions at all,
> redirect to mconca for a decision on that.  There was a flicker of interest
> earlier on bug 1314076 but not much recent activity.

As :aswan noted, bug 1314076 implied the low-level platform API was being kept around to help Firefox decide if it was okay to do (potentially battery-draining) background activities.  If that is the case, I'd still support exposing battery info via the WebExtensions API.

If, however, the WebExtension API is the *only* reason to keep the battery info around, then I'd probably reconsider bug 1314076 since it increases the WebExtensions maintenance and test burden for a nice but low-demand API.

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Yup, the low-level HAL battery code is still being used in QM, you're right. I misread that in bug 1314076, sorry about that.

That leaves the code in dom/battery (like BatteryManager). If we want to keep that around and expose it to extensions via navigator.getBattery, that's fine, just want to make sure we know that, so it isn't disappeared by those of us cleaning up and removing code. I don't believe it's being used anywhere else, and we could expose it to both Chrome and Extensions if that would be helpful. Just need to know who wants what.

Comment 10

[Mike Conca [:mconca]]

(In reply to Kyle Machulis [:qdot] [:kmachulis] (if a patch has no decent commit message, automatic r-) from comment #9)
> That leaves the code in dom/battery (like BatteryManager). If we want to
> keep that around and expose it to extensions via navigator.getBattery,
> that's fine, just want to make sure we know that, so it isn't disappeared by
> those of us cleaning up and removing code. I don't believe it's being used
> anywhere else, and we could expose it to both Chrome and Extensions if that
> would be helpful. Just need to know who wants what.

Thanks, Kyle. Since this last question has to do with the engineering architecture of how to best structure and expose the battery info via an API, I'm going to ping this NI back to aswan.

Comment 11

[Andrew Swan [:aswan]]

It sounds like the consensus is that we can expose this to extensions but it isn't a high priority for anybody.
I propose that we directly expose the existing navigator interface to pages with extension principals (and add a test of it).  That can happen over in bug 1314076.

Comment 12

[Anne (:annevk)]

jkt, could you please land the [ChromeOnly] change and file a follow-up for further cleanup after extensions are sorted or some such? That at least ensures sites don't end up feature testing on this incorrectly or some such.

Comment 13

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1441976 - Expose BatteryManager only to Chrome scripts

Comment 14

[Jonathan Kingston [:jkt] he/him]

Seeing if it just works tm, https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e7f9c9413f200ab6f624c69959611996aabb7a6

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f065877ca676
Expose BatteryManager only to Chrome scripts r=baku

Comment 16

[Cristina Coroiu [:ccoroiu]]

Backed out changeset f065877ca676 (bug 1441976) for wpt failures at battery-status/battery-interface-idlharness.https.window.html

Backout: https://hg.mozilla.org/integration/autoland/rev/78ef6366dfaf5a8142a1d5894c35b243fb6ef133

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=f065877ca676168781ca11f205cef8dc8eedcf54

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=268654386&repo=autoland&lineNumber=1713

[task 2019-09-26T20:43:36.694Z] 20:43:36 INFO - TEST-START | /battery-status/battery-interface-idlharness.https.window.html
[task 2019-09-26T20:43:36.698Z] 20:43:36 INFO - Closing window 14
[task 2019-09-26T20:43:37.360Z] 20:43:37 INFO -
[task 2019-09-26T20:43:37.360Z] 20:43:37 INFO - TEST-FAIL | /battery-status/battery-interface-idlharness.https.window.html | idl_test setup - promise_test: Unhandled rejection with value: object "TypeError: navigator.getBattery is not a function"
[task 2019-09-26T20:43:37.360Z] 20:43:37 INFO - TEST-PASS | /battery-status/battery-interface-idlharness.https.window.html | Partial interface Navigator: original interface defined
[task 2019-09-26T20:43:37.360Z] 20:43:37 INFO - TEST-UNEXPECTED-FAIL | /battery-status/battery-interface-idlharness.https.window.html | BatteryManager interface: existence and properties of interface object - assert_own_property: self does not have own property "BatteryManager" expected property "BatteryManager" missing
[task 2019-09-26T20:43:37.360Z] 20:43:37 INFO - IdlInterface.prototype.assert_interface_object_exists@https://web-platform.test:8443/resources/idlharness.js:1332:24
[task 2019-09-26T20:43:37.360Z] 20:43:37 INFO - IdlInterface.prototype.test_self/<@https://web-platform.test:8443/resources/idlharness.js:1543:14
[task 2019-09-26T20:43:37.360Z] 20:43:37 INFO - Test.prototype.step@https://web-platform.test:8443/resources/testharness.js:1905:25
[task 2019-09-26T20:43:37.360Z] 20:43:37 INFO - test@https://web-platform.test:8443/resources/testharness.js:544:30
[task 2019-09-26T20:43:37.360Z] 20:43:37 INFO - self.subsetTestByKey@https://web-platform.test:8443/resources/idlharness.js:54:14
[task 2019-09-26T20:43:37.360Z] 20:43:37 INFO - IdlInterface.prototype.test_self@https://web-platform.test:8443/resources/idlharness.js:1523:20
[task 2019-09-26T20:43:37.360Z] 20:43:37 INFO - IdlInterface.prototype.test@https://web-platform.test:8443/resources/idlharness.js:1498:14
[task 2019-09-26T20:43:37.360Z] 20:43:37 INFO - IdlArray.prototype.test@https://web-platform.test:8443/resources/idlharness.js:861:28
[task 2019-09-26T20:43:37.360Z] 20:43:37 INFO - idl_test/</<@https://web-platform.test:8443/resources/idlharness.js:3321:31
[task 2019-09-26T20:43:37.360Z] 20:43:37 INFO -

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:jkt, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84919209f983
Expose BatteryManager only to Chrome scripts r=baku

Comment 19

[Kohei Yoshino [:kohei]]

Updated the site compatibility note: https://www.fxsitecompat.dev/en-CA/docs/2016/battery-status-api-has-been-removed/

Comment 20

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/84919209f983

Comment 21

[Chris Mills [:cmills]]

I've updated the compat data for BatteryManager; I think this is all it needs:

https://github.com/mdn/browser-compat-data/pull/5294