-
Notifications
You must be signed in to change notification settings - Fork 59
emulation.setNetworkConditions:offline
#948
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
base: main
Are you sure you want to change the base?
Conversation
909666e
to
4b304a6
Compare
emulation.setNetworkConditions
3e9d492
to
7d5df6c
Compare
5a54342
to
08b8e65
Compare
emulation.setNetworkConditions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we discuss this again with the working group (if it was discussed recently, I'm missing the summary) ?
Notes from the Working Group meeting:
|
2283b57
to
56e6e63
Compare
emulation.setNetworkConditions
emulation.setNetworkConditions:offline
7e372ac
to
3f3075d
Compare
@juliandescottes @OrKoN could you please take another look? |
3f3075d
to
2f3394e
Compare
@juliandescottes PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update, and sorry for the delay. Overall I think the update for BiDi looks good, but I'm not sure how we should treat this PR since we have a dependency on HTML and Fetch.
I prefer to let James or other people more used to the spec world comment on this, my feeling is that we might need at least some agreement on the other PRs that they are ok with the direction. We can also discuss it next wednesday during the meeting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently I made some comments a while ago and didn't submit them…
index.bs
Outdated
|
||
1. If the result of [=WebDriver BiDi network is offline=] with |environment settings| is true: | ||
|
||
1. [=Make disappear=] |webSocket|. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this right? It's going to send a close frame to the server, which wouldn't happen if the browser was actually offline. Do browsers' existing offline modes actually send this frame when activated on a page with an open websocket?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to Fail the WebSocket connection
|
||
1. Let |emulated network conditions| be null. | ||
|
||
1. If |command parameters|["<code>networkConditions</code>"] is not null and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now the check for type can just be an assert. I see you're trying to define this for extensibility although it does make the API rather odd in the short term, and the obvious extensions into network conditions seem like they're going to be rather challenging to standardise. I wonder if we'd be better with just a network.setOffline({contexts, userContexts, offline: <bool>})
command for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't plan to extend for other network conditions (e.g. high-latency or low-bandwidth), I'd propose to:
- Move the method to
emulation.setNetworkOffline
, as it influences not only the network, but the navigator as well. - Allow for only
offline: true / null
, as we cannot emulate being online when offline, we can only restore the default behavior.
The Browser Testing and Tools Working Group just discussed The full IRC log of that discussion<tidoust> Topic: emulation.setNetworkConditions:offline<tidoust> github: https://github.com//pull/948 <jgraham> q+ <tidoust> sadym: Current version of spec is that no network emulation is pretty much airplane mode. <jgraham> ack next <tidoust> jgraham: I was definitely imagining something like "what if the network goes down at this point", rather than a clean network disconnection. <orkon> q+ <tidoust> ... What happens if you engage offline mode. What does the prior art look like? Closing connections gracefully? Or shutting them down? <sadym> q+ <jgraham> ack next <jdescottes> q+ <tidoust> orkon: We discussed this. If you go offline, usually it's not instant, you have timeouts on HTTP, TCP, etc. navigator.online does not report the status right away. <tidoust> ... What we discussed is that switching would only apply to new requests. <tidoust> ... This is a simpler model, and I don't see a strong use case for airplane mode where you emulate offline mode. <tidoust> ... You want to check that, e.g., your service worker picks up things correctly. <tidoust> ... It would be difficult to emulate the real circumstances for when you're going offline given the many ongoing connections that may be happening. <jgraham> ack next <tidoust> sadym: Emulate tunnel in the train vs. airplane is what we need to get consensus on, then. <jgraham> q+ <jgraham> ack next <tidoust> jdescottes: The situation in Firefox is that there are two ways to get offline. <tidoust> ... Two inconsistent features, including one in dev tools. <tidoust> ... I still think that we should do more than preventing next request. Just failing requests on beforeRequestSent is not enough. <tidoust> ... Some applications may be relying on other signals to switch to offline mode. <tidoust> ... Also, this PR depends on two other PRs, do we want to wait until these two PRs are reviewed? <jgraham> ack next <sadym> q+ <tidoust> jgraham: In most testing scenarios, I agree that simplifying things is probably enough. <tidoust> ... I believe the current PR covers the needs. <tidoust> ... In the context of the scripting API, we've sometimes landed updates even when the background is broken when there are inter-dependencies. Sometimes, we need to go back and clean up the other PRs. <sadym> q+ <tidoust> Blaze: Webkit uses this online/offline marker to switch mode. <tidoust> ... That should align well with this. <jgraham> ack next <tidoust> ... I don't see any blocker. |
86e4787
to
ccaddea
Compare
@jgraham could you please take another look? |
First step addressing #776.
Preview | Diff