1209947 - Remove nsISHEntry.mIsSubframe

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[:Gijs (he/him)]

See bug 1206879 comment 21 / 22:

(In reply to :Gijs Kruitbosch from comment #21)
> (In reply to Olli Pettay [:smaug] from comment #11)
> > Could you perhaps push patch without nsISHEntry.isSubFrame to tryserver to
> > see what all breaks?
> 
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8a1d43abb1c
> 
> "quite a bit, but not everything", it seems. Some of the identity box stuff
> looks concerning, though.
> 
> Do you want me to file a followup bug to actually do this?
> 
> remote:   https://hg.mozilla.org/integration/fx-team/rev/ba3339abba12

(In reply to Olli Pettay [:smaug] from comment #22)
> That looks quite isolated issues, or how to say. And better than I was
> afraid of.
> Please file a followup bug yes. I might try to look at it if I find time
> during next Q.
> Or feel free to take that bug ;)
> 
> 
> Thanks for doing that try-push!


(I'm a little too busy to chase this at the minute, I'm afraid, but it'd be worth doing from a code cleanup perspective)

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1209947 - Part 1: Skip more of XULBrowserWindow.onLocationChange for subframes,

With the changes in this patch stack, it will become more common for frontend
code to receive onLocationChange notifications for subframes, as we will now
correctly report events for oop subframes, and will also deliver notifications
for the first document loaded in subframes in order to update the current remote
URI on CanonicalBrowsingContext.

This change makes more of the callbacks fired by onLocationChange be guarded by
the isTopLevel check, including setting the "URL" crash annotation and updating
the macOS touchbar, which should hopefully both fix existing latent bugs, and
ensure that these issues don't occur with the more frequent onLocationChange
callbacks.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1209947 - Part 2: Fire location change events for all subframe navigations,

The check in this function was added in bug 82236 to avoid an issue in the
mozilla suite where the UI would update to a subframe's URI. This came up
previously in bug 1206879 when we observed we weren't sending OnLocationChange
events when we were expecting, which was causing issues for pushState location
changes. A workaround was added in that bug to avoid the issue for the specific
case of pushState.

This patch removes the redundant check, and reverts the workaround added in that
bug. Unfortunately, we aren't able to fully remove nsISHEntry::GetIsSubframe, as
it is now used by Browser{Parent,Child}::CanCancelContentJS.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dfab437ac709
Part 1: Skip more of XULBrowserWindow.onLocationChange for subframes, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/c7cfaeeb66be
Part 2: Fire location change events for all subframe navigations, r=smaug,Gijs

Comment 4

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/dfab437ac709
https://hg.mozilla.org/mozilla-central/rev/c7cfaeeb66be