Closed Bug 1351078 Opened 4 years ago Closed 2 years ago

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

Categories

(Toolkit :: General, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: florian, Assigned: Felipe, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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?
Flags: needinfo?(dteller)
Well, it was meant to be used by bug 978857, which would still be useful.
Flags: needinfo?(dteller)
(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.
Flags: needinfo?(dteller)
Sorry, it was bug 506975.
Flags: needinfo?(dteller)
Ok, let's package it using TESTING_JS_MODULES then.
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?
Flags: needinfo?(dteller)
Hmm I got confused, navigator.battery was non-standard, but navigator.getBattery() is still available for privileged code
Flags: needinfo?(dteller)
I still think that bug 506975 would be useful for our energy use.
(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?
Flags: needinfo?(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.
Mentor: florian
Flags: needinfo?(florian)
Blocks: 1527219

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)

Flags: needinfo?(dteller)

No objection from me.

Flags: needinfo?(dteller)

Thanks!

Assignee: nobody → felipc
Status: NEW → ASSIGNED
Keywords: good-first-bug
Priority: -- → P5
Summary: Stop shipping resource://gre/modules/Battery.jsm → Remove resource://gre/modules/Battery.jsm
Pushed by fgomes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3607e50ad27a
Remove unused Battery.jsm. r=Yoric

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

Flags: needinfo?(felipc)
Pushed by fgomes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f79d064a028c
Remove unused Battery.jsm. r=Yoric
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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

You need to log in before you can comment on or make changes to this bug.