Webdriver GoBack and GoForward commands wait for navigation complete #37950
Conversation
31f06ff to
19b8dd9
Compare
There was a problem hiding this comment.
Errors on this file can be fixed in: #37955
19b8dd9 to
f8142d3
Compare
|
Too many suspicious failed tests: https://github.com/longvatrong111/servo/actions/runs/16187886872 |
|
My suggestion for the design I think would address more edge cases:
|
This sounds reasonable to me.
Can you explain why it is better than let |
I spent a bunch of time looking into this (as well as the parts of the HTML specification that define firing
However, I did some more investigation and spec reading and came across w3c/webdriver#777, which is pretty conclusive that at least one other browser does not attempt to wait for pages to load when performing session history traversal with webdriver. Given that, I'm going to modify my suggestion to add the load status sender as an argument to servo/components/constellation/constellation.rs Line 4640 in 099d20f servo/components/constellation/constellation.rs Line 4746 in 099d20f The benefit of doing this in the constellation, rather than the embedding layer, is that 1) we won't time out waiting if we happen to hit any early returns in the session traversal code that skip notify_history_changed, and 2) we avoid the risk of accidentally triggering the LoadComplete via notify_history_changed from unrelated session history changes that might be racing with webdriver commands (eg. website JS that calls |
I see, thank you very much for your details. |
|
@jdm I have a suggestion to your solution. servo/components/constellation/constellation.rs Line 4746 in 099d20f But instead passing and using a load status sender, we can modify EmbedderMsg::HistoryChanged or add a new enum to EmbedderMsg.With this modification, we still have all benefit you mentioned and we can also avoid bring back webdriver load status sender to constellation. The response is a bit longer but it's ok for webdriver. On the orther hand, constellation is lighter. To implement your solution, I think we can send a response to webdriver at the end of: servo/components/constellation/constellation.rs Lines 1381 to 1383 in 099d20f |
a7fcf11 to
acc4487
Compare
|
@jdm your solution works well, and with #37955 it can pass most test of GoBack and GoForward. |
acc4487 to
902907e
Compare
902907e to
6f4f809
Compare
6f4f809 to
6496f32
Compare
|
The bad news is that I attempted to leave a review 12 hours ago where I said I had an idea I wanted to play with but I was ok with the current PR merging. The good news is that I have a proposal: longvatrong111#1 This uses your idea of always sending the notification after calling |
|
I got these results when running locally with that change: |
Signed-off-by: batu_hoang <longvatrong111@gmail.com>
…e that to notify webdriver. Signed-off-by: Josh Matthews <josh@joshmatthews.net>
Signed-off-by: Josh Matthews <josh@joshmatthews.net>
df6e64f to
d37a260
Compare
Signed-off-by: batu_hoang <hoang.binh.trong@huawei.com>
|
🔨 Triggering try run (#16287524471) for Linux (WPT) |
|
Update test expectation for |
|
Test results for linux-wpt from try job (#16287524471): Flaky unexpected result (19)
Stable unexpected results that are known to be intermittent (18)
Stable unexpected results (1)
|
|
|
|
@jdm seem like the test result of |
After sending
GoBackorGoForwardcommand, webdriver wait for the navigation complete.It can be achieved by waiting for
WebViewDelegate::notify_history_changedTesting:
tests/wpt/meta/webdriver/tests/classic/back/back.pytests/wpt/meta/webdriver/tests/classic/forward/forward.py