1351078 - Remove resource://gre/modules/Battery.jsm

Status: RESOLVED FIXED

(Toolkit :: General, enhancement, P5)

Description

[Florian Quèze [:florian]]

resource://gre/modules/Battery.jsm was introduced in bug 978857, and is only referenced by toolkit/modules/tests/browser/browser_Battery.js which tests it.

It was meant as a test helper, so it should be packaged using TESTING_JS_MODULES instead of EXTRA_JS_MODULES. But given that no other test seems to be using it, I think we could also just remove it. Yoric, you r+'ed this file in bug 978857, how do you feel about us removing it?

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Well, it was meant to be used by bug 978857, which would still be useful.

Comment 2

[Florian Quèze [:florian]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #1)
> Well, it was meant to be used by bug 978857, which would still be useful.

Wrong bug number I assume.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Sorry, it was bug 506975.

Comment 4

[Florian Quèze [:florian]]

Ok, let's package it using TESTING_JS_MODULES then.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

navigator.battery was removed in bug 1259335. Battery.jsm was only a stub implementation to return fake values during tests, but the navigator.battery API itself is totally gone now. In this case, I think we can remove this file, right?

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Hmm I got confused, navigator.battery was non-standard, but navigator.getBattery() is still available for privileged code

Comment 7

[David Teller [:Yoric] - still alive but not very active]

I still think that bug 506975 would be useful for our energy use.

Comment 8

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] (PTO until Jan 2) from comment #4)
> Ok, let's package it using TESTING_JS_MODULES then.

Can we still do this? Can you maybe write up a brief description of how to do this and make this a mentored bug?

Comment 9

[Florian Quèze [:florian]]

Battery.jsm is currently packaged at https://searchfox.org/mozilla-central/rev/ecf61f8f3904549f5d65a8a511dbd7ea4fd1a51d/toolkit/modules/moz.build#186

We should move this line to the TESTING_JS_MODULES section a few lines above.

The url to import in the code will then become "resource://testing-common/Battery.jsm" instead of the current "resource://gre/modules/Battery.jsm" used at https://searchfox.org/mozilla-central/source/toolkit/modules/tests/browser/browser_Battery.js#5

After doing this change, the file will no longer be shipped as part of Firefox, so it'll no longer be reported as unreferenced, ie https://searchfox.org/mozilla-central/rev/ecf61f8f3904549f5d65a8a511dbd7ea4fd1a51d/browser/base/content/test/static/browser_all_files_referenced.js#147-148 should be removed.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

So I propose that we go back on the decision and fully remove this Battery.jsm module. Its purpose was to be a wrapper around navigator.getBattery() that all code should use in order to allow testing to provide fake values for it. However, there are some issues:

• I'm inspecting code across the tree to remove usage of the hiddenDOMWindow (bug 1527219). This module should probably be fixed to use an existing browser window instead of the hidden window

• navigator.getBattery() got deprecated and unexposed to web content, so there really is no reason for this to be tied to navigator or a window. If someone needs this functionality in the future, it would be better if it got converted to a normal internal component instead of needing a window to use it.. And, in that case, there are other ways to create a mock component to override it on tests..

Given this, are you ok if we remove it? It would be easy to bring it back from history if needed (and hopefully, if someone does that, to rewrite it slightly to not use the hidden window)

Comment 11

[David Teller [:Yoric] - still alive but not very active]

No objection from me.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Thanks!

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1351078 - Remove unused Battery.jsm. r=Yoric

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3607e50ad27a
Remove unused Battery.jsm. r=Yoric

Comment 15

[Dorel Luca [:dluca]]

Backed out changeset 3607e50ad27a (bug 1351078) for build bustage. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=229926252&repo=autoland&lineNumber=1520

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=3607e50ad27a07aea93c2e8b81d8ae05d34fc148

Backout:
https://hg.mozilla.org/integration/autoland/rev/176ff7c8871e7df2977659d1f7c9107241bb6318

Comment 16

[:Felipe Gomes (needinfo for replies!)]

Sorry, I just forgot to remove the deleted test file from browser.ini

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f79d064a028c
Remove unused Battery.jsm. r=Yoric

Comment 18

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/f79d064a028c

Comment 19

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/f79d064a028c

Comment 20

[Florin Strugariu [:Bebe]]

== Change summary for alert #19615 (as of Mon, 25 Feb 2019 07:28:01 GMT) ==

Improvements:

2% tp5o_webext responsiveness linux64 pgo e10s stylo 1.49 -> 1.46

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=19615