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

Reduce LayoutTaskData to the fields used for RPC. #8378

Merged
merged 19 commits into from Nov 10, 2015
Merged

Conversation

@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 6, 2015

Review on Reviewable

@highfive
Copy link

highfive commented Nov 6, 2015

warning Warning warning

  • These commits modify layout code, but no reftests are modified. Please consider adding a reftest!
@eefriedman
Copy link
Contributor

eefriedman commented Nov 6, 2015

Nice simplification... except that I'll have to redo large chunks of it to rebase #7524 on top of this.

The problem is that if you have an &mut LayoutTask floating around, it's impossible to borrow the Stylist... and SharedLayoutContext does precisely that. The borrow checker just can't see it at the moment because SharedLayoutContext uses a raw pointer.


Review status: 0 of 9 files reviewed at latest revision, 1 unresolved discussion.


components/layout/layout_task.rs, line 332 [r1] (raw file):
Accidentally left this in?


Comments from the review on Reviewable.io

@eefriedman eefriedman self-assigned this Nov 6, 2015
@Ms2ger
Copy link
Contributor Author

Ms2ger commented Nov 6, 2015

I think anything that prevents us from using &mut LayoutTask is problematic; you see how much pointless locking I'm removing. I'll see if I can think of something for Stylist.


Review status: 0 of 9 files reviewed at latest revision, 1 unresolved discussion.


components/layout/layout_task.rs, line 332 [r1] (raw file):
No, I believe it should be used in some places where it isn't: #8358


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Nov 6, 2015

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

@bors-servo
Copy link
Contributor

bors-servo commented Nov 7, 2015

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

@Ms2ger Ms2ger force-pushed the Ms2ger:rwdata branch from 0d88a2d to 22f951a Nov 8, 2015
Ms2ger added 16 commits Nov 6, 2015
There is no good reason to have the two types.

This also means that the result of LayoutTask::profiler_metadata no longer
borrows the LayoutTask, which I'll need later.
…tTask::handle_request.

This ensures no fields of the LayoutTask are borrowed when calling repaint or
handle_request_helper, which I'll need later.
As the LayoutTask is uniquely owned, and we no longer have borrows of its
fields hanging around, we can use mutable references to simplify some code.
This matches LayoutTask::solve_constraints, and will be necessary when we
borrow parallel_traversal directly from the LayoutTask.
@bors-servo
Copy link
Contributor

bors-servo commented Nov 9, 2015

Testing commit 1919e19 with merge d24530a...

bors-servo added a commit that referenced this pull request Nov 9, 2015
Reduce LayoutTaskData to the fields used for RPC.

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

bors-servo commented Nov 9, 2015

💔 Test failed - mac-rel-wpt

@jdm
Copy link
Member

jdm commented Nov 9, 2015

Looking at the failure in the reftest analyzer, the screenshot is blank. Can you reproduce this failure?

@eefriedman
Copy link
Contributor

eefriedman commented Nov 9, 2015

The failed test is blank, white; given that the test has a green background, most likely reflow didn't finish before the screenshot was taken. Probably just an intermittent failure which happened to show up here, but I'll look a little more.

@eefriedman
Copy link
Contributor

eefriedman commented Nov 10, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Nov 10, 2015

Testing commit 1919e19 with merge 894ad95...

bors-servo added a commit that referenced this pull request Nov 10, 2015
Reduce LayoutTaskData to the fields used for RPC.

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

bors-servo commented Nov 10, 2015

💔 Test failed - gonk

@eefriedman
Copy link
Contributor

eefriedman commented Nov 10, 2015

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Nov 10, 2015

Testing commit 1919e19 with merge 0d21158...

bors-servo added a commit that referenced this pull request Nov 10, 2015
Reduce LayoutTaskData to the fields used for RPC.

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

bors-servo commented Nov 10, 2015

@bors-servo bors-servo merged commit 1919e19 into servo:master Nov 10, 2015
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable Review in progress: 0 of 9 files reviewed, 1 unresolved discussion
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@Ms2ger Ms2ger deleted the Ms2ger:rwdata branch Nov 10, 2015
simartin added a commit to simartin/servo that referenced this pull request Dec 12, 2015
simartin added a commit to simartin/servo that referenced this pull request Dec 12, 2015
simartin added a commit to simartin/servo that referenced this pull request Dec 12, 2015
simartin added a commit to simartin/servo that referenced this pull request Dec 12, 2015
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

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