1766217 - browsingContext.navigate should not use the unload timer

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, defect, P3)

Description

[Julian Descottes [:jdescottes]]

From Bug 1766043, when we trigger a navigation explicitly, we should always have a navigation and therefore the unload timer used in the ProgressListener should not be used.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #4)

Oh, we shouldn't use the unload timer for the navigate command given that it should always navigate. In cases when the target URL has already been loaded we should do the check similar to Marionette to see if a load in the browser will happen at all.

I would suggest that we file a new bug to specifically cover this.

Comment 1

[Julian Descottes [:jdescottes]]

Makes sense to me to not allow any unload timer here.

In cases when the target URL has already been loaded we should do the check similar to Marionette to see if a load in the browser will happen at all.

Looking at what marionette does, I'm not sure which check we should try to port over to BiDi.

We have the requireBeforeUnload flag, which is set to true for Marionette's navigateTo, which allows to avoid using the unload timer. And that is similar to the overall suggestion here. But that doesn't really handle "cases when the target URL has already been loaded".

And from driver's navigateTo, Marionette calls isLoadEventExpected but the only case where this would return false is if current.hash === future.hash. Why only compare hashes here? If we go from "http://first-url.com#hash1" to "http://second-url.com#hash1", it seems it will confuse this logic? The naming matches your description, but the implementation seems off (maybe I'm not reading it right).

Can you clarify which check we should reuse here?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Julian Descottes [:jdescottes] from comment #1)

We have the requireBeforeUnload flag, which is set to true for Marionette's navigateTo, which allows to avoid using the unload timer. And that is similar to the overall suggestion here. But that doesn't really handle "cases when the target URL has already been loaded".

Right, we have two different scenarios here. Using such a flag to not use the unload timer will be the first step but then we also have to make sure to not necessarily wait.

And from driver's navigateTo, Marionette calls isLoadEventExpected but the only case where this would return false is if current.hash === future.hash. Why only compare hashes here? If we go from "http://first-url.com#hash1" to "http://second-url.com#hash1", it seems it will confuse this logic? The naming matches your description, but the implementation seems off (maybe I'm not reading it right).

That actually is the method that I was thinking of. It's interesting that we do not have a check for an equal URL. Maybe it's somewhere else in Marionette? Regarding the hash that's a known bug which I wanted to see fixed for quite some time (bug 1611857) but never had the opportunity for. And no-one actually reported an issue for that yet, so it has lower priority.

Can you clarify which check we should reuse here?

I wonder if there is something else that we could utilize. I wonder how the webProgress listener behaves here. Does it still fire some state event changes that we could listen for? I'm not particularly happy with isLoadEventExpected these days given that there might always be something missing.

Comment 3

[Julian Descottes [:jdescottes]]

I should experiment a bit with context.loadURI.
My guess is that if you call loadURI for the same URL but without any hash, it would actually reload the page.
But if you have a hash, it might try to do a hash navigation. I will report after testing.

Comment 4

[Alexandra Borovova [:Sasha]]

The part about not reloading when browsingContext.navigate is called with the same hash in URL moved to bug 1767924

Comment 5

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1766217 - Add expectNavigation flag to Webdriver BiDi Navigate.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/301536aeafe4
Add expectNavigation flag to Webdriver BiDi Navigate. r=webdriver-reviewers,jdescottes

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44133fa22f2a
Fix lint failure r=fix CLOSED TREE

Comment 8

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/301536aeafe4
https://hg.mozilla.org/mozilla-central/rev/44133fa22f2a