Closed Bug 1441976 Opened 5 years ago Closed 4 years ago

Restrict BatteryManager to chrome script

Categories

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

Product:
Core
Component:
DOM: Device Interfaces
Version:
60 Branch
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla72
Tracking Flags:
Tracking Status
firefox60 --- wontfix
firefox72 --- fixed

People

(Reporter: jkt, Assigned: jkt)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(1 file)

Bug 1441976 - Expose BatteryManager only to Chrome scripts
47 bytes, text/x-phabricator-request
Details | Review 
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.
I think you mean making the BatteryManager restricted to chrome script?
Summary: Restrict BatteryManager to parent processes → Restrict BatteryManager to chrome script
Anyway, I think this should be as simple as adding [ChromeOnly] to BatterManager interface.
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.
Priority: -- → P3
: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.
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.
Assignee: nobody → kyle
Flags: needinfo?(dhylands)
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.
Flags: needinfo?(dhylands) → needinfo?(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.
Flags: needinfo?(aswan) → needinfo?(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.
Flags: needinfo?(mconca)
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.
Flags: needinfo?(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.
Flags: needinfo?(mconca) → needinfo?(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.
Flags: needinfo?(aswan)
Assignee: kyle → nobody

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.

Flags: needinfo?(jkt)
Assignee: nobody → jkt
Flags: needinfo?(jkt)
Status: NEW → ASSIGNED
Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f065877ca676
Expose BatteryManager only to Chrome scripts r=baku

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 -

Flags: needinfo?(jkt)

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.

Flags: needinfo?(jkt)
Type: enhancement → task
Keywords: site-compat
Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/84919209f983
Expose BatteryManager only to Chrome scripts r=baku

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

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox72: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Flags: needinfo?(jkt)

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

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

Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.