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@mozilla.org
https://mail.mozilla.org/listinfo/mobile-firefox-dev
_______________________________________________
mobile-firefox-dev mailing list
mobile-firefox-dev@mozilla.org
https://mail.mozilla.org/listinfo/mobile-firefox-dev
