Remove MozPowerManager and related things

RESOLVED FIXED in Firefox 57

Status

()

Core
DOM
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

10 months ago
In bug 1382099 gsvelto noticed that the MozPowerManager is no longer necessary.
(Assignee)

Comment 1

10 months ago
Created attachment 8888681 [details] [diff] [review]
Remove MozPowerManager and related things

gsvelto, I've taken a crack at this. I don't really know what I'm doing, I just
started trimming and didn't stop until it seemed complete. Is modifying the
.webidl files ok?
Attachment #8888681 - Flags: review?(gsvelto)
(In reply to Nicholas Nethercote [:njn] from comment #1)
> Created attachment 8888681 [details] [diff] [review]
> Remove MozPowerManager and related things
> 
> gsvelto, I've taken a crack at this. I don't really know what I'm doing, I
> just
> started trimming and didn't stop until it seemed complete. Is modifying the
> .webidl files ok?

Just had a quick look since I'm still on paternal leave, I'll dig deeper early next month when I'm back full time. I think more stuff can be trimmed from HAL but I'll have to check it. Also I need to check that Fennec isn't using wakelocks because IIRC it was at some point so we might have to keep those around. BTW see also bug 1369194 for something somewhat related.
(Assignee)

Comment 3

10 months ago
Created attachment 8889288 [details] [diff] [review]
Remove a bunch of unused HAL stuff

kanru, please review the sync-messages.ini changes, which require an IPC peer.

gsvelto, please review the rest.
Attachment #8889288 - Flags: review?(kchen)
Attachment #8889288 - Flags: review?(gsvelto)
Comment on attachment 8889288 [details] [diff] [review]
Remove a bunch of unused HAL stuff

Review of attachment 8889288 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the IPC change.

This patch is not complete. I think you will also remove all the users of the removed API?
Attachment #8889288 - Flags: review?(kchen) → review+
(Assignee)

Comment 5

10 months ago
> This patch is not complete. I think you will also remove all the users of
> the removed API?

I'm not sure if I understand your comment. There are no users of these APIs, which is why I was able to remove them. AS far as I can tell the patch is complete. (It builds on all platforms on try.)
(In reply to Nicholas Nethercote [:njn] from comment #5)
> > This patch is not complete. I think you will also remove all the users of
> > the removed API?
> 
> I'm not sure if I understand your comment. There are no users of these APIs,
> which is why I was able to remove them. AS far as I can tell the patch is
> complete. (It builds on all platforms on try.)

You're right. I missed your first patch :)

Updated

10 months ago
Back from parental leave... I'll review this tomorrow.
Comment on attachment 8888681 [details] [diff] [review]
Remove MozPowerManager and related things

Almost there but not quite; after these changes the power manager service has three methods that are now unused: restart(), reboot() and powerOff(), see [1]. The latter are implemented via hal::Reboot() and hal::PowerOff() which can also be removed and the former was a "special" restart function only ever used in gonk so it can also go.

[1] https://dxr.mozilla.org/mozilla-central/rev/36f95aeb4c77f7cf3b3366583008cd6e4b6b1dba/dom/power/nsIPowerManagerService.idl#25
Attachment #8888681 - Flags: review?(gsvelto) → review-
Comment on attachment 8889288 [details] [diff] [review]
Remove a bunch of unused HAL stuff

Clearing the review flag as more changes are required as per the comment above.
Attachment #8889288 - Flags: review?(gsvelto)
(Assignee)

Comment 10

10 months ago
Created attachment 8892375 [details] [diff] [review]
(part 1) - Remove MozPowerManager and related things

This is the same as the previous version, except for the commit log. I put the
nsIPowerManagerService changes in a separate patch.
Attachment #8892375 - Flags: review?(gsvelto)
(Assignee)

Updated

10 months ago
Attachment #8888681 - Attachment is obsolete: true
(Assignee)

Comment 11

10 months ago
Created attachment 8892376 [details] [diff] [review]
(part 2) - Remove nsIPowerManagerService::{powerOff,reboot,restart} and related things
Attachment #8892376 - Flags: review?(gsvelto)
(Assignee)

Comment 12

10 months ago
Created attachment 8892377 [details] [diff] [review]
(part 3) - Remove a bunch of unused HAL stuff
Attachment #8892377 - Flags: review?(gsvelto)
(Assignee)

Updated

10 months ago
Attachment #8889288 - Attachment is obsolete: true
Attachment #8892375 - Flags: review?(gsvelto) → review+
Comment on attachment 8892376 [details] [diff] [review]
(part 2) - Remove nsIPowerManagerService::{powerOff,reboot,restart} and related things

Review of attachment 8892376 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM save for the comment below. From what I can tell this should be all of it.

::: dom/ipc/ContentParent.cpp
@@ -680,5 @@
> -
> -  bool done = false;
> -  Monitor monitor("mozilla.dom.ContentParent.JoinAllSubprocesses");
> -  XRE_GetIOMessageLoop()->PostTask(NewRunnableFunction(
> -                                     &ContentParent::JoinProcessesIOThread,

ContentParent::JoinProcessesIOThread() seems unused now, let's remove that too.
Attachment #8892376 - Flags: review?(gsvelto) → review+
Comment on attachment 8892377 [details] [diff] [review]
(part 3) - Remove a bunch of unused HAL stuff

Review of attachment 8892377 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8892377 - Flags: review?(gsvelto) → review+
I hadn't noticed but there's a build failure on Android with the latest patch set, it should be trivial to fix though:

https://treeherder.mozilla.org/logviewer.html#?job_id=119958778&repo=try&lineNumber=17115
(Assignee)

Comment 16

10 months ago
Comment on attachment 8892375 [details] [diff] [review]
(part 1) - Remove MozPowerManager and related things

Review of attachment 8892375 [details] [diff] [review]:
-----------------------------------------------------------------

bz, the two .webidl files need review from a DOM peer. This is all b2g-only power stuff that is no longer needed.
Attachment #8892375 - Flags: review?(bzbarsky)
Comment on attachment 8892375 [details] [diff] [review]
(part 1) - Remove MozPowerManager and related things

r=me
Attachment #8892375 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 18

10 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/615e7773cd68625b1d8ce9c22df7186c7ae921ca
Bug 1382955 (part 1) - Remove MozPowerManager and related things. r=bz,gsvelto.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2345ddc27945fb93f71de800f0cf79184c3c1550
Bug 1382955 (part 2) - Remove nsIPowerManagerService::{powerOff,reboot,restart} and related things. r=gsvelto.

https://hg.mozilla.org/integration/mozilla-inbound/rev/df3915693fa3bec56bacc17a4f2588f99ab635b4
Bug 1382955 (part 3) - Remove a bunch of unused HAL stuff. r=gsvelto,kanru.

Comment 19

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/615e7773cd68
https://hg.mozilla.org/mozilla-central/rev/2345ddc27945
https://hg.mozilla.org/mozilla-central/rev/df3915693fa3
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.