1147749 - View source doesn't go through service worker interception

Status: RESOLVED FIXED

(Toolkit :: View Source, defect)

Description

[(no longer active)]

This means that if you try to view source a page that has been served through a service worker, you'll be viewing the source the latest version from the network.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: viewSource.patch

bz, I don't know if what I'm doing with the docshell makes sense or not.
It works, but maybe it's all an horrible hack.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: viewSource2.patch

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8660126 [details] [diff] [review]
viewSource2.patch

>+    * Returns true if this channel is the main for the current document.

"is the main one"?

>+    * This can happen also if the flags do not have the LOAD_DOCUMENT_URI bit.

Maybe "even if" instead of "also if".

>+    attribute boolean mainDocumentChannel;

I think this should be isMainDocumentChannel.

Also, the interaction with the loadFlags setter should be documented, so that people know they need to call this _after_ setting the loadFlags?

Alternately, we could just have the getter return true if either the setter was called with true or the load flags have LOAD_DOCUMENT_URI...  Either way, need to document the exact behavior.

Probably good to get a necko peer to review the necko bits.

>+    nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(mChannel);

Just use mHttpChannel, if it's non-null.

>+nsViewSourceChannel::GetMainDocumentChannel(bool* aValue)
>+nsViewSourceChannel::SetMainDocumentChannel(bool aValue)

Why not forward to the underlying HTTP channel?  Might be better that way.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: viewSource2.patch

Comment 5

[Jason Duell]

Comment on attachment 8661573 [details] [diff] [review]
viewSource2.patch

Review of attachment 8661573 [details] [diff] [review]:
-----------------------------------------------------------------

This patch really makes me want to gag :(  It's not clear to me why we're not allowed to put more than one channel with LOAD_DOCUMENT_URI into a loadGroup (there's no code in LoadGroup.cpp that forbids it), but I'm assuming there's some reason (addon compat?) why you didn't follow the (to my mind) cleaner approach of handing this in the docshell (as in your first patch).

But my track record of quibbling with :bz is poor, and I'm guessing there's a good reason for the change in strategy.  So I can live with this.  But I'd like the alternative API semantics that bz suggested (see comment below).  +r to next patch with no re-review needed if that's ok.

For now I'm giving this version a f- just because it makes me feel cleaner :)

::: netwerk/protocol/http/nsIHttpChannel.idl
@@ +254,5 @@
> +    * This can happen even if the flags do not have the LOAD_DOCUMENT_URI bit.
> +    * If you want to set this value, remember to do it after the loading flags
> +    * because this value is overwritten but those flags.
> +    */
> +    attribute boolean isMainDocumentChannel;

> Alternately, we could just have the getter return true if either the setter 
> was called with true or the load flags have LOAD_DOCUMENT_URI

I like this approach better.  So let's do

/** Indicates whether channel should be treated as the main one for the 
 *  current document.  If manually set to true, will always remain true.  Otherwise, 
 *  will be true iff LOAD_DOCUMENT_URI is set in the channel's loadflags.  

Rename mIsMainDocumentChannel -> mForceMainDocumentChannel and have the Getter function return true if it's set or if LOAD_DOCUMENT is set.

This is all assuming that no one will ever want to "force" not-main-document status on a channel.  It doesn't look like that will ever be the case.

Comment 6

[Boris Zbarsky [:bzbarsky]]

More that one LOAD_DOCUMENT_URI channel would probably make the logic in docloader very very confused.

Comment 7

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b082130e530

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9b082130e530