Closed
Bug 1382955
Opened 7 years ago
Closed 7 years ago
Remove MozPowerManager and related things
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(3 files, 2 obsolete files)
(deleted),
patch
|
gsvelto
:
review+
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gsvelto
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gsvelto
:
review+
|
Details | Diff | Splinter Review |
In bug 1382099 gsvelto noticed that the MozPowerManager is no longer necessary.
Assignee | ||
Comment 1•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
(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•7 years ago
|
||
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 4•7 years ago
|
||
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•7 years 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.)
Comment 6•7 years ago
|
||
(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 :)
Comment 7•7 years ago
|
||
Back from parental leave... I'll review this tomorrow.
Comment 8•7 years ago
|
||
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 9•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
Attachment #8888681 -
Attachment is obsolete: true
Assignee | ||
Comment 11•7 years ago
|
||
Attachment #8892376 -
Flags: review?(gsvelto)
Assignee | ||
Comment 12•7 years ago
|
||
Attachment #8892377 -
Flags: review?(gsvelto)
Assignee | ||
Updated•7 years ago
|
Attachment #8889288 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8892375 -
Flags: review?(gsvelto) → review+
Comment 13•7 years ago
|
||
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 14•7 years ago
|
||
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+
Comment 15•7 years ago
|
||
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•7 years 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 17•7 years ago
|
||
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•7 years 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•7 years 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
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•