Closed
Bug 601253
Opened 14 years ago
Closed 14 years ago
Ignore -private-toggle on the command line if permanent private browsing is on
Categories
(Firefox :: Private Browsing, defect)
Tracking
()
RESOLVED
FIXED
Firefox 4.0b8
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
Details
Attachments
(1 file, 2 obsolete files)
1.24 KB,
patch
|
Gavin
:
review+
benjamin
:
approval2.0+
|
Details | Diff | Splinter Review |
We currently toggle the PB mode if you specify -private-toggle on the command line. This shouldn't happen in permanent PB mode.
Assignee | ||
Comment 1•14 years ago
|
||
Attachment #480306 -
Flags: review?(dolske)
Comment 2•14 years ago
|
||
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•14 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•14 years ago
|
Whiteboard: [has patch][needs review dolske]
Comment 4•14 years ago
|
||
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•14 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•14 years ago
|
||
Attachment #480306 -
Attachment is obsolete: true
Attachment #482625 -
Flags: review?(gavin.sharp)
Attachment #480306 -
Flags: review?(gavin.sharp)
Comment 7•14 years ago
|
||
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•14 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•14 years ago
|
Whiteboard: [has patch][needs review dolske] → [has patch][needs review gavin]
Comment 9•14 years ago
|
||
The command-line-startup case is never hit at all for remoting.
Assignee | ||
Comment 10•14 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>
Comment 11•14 years ago
|
||
That's after we do the remoting stuff (nativeApp->Start() and RemoteCommandLine()).
Comment 12•14 years ago
|
||
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•14 years ago
|
||
Right, my bad.
Attachment #482625 -
Attachment is obsolete: true
Attachment #485877 -
Flags: review?(gavin.sharp)
Updated•14 years ago
|
Attachment #485877 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [has patch][needs review gavin] → [has patch][needs approval]
Assignee | ||
Updated•14 years ago
|
Attachment #485877 -
Flags: approval2.0?
Updated•14 years ago
|
Attachment #485877 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [has patch][needs approval] → [needs landing]
Assignee | ||
Comment 14•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → Firefox 4.0b8
You need to log in
before you can comment on or make changes to this bug.
Description
•