1499895 - Add triggering page URI to onLoadRequest

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P3)

Description

[Eugen Sawin [:esawin]]

It can be useful to know the origin URI of a load request.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1499895-1.0-Add-triggering-URI-to-onLoadRequest..patch

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.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

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...).

Comment 3

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1499895-1.1-Add-triggering-URI-to-onLoadRequest..patch

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.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

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, ... })

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 6

[Eugen Sawin [:esawin]]

(In reply to James Willcox (:snorp) ([e-mail]) 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.

Comment 7

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1499895-1.2-Add-triggering-URI-to-onLoadRequest..patch

Addressed comments.

Comment 8

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1499895-2.0-Update-onLoadRequest-tests.-r-jchen.patch

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/45e3efb5e076
https://hg.mozilla.org/mozilla-central/rev/b9614de52765