Closed Bug 928995 Opened 10 years ago Closed 10 years ago

Let PreallocatedProcessManager manage Nuwa process.

Categories

(Core :: IPC, defect)

Product:
Core
Component:
IPC
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla28

People

(Reporter: kk1fff, Assigned: kk1fff)

References

Details

Attachments

(1 file, 3 obsolete files)

Patch v.check-in r=bent
23.26 KB, patch
Details | Diff | Splinter Review 
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: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

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