Ignore -private-toggle on the command line if permanent private browsing is on

RESOLVED FIXED in Firefox 4.0b8

Status

()

Firefox
Private Browsing
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

Trunk
Firefox 4.0b8
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
We currently toggle the PB mode if you specify -private-toggle on the command line.  This shouldn't happen in permanent PB mode.
(Assignee)

Updated

7 years ago
Blocks: 601255
(Assignee)

Comment 1

7 years ago
Created attachment 480306 [details] [diff] [review]
Patch (v1)
Attachment #480306 - Flags: review?(dolske)
Hmm, instead of just ignoring the flag, should we throw (per nsICommandLineHandler.idl) so that startup aborts and the app quits?

[I guess it's not a big  deal, though. Starting in PB mode when you were expecting non-PB mode isn't a big deal. Obviously this would be different if the situation was about starting in non-PB mode when the user expected PB mode.]
(Assignee)

Comment 3

7 years ago
(In reply to comment #2)
> Hmm, instead of just ignoring the flag, should we throw (per
> nsICommandLineHandler.idl) so that startup aborts and the app quits?

Hmm, I'm not sure if that's needed.  The app will quit anyways, right?  However, if you want, I can definitely make that change...
(Assignee)

Updated

7 years ago
Whiteboard: [has patch][needs review dolske]
Comment on attachment 480306 [details] [diff] [review]
Patch (v1)

Yeah, worth throwing I think since it's a nonsensical commandline, according to how permanent PB mode works.

Looks fine otherwise, but I'm going to bounce over to gavin since I've not really familiar with the command line stuff. Why does the existing code have both an observer and nsICommandLineHandler? Does one get called for the initial app launch, and the other for remote invocations or something like that?
Attachment #480306 - Flags: review?(gavin.sharp)
Attachment #480306 - Flags: review?(dolske)
Attachment #480306 - Flags: review+
(Assignee)

Comment 5

7 years ago
(In reply to comment #4)
> Comment on attachment 480306 [details] [diff] [review]
> Patch (v1)
> 
> Yeah, worth throwing I think since it's a nonsensical commandline, according to
> how permanent PB mode works.

OK, will submit a new patch with that change.

> Looks fine otherwise, but I'm going to bounce over to gavin since I've not
> really familiar with the command line stuff. Why does the existing code have
> both an observer and nsICommandLineHandler? Does one get called for the initial
> app launch, and the other for remote invocations or something like that?

"command-line-startup" is fired before "final-ui-startup".  After that notification, it's too late for us to initialize permanent PB mode.
(Assignee)

Comment 6

7 years ago
Created attachment 482625 [details] [diff] [review]
Patch (v2)
Attachment #480306 - Attachment is obsolete: true
Attachment #482625 - Flags: review?(gavin.sharp)
Attachment #480306 - Flags: review?(gavin.sharp)
Comment on attachment 482625 [details] [diff] [review]
Patch (v2)

>diff --git a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js

>       case "command-line-startup":
>         this._obs.removeObserver(this, "command-line-startup");
>         aSubject.QueryInterface(Ci.nsICommandLine);
>         if (aSubject.findFlag("private", false) >= 0) {
>           this.privateBrowsingEnabled = true;
>           this._autoStarted = true;
>           this._lastChangedByCommandLine = true;
>         }
>-        else if (aSubject.findFlag("private-toggle", false) >= 0) {
>+        else if (aSubject.findFlag("private-toggle", false) >= 0 &&
>+                 !this._autoStarted) {
>           this._lastChangedByCommandLine = true;
>         }

I don't really understand the reasoning behind this change. If the idea is to avoid setting lastChangedByCommandLine if we're already in PB mode due to the pref being set, then why not also do this for the -private flag?
(Assignee)

Comment 8

7 years ago
(In reply to comment #7)
> Comment on attachment 482625 [details] [diff] [review]
> Patch (v2)
> 
> >diff --git a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
> 
> >       case "command-line-startup":
> >         this._obs.removeObserver(this, "command-line-startup");
> >         aSubject.QueryInterface(Ci.nsICommandLine);
> >         if (aSubject.findFlag("private", false) >= 0) {
> >           this.privateBrowsingEnabled = true;
> >           this._autoStarted = true;
> >           this._lastChangedByCommandLine = true;
> >         }
> >-        else if (aSubject.findFlag("private-toggle", false) >= 0) {
> >+        else if (aSubject.findFlag("private-toggle", false) >= 0 &&
> >+                 !this._autoStarted) {
> >           this._lastChangedByCommandLine = true;
> >         }
> 
> I don't really understand the reasoning behind this change. If the idea is to
> avoid setting lastChangedByCommandLine if we're already in PB mode due to the
> pref being set, then why not also do this for the -private flag?

That is the idea behind this change, and the reason why we don't do this for -private is that firstly I'm not changing the behavior of -private in this bug, and secondly the -private flag is not remotable, and therefore is not subject to this problem in the first place.
(Assignee)

Updated

7 years ago
Whiteboard: [has patch][needs review dolske] → [has patch][needs review gavin]
The command-line-startup case is never hit at all for remoting.
(Assignee)

Comment 10

7 years ago
(In reply to comment #9)
> The command-line-startup case is never hit at all for remoting.

Is it not?

<http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#3584>
That's after we do the remoting stuff (nativeApp->Start() and RemoteCommandLine()).
Comment on attachment 482625 [details] [diff] [review]
Patch (v2)

But even ignoring all of that, the change isn't related to this bug at all, as far as I can see. Given that, and the fact that it also introduces an inconsistency between -private and -private-browsing, it seems pretty clear that it shouldn't be made here.

The other hunk in this patch looks fine, you can have r+ on just that.
Attachment #482625 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 13

7 years ago
Created attachment 485877 [details] [diff] [review]
Patch (v3)

Right, my bad.
Attachment #482625 - Attachment is obsolete: true
Attachment #485877 - Flags: review?(gavin.sharp)
Attachment #485877 - Flags: review?(gavin.sharp) → review+
(Assignee)

Updated

7 years ago
Whiteboard: [has patch][needs review gavin] → [has patch][needs approval]
(Assignee)

Updated

7 years ago
Attachment #485877 - Flags: approval2.0?

Updated

7 years ago
Attachment #485877 - Flags: approval2.0? → approval2.0+
(Assignee)

Updated

7 years ago
Whiteboard: [has patch][needs approval] → [needs landing]
(Assignee)

Comment 14

7 years ago
http://hg.mozilla.org/mozilla-central/rev/488177edfe60
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → Firefox 4.0b8

Updated

7 years ago
Depends on: 632039
You need to log in before you can comment on or make changes to this bug.