Closed
Bug 471997
Opened 16 years ago
Closed 15 years ago
Add command line argument to start directly into Private Browsing mode
Categories
(Firefox :: Private Browsing, defect, P3)
Tracking
()
RESOLVED
FIXED
Firefox 3.6b1
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta1-fixed |
status1.9.1 | --- | wontfix |
People
(Reporter: whimboo, Assigned: william.jon.mccann)
References
Details
(Keywords: dev-doc-complete, user-doc-needed)
Attachments
(1 file, 9 obsolete files)
(deleted),
patch
|
mconnor
:
review+
|
Details | Diff | Splinter Review |
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.
Flags: wanted-firefox3.1?
Comment 1•16 years ago
|
||
Or/and an option in the Start menu -> All Programs.
Comment 2•16 years ago
|
||
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.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Updated•16 years ago
|
Whiteboard: [PB-P2]
Assignee | ||
Comment 3•16 years ago
|
||
How about something like this?
Comment 4•16 years ago
|
||
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•16 years ago
|
||
William, feel free to assign this bug to yourself if you're going to update your patch. :-)
Assignee | ||
Comment 6•16 years ago
|
||
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•16 years ago
|
||
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?
Assignee: ehsan.akhgari → william.jon.mccann
Whiteboard: [PB-P2]
Assignee | ||
Comment 8•16 years ago
|
||
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•16 years ago
|
||
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. :-)
Updated•16 years ago
|
Attachment #362926 -
Flags: review+
Comment 11•16 years ago
|
||
Comment on attachment 362926 [details] [diff] [review]
updated patch
Looks good!
You also need a review from a browser peer...
Updated•16 years ago
|
Attachment #362926 -
Flags: review?(mconnor)
Updated•16 years ago
|
Whiteboard: [has patch][needs review mconnor]
Comment 12•16 years ago
|
||
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
Attachment #362926 -
Flags: review?(mconnor) → review-
Assignee | ||
Comment 13•16 years ago
|
||
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?
Attachment #362926 -
Attachment is obsolete: true
Updated•16 years ago
|
Attachment #362966 -
Flags: review?(mconnor)
Comment 14•16 years ago
|
||
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•16 years ago
|
||
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.
Attachment #362966 -
Flags: review?(mconnor) → review+
Comment 16•16 years ago
|
||
Or, even better, just reuse this._autoStarted
Comment 17•16 years ago
|
||
William, can you also write a unit test for this? I think it should be fairly trivial to test automatically...
Whiteboard: [has patch][needs review mconnor] → [has new patch]
Assignee | ||
Comment 18•16 years ago
|
||
Reuses autoStarted as suggested.
Attachment #362966 -
Attachment is obsolete: true
Comment 19•16 years ago
|
||
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.
Assignee | ||
Comment 20•16 years ago
|
||
Sure, do you have any references for how that is done?
Comment 21•16 years ago
|
||
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•16 years ago
|
||
This will probably miss 3.1 unless the test gets written real soon.
Flags: wanted-firefox3.1? → wanted-firefox3.1-
Priority: -- → P3
Whiteboard: [has new patch] → [has patch][has reviews][needs test]
Target Milestone: --- → Firefox 3.2a1
Assignee | ||
Comment 23•16 years ago
|
||
Ok, I'd really like to get this in. I'm working on it now. When is the deadline?
Assignee | ||
Comment 24•16 years ago
|
||
Sorry, I'm new to this. Would something like this work?
Comment 25•16 years ago
|
||
(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•16 years ago
|
||
One point though, QueryInterface should support nsISupports as well :-)
Assignee | ||
Comment 27•16 years ago
|
||
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.
Reporter | ||
Comment 28•16 years ago
|
||
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
Assignee | ||
Comment 29•16 years ago
|
||
(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.
Assignee | ||
Comment 30•16 years ago
|
||
Ok, nevermind, google found this one:
https://developer.mozilla.org/En/JavaScript_style_guide
Assignee | ||
Comment 31•16 years ago
|
||
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
Attachment #363203 -
Attachment is obsolete: true
Attachment #363844 -
Attachment is obsolete: true
Assignee | ||
Comment 32•16 years ago
|
||
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
Assignee | ||
Updated•16 years ago
|
Attachment #363933 -
Attachment is obsolete: true
Assignee | ||
Comment 33•16 years ago
|
||
Forgot to hg add unit test in last patch.
Attachment #364205 -
Attachment is obsolete: true
Comment 34•16 years ago
|
||
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. :-)
Attachment #364218 -
Attachment is obsolete: true
Attachment #364385 -
Flags: review?(mconnor)
Updated•16 years ago
|
Whiteboard: [has patch][has reviews][needs test] → [has patch][needs review mconnor]
Assignee | ||
Comment 35•16 years ago
|
||
Ehsan, thanks so much for finishing this off :-)
Assignee | ||
Comment 36•16 years ago
|
||
Is there still hope that this will get into 3.1?
Comment 37•16 years ago
|
||
mconnor: ping?
Comment 38•16 years ago
|
||
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.
Assignee | ||
Comment 39•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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.
Attachment #364385 -
Attachment is obsolete: true
Attachment #376613 -
Flags: review?(mconnor)
Attachment #364385 -
Flags: review?(mconnor)
Assignee | ||
Comment 42•15 years ago
|
||
Ping.
Comment 43•15 years ago
|
||
Requesting blocking status for 3.6, we could really use this as a win7 Jump List option.
Blocks: 473045
Flags: wanted-firefox3.6?
Updated•15 years ago
|
Flags: blocking-firefox3.6?
Comment 44•15 years ago
|
||
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•15 years ago
|
||
(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...
Whiteboard: [has patch][needs review mconnor] → [has patch][needs r=mconnor]
Target Milestone: Firefox 3.6a1 → ---
Comment 46•15 years ago
|
||
(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•15 years ago
|
||
(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?
Updated•15 years ago
|
Attachment #376613 -
Flags: review?(mconnor) → review+
Comment 48•15 years ago
|
||
William, thanks for your efforts!
http://hg.mozilla.org/mozilla-central/rev/d907a89b9b87
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch][needs r=mconnor]
Target Milestone: --- → Firefox 3.7
Updated•15 years ago
|
Attachment #376613 -
Flags: approval1.9.2?
Comment 49•15 years ago
|
||
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•15 years ago
|
||
(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
Assignee | ||
Comment 51•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
(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•15 years ago
|
||
Comment 55•15 years ago
|
||
Should be documented in <https://developer.mozilla.org/en/Command_Line_Options>.
Keywords: dev-doc-needed
Comment 56•15 years ago
|
||
Probably also useful for SUMO. But please hold off docs until Bug 506955 is resolved
Keywords: user-doc-needed
Updated•15 years ago
|
status1.9.1:
--- → wontfix
Updated•15 years ago
|
Whiteboard: [doc-waiting-landing]
Updated•15 years ago
|
Flags: wanted-firefox3.6?
Flags: wanted-firefox3.6+
Flags: blocking-firefox3.6?
Flags: blocking-firefox3.6+
Comment 57•15 years ago
|
||
(no longer needs approval as it's a blocker, so please land on 1.9.2 ASAP)
Target Milestone: Firefox 3.7 → Firefox 3.6b1
Comment 58•15 years ago
|
||
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'.
Updated•15 years ago
|
status1.9.2:
--- → beta1-fixed
Updated•15 years ago
|
Attachment #376613 -
Flags: approval1.9.2?
Comment 59•15 years ago
|
||
Landed on 1.9.2 as: <http://hg.mozilla.org/releases/mozilla-1.9.2/rev/809b137abeda>.
Updated•15 years ago
|
Whiteboard: [doc-waiting-landing]
Updated•15 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Comment 60•15 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•