234257 - nsIDocumentLoader/nsDocLoader misc cleanup (uriloader/base)

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

o) nsIURILoader::GetLoadGroupForContext has no reason to exist
it is totally stupid that users of uriloader have to get the loadgroup from
uriloader and set it on the channel, only to pass that to the uriloader again.

o) nsIDocumentLoader has some functions that are really internal functions and
shouldn't be on an interface

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

oh yeah, nsDocLoaderImpl::GetContentViewerContainer was unused so I removed it too

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 3

[Boris Zbarsky [:bzbarsky]]

<biesi> bz: bah, can you comment in the bug on which I requested review from me to
+change CalculateMaxProgress as well?

Comment 4

[Darin Fisher]

Comment on attachment 141376 [details] [diff] [review]
patch


>+  if (mMaxSelfProgress >= 0 && newMaxTotal >= 0)
>+    return newMaxTotal + mMaxSelfProgress;
>+  
>+  return -1;
> }

combine these into a single return statement?  there are a couple
comptrs on the stack ;)


>Index: uriloader/base/nsIDocumentLoader.idl


> [scriptable, uuid(f43ba260-0737-11d2-beb9-00805f8a66dc)]
> interface nsIDocumentLoader : nsISupports
> {
>   void stop();
>-  boolean isBusy();

rev the IID for this interface?  better safe than sorry! :)


>Index: uriloader/base/nsIURILoader.idl

rev this guy's IID too?


>Index: docshell/base/nsDocShell.cpp

>+        nsCOMPtr<nsIInterfaceRequestor> requestor(do_QueryInterface(mLoadCookie));
>+        if (requestor)
>+          return requestor->GetInterface(aIID, aSink);
>+
>+        return NS_ERROR_FAILURE;

coalesce return statements?


> NS_IMETHODIMP
>-nsDocShell::LoadStream(nsIInputStream *aStream, nsIURI * aURI,
>-                       const nsACString &aContentType,
>-                       const nsACString &aContentCharset,
>-                       nsIDocShellLoadInfo * aLoadInfo)
>-{

i seem to recall an embedder that was using this... ah, photon ;-)

my point is that this might still be a useful method.


sr=darin

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

hm, I thought I had changed those IIDs already...

will make those changes, I'll upload a new patch after bz reviews (or before he
reviews, if you want)

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

well, I changed my mind :) and attached the patch with the changes now.

Comment 7

[Boris Zbarsky [:bzbarsky]]

biesi, I don't know when I'll be able to get to this... I have some non-mozilla
stuff I need to deal with.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Hrm... So we no longer call Destroy() on the docloader when the docshell dies? 
And more to the point we don't null out its container?  That's no good... it'll
end up with a stale ref to the docshell, no?

Also, why is it ok to not give the channel a loadgroup at creation time?  I
suppose the code in OpenURI handles that?

Finally, you left some of the SetupLoadCookie callers sitting around but not
others.  I would say we should just decide what the deal with the load cookie
thing is and whether we need this funky opaque api to do it.  Then make changes
from there as needed.

Finally, why remove the isBusy() method from the docloader interface?

Comment 9

[Boris Zbarsky [:bzbarsky]]

In short, I feel that we should figure out why docloader exists, what exactly it
should do, whether it needs to be scriptable, whether it needs to be public,
whether embedders use it, and whether we need it.  If nothing else, the way it's
used as both a service and an instance is just broken.....

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

bz:
(In reply to comment #8)
> Hrm... So we no longer call Destroy() on the docloader when the docshell dies?

We still do. See nsWebShell.cpp Line 189
 
> And more to the point we don't null out its container?  That's no good... it'll
> end up with a stale ref to the docshell, no?

webshell line 188

> Also, why is it ok to not give the channel a loadgroup at creation time? 

I don't think uriloader callers should be aware that a loadgroup even exists...

> I suppose the code in OpenURI handles that?

Correct, following the comment I added to nsIURILoader::OpenURI.


> Finally, you left some of the SetupLoadCookie callers sitting around but not
> others. 

I only removed the one that was in the function I was removing... The point was
not to remove a SetupLoadCookie caller, but to remove this function.

> I would say we should just decide what the deal with the load cookie
> thing is and whether we need this funky opaque api to do it.  Then make changes
> from there as needed.

well, I wanted to remove the GetLoadGroup call here. can we deal with
loadcookies separately?

> Finally, why remove the isBusy() method from the docloader interface?

why leave it there? that it's unused (outside of docloader) seems to  be a good
indication that it's unneeded.

(In reply to comment #9)
> In short, I feel that we should figure out why docloader exists, what exactly it
> should do, whether it needs to be scriptable, whether it needs to be public,
> whether embedders use it, and whether we need it.  If nothing else, the way it's
> used as both a service and an instance is just broken.....

hm, this bug wasn't meant as a "think about the entire docloader" forum.
although we can make it one if you want to :)

Comment 11

[Boris Zbarsky [:bzbarsky]]

I just feel that making interface changes is silly unless we first decide what
the interface is for...

Comment 12

[Darin Fisher]

(In reply to comment #11)
> I just feel that making interface changes is silly unless we first decide what
> the interface is for...

nsIDocumentLoader seems like a crufty old interface that had a bunch of stuff
dumped on it.  Some of the functions aren't even used or can be deCOMtaminated.
 I think we should view biesi's patch as a good step along the path to cleaning
up the code in uriloader/base.  I don't think we need one final uber patch here.
 Let's do it incrementally.  (i.e., this patch is holding up other work I want
to do in uriloader/base, so can we please land it? ;-)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 141403 [details] [diff] [review]
patch v2

r=bzbarsky (and nice catch about the useless mDocLoader member!), but I would
like to have this conversation about docloader's purpose in life and its
relationship to docshell/webshell/uriloader.  Could we set a time and channel
to do this on irc?

Comment 14

[Darin Fisher]

Comment on attachment 141403 [details] [diff] [review]
patch v2

sr=darin

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch merged to trunk

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in docshell/base/nsDocShell.cpp;
/cvsroot/mozilla/docshell/base/nsDocShell.cpp,v  <--  nsDocShell.cpp
new revision: 1.627; previous revision: 1.626
done
Checking in docshell/base/nsDocShell.h;
/cvsroot/mozilla/docshell/base/nsDocShell.h,v  <--  nsDocShell.h
new revision: 1.163; previous revision: 1.162
done
Checking in mailnews/compose/src/nsURLFetcher.cpp;
/cvsroot/mozilla/mailnews/compose/src/nsURLFetcher.cpp,v  <--  nsURLFetcher.cpp
new revision: 1.71; previous revision: 1.70
done
Checking in uriloader/base/nsDocLoader.cpp;
/cvsroot/mozilla/uriloader/base/nsDocLoader.cpp,v  <--  nsDocLoader.cpp
new revision: 3.277; previous revision: 3.276
done
Checking in uriloader/base/nsDocLoader.h;
/cvsroot/mozilla/uriloader/base/nsDocLoader.h,v  <--  nsDocLoader.h
new revision: 1.27; previous revision: 1.26
done
Checking in uriloader/base/nsIDocumentLoader.idl;
/cvsroot/mozilla/uriloader/base/nsIDocumentLoader.idl,v  <--  nsIDocumentLoader.idl
new revision: 1.10; previous revision: 1.9
done
Checking in uriloader/base/nsIURILoader.idl;
/cvsroot/mozilla/uriloader/base/nsIURILoader.idl,v  <--  nsIURILoader.idl
new revision: 1.31; previous revision: 1.30
done
Checking in uriloader/base/nsURILoader.cpp;
/cvsroot/mozilla/uriloader/base/nsURILoader.cpp,v  <--  nsURILoader.cpp
new revision: 1.130; previous revision: 1.129
done

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: partial backout of nsDocShell.cpp change

I had to check this patch in, which backs out the first hunk of the
nsDocShell.cpp change. it broke embedding on at least linux/gtk2 and mac, but
likely cross-platform, because nsWebBrowser broke.

I suspect mLoadCookie isn't set yet at this point, and the old code used
GetDocumentLoader to force creation of one. unfortunate that I have to do this,
but oh well :(