Closed Bug 1545170 Opened 1 year ago Closed 1 year ago

Confusing name: NavigationDelegate.LoadRequest.isRedirect

Categories

(GeckoView :: General, enhancement, P2)

Unspecified
Android
enhancement

Tracking

(firefox66 wontfix, firefox67 wontfix, firefox68 fixed)

RESOLVED FIXED
mozilla68
Tracking Status
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: sebastian, Assigned: mbrubeck)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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?

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.

Assignee: nobody → 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...

Type: defect → enhancement
OS: All → Android
Priority: -- → P2
Pushed by mbrubeck@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9145204dc6e5
Fix outdated javadoc comment for LoadRequest.isRedirect. r=geckoview-reviewers,esawin
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Attachment #9062619 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.