471997 - Add command line argument to start directly into Private Browsing mode

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090103 Shiretoko/3.1b3pre ID:20090103020545

Yesterday I had a talk with Aleksej on IRC and he pointed out that it would be good to have a command line argument for starting directly into the Private Browsing mode. Accidentally I miss-read bug 460346 and said it wouldn't be necessary. But bug 460346 is about staying in PB mode all the time. There is no way to leave it.

So it would really make sense to have a command line argument for an one time start of Firefox. With the current implementation you cannot be sure what will opened when you start the browser. Means which sites are open currently. If you are travelling and you have some ppl around you watching your screen, you will probably not show your last session.

No idea, if something will be possible for 3.1 but asking for wanted1.9.1 anyway.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Or/and an option in the Start menu -> All Programs.

Comment 2

[(no longer active)]

I think we can get this for 3.1.  I think the use case is valid, and it will allow people to even set up launchers with both -private and -no-remote so that they can have private browsing sessions side by side with their normal browsing session (albeit not using the same profile).

I'll take this.

Comment 3

[William Jon McCann]

Attachment: patch

How about something like this?

Comment 4

[(no longer active)]

Comment on attachment 362852 [details] [diff] [review]
patch

>+    if (this._starting) {
>+      // Interpret setting during startup as enabling autostart
>+      this._autoStart = true;
>+      return;
>+    }
>+

It looks very good, except for this part.  We don't want the -private command line to trigger "auto-start", because auto-start is really about an "always-on" private browsing mode, not one which starts automatically at startup (for example, in auto-start mode we don't allow leaving the private browsing mode, whereas we would want to allow this for the -private command line handker.)  See bug 471998 which is filed to eliminate this naming confusion (which is my fault).

So, you need to keep the |_starting| logic as you have implemented in this patch, and check that in addition to _autoStart where appropriate.

Comment 5

[(no longer active)]

William, feel free to assign this bug to yourself if you're going to update your patch.  :-)

Comment 6

[William Jon McCann]

Thanks for the review.  So, actually for my use case I did want to have exactly the same behavior as always-on mode (for a kiosk) without having to use a preference.  Is there another way I can achieve that?

Maybe we could have two options?  -private and -private-always-on ?

I'll be happy to update the patch.

Comment 7

[(no longer active)]

Hmm, well if that's also desired, I suggest having a single -private command line option which can take an optional param, and let that param be "alwayson".  Something like:

firefox -private
starts in private browsing mode

firefox -private alwayson
starts in "always-on" private browsing mode

But first, would you mind explaining why this use case would be useful to you?  What's wrong with setting a preference?

Comment 8

[William Jon McCann]

A preference is not ideal for this for the same reasons that are mentioned in the first comment of this bug.  I would like to have the operation of the browser determined by command line options so that separate options can be configured in the menus for standard firefox and kiosk firefox (I'm also using another extension that adds a -kiosk command line option).

I suppose that I can just make my other extension veto leaving private browsing mode and that would work just as well as -private=alwayson.

I'll update the patch.  Thanks.

Comment 9

[(no longer active)]

Like comment 0 mentions, "bug 460346 is about staying in PB mode all the time", but any command line option affects only *that* run of the browser, so I think mixing these ideas is a bit illogical.  At least it's not appropriate for inclusion in the core product.

(In reply to comment #8)
> I suppose that I can just make my other extension veto leaving private browsing
> mode and that would work just as well as -private=alwayson.

That would work, but it's also kind of scary, unless your extension advertises the private browsing mode as "the kiosk mode", otherwise the user may get frustrated if she tries to leave the private browsing mode and your extension just blocks that request.  Not that it's relevant to this bug, though.  :-)

Comment 10

[William Jon McCann]

Attachment: updated patch

How's this?

Comment 11

[(no longer active)]

Comment on attachment 362926 [details] [diff] [review]
updated patch

Looks good!

You also need a review from a browser peer...

Comment 12

[Mike Connor [:mconnor]]

Comment on attachment 362926 [details] [diff] [review]
updated patch

There's no need for a separate component, you can just add this around http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#492

Comment 13

[William Jon McCann]

Attachment: updated patch

Thanks for the review Mike.  Makes sense.  I think I was just following the example used in the shell component.  Here's the updated patch.  Is this what you mean?

Comment 14

[Johnathan Nightingale [:johnath]]

Comment on attachment 362966 [details] [diff] [review]
updated patch

Thanks for the updated patch, William. I'll request review on it so that it shows up in mconnor's queue

Comment 15

[Mike Connor [:mconnor]]

Comment on attachment 362966 [details] [diff] [review]
updated patch

this._autoStart is only used in one function now, we can kill it and just use a local var now.  With that fixed, r=me.

Comment 16

[Mike Connor [:mconnor]]

Or, even better, just reuse this._autoStarted

Comment 17

[(no longer active)]

William, can you also write a unit test for this?  I think it should be fairly trivial to test automatically...

Comment 18

[William Jon McCann]

Attachment: updated patch

Reuses autoStarted as suggested.

Comment 19

[(no longer active)]

The patch looks fine, except for the missing test.  Please let me know if you're going to write it, otherwise I can look into it myself.

Comment 20

[William Jon McCann]

Sure, do you have any references for how that is done?

Comment 21

[(no longer active)]

Sorry I missed this comment.

I'm not sure if you're familiar with xpcshell-based unit tests or not, but here is the introductory docs: <https://developer.mozilla.org/en/Writing_xpcshell-based_unit_tests>

For this test, you need to implement a nsICommandLine object in JavaScript, which only has one command line parameter: "-private".  Then, you need to get a reference to the browser command handler XPCOM object, and pass your nsICommandLine object to this object's handle function. After that, you only need to make sure the PB service's privateBrowsingEnabled property is true.

Comment 22

[Mike Connor [:mconnor]]

This will probably miss 3.1 unless the test gets written real soon.

Comment 23

[William Jon McCann]

Ok, I'd really like to get this in.  I'm working on it now.  When is the deadline?

Comment 24

[William Jon McCann]

Attachment: possible unit test

Sorry, I'm new to this.  Would something like this work?

Comment 25

[(no longer active)]

(In reply to comment #24)
> Created an attachment (id=363844) [details]
> possible unit test
> 
> Sorry, I'm new to this.  Would something like this work?

Yes, exactly what I had in mind.  I have not tried running the test though, but if you have tested it, then r=me on the latest version of the patch with this test.  Please submit the whole thing as a patch and ask for mconnor's review as well so that we can land this ASAP.

Comment 26

[(no longer active)]

One point though, QueryInterface should support nsISupports as well :-)

Comment 27

[William Jon McCann]

Ok, thanks.  Just wanted to make sure I was on the right track.  I'll add it to the patch and give it a test in the morning.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 363844 [details]
possible unit test

> * The Initial Developer of the Original Code is
> * Ehsan Akhgari.

Not really, or? :)

> * Portions created by the Initial Developer are Copyright (C) 2008

2009

>function testprivatecl() {}
>testprivatecl.prototype = {
>    _arguments: [ "-private" ],
>    get length TPC_get_length() {
>        return this._arguments.length;
>    },
[...]

Please obey the code guidelines. Otherwise you will not get a r+ I believe.

See http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/test/unit/test_privatebrowsing.js

Comment 29

[William Jon McCann]

(In reply to comment #28)
> 
> Please obey the code guidelines. Otherwise you will not get a r+ I believe.
> 
> See
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/test/unit/test_privatebrowsing.js

Sorry.  Do you have a reference to these code guidelines?  Or would you mind being a bit more specific about what I should change?

Thanks.

Comment 30

[William Jon McCann]

Ok, nevermind, google found this one:
https://developer.mozilla.org/En/JavaScript_style_guide

Comment 31

[William Jon McCann]

Attachment: patch w/unit test

Here's the patch but I'm having trouble testing it because even though firefox built fine make check gives me:
+++ Failed to get ScriptSecurityManager service, running without principals+++ Failed to get backstage pass from rtsvc: 80040111

Comment 32

[William Jon McCann]

Attachment: updated patch

Ok, sorry for the iterations.  I've finally gotten an environment where i can run the unit tests.  (side note that make clean seems to remove more files than it should)

Here's an updated patch that passes all the tests except the one it adds.  It fails because:
TypeError: Components.classes['@mozilla.org/browswer/clh;1'] is undefined

Comment 33

[William Jon McCann]

Attachment: this time with unit test

Forgot to hg add unit test in last patch.

Comment 34

[(no longer active)]

Attachment: Patch + test

As it turns out, the correct way to run the command line handlers is to go through the category manager route.  I modified the test to correct the implementation of nsICommandLine, use catman to handle the command line, make sure autoStarted is not set with command line, and also cleaned up the test to better match our coding styles.

This is ready for mconnor's final review.

Thanks for your great work on this patch, William, and I hope you don't mind me stepping on your toes here.  :-)

Comment 35

[William Jon McCann]

Ehsan, thanks so much for finishing this off :-)

Comment 36

[William Jon McCann]

Is there still hope that this will get into 3.1?

Comment 37

[(no longer active)]

mconnor: ping?

Comment 38

[Mike Connor [:mconnor]]

Okay, so, with the changes in bug 462041 I don't know if this patch is sufficient, depending on desired use case.  Also, the final-ui-startup piece is wrong, so this probably behaves in such a way that it works like just enabling PB mode, but not in a way that it looks like autostarted.

Comment 39

[William Jon McCann]

So the question is should -private:
 a) simply enable PB mode at startup
 b) enable always-on PB mode (very very confusingly called autostart)

I think I'd prefer b) (and seems like Mike does too in comment #38) however earlier comments from Ehsan seemed to prefer a).

I tend to think of command line options as per-instance preferences (ignoring remote).  Changing something in the preferences is global and except for special cases I doubt you'd want to have all browser instances using always-on PB mode.  I think the use case for having a -private option is more for setting up a menu item or launcher that is labelled (or known to be) a private browsing mode (and therefore always-on).  I have a harder time imagining a reason why I'd want to start in Private mode but then leave it later.

Either way, I think I'll be able to work around it to accomplish what I need.  I just need to know what you'd prefer to have here.  Or perhaps I'm misunderstanding and there is no disagreement.

Comment 40

[Mike Connor [:mconnor]]

I definitely want b) here, I am pretty sure that's the right expression of this switch, and is more likely to be useful.  Ehsan, thoughts?

Comment 41

[(no longer active)]

Attachment: New patch

After some thought, I changed my mind, and I also vote for (b).

Here is a new patch which implements that idea, plus the automated tests needed for it.

Comment 42

[William Jon McCann]

Ping.

Comment 43

[Jim Mathies [:jimm]]

Requesting blocking status for 3.6, we could really use this as a win7 Jump List option.

Comment 44

[Jim Mathies [:jimm]]

Ehsan, will this also work if the browser is already open, or is this more like the profile manager where if you launch with the -private option and the browser is already open, private browsing isn't started?

Comment 45

[(no longer active)]

(In reply to comment #43)
> Requesting blocking status for 3.6, we could really use this as a win7 Jump
> List option.

I don't know much about the Jump List option.  Jim, would you care to elaborate how this might be useful in that context?

(In reply to comment #44)
> Ehsan, will this also work if the browser is already open, or is this more like
> the profile manager where if you launch with the -private option and the
> browser is already open, private browsing isn't started?

With the current patch, it behaves like the latter (it won't do anything if the browser is already open).  If the former behavior is desired for you, we can get it on a follow-up bug...

Comment 46

[Jim Mathies [:jimm]]

(In reply to comment #45)
> (In reply to comment #43)
> > Requesting blocking status for 3.6, we could really use this as a win7 Jump
> > List option.
> 
> I don't know much about the Jump List option.  Jim, would you care to elaborate
> how this might be useful in that context?

It's a new win7 feature where down on the task bar, if you right click on the icon of the app, there are shortcuts to app specific functionality. Google has some images of these indexed - 

http://images.google.com/images?q=Jump+List

for ie8, 

http://www.redmondpie.com/wp-content/uploads/2009/04/windows77.png

The simplest way to put one of these together is through an app command based on a command line parameter.

For parity with ie, and generally for better user experience, I'd like to add a "private browsing" jump list item for 3.6. (That work is taking place in bug 473045). We've got command line options for creating new tabs and for new windows, but nothing for private browsing. The thing is if we add it, it has to always work - meaning Fx needs to restart when it's already running just like it does for the menu option.  

I was planning on adding the profile manager as well, but it suffers from the limitation that if we're already running, the manager doesn't come up. So I had to make that a power user option that you enable through prefs. For private browsing, it'd be great if we could go ahead and get it working fully so we can make the jump list item a default.

> With the current patch, it behaves like the latter (it won't do anything if the
> browser is already open).  If the former behavior is desired for you, we can
> get it on a follow-up bug...

If that's possible that would be great! Otherwise I can disable displaying it by default for now and file another bug to get it working in another release.

Comment 47

[(no longer active)]

(In reply to comment #46)
> (In reply to comment #45)
> > (In reply to comment #43)
> > > Requesting blocking status for 3.6, we could really use this as a win7 Jump
> > > List option.
> > 
> > I don't know much about the Jump List option.  Jim, would you care to elaborate
> > how this might be useful in that context?
> 
> It's a new win7 feature where down on the task bar, if you right click on the
> icon of the app, there are shortcuts to app specific functionality. Google has
> some images of these indexed - 

Thanks for the info!

> For parity with ie, and generally for better user experience, I'd like to add a
> "private browsing" jump list item for 3.6. (That work is taking place in bug
> 473045). We've got command line options for creating new tabs and for new
> windows, but nothing for private browsing. The thing is if we add it, it has to
> always work - meaning Fx needs to restart when it's already running just like
> it does for the menu option.  

Yes, I think this is a good idea.

> I was planning on adding the profile manager as well, but it suffers from the
> limitation that if we're already running, the manager doesn't come up. So I had
> to make that a power user option that you enable through prefs. For private
> browsing, it'd be great if we could go ahead and get it working fully so we can
> make the jump list item a default.

For profile manager, I don't think we can switch profiles at runtime, so I don't think we can support this when Firefox is running.

Can't Jump List commands be disabled/hidden when Firefox is already running?

> > With the current patch, it behaves like the latter (it won't do anything if the
> > browser is already open).  If the former behavior is desired for you, we can
> > get it on a follow-up bug...
> 
> If that's possible that would be great! Otherwise I can disable displaying it
> by default for now and file another bug to get it working in another release.

I don't think it should be that much complicated.  I've filed bug 506955.  I'll try to take that bug soon!

Is there a deadline for getting this done?

Comment 48

[(no longer active)]

William, thanks for your efforts!

http://hg.mozilla.org/mozilla-central/rev/d907a89b9b87

Comment 49

[Jim Jeffery not reading bug-mail 1/2/11]

What is the command line to start up PB ?  Firefox.exe -private starts the browser, but nothing shows I'm in PB mode, and all my tabs are present, shouldn't I be seeing the PB welcome page ?  

Closing the browser after starting as above I am not prompted to save,quit,cancel, it just closes, leading me to believe I'm actually in PB mode...but I should not be seeing my previous tabs..

Tested using the latest hourly:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20090816 Minefield/3.7a1pre Firefox/3.7 (.NET CLR 3.5.30729) ID:20090816114218

cset: http://hg.mozilla.org/mozilla-central/rev/d907a89b9b87

Comment 50

[Nochum Sossonko [:Natch]]

(In reply to comment #49)
Best way to test would be starting with -private then navigating to about:privatebrowsing, it should tell you if you're in or out of pb mode. This is verified fixed for me.

You can also check the Tools menu and see if the "Stop Private Browsing" menuitem is grayed out (disabled).

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a1pre) Gecko/20090816 Minefield/3.7a1pre

Comment 51

[William Jon McCann]

The point was to *not* show the PB welcome page but just to load the home page or the page requested on the command line directly.  The launcher itself that uses a -private argument should advertise the private mode.  One important use case for this option is to run firefox in a kiosk mode and go directly to the start page.

Comment 52

[Jim Jeffery not reading bug-mail 1/2/11]

Well I can say it does not go to the home page but opens all my previous tabs.  This is not right IMO. Should I just file a bug blocking this one because its not working as you state ?  No home page is shown....

Comment 53

[(no longer active)]

(In reply to comment #52)
> Well I can say it does not go to the home page but opens all my previous tabs. 
> This is not right IMO. Should I just file a bug blocking this one because its
> not working as you state ?  No home page is shown....

Please file a separate bug and we will discuss this further there.

Comment 54

[Jim Jeffery not reading bug-mail 1/2/11]

Filed: https://bugzilla.mozilla.org/show_bug.cgi?id=510881

Comment 55

[(no longer active)]

Should be documented in <https://developer.mozilla.org/en/Command_Line_Options>.

Comment 56

[Mike Connor [:mconnor]]

Probably also useful for SUMO.  But please hold off docs until Bug 506955 is resolved

Comment 57

[Mike Beltzner [:beltzner, not reading bugmail]]

(no longer needs approval as it's a blocker, so please land on 1.9.2 ASAP)

Comment 58

[Jim Jeffery not reading bug-mail 1/2/11]

Your really going to push this into the release build and not fix the bug in comment #54 ?  This has serious security implications in my mind, yet bug 410881 has been minused ?? It needs the 2nd review and pushed ASAP in conjunction with bug.. or I'm afraid this is going to bite Mozilla once 3.6 comes out in the form of 'bad press'.

Comment 59

[(no longer active)]

Landed on 1.9.2 as: <http://hg.mozilla.org/releases/mozilla-1.9.2/rev/809b137abeda>.

Comment 60

[[not reading bugmail]]

Filed bug 516737 because I got the same results testing the latest trunk nightly as comment 49.  Then eventually I got the results from comment 50.  The arguments don't seem to take effect right away or are affected by running -profilemanager first.