Closed Bug 826063 Opened 7 years ago Closed 7 years ago

Provide scriptable alternative to NS_UsePrivateBrowsing

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: nmaier, Assigned: nmaier)

References

Details

Attachments

(2 files, 3 obsolete files)

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?
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.
Component: Private Browsing → Networking
Product: Firefox → Core
Version: unspecified → Trunk
(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.
(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...
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
Attachment #697454 - Flags: feedback?(ehsan)
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?
Attachment #697458 - Flags: feedback?(ehsan)
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
Assignee: nobody → MaierMan
Attachment #697456 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #697541 - Flags: review?(ehsan)
Attachment #697459 - Flags: review?(ehsan)
Attachment #697458 - Flags: feedback?(ehsan) → review?(ehsan)
Attachment #697454 - Flags: feedback?(ehsan) → review?(ehsan)
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!
Attachment #697454 - Flags: review?(ehsan) → review+
Attachment #697541 - Flags: review?(ehsan) → review+
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.
Attachment #697458 - Flags: review?(ehsan) → review-
Attachment #697459 - Flags: review?(ehsan) → review-
Thanks for working on this, Nils!  :-)
Does this need sr?
Attachment #697458 - Attachment is obsolete: true
Attachment #697459 - Attachment is obsolete: true
(In reply to comment #12)
> Does this need sr?

No.
checkin-needed then for Part 1,2 :D
Green try: https://tbpl.mozilla.org/?tree=Try&rev=89e1a8de0888
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4b6bc165770f
https://hg.mozilla.org/mozilla-central/rev/fc816bcb876c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
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.
(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. :(
You need to log in before you can comment on or make changes to this bug.