-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Move event state from Node to Element #7935
Conversation
@@ -20,7 +20,7 @@ use dom::bindings::codegen::Bindings::HTMLTemplateElementBinding::HTMLTemplateEl | |||
use dom::bindings::codegen::Bindings::NamedNodeMapBinding::NamedNodeMapMethods; | |||
use dom::bindings::codegen::Bindings::NodeBinding::NodeMethods; | |||
use dom::bindings::codegen::InheritTypes::{CharacterDataCast, DocumentDerived, ElementCast}; | |||
use dom::bindings::codegen::InheritTypes::{ElementDerived, EventTargetCast, HTMLAnchorElementCast}; | |||
use dom::bindings::codegen::InheritTypes::{ElementDerived, EventTargetCast, HTMLAnchorElementCast, HTMLFieldSetElementDerived, HTMLLegendElementDerived, HTMLOptGroupElementDerived}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://travis-ci.org/servo/servo/jobs/84430060
The tidy check failed because these imports aren't sorted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the imports need to be sorted, so check out the next line and notice how those fall alphabetically before some of the imports on this line. Let me know if you need help
e1c8f9a
to
e593ea7
Compare
test-unit says:
|
☔ The latest upstream changes (presumably #7932) made this pull request unmergeable. Please resolve the merge conflicts. |
e593ea7
to
0bef174
Compare
r? @jdm |
☔ The latest upstream changes (presumably #7873) made this pull request unmergeable. Please resolve the merge conflicts. |
r? @nox |
@bors-servo r+ -S-awaiting-review Reviewed 14 of 14 files at r1, 2 of 2 files at r2, 4 of 4 files at r3. Comments from the review on Reviewable.io |
📌 Commit 0bef174 has been approved by |
That looks legit. |
I didn't realize that Cell.get() returns a copy rather than a ref. Updating. |
Conceptually they belong there, rather than on |Node|. Fixes servo#7934.
This is a simple cut/paste.
After rebasing, this suddenly became a problem again, even though there's no actual size increase here (we're shrinking NodeFlags by 1 byte, and adding 1 byte of EventState). Moving the NodeFlags to the end of Node and the EventState bits to the beginning of Element doesn't seem to helper either. This is probably a padding issue that's worth investigating at some point, but given the level of churn in this code it doesn't seem worth it to fuss to much over this right now.
Though I'm confused as to why this didn't surface before. Does CI only run when the PR Is actually about to be merged? if so, is there a tryserver-like way that I can get test results before that stage? |
b9cba75
to
69255ed
Compare
r? @nox |
You can get a reviewer to run |
@bors-servo r+ Thanks for your contribution. Reviewed 1 of 1 files at r6. Comments from the review on Reviewable.io |
📌 Commit 69255ed has been approved by |
Move event state from Node to Element Just getting my feet wet with Rust here. Please feel free to nit the hell out of it stylistically and idiomatically. :-) <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7935) <!-- Reviewable:end -->
Move event state from Node to Element Just getting my feet wet with Rust here. Please feel free to nit the hell out of it stylistically and idiomatically. :-) <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7935) <!-- Reviewable:end -->
☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt |
Just getting my feet wet with Rust here. Please feel free to nit the hell out of it stylistically and idiomatically. :-)