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

Dump initial prototype of devtools server into the build. Expect lies if... #3172

Merged
merged 7 commits into from Sep 19, 2014

Conversation

@jdm
Copy link
Member

jdm commented Aug 27, 2014

... you try to use it for anything real.

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Aug 27, 2014

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

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.

@highfive
Copy link

highfive commented Aug 27, 2014

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@jdm
Copy link
Member Author

jdm commented Aug 27, 2014

This isn't in the greatest state to be merged right now (for example, I hacked around the inability to call get_type_id in the AnyRefExt implementation by making is return true unconditionally), but this is something I'd like to get in the tree so we can start iterating on it. I'm going to write up documentation about my plans for the design and mini-projects that interested parties could work on.

@tetsuharuohzeki
Copy link
Member

tetsuharuohzeki commented Aug 27, 2014

we should refer to documents of remote debugging protocol in the code comment.

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Aug 27, 2014

cc @glennw @SimonSapin @mbrubeck (since we were talking about how awesome this would be last night...)

@jdm
Copy link
Member Author

jdm commented Aug 27, 2014

@jdm
Copy link
Member Author

jdm commented Sep 1, 2014

This is in a good state for review at this point. Who wants to take it on? @Ms2ger should do the script task bits, but the actual devtools server implementation is open. @larsbergstrom? @metajack? @glennw?

@jdm jdm force-pushed the jdm:devtools branch 2 times, most recently from a854d5c to 4af3495 Sep 2, 2014
}
}
}
let mut new_actors = self.new_actors.borrow_mut();

This comment has been minimized.

Copy link
@glennw

glennw Sep 4, 2014

Member

Just for my own understanding, why/when would you want to register an actor later?

This comment has been minimized.

Copy link
@jdm

jdm Sep 4, 2014

Author Member

I haven't found a way pass a mutable ActorRegistry reference to handle_message, since self.actors is currently frozen. This means that it's impossible to register new actors in the same vector, hence the delayed registration.

json::Object(m)
} else if val.is_infinite() {
let mut m = TreeMap::new();
if val < 0. {

This comment has been minimized.

Copy link
@glennw

glennw Sep 4, 2014

Member

Is this condition around the wrong way?

This comment has been minimized.

Copy link
@jdm

jdm Sep 4, 2014

Author Member

Yes.

@jdm jdm force-pushed the jdm:devtools branch 4 times, most recently from 4a4a702 to 6d57223 Sep 15, 2014
@jdm jdm force-pushed the jdm:devtools branch from 6d57223 to fae7ce3 Sep 18, 2014
jdm added a commit that referenced this pull request Sep 19, 2014
Dump initial prototype of devtools server into the build. Expect lies if...
@jdm jdm merged commit b82c0dc into servo:master Sep 19, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.