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

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P5
normal
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: florian, Assigned: Felipe, Mentored)

Tracking

(Blocks 1 bug)

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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)
(Reporter)

Comment 2

2 years ago
(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)
(Reporter)

Comment 4

2 years ago
Ok, let's package it using TESTING_JS_MODULES then.
(Assignee)

Comment 5

11 months ago
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)
(Assignee)

Comment 6

11 months ago
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.

Comment 8

4 months ago
(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)
(Reporter)

Comment 9

4 months ago
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)
(Assignee)

Updated

2 months ago
Keywords: good-first-bug
(Assignee)

Updated

2 months ago
Blocks: 1527219
(Assignee)

Comment 10

2 months ago

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)
(Assignee)

Comment 12

2 months ago

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

Comment 14

2 months ago
Pushed by fgomes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3607e50ad27a
Remove unused Battery.jsm. r=Yoric
(Assignee)

Comment 16

2 months ago

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

Flags: needinfo?(felipc)

Comment 17

2 months ago
Pushed by fgomes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f79d064a028c
Remove unused Battery.jsm. r=Yoric

Comment 18

2 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months 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.