DocShell/IRC Logs
< DocShell
Jump to navigation
Jump to search
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