826063 - Provide scriptable alternative to NS_UsePrivateBrowsing

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Nils Maier [:nmaier]]

Add-ons and maybe other javascript code may want to find out what channel is private for various reasons from an nsIChannel ref alone.
In my use case, one of my add-ons (DownThemAll!) retrieves and caches some information such as uris and/or post data from channels via http-request observers (in js code module). The add-on may later retrieves that information to construct downloads.

Right now it is not possible to fully re-implement NS_UsePrivateBrowsing in javascript because nsIPrivateBrowsingChannel::isPrivateModeOverriden is [noscript]. Other than this, it wouldn't be actually pretty user friendly having to implement this stuff yourself.

I could imagine amending the interface like so:
interface nsIPrivateBrowsingChannel : nsISupports
{
...
  readonly attribute boolean isChannelPrivate; // corresponding to isWindowPrivate
}
with an implementation that simply calls NS_UsePrivateBrowsing(this);
Bonus points for an implementation of PrivateBrowsingUtils.isChannelPrivate(aChannel)

Would this be a viable approach?

Comment 1

[(no longer active)]

Given the number of different things implementing nsIPrivateBrowsingChannel, I'd rather just make isPrivateModeOverriden scriptable if you really need that, unless Josh or Jason disagree.

Comment 2

[Nils Maier [:nmaier]]

(In reply to :Ehsan Akhgari from comment #1)
> Given the number of different things implementing nsIPrivateBrowsingChannel,

The thing can be actually implemented just once in PrivateBrowsingChannel.h, just like isPrivateModeOverriden
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/PrivateBrowsingChannel.h#50

> I'd rather just make isPrivateModeOverriden scriptable if you really need
> that, unless Josh or Jason disagree.

Fine with me, too, as long as it enables me to implement NS_UsePrivateBrowsing in js.

Comment 3

[(no longer active)]

(In reply to comment #2)
> > I'd rather just make isPrivateModeOverriden scriptable if you really need
> > that, unless Josh or Jason disagree.
> 
> Fine with me, too, as long as it enables me to implement NS_UsePrivateBrowsing
> in js.

We can't call NS_UsePrivateBrowsing there since we won't have an nsIChannel* to pass to it...

Comment 4

[Nils Maier [:nmaier]]

Attachment: Part 1: Add and implement nsIPrivateBrowsingChannel::isChannelPrivate

Actually we do have a channel, namely this.
This patch implements ::isChannelPrivate. The cast is analog to what the rest of the class methods already do anyway.

Successful test build on my mac. Try forthcoming: https://tbpl.mozilla.org/?tree=Try&rev=2cbdaa76ef5c

Comment 5

[Nils Maier [:nmaier]]

Attachment: Part 2: Add tests for nsIPrivateBrowsingChannel::isChannelPrivate

Comment 6

[Nils Maier [:nmaier]]

Attachment: Part 3: Add PrivateBrowsingUtils.isChannelPrivate()

Add isChannelPrivate() convenience function to PrivateBrowsingUtils.
That function doesn't do much, but I figured it would be nice to have around for add-on authors?

Comment 7

[Nils Maier [:nmaier]]

Attachment: Part 4: Add PrivateBrowsingUtils.isChannelPrivate() tests

Comment 8

[Nils Maier [:nmaier]]

Attachment: Part 2: Add tests for nsIPrivateBrowsingChannel::isChannelPrivate

Alrighty, I was a little over-zealous and had all those map-into-jarchannels/omnijar URIs in the test, and that does not work obviously, nor should it.

Green of of all: https://tbpl.mozilla.org/?tree=Try&rev=89e1a8de0888

Comment 9

[(no longer active)]

Comment on attachment 697454 [details] [diff] [review]
Part 1: Add and implement nsIPrivateBrowsingChannel::isChannelPrivate

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

Ah, I forgot about the Channel template argument!

Comment 10

[(no longer active)]

Comment on attachment 697458 [details] [diff] [review]
Part 3: Add PrivateBrowsingUtils.isChannelPrivate()

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

Let's not do this....  I'd like to keep PrivateBrowsingUtils simple.  isChannelPrivate() is something that hopefully very few add-ons will ever need to use.

Comment 11

[(no longer active)]

Thanks for working on this, Nils!  :-)

Comment 12

[Nils Maier [:nmaier]]

Does this need sr?

Comment 13

[(no longer active)]

(In reply to comment #12)
> Does this need sr?

No.

Comment 14

[Nils Maier [:nmaier]]

checkin-needed then for Part 1,2 :D
Green try: https://tbpl.mozilla.org/?tree=Try&rev=89e1a8de0888

Comment 15

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b6bc165770f
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc816bcb876c

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4b6bc165770f
https://hg.mozilla.org/mozilla-central/rev/fc816bcb876c

Comment 17

[Jason Duell]

Comment on attachment 697454 [details] [diff] [review]
Part 1: Add and implement nsIPrivateBrowsingChannel::isChannelPrivate

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

::: netwerk/base/src/PrivateBrowsingChannel.h
@@ +49,5 @@
>  
> +  NS_IMETHOD GetIsChannelPrivate(bool *aResult)
> +  {
> +      NS_ENSURE_ARG_POINTER(aResult);
> +      *aResult = NS_UsePrivateBrowsing(static_cast<Channel*>(this));

Late to the party here... FWIW I'd be happy if we just removed NS_UsePrivateBrowsing and put its body in this method (and maybe provided a simple C++-only accessor method that avoided xpcom goop and just returned the bool).  I'm also too busy to bother with it any time soon, but if Nils or someone else has time, go for it.

Comment 18

[(no longer active)]

(In reply to comment #17)
> Comment on attachment 697454 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=697454
> Part 1: Add and implement nsIPrivateBrowsingChannel::isChannelPrivate
> 
> Review of attachment 697454 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/src/PrivateBrowsingChannel.h
> @@ +49,5 @@
> >  
> > +  NS_IMETHOD GetIsChannelPrivate(bool *aResult)
> > +  {
> > +      NS_ENSURE_ARG_POINTER(aResult);
> > +      *aResult = NS_UsePrivateBrowsing(static_cast<Channel*>(this));
> 
> Late to the party here... FWIW I'd be happy if we just removed
> NS_UsePrivateBrowsing and put its body in this method (and maybe provided a
> simple C++-only accessor method that avoided xpcom goop and just returned the
> bool).  I'm also too busy to bother with it any time soon, but if Nils or
> someone else has time, go for it.

Hmm, that would make for uglier code, because the callers would all look more XPCOMy than necessary. :(