Loading…
Fix for #1072: Listen to DOMTitleChanged #1084
3 participants
Hi @AlexVallat. I know working to port to Fennec is quite tricky, so I hope this isn't a bothersome intervention...
I'm curious, does pageshow not fire when navigating to a cached page? It looks like one can indeed listen for pageshow on a Browser, and Mozilla's documentation claims it's just like load except that it fires even when the page is loaded from cache:
pageshoweventThis event works the same as the
loadevent, except that it fires every time the page is loaded (whereas theloadevent doesn’t fire [...] when the page is loaded from cache).The first time the page loads, the
pageshowevent fires just after the firing of theloadevent. Thepageshowevent uses a boolean property calledpersistedthat is set tofalseon the initial load [... and] set totruewhen the page is cached.
Here's where I was reading about this.
Like I said, you probably have already researched this more than I can imagine — I'm just curious to hear your thoughts about that.
Thanks for the work on Fennec!
Not at all, any ideas are worth a try! pageshow does fire when navigating to a cached page, but the reason we don't use it is that it fires after the page has loaded, not before. It is therefore unsuitable for setting the page URL as all the requests that need to be blocked will have already been made.
DOMTitleChanged is the first event that fires, before almost everything else. Scripts loaded in <head> may still be fetched before it fires, which is why I wanted an httpObserver-based page URL setting instead - this catches the motherjones.com amazon aws script not being blocked issue.
@AlexVallat Have you tried using nsIWebProgressListener instead (which is available for <browser>, which is present in Fennec too)? That would give the exact same results as addTabsProgressListener.
Looking at it, the only problem I see, that it is not e10s friendly, but neither is DOMTitleChanged.
Anyway, you forgot cleanup for the DOMTitleChanged event.
Anyway, you forgot cleanup for the
DOMTitleChangedevent.
Good point, I've put that in now.
I'll give nsIWebProgressListener a try in another stream, and let you know if it works out.
Right, I've created a webProgressListener branch which uses one of those instead of a tabsProgressListener, DOMTitleChanged event listener, and the main-frame check in httpObserver.
The addProgressListener method is not available at the tabContainer/tabBrowser level for Fennec, so one must be added to each tab.
As far as I can tell from the documentation available, it is a natively content process thing but which is always shimmed to be available to the chrome process too. Therefore I think it would be preferrable, e10s-wise, to put the listener in the frame script and send a message.
Adding the listener to the framescript means it will be in every tab naturally, so that works out nicely too.
It will work equally well for Firefox as well as Fennec, so feel free to try it out. From my testing it appears to work just fine, but as to whether this is a better solution than the other, I'm really not sure. On the one hand, having only one place rather than three for the navigation change reporting is nice. On the other, going through messaging and loading the listener into every tab is not so nice, but then again is probably closer to the 'right way to do it' for e10s...
TabsProgressListener works well for desktop Firefox, I wouldn't change that because of Fennec, so these workarounds should be done only for Fennec.
I would have thought having a single code path would be better than maintaining two, personally - far more likely to discover and propagate fixes if it's shared. But, if separate is the way you prefer it then fair enough.
If you think that the webProgressListener solution as I've implemented would actually be worth replacing the existing fennec implementation for, let me know and I can change it to be fennec-specific.
a single code path would be better than maintaining two
I would agree, but TabsProgressListener requires only one listener, and it runs in background page, so no messaging, and it will run right after main_frame.
I'm not sure that the async message will always reach the background page before any other sync message (for other net requests from shouldLoad), meaning some request could have a different page context when matching filters against them.
I'm sure I remember reading somewhere that message order was guaranteed, but I can't find that documentation now. We could switch it to a sync message, but I thought I'd at least try it as async before making it sync if it didn't need to be.
I agree that it's not clear cut whether this is a better method or not, but for me the argument is the same for both desktop and fennec - if, on balance, it's not a better method (due to messaging and number of listeners, for example) then there's no need to implement it for fennec either.
I just tried sending an async and a sync message, and they indeed arrived in order, which is unexpected, but because of this it probably wouldn't be a problem.
Alright, then let's revisit this issue when it will actually cause a real world problem.
When a page is loaded without any http request being made the onLocationChange from httpObserver is never triggered. Adding back in the DOMTitleChanged event handler will ensure onLocationChange is also called in those circumstances.
On the down side, it means that most pages will have onLocationChange called twice. This appears to be harmless, but if you know it to be an issue then further work would be required.