nsViewSourceChannel needs to implement nsICachingChannel




17 years ago
17 years ago


(Reporter: bzbarsky, Assigned: bzbarsky)



Firefox Tracking Flags

(Not tracked)



(2 attachments)



17 years ago
To be able to load view source from cache, view source channels need to
implement nsICachingChannel.

Comment 1

17 years ago
Created attachment 48905 [details] [diff] [review]
proposed patch -- add nsIHttpChannel and nsICachingChannel so the docshell code will do the right things

Comment 2

17 years ago
Chak?  review?
Blocks: 55583
Keywords: patch, review

Comment 3

17 years ago
Are these changes going to fix bug 55583 or will there be something more to do?
[BTW: if yes, is nsICachingChannel something new?!]

Comment 4

17 years ago
There will be more.  Here's the full list for bug 55583:

1)  Check this in
2)  Check in interface changes to allow passing postdata and cachkeys to loadUri
    (or something equivalent)
3)  Check in JS changes to pass these in when doing viewsource

nsDocShell.cpp does the following at the moment:

open a channel;
if (channel can QI to nsIHttpChannel) {
  rewind postdata if we have it; // needed to properly repost if we repost
  if (channel can QI to nsICachingChannel && cache key passed in) {
    use cachekey to load document (prompting for repost if we have post data
    and can't load from cache);

So making nsIViewSourceChannel load from cache involves being able to get into
the middle part of that loop.

Comment 5

17 years ago
r=chak on the first patch 

Comment 6

17 years ago
Comment on attachment 48905 [details] [diff] [review]
proposed patch -- add nsIHttpChannel and nsICachingChannel so the docshell code will do the right things

looks like it would be nice if nsIHttpChannel and nsIViewSourceChannel 
didn't inherit from nsIChannel (sigh)

also, are you sure it is correct to have the view source channel appear to 
implement nsIHttpChannel and nsICachingChannel when mHttpChannel and/or 
mCachingChannel are NULL?

Comment 7

17 years ago
Well... I've checked all the callers and they do error-checking properly.  But
on third thought that does make me a little queasy.  Another approach coming up.

Comment 8

17 years ago
Created attachment 50152 [details] [diff] [review]
A patch that's a little more clever with its QI implementation

Comment 9

17 years ago
ccing darin for his comments

Comment 10

17 years ago
yeah, that seems better... i wonder if it is "OK" to hard code knowledge of the
inner workings of the QI macros.  i think it is worth it, considering the
benefit of not having to completely write your own QI impl, but perhaps you
should seek out a second opinion.

Comment 11

17 years ago
I talked with Shaver on IRC before posting that patch.  He was OK with it.  In
fact he vastly preferred it to the first attempt.

Comment 12

17 years ago
well then, r/sr=darin

Comment 13

17 years ago
checked in.
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.