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

Replace XHR events for generic ones in ScriptTask #4474

Merged
merged 1 commit into from
Dec 24, 2014
Merged

Replace XHR events for generic ones in ScriptTask #4474

merged 1 commit into from
Dec 24, 2014

Conversation

thiagopnts
Copy link
Contributor

This refs #3735. As discussed in the issue, I did it cloning when I couldn't dereference an attribute. The trait method should be on &self for object-safety reasons.

@jdm
Copy link
Member

jdm commented Dec 23, 2014

@jdm jdm added the S-awaiting-review There is new code that needs to be reviewed. label Dec 23, 2014
@thiagopnts
Copy link
Contributor Author

Ops, my bad I think I forgot to run everything after removing the older XHR msgs at the end. I'll fix this and squash.

@jdm jdm closed this Dec 23, 2014
@jdm jdm reopened this Dec 23, 2014
@hoppipolla-critic-bot
Copy link

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

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.

fixup! Replace XHR events for generic ones in ScriptTask

fixup! Replace XHR events for generic ones in ScriptTask
@thiagopnts
Copy link
Contributor Author

I fixed the issue and squashed it

@jdm
Copy link
Member

jdm commented Dec 24, 2014

In general it's preferred to make additional commits to address review comments, and then we squash once the review is complete. However, the result here looks fine to me this time.

@jdm jdm added S-awaiting-review There is new code that needs to be reviewed. 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 Dec 24, 2014
bors-servo pushed a commit that referenced this pull request Dec 24, 2014
This refs #3735. As discussed in the issue, I did it cloning when I couldn't dereference an attribute. The trait method should be on `&self` for object-safety reasons.
bors-servo pushed a commit that referenced this pull request Dec 24, 2014
This refs #3735. As discussed in the issue, I did it cloning when I couldn't dereference an attribute. The trait method should be on `&self` for object-safety reasons.
@bors-servo bors-servo closed this Dec 24, 2014
@bors-servo bors-servo merged commit 271aa27 into servo:master Dec 24, 2014
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