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

script: Split `Pipeline::create` into chrome process and content process parts. #6618

Merged
merged 2 commits into from Jul 15, 2015

Conversation

@pcwalton
Copy link
Contributor

pcwalton commented Jul 14, 2015

This will make it easier to adapt to IPC.

The trickiest part here was to make script tasks spawn new layout tasks
directly instead of having the pipeline do it for them. The latter
approach will not work in multiprocess mode, because layout and script
must run in the same address space and the pipeline cannot inject tasks
into another process.

r? @larsbergstrom

Review on Reviewable

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Jul 14, 2015

Critic review: https://critic.hoppipolla.co.uk/r/5544

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Jul 14, 2015

Reviewed 11 of 11 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from the review on Reviewable.io

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Jul 14, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Jul 14, 2015

📌 Commit 4481509 has been approved by larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented Jul 14, 2015

Testing commit 4481509 with merge 01502c0...

bors-servo pushed a commit that referenced this pull request Jul 14, 2015
…bergstrom

script: Split `Pipeline::create` into chrome process and content process parts.

This will make it easier to adapt to IPC.

The trickiest part here was to make script tasks spawn new layout tasks
directly instead of having the pipeline do it for them. The latter
approach will not work in multiprocess mode, because layout and script
must run in the same address space and the pipeline cannot inject tasks
into another process.

r? @larsbergstrom

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6618)
<!-- Reviewable:end -->
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Jul 14, 2015

4.84s$ ./mach test-tidy

components/compositing/pipeline.rs:271: trailing whitespace

components/compositing/pipeline.rs:278: trailing whitespace

Fix then r=me

@bors-servo
Copy link
Contributor

bors-servo commented Jul 14, 2015

💔 Test failed - linux1

@jdm jdm added S-fails-tidy and removed S-awaiting-review labels Jul 14, 2015
@pcwalton pcwalton force-pushed the pcwalton:content-process-code-shuffling branch from 4481509 to 401a3d8 Jul 14, 2015
@pcwalton
Copy link
Contributor Author

pcwalton commented Jul 14, 2015

@bors-servo: r=larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented Jul 14, 2015

📌 Commit 401a3d8 has been approved by larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented Jul 14, 2015

Testing commit 401a3d8 with merge 477937c...

bors-servo pushed a commit that referenced this pull request Jul 14, 2015
…bergstrom

script: Split `Pipeline::create` into chrome process and content process parts.

This will make it easier to adapt to IPC.

The trickiest part here was to make script tasks spawn new layout tasks
directly instead of having the pipeline do it for them. The latter
approach will not work in multiprocess mode, because layout and script
must run in the same address space and the pipeline cannot inject tasks
into another process.

r? @larsbergstrom

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6618)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 14, 2015

💔 Test failed - mac1

@jdm jdm added S-tests-failed and removed S-awaiting-merge labels Jul 14, 2015
process parts.

This will make it easier to adapt to IPC.

The trickiest part here was to make script tasks spawn new layout tasks
directly instead of having the pipeline do it for them. The latter
approach will not work in multiprocess mode, because layout and script
must run in the same address space and the pipeline cannot inject tasks
into another process.
@bors-servo
Copy link
Contributor

bors-servo commented Jul 14, 2015

The latest upstream changes (presumably #6626) made this pull request unmergeable. Please resolve the merge conflicts.

iframe case instead of creating a new one.

Fixes test timeouts.
@pcwalton pcwalton force-pushed the pcwalton:content-process-code-shuffling branch from 401a3d8 to c2195c4 Jul 14, 2015
@pcwalton
Copy link
Contributor Author

pcwalton commented Jul 14, 2015

Should fix the timeouts. r? @larsbergstrom

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Jul 15, 2015

Reviewed 1 of 11 files at r1, 11 of 11 files at r2.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Jul 15, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Jul 15, 2015

📌 Commit c2195c4 has been approved by larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented Jul 15, 2015

Testing commit c2195c4 with merge e06eaa0...

bors-servo pushed a commit that referenced this pull request Jul 15, 2015
…bergstrom

script: Split `Pipeline::create` into chrome process and content process parts.

This will make it easier to adapt to IPC.

The trickiest part here was to make script tasks spawn new layout tasks
directly instead of having the pipeline do it for them. The latter
approach will not work in multiprocess mode, because layout and script
must run in the same address space and the pipeline cannot inject tasks
into another process.

r? @larsbergstrom

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6618)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 15, 2015

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2, mac3

@bors-servo bors-servo merged commit c2195c4 into servo:master Jul 15, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.