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

Allows object evaluation in devtools -- Closes #6724 #7101

Merged
merged 1 commit into from Aug 10, 2015

Conversation

@HarryLovesCode
Copy link
Contributor

HarryLovesCode commented Aug 8, 2015

The purpose of this is to fix how objects were previously evaluated in
the developer tools.

  • Before this, evaluating an object such as the window would panic!
  • After this, evaluating an object such as the window outputs [object Window]

A few things to note:

  • This commit contains unsafe code.
  • This does not contain a test because the developer tools cannot be properly tested until #5971 lands.

Review on Reviewable

@HarryLovesCode HarryLovesCode changed the title Allows object evaluation in devtools (Fixes #6724) Allows object evaluation in devtools -- Closes #6724 Aug 8, 2015
@highfive
Copy link

highfive commented Aug 9, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 9, 2015

r? @jdm

@jdm
Copy link
Member

jdm commented Aug 9, 2015

Great work @HarryLovesCode! A few small changes and this should be ready to merge :)
-S-awaiting-review +S-needs-code-changes


Reviewed 5 of 5 files at r1.
Review status: 4 of 5 files reviewed at latest revision, 10 unresolved discussions, all commit checks successful.


components/devtools/actors/object.rs, line 23 [r1] (raw file):
This should be Ok(false) since we don't actually handle any messages.


components/devtools/lib.rs, line 47 [r1] (raw file):
nit: add this in alphabetical order (and move the network_event import appropriately)


components/devtools/lib.rs, line 79 [r1] (raw file):
nit: add this in alphabetical order (and move network_event appropriately)


components/devtools/lib.rs, line 222 [r1] (raw file):
No need for this.


components/devtools/lib.rs, line 262 [r1] (raw file):
No need for this.


components/devtools_traits/lib.rs, line 80 [r1] (raw file):
Let's make this ActorValue { class: String, uuid: String } for clarity.


components/script/devtools.rs, line 27 [r1] (raw file):
nit: add uuid after the std imports.


components/script/devtools.rs, line 47 [r1] (raw file):
Let's keep this as else { with the assert.


components/script/devtools.rs, line 48 [r1] (raw file):
Let's just wrap about the ObjectClassName call instead of the whole block.


components/script/devtools.rs, line 50 [r1] (raw file):
This can be rval.handle() instead of creating one by hand.


Comments from the review on Reviewable.io

@HarryLovesCode
Copy link
Contributor Author

HarryLovesCode commented Aug 9, 2015

Review status: 4 of 5 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


components/devtools/lib.rs, line 222 [r1] (raw file):
I think this review is on a previous commit. Already removed this.


components/devtools/lib.rs, line 262 [r1] (raw file):
I think this review is on a previous commit. Already removed this.


Comments from the review on Reviewable.io

@HarryLovesCode
Copy link
Contributor Author

HarryLovesCode commented Aug 9, 2015

Review status: 4 of 5 files reviewed at latest revision, all discussions resolved, all commit checks successful.


components/script/devtools.rs, line 50 [r1] (raw file):
I'm not sure about this. rval.handle() is of type js::jsapi::Handle<js::jsapi::Value> and ObjectClassName expects this type: js::jsapi::Handle<*mut js::jsapi::JSObject>. (Sorry about any typos. Reviewable keeps scrolling everytime I press a key)


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Aug 9, 2015

You are correct.


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Aug 9, 2015

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


components/script/devtools.rs, line 50 [r1] (raw file):
Oh, my mistake. Then I'd prefer

let obj = RootedObject::new(cx, rval.get().to_object());
let class_name = ObjectClassName(cx, obj.handle());

Comments from the review on Reviewable.io

The purpose of this is to fix how objects were previously evaluated in
the developer tools.

- Before this, evaluating an object such as the `window` would `panic!`
- After this, evaluating an object such as the `window` outputs `[object
  Window]`

A few things to note:

- This commit contains `unsafe` code.
- This does not contain a test because the developer tools cannot be properly tested until #5971 lands.
@HarryLovesCode
Copy link
Contributor Author

HarryLovesCode commented Aug 9, 2015

ping @jdm

@jdm
Copy link
Member

jdm commented Aug 9, 2015

@bors-servo: r+
Thanks for the patch!


Reviewed 5 of 5 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Aug 9, 2015

📌 Commit e0f007a has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Aug 9, 2015

Testing commit e0f007a with merge f75f019...

bors-servo pushed a commit that referenced this pull request Aug 9, 2015
Allows object evaluation in devtools -- Closes #6724

The purpose of this is to fix how objects were previously evaluated in
the developer tools.

- Before this, evaluating an object such as the `window` would `panic!`
- After this, evaluating an object such as the `window` outputs `[object
  Window]`

A few things to note:

- This commit contains `unsafe` code.
- This does not contain a test because the developer tools cannot be properly tested until #5971 lands.

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

bors-servo commented Aug 9, 2015

💔 Test failed - mac2

@HarryLovesCode
Copy link
Contributor Author

HarryLovesCode commented Aug 9, 2015

The mac2 machine failed before compilation even began. Can we rerun that?

@metajack
Copy link
Contributor

metajack commented Aug 10, 2015

@bors-servo retry

  • failed during git checkout
@bors-servo
Copy link
Contributor

bors-servo commented Aug 10, 2015

Testing commit e0f007a with merge f77973c...

bors-servo pushed a commit that referenced this pull request Aug 10, 2015
Allows object evaluation in devtools -- Closes #6724

The purpose of this is to fix how objects were previously evaluated in
the developer tools.

- Before this, evaluating an object such as the `window` would `panic!`
- After this, evaluating an object such as the `window` outputs `[object
  Window]`

A few things to note:

- This commit contains `unsafe` code.
- This does not contain a test because the developer tools cannot be properly tested until #5971 lands.

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

bors-servo commented Aug 10, 2015

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

@bors-servo bors-servo merged commit e0f007a into servo:master Aug 10, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
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.