DocShell/IRC Logs
From MozillaWiki
< DocShell
DocShell IRC Logs
This page contains some IRC logs of discussions concerning docshell development. Some of the mentioned plans have now been implemented.
DocShell/DocLoader relationship: 2004-09-22 23:07 +0200
Sep 22 23:07:35 --> Sie schreiben nun in #docloader Sep 22 23:08:22 --> bz (~bzbarsky@adsl-68-77-24-238.dsl.emhril.ameritech.net) hat #docloader betreten Sep 22 23:08:23 <bz> ok Sep 22 23:08:29 <darin> what are the major issues we need to deal with? or can we just punt since this is like not a public interface? Sep 22 23:08:39 <bz> Like I said, I may need to go soon Sep 22 23:08:43 <darin> is it not enough to just widdle away the interface over time? Sep 22 23:08:44 <bz> but I'm not sure when, so... ;) Sep 22 23:08:50 <biesi> so firstly... Sep 22 23:08:50 <darin> ok.. no problem Sep 22 23:08:55 <biesi> why can't docshell send this progress stuff? Sep 22 23:08:58 <darin> just wanted to hear the ideas that you guys have Sep 22 23:09:00 <bz> that was my question. ;) Sep 22 23:09:06 <bz> that is, biesi's question. Sep 22 23:09:19 <darin> so.. why isn't the docloader the docshell? Sep 22 23:09:27 <bz> Hesitant as I am to pile more crap on docshell Sep 22 23:09:40 <darin> well.. there's all the documentopeninfo stuff.. Sep 22 23:09:41 <bz> What we have now are two isomorphic trees that can't exist without each other Sep 22 23:09:49 <biesi> documentopeninfo is unrelated to docloader, isn't it? Sep 22 23:09:54 <darin> right right Sep 22 23:09:58 * darin got confused for a minute Sep 22 23:10:34 <darin> docshell is the nsIInterfaceRequestor, so he can be the nsIProgressEventSink and the nsISecurityEventSink Sep 22 23:10:38 <bz> Also note that nsIDocumentLoaderFactory has nothing to do with this Sep 22 23:10:45 <darin> right Sep 22 23:10:47 <bz> in spite of name. Sep 22 23:10:49 <bz> ok. Sep 22 23:11:00 <darin> is it correct for docshell to also be a nsIWebProgress? Sep 22 23:11:01 <bz> So is my claim that docshell always has a docloader correct? Sep 22 23:11:11 <darin> it would seem to be true Sep 22 23:11:18 <darin> i wonder about the webprogress parent child relationship Sep 22 23:11:24 <darin> does it correspond 1-1 to docshell tree Sep 22 23:11:50 <bz> you mean docloader? Sep 22 23:12:00 <bz> nsIWebProgress has no such relationships Sep 22 23:12:08 <darin> right Sep 22 23:12:17 <darin> well... actually Sep 22 23:12:18 <biesi> well, there's also the docloader service Sep 22 23:12:25 <biesi> which is, I think, parent of all other docloaders Sep 22 23:12:29 <bz> right. Sep 22 23:12:31 <biesi> so the trees are not quite isomorphic Sep 22 23:12:36 <darin> nsIWebProgressListener and nsIWebProgress implies a nsIWebProgress tree Sep 22 23:12:46 <bz> The question is whether anyone but URILoader uses the service Sep 22 23:13:07 <darin> the docloader service is used to attach a nsIWebProgressListener to observe all events in the system Sep 22 23:13:18 <darin> there's plenty of code that depends on that Sep 22 23:13:41 <biesi> yeah. but that's via nsIWebProgress Sep 22 23:13:47 <darin> true Sep 22 23:14:09 <darin> so assuming we aren't rocking the world too badly, then there will need to be a root nsIWebProgress impl Sep 22 23:14:10 <bz> http://lxr.mozilla.org/seamonkey/ident?i=NS_DOCUMENTLOADER_SERVICE_CONTRACTID Sep 22 23:14:15 <bz> OK, we have some people relying on that... Sep 22 23:14:25 <biesi> er, we aren't the only ones Sep 22 23:14:44 <darin> i think the ClassID is used in some places too Sep 22 23:14:51 <darin> right, extensions! Sep 22 23:15:00 <biesi> the classid? that totally sucks Sep 22 23:15:06 <biesi> arg Sep 22 23:15:12 <bz> if it's in our code, we can fix it Sep 22 23:15:18 <bz> would extensions assume nsIDocumentLoader? Sep 22 23:15:19 <darin> i know of an example in our code Sep 22 23:15:24 <bz> Or just get it as an nsIWebProgress? Sep 22 23:15:28 <darin> no.. extensions should not need nsIDocumentLoader Sep 22 23:15:32 <biesi> they would likely want webprogress Sep 22 23:15:35 <darin> right Sep 22 23:15:38 * bz _hopes_ extensions would not need the class id Sep 22 23:15:39 <biesi> since they just want to register as listener Sep 22 23:15:41 <bz> so we can have a real service Sep 22 23:15:58 <darin> let's assume that we can kill the ClassID Sep 22 23:16:01 <bz> that "root" docloaders (aka docshells) register with Sep 22 23:16:03 <bz> if desired Sep 22 23:16:15 <biesi> does it need a list of children? Sep 22 23:16:48 <bz> makes total progress stuff work, no? Sep 22 23:16:51 <bz> Otherwise, how to do it? Sep 22 23:17:08 <darin> nsIDocumentLoader::Stop trickles down to the children Sep 22 23:17:28 <biesi> Stop can likely be killed Sep 22 23:17:29 <bz> whereas webnavigation::stop does not? Sep 22 23:17:30 <biesi> use the loadgroup Sep 22 23:17:33 <darin> IsBusy also depends on the childlist chain Sep 22 23:17:38 <darin> right Sep 22 23:17:39 <biesi> same for isbusy Sep 22 23:17:49 <darin> wait.. there is one loadgroup for each webprogress right? Sep 22 23:17:51 <bz> isbusy is dead once that patch gets darin's sr Sep 22 23:17:55 <biesi> hmm, maxtotalprogress can be an issue... Sep 22 23:17:55 <darin> ok Sep 22 23:18:00 <bz> darin: there's one loadgroup per docloader, yes. Sep 22 23:18:06 <biesi> ah, yeah... Sep 22 23:18:13 <bz> darin: with the loadgroups in a tree too, which is fine. Sep 22 23:18:15 <darin> so you'd have to have a tree of loadgroups Sep 22 23:18:15 <biesi> are the child loadgroups elements of the parent loadgroup? Sep 22 23:18:22 <bz> darin: you already do, no? Sep 22 23:18:23 <darin> no, but they could be Sep 22 23:18:28 <bz> oh, they're not? Sep 22 23:18:33 <darin> i don't think so Sep 22 23:18:33 <biesi> so then a cancel() on the top could kill all loads Sep 22 23:18:37 <darin> right Sep 22 23:18:38 * bz sorta assumed that would be the simple way to implement a lot of this stuff. Sep 22 23:18:57 <darin> when i made nsILoadGroup inherit from nsIRequest, that was something in the back of my mind Sep 22 23:19:04 <darin> i don't think it ever got implemented though Sep 22 23:19:12 <darin> because we have docloader::stop walk the childlist Sep 22 23:19:23 <darin> which calls cancel on each loadgorup Sep 22 23:19:30 <bz> Right. Sep 22 23:19:33 <darin> we could get rid of that if the loadgroups were in a common loadgroup Sep 22 23:19:36 <bz> So maybe that should be the first step? Sep 22 23:19:51 <bz> put loadgroups in the parents, modify docloader to not do the child walk in stop? Sep 22 23:20:00 <bz> This is if we want to do incremental betterment here. Sep 22 23:20:09 <bz> I think this change is a good one no matter what else we do with docloader. Sep 22 23:20:12 <biesi> well, if 0th step is check my patch in ;) Sep 22 23:20:31 <biesi> yeah, that first step sounds like a good idea Sep 22 23:20:40 <bz> biesi: get darin to sr. ;) Sep 22 23:20:45 <biesi> you r'd? Sep 22 23:20:47 <bz> yep Sep 22 23:20:54 <bz> back when I first pinged you in Mozilla Sep 22 23:20:58 <biesi> ohh :) Sep 22 23:21:25 * biesi looks at the darin: superreview+ in the bug Sep 22 23:21:31 <darin> i will sr Sep 22 23:21:48 <biesi> it looks like you did already Sep 22 23:21:52 <darin> haven't had a chance to read bugmail yet today :( Sep 22 23:21:53 <biesi> but feel free to re-sr :) Sep 22 23:22:02 <darin> oh.. hmm. Sep 22 23:22:18 <biesi> (long ago, on v1 of the patch) Sep 22 23:22:22 <darin> oh, right Sep 22 23:22:25 <darin> which bug is this? Sep 22 23:22:30 <biesi> https://bugzilla.mozilla.org/show_bug.cgi?id=234257 Sep 22 23:22:37 * biesi goes to figure out if he promised any changes to bz Sep 22 23:22:43 <bz> not that I recall. Sep 22 23:22:59 <bz> So there is the question of what our goal is Sep 22 23:23:10 <bz> I think we're aiming to reduce the number of interfaces floating about Sep 22 23:23:18 <bz> and reduce the number of "global" objects involved. Sep 22 23:23:33 <bz> Esp. since that makes ref-cycle-avoidance easier. Sep 22 23:23:38 <biesi> yeah Sep 22 23:24:12 <bz> So possible impl plan right now is: Sep 22 23:24:20 <bz> 1) Move docloader methods into docshell Sep 22 23:24:48 <bz> 2) Make the "docloader service" (with same contractid?) a real service that root docshells register with and report progress to Sep 22 23:25:00 <bz> and make it implement nsIWebProgress. Sep 22 23:25:22 <bz> This service is the one thing that makes me not so happy about this... :( Sep 22 23:25:25 <biesi> I think you don't need the "register with" part Sep 22 23:25:39 <bz> oh, because no need to know about kids? Sep 22 23:25:44 <biesi> oh wait Sep 22 23:25:50 <biesi> there's still the maxtotalprogress issue Sep 22 23:28:02 <biesi> currently, nsdocloader asks the children by calling an nsDocLoaderImpl fcn on them... Sep 22 23:28:11 <bz> yeah Sep 22 23:28:34 <bz> But all that does is add up mMaxSelfProgress over them all Sep 22 23:29:14 <biesi> but if you distribute that over two classes (docshell + the service) that's not so easy Sep 22 23:29:36 <bz> well, can't you keep a running total of mMaxSelfProgress's over all kids? Sep 22 23:29:44 <bz> Based on past OnProgress() notifications? Sep 22 23:29:54 * bz agrees that this is the one hard part Sep 22 23:30:06 <biesi> hmm Sep 22 23:30:32 <biesi> that's an interesting idea... Sep 22 23:35:29 <biesi> so each docshell keeps an (weak?) nsIWebProgressListener* mParent pointer, and the toplevel docshells have one to the service Sep 22 23:36:28 <biesi> hmm, do chrome docshells currently report progress? would it improve Tp/Txul not to? Sep 22 23:36:57 <bz> docshells could just not have this pointer at all. Sep 22 23:37:05 * darin catches up with the backlog Sep 22 23:37:06 <bz> and QI mParent as needed, and if mParent is null getService Sep 22 23:37:52 <biesi> I liked the thought of avoiding that checks, but sure, that works Sep 22 23:38:02 <biesi> but be careful, progress is reported a lot Sep 22 23:38:07 <biesi> during pageload Sep 22 23:38:15 <bz> hmmm... Sep 22 23:38:44 <bz> one sec Sep 22 23:38:46 <bz> reading code Sep 22 23:38:46 <darin> so who's going to be cutting these patches? ;-) Sep 22 23:39:26 <bz> http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.h#432 Sep 22 23:39:33 <bz> 432 nsIDocShellTreeItem * mParent; // Weak Reference Sep 22 23:39:48 <bz> There's no reason that can't be a nsDocShell* Sep 22 23:39:55 <bz> and then if mParent is non-null, we just call it. Sep 22 23:40:08 <bz> if it's null, we have a class static (cleaned up on shutdown) pointing to the service Sep 22 23:40:12 <bz> fast, and all Sep 22 23:40:21 <bz> darin: well, that's a good question. Sep 22 23:40:27 <bz> darin: biesi or I, sounds like... ;) Sep 22 23:40:38 <bz> darin: assuming we come to an agreement about general direction Sep 22 23:40:55 <biesi> bz, that'd work Sep 22 23:41:46 <biesi> so docshell would be an iwebprogresslistener an iprogresseventsink? Sep 22 23:42:51 <biesi> er, _and_ an iprogresseventsink Sep 22 23:44:12 <darin> biesi: so, why does your patch need that call to SetLoadGroup? Sep 22 23:44:21 * darin doesn't see a SetLoadGroup being removed Sep 22 23:44:47 <biesi> the removed setloadgroup was part of NS_NewChannel, looks like Sep 22 23:44:52 <darin> oh.. ok Sep 22 23:47:42 <darin> sr marked in the bug Sep 22 23:47:53 * biesi notices something... Sep 22 23:48:03 <biesi> is it possible that it's not actually the docloader that's set as notificationcallback? Sep 22 23:52:59 <darin> it is the docshell, no? Sep 22 23:53:15 <darin> line 4552 of nsDocShell.cpp Sep 22 23:53:38 <darin> oh, hmm.. that's interesting Sep 22 23:53:42 <darin> maybe a bug even Sep 22 23:53:53 <darin> in the other place we set an InterfaceRequestorProxy as the notification callbacks Sep 22 23:54:05 <darin> oh, but that's for the loadgroup Sep 22 23:55:11 <biesi> http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#5485 Sep 22 23:55:24 <biesi> is that a good idea? Sep 22 23:55:57 <biesi> it means each caller of nsIURILoader::Open has to forward progresseventsink requests to the docloader Sep 22 23:56:06 <biesi> assuming he wants stuff to behave correctly Sep 22 23:56:50 <darin> right Sep 22 23:56:53 <darin> it's not ideal really Sep 22 23:57:02 <darin> oh wait Sep 22 23:57:31 <darin> nsDocShell::GetInterface returns the docloader for nsIProgressEventSink, no? Sep 22 23:57:42 <biesi> yes, docshell does Sep 22 23:58:07 <biesi> what about the others? Sep 22 23:58:17 <darin> are there other callers of nsIURILoader::open ? Sep 22 23:58:27 <biesi> yeah Sep 22 23:58:35 <biesi> xremote, some mailnews stuff Sep 22 23:58:56 <darin> oh yeah.. true Sep 22 23:59:00 * darin lxr's and finds lots Sep 23 00:00:09 <darin> hmm.. actually not that many Sep 23 00:00:18 <darin> most references to nsIURILoader are for registering content listeners Sep 23 00:00:26 <darin> but yeah.. xremote and nsURLFetcher (what is that for?) Sep 23 00:01:01 * bz has to go Sep 23 00:01:05 * biesi has no idea what urlfetcher does Sep 23 00:01:07 <bz> email me the log, please? Sep 23 00:01:16 <biesi> the rest of it, or all? Sep 23 00:07:25 <biesi> darin, so what do you think about setting the docloader as notificationcallbacks? Sep 23 00:08:05 <darin> setting the docloader to be the notification callbacks for the channel? Sep 23 00:08:09 <biesi> yes Sep 23 00:08:14 <darin> (because it is already the notification callbacks for the loadgroup) Sep 23 00:08:26 <biesi> oh Sep 23 00:08:36 <darin> but we use the notification callbacks to get to the docshell Sep 23 00:08:36 <biesi> hmm, then that may not be needed Sep 23 00:08:39 <darin> see nsCookiePermission.cpp Sep 23 00:08:53 <darin> can we just make the docshell skip nsIProgressEventSink ? ;-) Sep 23 00:08:55 <darin> etc. Sep 23 00:08:59 <biesi> that sounds like a good idea Sep 23 00:09:12 <biesi> because the code it has for that is somewhat sucky :) Sep 23 00:09:18 <darin> so long as all of the channels do the right thing ;-) Sep 23 00:09:52 <biesi> they better! :) Sep 23 00:09:55 <darin> we should probably write a common routine (stick it in nsNetUtil.h) for querying notification callbacks on a channel Sep 23 00:10:10 <biesi> yeah Sep 23 00:10:17 <darin> they don't Sep 23 00:10:23 * darin points to nsFileChannel.cpp Sep 23 00:10:27 <biesi> eww Sep 23 00:10:33 <darin> so.. we'd need to patch that ;-) Sep 23 00:10:41 <biesi> then there's externalprotocolchannel (sp) which I filed a bug about... Sep 23 00:10:46 * darin blames himself for not being strict about that Sep 23 00:12:38 <biesi> hmm, ftpchannel too, if I'm interpreting grep correctly... Sep 23 00:13:47 <biesi> !!! not even HTTP gets it right Sep 23 00:13:53 <biesi> nsHttpChannel::SetNotificationCallbacks(nsIInterfaceRequestor *callbacks) Sep 23 00:13:53 <biesi> { Sep 23 00:13:53 <biesi> mCallbacks = callbacks; Sep 23 00:13:53 <biesi> Sep 23 00:13:53 <biesi> mHttpEventSink = do_GetInterface(mCallbacks); Sep 23 00:13:53 <biesi> mProgressSink = do_GetInterface(mCallbacks); Sep 23 00:14:23 <darin> yikes Sep 23 00:14:40 <darin> http gets it right only for nsIPrompt, nsIAuthPrompt, nsIAuthPromptProvider, etc. Sep 23 00:14:48 <darin> ugh.. i suck :( Sep 23 00:15:00 * darin files a bu Sep 23 00:15:01 <darin> bug Sep 23 00:16:07 <biesi> yeah, http is the "best" of the channels at this ;) Sep 23 00:16:22 <biesi> the rest totally suck Sep 23 00:17:47 <darin> https://bugzilla.mozilla.org/show_bug.cgi?id=261083 Sep 23 00:18:16 <biesi> good Sep 23 00:19:03 * biesi goes to reassign his review comments bug to him, because it looks like it won't get fixed otherwise... Sep 23 00:20:15 <darin> to me? Sep 23 00:20:26 <biesi> no, to me Sep 23 00:20:31 <biesi> unless you want it :) Sep 23 00:20:58 <darin> you can have it :) Sep 23 00:21:22 <biesi> thought so :) Sep 23 00:23:58 <biesi> hmm, extensions/datetime and finger get it wrong too Sep 23 00:24:06 * biesi wonders about mailnews Sep 23 00:25:22 <biesi> well, hardly surprising, it does too Sep 23 00:25:35 <darin> ;-) Sep 23 00:26:25 * darin starts cutting a patch Sep 23 00:32:00 * biesi goes to file bugs to remove progresseventsink from docshell::Getinterface, and to put loadgroups into parent loadgroups Sep 23 00:33:02 <darin> ok Sep 23 00:54:44 <darin> there are some issues with making this change to always query the loadgroup Sep 23 00:55:02 <darin> whereas today we would query the notification callbacks when they are set, we cannot do that now... b/c the loadgroup may not be set Sep 23 00:55:10 <darin> we need to defer all of that until AsyncOpen / Open gets called Sep 23 00:55:55 <biesi> hm? Sep 23 00:56:00 <biesi> oh, true :/ Sep 23 00:56:36 <darin> yeah.. not a problem really Sep 23 00:56:58 <darin> i'll just add code to the top of AsyncOpen (and in some cases Open) that configures things like mProgressSink, etc. Sep 23 00:57:05 <biesi> ah, yeah, true Sep 23 00:57:52 * darin notices that he's practically eliminating all calls to do_GetInterface from necko Sep 23 01:04:33 <darin> man ftp is a mess Sep 23 01:05:06 * darin decides to get rid of all the useless mutex locking in ftp as well Sep 23 01:23:20 <-- bz hat sich getrennt (Ping timeout: 378 seconds)
Concrete plans: 2004-10-18 18:10 -0600
<bz> biesi: I had a scary thought just now.. * biesi checks whether this container is indeed the docshell <bz> biesi: What we _really_ want is for every docshell to be a docloader, right? <bz> biesi: but not all docloaders will be docshells? <biesi> bz, only one docloader won't be a docshell, right? <bz> biesi: right <bz> biesi: the point is, perhaps docshell should just inherit from docloader... <bz> biesi: and you can get back and forth with getinterface as needed <bz> biesi: that is, CallGetI to get from docloader to docshell <bz> biesi: nothing needed in the other direction <biesi> bz, like class nsDocShell : public nsDocLoaderImpl? <bz> biesi yes <bz> biesi: with fixes to QI appropriately and all that <biesi> that's an interesting idea <biesi> dso-wise it works <bz> biesi: right <biesi> bz, that may be the best solution