Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should the timer scheduler live in the chrome process? #15219

Open
Ms2ger opened this issue Jan 25, 2017 · 8 comments
Open

Should the timer scheduler live in the chrome process? #15219

Ms2ger opened this issue Jan 25, 2017 · 8 comments

Comments

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Jan 25, 2017

It currently lives along the constellation, but it seems to be used only from the script crate; it might be worth having one per content process instead.

@jdm
Copy link
Member

@jdm jdm commented Jan 25, 2017

I agree; it will be more efficient to have it exist in content processes.

@jdm
Copy link
Member

@jdm jdm commented Jan 27, 2017

I propose to solve this by:

  • making the scheduler_chan in Constellation an Option<Sender<TimerEventRequest>>
  • add a Sender<TimerEventRequest> argument to UnprivilegedPipelineContent::start_all
  • removing the scheduler_chan member from UnprivilegedPipelineContent
  • make TimerScheduler::start return a Sender<TimerEventRequest>
  • call TimerScheduler::start in run_content_process and pass the resulting channel to start_all
  • call TimerScheduler::start in Browser::new and store the resulting channel in Constellation's optional field
  • unwrap the optional field and pass the value to start_all in Pipeline::spawn

Code: components/constellation/constellation.rs, components/constellation/pipeline.rs, components/servo/lib.rs

@Seris would you like to work on this?

@Seris
Copy link

@Seris Seris commented Jan 27, 2017

I'll work on this !
Thanks for the quick response !

@jdm jdm added the C-assigned label Jan 27, 2017
@jdm
Copy link
Member

@jdm jdm commented Mar 16, 2017

@Seris Did you make any progress here? Any questions?

@Seris
Copy link

@Seris Seris commented Mar 22, 2017

I've worked a bit on it and I think I understand what I need to do. But I can't work on it right now ! It's a hard semester and I have a lot of exams and projects. I'll be working again on the issue after April 17 if it does not require more attention right now ^^

Sorry for not informing you earlier !

@KiChjang KiChjang added C-assigned and removed C-assigned labels Mar 23, 2017
@jdm jdm removed the C-assigned label Mar 27, 2017
@ibmandura
Copy link

@ibmandura ibmandura commented Jun 16, 2017

I would like to tackle this one

@jdm jdm added the C-assigned label Jun 16, 2017
@ibmandura
Copy link

@ibmandura ibmandura commented Jun 18, 2017

@jdm Here are few questions to see if I understood everything:

  • Type of scheduler_chan should be Option<Sender<TimerSchedulerMsg>> instead of TimerEventRequest?
  • Should field scheduler_chan be removed from the WorkerGlobalScopeInit since it needs to derive Serialize and Deserialize traits? I don't see a good way to implement Serialize for Sender<TimerSchedulerMsg>.
    I suggest adding scheduler_chan as a member to WorkletGlobalScopeInit (and maybe SWManagerSenders, but we are getting that from unprivileged_content).
  • What would be the least time-consuming way to test this?
@jdm
Copy link
Member

@jdm jdm commented Jun 18, 2017

  • The type of the channel has changed since I first filed this; now Sender<TimerEventRequest> is appropriate.
  • Yes, you can remove it from WorkerGlobalScopeInit. We should be able to use an existing value in every place that tries to get it from that structure instead.
  • I recommend running ./mach test-wpt tests/wpt/web-platform-tests/html/webappapis/timers. If those pass then you can go ahead and make a pull request with your changes.
@ibmandura ibmandura mentioned this issue Jun 18, 2017
2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.