Closed Bug 928995 Opened 7 years ago Closed 7 years ago

Let PreallocatedProcessManager manage Nuwa process.

Categories

(Core :: IPC, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: kk1fff, Assigned: kk1fff)

References

Details

Attachments

(1 file, 3 obsolete files)

Most of logic of nuwa is implemented in ContentParent, we should move some of them into PreallocateProcessManager to let PreallocateProcessManager control nuwa process.
No longer blocks: Nuwa
Depends on: Nuwa
Attached patch Proposed Patch (obsolete) — Splinter Review
Moving Nuwa process logic to PreallocatedProcessManager, use PreallocatedProcessManager to control the process.
Attachment #821693 - Flags: feedback?(cyu)
Attached patch Proposed Patch (obsolete) — Splinter Review
Assignee: nobody → pwang
Attachment #821693 - Attachment is obsolete: true
Attachment #821693 - Flags: feedback?(cyu)
Attachment #823989 - Flags: feedback?(cyu)
Comment on attachment 823989 [details] [diff] [review]
Proposed Patch

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

Sorry for taking so long. I took a quick glance, and generally, the patch looks good. Some suggestions:

- I don't think we need PreallocatedProcessManager::ScheduleDelayedNuwaFork(). This should be encapsulated in AllocAfterDelay().
- We don't need to #include Nuwa.h in ContentParent or PreallocatedProcessManager.
Attachment #823989 - Flags: feedback?(cyu) → feedback+
Attached patch Proposed Patch (obsolete) — Splinter Review
Thanks, Cervantes! This is updated version.
Attachment #823989 - Attachment is obsolete: true
Comment on attachment 828529 [details] [diff] [review]
Proposed Patch

Hi Ben, the propurse of this patch is to move Nuwa logic to preallocated process manager, since Nuwa is a preallocated process, and this would make 'dom.ipc.processPrelaunch.enabled' preference able to control Nuwa, which is necessary in Nuwa test case. Would you review this patch?  Thanks!
Attachment #828529 - Flags: review?(bent.mozilla)
Comment on attachment 828529 [details] [diff] [review]
Proposed Patch

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

It's not clear from the patch if anything has really changed in the code that was moved from ContentParent to PreallocatedProcessManager... Has anything changed? Or was it simply moved?
It is just moving code. The only change is to allow us to enable or disable Nuwa process by controlling value of 'dom.ipc.processPrelaunch.enabled'.
Comment on attachment 828529 [details] [diff] [review]
Proposed Patch

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

Ok!
Attachment #828529 - Flags: review?(bent.mozilla) → review+
Thanks, Ben!
Attachment #828529 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/5cbb679dda59
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.