devtools: Implement frame scoped evaluation#42936
Conversation
|
This looks fine(screenshot from whattimeisit.io) 
This is not ideal. Firefox does not show any value for 
|
When paused at a breakpoint, hovering over local variables now shows their value. Previously `eval` used executeInGlobal() which could not access local scope, so hovering showed ReferenceError instead of the actual value. Signed-off-by: atbrakhi <[email protected]>
atbrakhi
force-pushed
the
reference_error
branch
from
ffd0ebb to
e1d4ad2
Compare
| }; | ||
|
|
||
| // TODO: Catch and return exception values from JS evaluation | ||
| // TODO: This should have a has_exception field |
There was a problem hiding this comment.
I needed to do what this comment said, to introduce a has_exception field in EvaluateJSReply. Introducing this resolved the reference error we see in screenshot of the previous comment.
servo-highfive
added
the
S-awaiting-review
|
@eerii i think this is ready for review now :) |
eerii
left a comment
eerii
left a comment
There was a problem hiding this comment.
Looks great! I tested it locally :)
| /// TODO: generalize this beyond the EvaluateJS message? | ||
| #[derive(Debug, Deserialize, Serialize)] | ||
| pub enum EvaluateJSReply { | ||
| pub enum EvaluateJSReplyValue { |
There was a problem hiding this comment.
Very small nits, but maybe this can be simplified to EvaluateJSValue, or even JSValue if we are going to use it outside of eval (though that might be confused with the other JSValue).
Also, I'm not sure why clippy isn't complaining, but since all of the variants end with Value we should probably remove the suffix and have EvaluateJSValue::Void, and so on. The lint is enum_variant_names.
There was a problem hiding this comment.
I spent lots of time on this renaming. My other option was also EvaluateJSValue but i didn't wanted to give the feel that this is all the Evaluate js value as we have a lot more then that. Hence I couldn't convince myself to rename it to EvaluateJSValue. But i will agree that i am also not a big fan of EvaluateJSReplyValue.
I think we will be touching this function more in coming weeks, how do you feel about landing it the way it is and renaming it later?...i am hoping to find something better
There was a problem hiding this comment.
Yeah, it is a tricky naming choice indeed. I agree that we should think about it and rename it later :)
servo-highfive
removed
the
S-awaiting-review
servo-highfive
added
the
S-awaiting-merge
servo-highfive
removed
the
S-awaiting-merge
Class name in `ObjectActor` was harcoded to `Window`, which is wrong. This change removes the hardcoded value and replaces it with actual class name. **Before:** <img width="320" height="163" alt="image" src="https://github.com/user-attachments/assets/f94df58a-5309-4fd1-bb02-aa06dfeb32c3" /> **After:** <img width="353" height="138" alt="image" src="https://github.com/user-attachments/assets/ad129676-8c3d-48e2-a543-1447c75c40f0" /> Testing: Existing test passes Fixes: part of #36027 Depends on #42936 Signed-off-by: atbrakhi <[email protected]>
When paused at a breakpoint, hovering over local variables now shows their value. Previously `eval` used `executeInGlobal()` which could not access local scope, so hovering showed `ReferenceError` instead of the actual value. https://github.com/user-attachments/assets/05247b82-4e4d-422d-a428-63e46b55d55f Testing: Added a new test Fixes: Part servo#36027 Signed-off-by: atbrakhi <[email protected]>
Class name in `ObjectActor` was harcoded to `Window`, which is wrong. This change removes the hardcoded value and replaces it with actual class name. **Before:** <img width="320" height="163" alt="image" src="https://github.com/user-attachments/assets/f94df58a-5309-4fd1-bb02-aa06dfeb32c3" /> **After:** <img width="353" height="138" alt="image" src="https://github.com/user-attachments/assets/ad129676-8c3d-48e2-a543-1447c75c40f0" /> Testing: Existing test passes Fixes: part of servo#36027 Depends on servo#42936 Signed-off-by: atbrakhi <[email protected]>
3 participants
When paused at a breakpoint, hovering over local variables now shows their value. Previously
evalusedexecuteInGlobal()which could not access local scope, so hovering showedReferenceErrorinstead of the actual value.frame-scope.mov
Testing: Added a new test
Fixes: Part #36027