Add triggering page URI to onLoadRequest

RESOLVED FIXED in Firefox 64

Status

enhancement
P3
normal
RESOLVED FIXED
7 months ago
5 months ago

People

(Reporter: esawin, Assigned: esawin)

Tracking

51 Branch
mozilla64
All
Android

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

7 months ago
It can be useful to know the origin URI of a load request.
Assignee

Comment 1

7 months ago
Add triggeringUri (or maybe better originUri?) to onLoadRequest.

There is more information we could expose from the triggering principal, if there are valid use cases for it.
Attachment #9018084 - Flags: review?(snorp)
Attachment #9018084 - Flags: review?(nchen)
Comment on attachment 9018084 [details] [diff] [review]
0001-Bug-1499895-1.0-Add-triggering-URI-to-onLoadRequest..patch

Review of attachment 9018084 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but if we're gonna break the API let's try to do it (almost) for the last time? r- just so we can see what that API will look like.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +2584,5 @@
>           *         not the load was handled. If unhandled, Gecko will continue the
>           *         load as normal. If handled (true value), Gecko will abandon the load.
>           *         A null return value is interpreted as false (unhandled).
>           */
>          @Nullable GeckoResult<Boolean> onLoadRequest(@NonNull GeckoSession session,

The number of arguments here is starting to get out of hand. What do you think about putting all of this into a LoadRequest class (or a better name? LoadInfo? I dunno...).
Attachment #9018084 - Flags: review?(snorp) → review-
Assignee

Comment 3

7 months ago
Add LoadRequest class for request details.

I've also switched flags to a boolean (isUserTriggered), assuming that we are not going to expose many flags, the boolean is more discoverable and convenient for the user.
Attachment #9018084 - Attachment is obsolete: true
Attachment #9018084 - Flags: review?(nchen)
Attachment #9018266 - Flags: review?(snorp)
Attachment #9018266 - Flags: review?(nchen)
Comment on attachment 9018266 [details] [diff] [review]
0001-Bug-1499895-1.1-Add-triggering-URI-to-onLoadRequest..patch

Review of attachment 9018266 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, I do think this improves the API a bit.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +2570,5 @@
> +            /**
> +             * The URI to be loaded.
> +             */
> +            public final @NonNull String uri;
> +            /**

I'd prefer a blank line here but NBD

@@ +2581,5 @@
> +             * One of {@link #TARGET_WINDOW_NONE TARGET_WINDOW_*}.
> +             */
> +            public final @TargetWindow int target;
> +            /**
> +             * True iff the request was triggered by user interaction.

Probably better to just use the full "if and only if" here, not everyone studied CS or Math :)

@@ +2583,5 @@
> +            public final @TargetWindow int target;
> +            /**
> +             * True iff the request was triggered by user interaction.
> +             */
> +            public final boolean isUserTriggered;

Are we sure we can't imagine any other values here? There is a balance between API ergonomics and future-proofing, but just want to make sure we're in the right place on this instance. I guess we could always add more booleans :)

::: mobile/android/modules/geckoview/LoadURIDelegate.jsm
@@ +18,5 @@
>  const LoadURIDelegate = {
>    // Delegate URI loading to the app.
>    // Return whether the loading has been handled.
> +  load: function(aWindow, aEventDispatcher, aUri, aWhere, aFlags,
> +                 aTriggeringPrincipal) {

This function is getting to be unwieldy with arguments too. Not sure it would help much, but we could pack all of this into an object, and then you could destructure it with ({ window, eventDispatcher, ... })
Attachment #9018266 - Flags: review?(snorp) → review+
Comment on attachment 9018266 [details] [diff] [review]
0001-Bug-1499895-1.1-Add-triggering-URI-to-onLoadRequest..patch

Review of attachment 9018266 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +2545,2 @@
>           */
> +        public class LoadRequest {

`static`

::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm
@@ +189,5 @@
>                                         name=${aName}`;
>  
>      if (LoadURIDelegate.load(this.window, this.eventDispatcher,
> +                             aUri, aWhere, aFlags,
> +                             /* triggeringPrincipal */ null)) {

The triggering principal is in `aParams.triggeringPrincipal`. We need to also pass in the triggering principal for `openURIInFrame`.

::: mobile/android/modules/geckoview/LoadURIDelegate.jsm
@@ +30,5 @@
>        type: "GeckoView:OnLoadRequest",
>        uri: aUri ? aUri.displaySpec : "",
>        where: aWhere,
> +      flags: aFlags,
> +      originUri: originUri && originUri.displaySpec

"Origin URI" has a specific meaning when it comes to web content, so `aTriggeringPrincipal.originNoSuffix` is probably best here.
Attachment #9018266 - Flags: review?(nchen) → review+
Assignee

Comment 6

7 months ago
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #4)
> Probably better to just use the full "if and only if" here, not everyone
> studied CS or Math :)

I expect there to be two groups of people, those who will understand it and those who will think it's a typo but still infer the correct meaning :)
I'll change it to if and only if to avoid confusion.
 
> Are we sure we can't imagine any other values here? There is a balance
> between API ergonomics and future-proofing, but just want to make sure we're
> in the right place on this instance. I guess we could always add more
> booleans :)

It was originally a boolean but boolean method arguments aren't very descriptive and extensible.
The boolean class member however seems more convenient to me than the flag.

Since we haven't had any requests in the past few months to add more flag values, I predict that the set size will not exceed 5 in the near future, which is a reasonable number to have as dedicated booleans.
I don't have a strong opinion on this, though.

> This function is getting to be unwieldy with arguments too. Not sure it
> would help much, but we could pack all of this into an object, and then you
> could destructure it with ({ window, eventDispatcher, ... })

Yeah, but unless we introduce an actual class for this, I'm not seeing much value in packing stuff together in JS.
Assignee

Comment 7

7 months ago
Addressed comments.
Attachment #9018266 - Attachment is obsolete: true
Attachment #9018420 - Flags: review?(nchen)
Comment on attachment 9018420 [details] [diff] [review]
0001-Bug-1499895-1.2-Add-triggering-URI-to-onLoadRequest..patch

Review of attachment 9018420 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/modules/geckoview/LoadURIDelegate.jsm
@@ +25,5 @@
>      }
>  
> +    const triggerUri = aTriggeringPrincipal &&
> +                       !aTriggeringPrincipal.isNullPrincipal &&
> +                       aTriggeringPrincipal.URI;

`(aTriggeringPrincipal.isNullPrincipal ? null : aTriggeringPrincipal.URI)`

Otherwise `triggerUri` will be `false` (which is unexpected) when it's a null principal.

@@ +32,5 @@
>        type: "GeckoView:OnLoadRequest",
>        uri: aUri ? aUri.displaySpec : "",
>        where: aWhere,
> +      flags: aFlags,
> +      triggerUri: triggerUri && triggerUri.displaySpec

Trailing comma
Attachment #9018420 - Flags: review?(nchen) → review+
Attachment #9018421 - Flags: review?(nchen) → review+

Comment 10

7 months ago
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45e3efb5e076
[1.3] Add triggering URI to onLoadRequest. r=snorp,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9614de52765
[2.1] Update onLoadRequest tests. r=jchen

Comment 11

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/45e3efb5e076
https://hg.mozilla.org/mozilla-central/rev/b9614de52765
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64

Updated

5 months ago
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 64 → mozilla64
You need to log in before you can comment on or make changes to this bug.