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

Make Attr::prefix an Atom #5777

Merged
merged 3 commits into from Apr 27, 2015
Merged

Make Attr::prefix an Atom #5777

merged 3 commits into from Apr 27, 2015

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Apr 21, 2015

Rebase of https://github.com/Ms2ger/servo/commits/ICE-attr-prefix-atom

Some of the changes weren't necessary since the internals had been refactored some in the interim. In any case, I was unable to reproduce the ICE reported in rust-lang/rust#18957.

@Ms2ger

Review on Reviewable

@highfive
Copy link

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

@jdm
Copy link
Member

jdm commented Apr 21, 2015

That.. is dedication. Thanks @tamird!

@tamird tamird mentioned this pull request Apr 21, 2015
@bors-servo
Copy link
Contributor

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

@tamird
Copy link
Contributor Author

tamird commented Apr 27, 2015

@jdm @Ms2ger any interest in this?

@jdm
Copy link
Member

jdm commented Apr 27, 2015

Yes, I'll make sure it's reviewed by the end of tomorrow.

@jdm jdm self-assigned this Apr 27, 2015
@jdm
Copy link
Member

jdm commented Apr 27, 2015

Reviewed files:

  • components/script/dom/attr.rs @ r2
  • components/script/dom/bindings/utils.rs @ r2
  • components/script/dom/create.rs @ r3
  • components/script/dom/document.rs @ r3
  • components/script/dom/element.rs @ r3
  • components/script/dom/node.rs @ r3

components/script/dom/node.rs, line 1737 [r3] (raw file):
Atom::from_slice(&p)



Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Apr 27, 2015

-S-awaiting-review +S-needs-code-changes


Comments from the review on Reviewable.io

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 27, 2015
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Apr 27, 2015
@tamird
Copy link
Contributor Author

tamird commented Apr 27, 2015

@jdm
Copy link
Member

jdm commented Apr 27, 2015

@bors-servo: r+

-S-awaiting-review +S-awaiting-merge


Reviewed files:

  • components/script/dom/attr.rs @ r4
  • components/script/dom/create.rs @ r4
  • components/script/dom/document.rs @ r4
  • components/script/dom/element.rs @ r4
  • components/script/dom/node.rs @ r4

Comments from the review on Reviewable.io

@jdm jdm 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 Apr 27, 2015
@bors-servo
Copy link
Contributor

📌 Commit 9435565 has been approved by jdm

@bors-servo
Copy link
Contributor

⌛ Testing commit 9435565 with merge b0a7d1b...

bors-servo pushed a commit that referenced this pull request Apr 27, 2015
Rebase of https://github.com/Ms2ger/servo/commits/ICE-attr-prefix-atom

Some of the changes weren't necessary since the internals had been refactored some in the interim. In any case, I was unable to reproduce the ICE reported in rust-lang/rust#18957.

@Ms2ger

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

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

@bors-servo bors-servo merged commit 9435565 into servo:master Apr 27, 2015
@tamird tamird deleted the ICE-attr-prefix-atom branch April 27, 2015 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants