GeckoView Breaking Change Notice: NavigationDelegate.onLoadRequest will be called for iFrames.

Ben Bucksch ben.bucksch at beonex.com
Wed May 13 22:34:40 UTC 2020


If you allow a little feedback:
load listeners in XUL work similarly. However, in the past, that has 
many times led to bugs in our code. We tested with a normal document, 
and it worked fine, and then much much later there was an iframe and it 
triggered the listener, and our code didn't expect that, and it broke in 
subtle but bad ways. This was despite the fact that this behavior was 
documented. We just didn't expect it and didn't think of it.

Now that we know that, of course we check for that. However, in almost 
all of our code, we always care about the top level load listeners only. 
So, we always have to add that check for the top level. That's a little 
annoying, because it clutters the code.

To avoid that, both to surprise new developers - and testers, because 
nobody thinks of testing this specific case -, and to avoid code clutter 
in the majority case, here's a proposal:

Instead of modifying the existing listener, could you just add a new 
listener with a new name, specifically for this case? And the existing 
one keeps its current behavior?

Because it my experience, you almost always care only about the top 
level. Be it URL bar or security indicators, or progress listeners, or a 
load finished listener. Very rarely do we care about iframes.

On 13.05.20 23:53, Dylan Roeh wrote:
> In order to fix a bug with handling unknown protocols in iFrames[0], we're
> going to be calling NavigationDelegate.onLoadRequest for iFrame navigations in
> addition to the top-level navigations for which we currently call it. To make
> this usable, the LoadRequest class will be getting an additional field,
> isTopLevel, which indicates whether the request is top-level or not. To
> maintain your present behavior once this change has landed, you can simply
> check if a request is non-top-level and allow it if so at the beginning of your
> onLoadRequest implementation, eg:
>
> @Override
> GeckoResult<AllowOrDeny> onLoadRequest(GeckoSession session, LoadRequest req) {
>      if (!req.isTopLevel) {
>          return GeckoResult.fromValue(AllowOrDeny.ALLOW);
>      }
>      // Handle top-level load requests here.
> }
>
> This patch should be landing in nightly (78) in the next few days; I will send
> a follow-up message to this mailing list once it does. Please let me know if
> you have any questions/concerns by responding to this email or asking on Matrix
> at https://chat.mozilla.org/#/room/#geckoview:mozilla.org
>
> [0]: https://bugzilla.mozilla.org/show_bug.cgi?id=1596825
>
> Regards,
> Dylan Roeh
> _______________________________________________
> mobile-firefox-dev mailing list
> mobile-firefox-dev at mozilla.org
> https://mail.mozilla.org/listinfo/mobile-firefox-dev
>



More information about the mobile-firefox-dev mailing list