View source for DocShell/IRC Logs

= 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