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.

Sign up for GitHub

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

Jump to bottom

Use associated types for layout wrapper trait, and generalized ThreadSafeLayoutFoo #8639

Merged
merged 4 commits into from
Nov 23, 2015
Merged

Use associated types for layout wrapper trait, and generalized ThreadSafeLayoutFoo #8639

merged 4 commits into from
Nov 23, 2015

Conversation

bholley
Copy link
Contributor

@bholley bholley commented Nov 21, 2015

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 21, 2015
@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify layout code, but no reftests are modified. Please consider adding a reftest!

@bholley
Copy link
Contributor Author

bholley commented Nov 21, 2015

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit e7d1603 with merge 2258017...

bors-servo pushed a commit that referenced this pull request Nov 21, 2015
Auto merge of #8639 - bholley:generalize_wrappers2, r=<try>
2258017 
Use associated types for layout wrapper trait, and generalized ThreadSafeLayoutFoo
@bholley
Copy link
Contributor Author

bholley commented Nov 21, 2015

This is a continuation of the work in #8595.

@bors-servo
Copy link
Contributor

💔 Test failed - linux-dev

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 21, 2015
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Nov 21, 2015
@bholley
Copy link
Contributor Author

bholley commented Nov 21, 2015

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit afa3834 with merge dead8a5...

bors-servo pushed a commit that referenced this pull request Nov 21, 2015
Auto merge of #8639 - bholley:generalize_wrappers2, r=<try>
dead8a5 
Use associated types for layout wrapper trait, and generalized ThreadSafeLayoutFoo

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

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

pcwalton
pcwalton reviewed Nov 21, 2015
View reviewed changes
components/layout/wrapper.rs
}

// These can violate the thread-safety and therefore are not public.
trait DangerousThreadSafeLayoutNode<'ln> : ThreadSafeLayoutNode<'ln> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So as I typed this I realized why you didn't use UnsafeThreadSafeLayoutNode, as it's a bit of an oxymoron of a name :) But I still lean toward consistency in use of "Unsafe". How about MemoryUnsafeThreadSafeLayoutNode, to maintain consistency in naming without the paradox?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could, but we already have something called UnsafeLayoutNode, which would be super confusing IMO - that's why I opted for a different name. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And in general, this name doesn't escape anywhere and is used in only a few places in this file, so I think having it be consistent is less important.

@bholley
Copy link
Contributor Author

bholley commented Nov 21, 2015

Since you're looking at it... @pcwalton r?

@pcwalton
Copy link
Contributor

Sure, seems fine.

@pcwalton
Copy link
Contributor

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit afa3834 has been approved by pcwalton

@bors-servo
Copy link
Contributor

⌛ Testing commit afa3834 with merge 711f516...

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 23, 2015
bors-servo pushed a commit that referenced this pull request Nov 23, 2015
Auto merge of #8639 - bholley:generalize_wrappers2, r=pcwalton
711f516 
Use associated types for layout wrapper trait, and generalized ThreadSafeLayoutFoo

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

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

@bors-servo bors-servo merged commit afa3834 into servo:master Nov 23, 2015
@bholley bholley deleted the generalize_wrappers2 branch October 30, 2016 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pcwalton pcwalton

Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bholley @highfive @bors-servo @pcwalton