Skip to content

/ servo

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

Implemented Document.currentScript #5066

Closed
wants to merge 1 commit into from
Closed

Implemented Document.currentScript #5066

wants to merge 1 commit into from

Conversation

@luniv
Copy link
Contributor

luniv commented Feb 25, 2015

Implements #5057 (Document.currentScript)
@highfive
Copy link

highfive commented Feb 25, 2015

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @larsbergstrom (or someone else) soon.
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Feb 25, 2015

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

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.
@jdm
Copy link
Member

jdm commented Feb 25, 2015

Exciting! There are comments on the Critic review; make sure to click the 'sign in' link so you get future updates by email.
@luniv
Copy link
Contributor Author

luniv commented Feb 25, 2015

Just realized that I have the wrong email in the commits (defaulted to my work email). Is there a way to to rewrite & push the commits without breaking the code review?
@jdm
Copy link
Member

jdm commented Feb 25, 2015

Nope. Just leave it until the review is complete, and then you can force push with the amended author.
@Ms2ger
Copy link
Contributor

Ms2ger commented Feb 26, 2015

@luniv: good to go; please squash the two commits and update your email address.
@luniv luniv force-pushed the luniv:document-currentscript branch from ea9c1ab to df6962d Feb 26, 2015
@luniv
Copy link
Contributor Author

luniv commented Feb 26, 2015

Done!
@Ms2ger
Copy link
Contributor

Ms2ger commented Feb 26, 2015

@luniv: I'm afraid it needs a rebase too
@jdm jdm added S-needs-rebase and removed S-needs-squash labels Feb 26, 2015
@luniv luniv force-pushed the luniv:document-currentscript branch from df6962d to 5996e42 Feb 27, 2015
@luniv
Copy link
Contributor Author

luniv commented Feb 27, 2015

Rebased.
@jdm jdm added S-awaiting-merge and removed S-needs-rebase labels Feb 27, 2015
bors-servo pushed a commit that referenced this pull request Feb 27, 2015
bors-servo
auto merge of #5066 : luniv/servo/document-currentscript, r=Ms2ger
Loading status checks…
1d9096c 
Implements #5057 (Document.currentScript)
@luniv
Implement Document.currentScript
Loading status checks…
5f5d124
@luniv
Copy link
Contributor Author

luniv commented Feb 27, 2015

Looks like I forgot to remove some expected fails from html/dom/interfaces.html.

Do I update the commit and just push it again?
@jdm
Copy link
Member

jdm commented Feb 27, 2015

Yep, just amend the commit and force push.
@luniv luniv force-pushed the luniv:document-currentscript branch from 5996e42 to 5f5d124 Feb 27, 2015
@luniv
Copy link
Contributor Author

luniv commented Feb 27, 2015

Done.
@jdm

This comment has been minimized.

Sign in to view
Copy link

jdm commented on 5f5d124 Feb 27, 2015

r+
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented on 5f5d124 Feb 27, 2015

saw approval from jdm
at luniv@5f5d124

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Feb 27, 2015

merging luniv/servo/document-currentscript = 5f5d124 into auto

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Feb 27, 2015

luniv/servo/document-currentscript = 5f5d124 merged ok, testing candidate = 26567ef

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Feb 27, 2015

all tests pass:
success: http://build.servo.org/builders/linux1/builds/873
success: http://build.servo.org/builders/linux2/builds/871
success: http://build.servo.org/builders/android/builds/907
success: http://build.servo.org/builders/mac1/builds/1088
success: http://build.servo.org/builders/mac2/builds/1088
success: http://build.servo.org/builders/gonk/builds/59

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Feb 27, 2015

fast-forwarding master to auto = 26567ef
bors-servo pushed a commit that referenced this pull request Feb 27, 2015
bors-servo
auto merge of #5066 : luniv/servo/document-currentscript, r=jdm
Loading status checks…
26567ef 
Implements #5057 (Document.currentScript)
@bors-servo bors-servo closed this Feb 27, 2015
@luniv luniv deleted the luniv:document-currentscript branch Feb 27, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Feb 27, 2015

\o/ Thanks!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@luniv @highfive @hoppipolla-critic-bot @jdm @Ms2ger @bors-servo
You can’t perform that action at this time.