Jonathan Kingston [:jkt] he/him Assignee
	Description • 6 years ago

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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 1 • 6 years ago

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 2 • 6 years ago

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

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 3 • 6 years ago

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.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 4 • 6 years ago

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

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 5 • 6 years ago

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.

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 6 • 6 years ago

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.

	Andrew Swan [:aswan]
	Comment 7 • 6 years ago

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.

	Mike Conca [:mconca]
	Comment 8 • 6 years ago

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

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 9 • 6 years ago

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.

	Mike Conca [:mconca]
	Comment 10 • 6 years ago

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

	Andrew Swan [:aswan]
	Comment 11 • 6 years ago

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.

	Pulsebot
	Comment 15 • 5 years ago

Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f065877ca676
Expose BatteryManager only to Chrome scripts r=baku

	Pulsebot
	Comment 18 • 5 years ago

Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/84919209f983
Expose BatteryManager only to Chrome scripts r=baku