Closed Bug 601253 Opened 12 years ago Closed 12 years ago

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

Categories

(Firefox :: Private Browsing, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b8

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(1 file, 2 obsolete files)

We currently toggle the PB mode if you specify -private-toggle on the command line.  This shouldn't happen in permanent PB mode.
Blocks: 601255
Attached patch Patch (v1) (obsolete) — Splinter Review
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.]
(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...
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+
(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.
Attached patch Patch (v2) (obsolete) — Splinter Review
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?
(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.
Whiteboard: [has patch][needs review dolske] → [has patch][needs review gavin]
The command-line-startup case is never hit at all for remoting.
(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-
Attached patch Patch (v3)Splinter Review
Right, my bad.
Attachment #482625 - Attachment is obsolete: true
Attachment #485877 - Flags: review?(gavin.sharp)
Attachment #485877 - Flags: review?(gavin.sharp) → review+
Whiteboard: [has patch][needs review gavin] → [has patch][needs approval]
Attachment #485877 - Flags: approval2.0?
Attachment #485877 - Flags: approval2.0? → approval2.0+
Whiteboard: [has patch][needs approval] → [needs landing]
http://hg.mozilla.org/mozilla-central/rev/488177edfe60
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → Firefox 4.0b8
Depends on: 632039
You need to log in before you can comment on or make changes to this bug.