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

Improve devtools experience when navigating #26325

Merged
merged 7 commits into from Apr 28, 2020
Merged

Conversation

@jdm
Copy link
Member

jdm commented Apr 26, 2020

The primary motivation for this work was to fix #15425, and these changes make it possible to use the devtools to meaningfully inspect multiple pages when navigating between them. Navigating through session history is not yet supported.

These changes also include improvements to the dedicated worker support, which broke at some point. We now can observe console messages in workers.

@highfive
Copy link

highfive commented Apr 26, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/worker.rs, components/script/serviceworker_manager.rs, components/script/dom/serviceworkerregistration.rs, components/script/dom/workerglobalscope.rs, components/script/dom/dedicatedworkerglobalscope.rs and 2 more
  • @KiChjang: components/script/dom/worker.rs, components/script/serviceworker_manager.rs, components/script/dom/serviceworkerregistration.rs, components/script/dom/workerglobalscope.rs, components/script/dom/dedicatedworkerglobalscope.rs and 2 more
@highfive
Copy link

highfive commented Apr 26, 2020

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@jdm jdm changed the title Improve devtools experience Improve devtools experience when navigating Apr 26, 2020
jdm added 2 commits Apr 25, 2020
Break the association between pipelines and browsing context actors.
Now there is one browsing context actor per actual browsing context,
and individual actors keep track of known pipelines as necessary.
There is also one console/performance/timeline/inspector/etc. actor
per browsing context.

This also centralizes more information in the browsing context actor.
Rather than duplicating state for the active pipeline in actors that
need to use it, each actor now remembers the name of its associated
browsing context actor and obtains that state whenever it's necessary.
@jdm jdm force-pushed the jdm:devtools-nav branch from b488636 to 76a6080 Apr 26, 2020
@jdm
Copy link
Member Author

jdm commented Apr 27, 2020

r? @asajeffrey (due to highfive's severe but fair random selection)

@gterzian
Copy link
Member

gterzian commented Apr 28, 2020

I would be happy to review this if @asajeffrey has other things to do, I started casually looking at this yesterday already...

In any case, I do have one suggestion: instead of using the BrowsingContextId as a kind of unique identifier that can survive navigation from one pipeline to another, how about we introduce a new DevtoolsId or similar in the msg crate, to be used for that purpose?

(note that worker threads have their own pipeline namespace, which would be necessary to create such an id, since #23934)

I think that would make it easier to use in the context of the Service worker as well, since unlike the dedicated worker that one cannot easily be tied to a BC.

@jdm
Copy link
Member Author

jdm commented Apr 28, 2020

I would rather file that as a possible improvement, since service worker support is only hypothetical right now.

Copy link
Member

gterzian left a comment

Sounds good, then as far as I can tell it looks good to me.

};
self.streams.borrow_mut().push(stream.try_clone().unwrap());
stream.write_json_packet(&msg);
// FIXME: fix messages to not requir forging a pipeline for worker messages

This comment has been minimized.

Copy link
@gterzian

gterzian Apr 28, 2020

Member

nit: typo

@jdm jdm force-pushed the jdm:devtools-nav branch from 76a6080 to b9bf9f8 Apr 28, 2020
@jdm
Copy link
Member Author

jdm commented Apr 28, 2020

@bors-servo r=gterzian
Thanks for reviewing!

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

📌 Commit b9bf9f8 has been approved by gterzian

@highfive highfive assigned gterzian and unassigned asajeffrey Apr 28, 2020
bors-servo added a commit that referenced this pull request Apr 28, 2020
Improve devtools experience when navigating

The primary motivation for this work was to fix #15425, and these changes make it possible to use the devtools to meaningfully inspect multiple pages when navigating between them. Navigating through session history is not yet supported.

These changes also include improvements to the dedicated worker support, which broke at some point. We now can observe console messages in workers.
@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

Testing commit b9bf9f8 with merge 692a2e9...

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member Author

jdm commented Apr 28, 2020

@bors-servo r=gterzian

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

📌 Commit 02ce618 has been approved by gterzian

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

Testing commit 02ce618 with merge e0c5dbe...

bors-servo added a commit that referenced this pull request Apr 28, 2020
Improve devtools experience when navigating

The primary motivation for this work was to fix #15425, and these changes make it possible to use the devtools to meaningfully inspect multiple pages when navigating between them. Navigating through session history is not yet supported.

These changes also include improvements to the dedicated worker support, which broke at some point. We now can observe console messages in workers.
@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member Author

jdm commented Apr 28, 2020

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

Testing commit 02ce618 with merge 1aab10f...

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

☀️ Test successful - status-taskcluster
Approved by: gterzian
Pushing 1aab10f to master...

@bors-servo bors-servo merged commit 1aab10f into servo:master Apr 28, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
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.

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