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
