nsViewSourceChannel needs to implement nsICachingChannel

RESOLVED FIXED

Status

()

RESOLVED FIXED
18 years ago
18 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

To be able to load view source from cache, view source channels need to
implement nsICachingChannel.
Created attachment 48905 [details] [diff] [review]
proposed patch -- add nsIHttpChannel and nsICachingChannel so the docshell code will do the right things
Chak?  review?
Blocks: 55583
Keywords: patch, review

Comment 3

18 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?!]
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

18 years ago
r=chak on the first patch 

Comment 6

18 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?
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.
Created attachment 50152 [details] [diff] [review]
A patch that's a little more clever with its QI implementation
ccing darin for his comments

Comment 10

18 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.
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

18 years ago
well then, r/sr=darin
checked in.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.