1545170 - Confusing name: NavigationDelegate.LoadRequest.isRedirect

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P2)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

NavigationDelegate.LoadRequest.isRedirect: I was expecting this to be a boolean indicating whether the request is a redirect (e.g. HTTP 302) and I couldn't understand how some code in Focus could work the way it worked. Then I noticed the javadoc:

/**
 * True if and only if the request was triggered by user interaction.
 */
public final boolean isRedirect;

With that I could understand the code in Focus. So I assume isRedirect is actually doing what the javadoc says and not about redirects at all?

Comment 1

[Matt Brubeck (:mbrubeck)]

Bug 1487542 removed the isUserTriggered flag and added isRedirect, but I accidentally left that old comment in place.

If you request a URL "http://foo" and this returns a redirect to "http://bar", then onLoadRequest will be called twice:

1. First, with uri = "http://foo" and isRedirect = false
2. Second with uri = "http://bar" and isRedirect = true

The source of the confusion is probably that isRedirect is set to true for requests that are the result of a redirect rather than ones that cause a redirect. I'll fix the javadoc to explain this.

Comment 2

[Matt Brubeck (:mbrubeck)]

Focus should have replaced isUserTriggered with !isRedirect. It looks like they used the opposite value isRedirect. If the behavior ended up correct anyways, maybe it's because the code doesn't care about the order of the requests, but only that one of the pair is set to true...

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: Bug 1545170 - Fix outdated javadoc comment for LoadRequest.isRedirect. r?snorp

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9145204dc6e5
Fix outdated javadoc comment for LoadRequest.isRedirect. r=geckoview-reviewers,esawin

Comment 5

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/9145204dc6e5

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: Bug 1545170 - Fix outdated javadoc comment for LoadRequest.isRedirect. r?snorp