E54D DevTools: Implement support for showing `source_content` in `Debugger > Source` panel by atbrakhi · Pull Request #36774 · servo/servo · GitHub
[go: up one dir, main page]
Skip to content

DevTools: Implement support for showing source_content in Debugger > Source panel #36774

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Jun 13, 2025
Merged

DevTools: Implement support for showing source_content in Debugger > Source panel #36774

merged 10 commits into from
Jun 13, 2025

Conversation

atbrakhi
Copy link
Member
@atbrakhi atbrakhi commented Apr 30, 2025 •
edited
Loading

This patch adds support for showing source_content in Debugger > Source panel. This works by handling the clients source messages in the source actor. These source actors are already advertised as resource via the watcher, populating the source list. We also update the sources handler in thread actor for future work in thread debugging.

Note: while this PR also adds support for showing worker script source_content, worker has been broken (See #37012). I was able to confirm the content_type and source_content for worker script in logs.

image

Fixes: part of #36027

delan
delan reviewed May 6, 2025
View reviewed changes
@atbrakhi atbrakhi marked this pull request as ready for review May 15, 2025 11:17
@atbrakhi atbrakhi requested a review from gterzian as a code owner May 15, 2025 11:17
@atbrakhi atbrakhi force-pushed the source_panel_code branch from 751c944 to c5c54ad Compare May 15, 2025 11:27
@atbrakhi atbrakhi mentioned this pull request May 9, 2025
Open
81 tasks
@atbrakhi atbrakhi force-pushed the source_panel_code branch from c5c54ad to dfef24d Compare May 15, 2025 11:43
@atbrakhi atbrakhi force-pushed the source_panel_code branch from dfef24d to 53a52e1 Compare May 27, 2025 10:14
delan
delan reviewed May 28, 2025
View reviewed changes
Copy link
Member
@delan delan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming back to the patch a month later, I’ve completely forgotten the sequence of messages needed to send source contents to the Debugger > Sources panel, and I’ll admit I struggled to relearn that from our code.

Can we add an automated test as part of this patch? I was not as concerned about this when we were last pairing, but I think now more than ever it’s important that we land new features with tests. This both prevents regressions and documents the minimal message sequence needed to get the feature working.

Also can we document why we’re adding each message handler? Ideally with links to Firefox docs or source code, but at the very least that they are needed for sending source contents to Debugger > Sources. As the actors get more complex, it gets harder to understand which features depend on which messages.

atbrakhi added 9 commits June 12, 2025 12:34
@atbrakhi
Implement support for showing source content in debugger>source
39aad47 
Signed-off-by: atbrakhi <atbrakhi@igalia.com>
@atbrakhi
handle_script_source_info should send source_actor_name not thread
3696a76 
Signed-off-by: atbrakhi <atbrakhi@igalia.com>
@atbrakhi
pass correct actor name to SourceData
dcfb090 
Signed-off-by: atbrakhi <atbrakhi@igalia.com>
@atbrakhi
fmt
8caa90b 
Signed-off-by: atbrakhi <atbrakhi@igalia.com>
@atbrakhi
register source_actor in source and not in handle_script
a73a141 
Signed-off-by: atbrakhi <atbrakhi@igalia.com>
@atbrakhi
send source_content for worker script
4340f39 
Signed-off-by: atbrakhi <atbrakhi@igalia.com>
@atbrakhi
send content_type via script
279ebdc 
Signed-off-by: atbrakhi <atbrakhi@igalia.com>
@atbrakhi
review fix
df23102 
Signed-off-by: atbrakhi <atbrakhi@igalia.com>
@atbrakhi
add tests for source content
0f7607b 
Signed-off-by: atbrakhi <atbrakhi@igalia.com>
@atbrakhi atbrakhi force-pushed the source_panel_code branch from 53a52e1 to 0f7607b Compare June 12, 2025 10:51
@atbrakhi
Copy link
Member Author
atbrakhi commented Jun 12, 2025 •
edited
Loading

Coming back to the patch a month later, I’ve completely forgotten the sequence of messages needed to send source contents to the Debugger > Sources panel, and I’ll admit I struggled to relearn that from our code.

Can we add an automated test as part of this patch? I was not as concerned about this when we were last pairing, but I think now more than ever it’s important that we land new features with tests. This both prevents regressions and documents the minimal message sequence needed to get the feature working.

Also can we document why we’re adding each message handler? Ideally with links to Firefox docs or source code, but at the very least that they are needed for sending source contents to Debugger > Sources. As the actors get more complex, it gets harder to understand which features depend on which messages.

Hmm, we can indeed add more comments. In general, when I am implementing a feature, I mostly look at the Firefox logs and its expectations for a certain actor and then implement it. This doesn’t necessarily involve reading the Firefox source code all the time. I find myself searching the Firefox DevTools code when I have implemented an actor as per the Firefox logs but it is not working, then I look into the Firefox code. But I agree that we should add more comments. Happy to hear what else we can do.

I have added some tests to test the source_content. I will be adding source content related tests with #36874 that is the next thing i am working on

delan
delan approved these changes Jun 13, 2025
View reviewed changes
Copy link
Member
@delan delan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, with a few minor comments :)

@atbrakhi
review fix
9c8cd3b 
Signed-off-by: atbrakhi <atbrakhi@igalia.com>
@atbrakhi atbrakhi force-pushed the source_panel_code branch from c6d7d4a to 9c8cd3b Compare June 13, 2025 08:57
@atbrakhi atbrakhi enabled auto-merge June 13, 2025 09:22
@atbrakhi atbrakhi added this pull request to the merge queue Jun 13, 2025
Merged via the queue into servo:main with commit 7a801f0 Jun 13, 2025
21 checks passed
@atbrakhi atbrakhi deleted the source_panel_code branch June 13, 2025 10:02
PotatoCP pushed a commit to PotatoCP/servo that referenced this pull request Jun 18, 2025
@atbrakhi @PotatoCP
DevTools: Implement support for showing source_content in `Debugger…
1b57ca3 
… > Source` panel (servo#36774)

This patch adds support for showing source_content in `Debugger >
Source` panel. This works by handling the clients `source` messages in
the source actor. These source actors are already advertised as resource
via the watcher, populating the source list. We also update the
`sources` handler in thread actor for future work in thread debugging.

Note: while this PR also adds support for showing worker script
source_content, worker has been broken (See
servo#37012). I was able to confirm the
`content_type` and `source_content` for worker script in logs.


![image](https://github.com/user-attachments/assets/bd53ea29-003a-4b5e-a3e8-6e280afa4671)

Fixes: part of servo#36027

---------

Signed-off-by: atbrakhi <atbrakhi@igalia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@delan delan delan approved these changes

@gterzian gterzian Awaiting requested review from gterzian gterzian is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@atbrakhi @delan
0