Closed Bug 837510 Opened 7 years ago Closed 7 years ago

Add File - New - Private Window menuitem

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.18

People

(Reporter: neil, Assigned: neil)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

This belongs below File - New - Browser Window. In order to achieve this for the navigator window itself, I have moved File - New - Browser Tab before them both.
Attached patch Proposed patch (obsolete) — Splinter Review
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #709499 - Flags: review?(philip.chee)
Attachment #709499 - Flags: review?(iann_bugzilla)
Comment on attachment 709499 [details] [diff] [review]
Proposed patch

>+function OpenPrivateWindow()
>+{
>+  window.openDialog(getBrowserURL(), "_blank",
>+                    "private,chrome,all,dialog=no", "about:privatebrowsing");
>+}

I might want to tweak this for the "Open Link in Private Window" context menuitem, something like this:

function OpenPrivateWindow(aUrl, aDoc)
{
  var referrer = null;
  if (aDoc) {
    urlSecurityCheck(aURL, aDoc.nodePrincipal,
                     Components.interfaces.nsIScriptSecurityManager.STANDARD);
    referrer = aDoc.documentURIObject;
  } else if (!aUrl) {
    aUrl = "about:privatebrowsing";
  }
  window.openDialog(getBrowserURL(), "_blank",
                    "private,chrome,all,dialog=no", aUrl, null, referrer);
}
Attached patch Revised patchSplinter Review
Added non-private in more places to ensure that windows aren't opened privately by mistake. Also created openNewPrivateWith to share code with openNewWindowWith instead of copying it into OpenPrivateWindow (for use by the context menu in a followup bug).
Attachment #709499 - Attachment is obsolete: true
Attachment #709499 - Flags: review?(philip.chee)
Attachment #709499 - Flags: review?(iann_bugzilla)
Attachment #713193 - Flags: review?(philip.chee)
Attachment #713193 - Flags: review?(iann_bugzilla)
Blocks: 841230
Comment on attachment 713193 [details] [diff] [review]
Revised patch

> +          <menuitem id="menu_newPrivate"/>
....
> +    <key id="key_newPrivate"/>
....
> +  <command id="cmd_newPrivate"
> +           oncommand="OpenPrivateWindow();"/>
....

I prefer "menu_newPrivateWindow", "key_newPrivateWindow", "cmd_newPrivateWindow", etc

> +function OpenPrivateWindow()
> +{
> +  window.openDialog(getBrowserURL(), "_blank",
> +                    "private,chrome,all,dialog=no", "about:privatebrowsing");
> +}
> +
>  function OpenBrowserWindow()
[Consider supporting the Firefox syntax OpenBrowserWindow({private: true}) for extension compatibility]

> +    var features = "private,chrome,all,dialog=no";
> +    if (aType != kNewPrivate)
> +      features = "non-" + features;
This is too clever by half but it makes the code less readable. I prefer something clearer e.g.

var features = aType != kNewPrivate ? "non-private" : "private;
features += ",chrome,all,dialog=no"

> +<!ENTITY newPrivateCmd.accesskey "P">
>  <!ENTITY printSetupCmd.label "Page Setup">
>  <!ENTITY printSetupCmd.accesskey "u">
>  <!ENTITY printPreviewCmd.label "Print Preview">
>  <!ENTITY printPreviewCmd.accesskey "v">
>  <!ENTITY printCmd.label "Print">
Your patch seems to have swallowed the ellipsis in "Page Setup…" and "Print Preview…".

From Comment 2:
> function OpenPrivateWindow(aUrl, aDoc)
> {
>   var referrer = null;
>   if (aDoc) {
>     urlSecurityCheck(aURL, aDoc.nodePrincipal,
>                      Components.interfaces.nsIScriptSecurityManager.STANDARD);

AFAIK nsIScriptSecurityManager.STANDARD is the default.
Attachment #713193 - Flags: review?(philip.chee) → review+
Comment on attachment 713193 [details] [diff] [review]
Revised patch

> const kPrivateWindow = 5;

Fri Feb 15 2013 01:15:39
Error: ReferenceError: kNewPrivate is not defined
Source file: chrome://communicator/content/utilityOverlay.js
Line: 1050
Attachment #713193 - Flags: review+
> const kPrivateWindow = 5;
How about kNewPrivateWindow ?
> +<!ENTITY newPrivateCmd.label "Private Window">
> +<!ENTITY newPrivateCmd.key "P">
> +<!ENTITY newPrivateCmd.accesskey "P">
This access key clashes with File->New->Composer _P_age.
(In reply to Philip Chee from comment #4)
> I prefer "menu_newPrivateWindow", "key_newPrivateWindow",
> "cmd_newPrivateWindow", etc
None of the related commands include the suffix "Window" ... perhaps we should rename the feature to "Stealth" or "Secret" or something?

> >  function OpenBrowserWindow()
> [Consider supporting the Firefox syntax OpenBrowserWindow({private: true})
> for extension compatibility]
Slippery slope...

> Your patch seems to have swallowed the ellipsis in "Page Setup…" and "Print
> Preview…".
It's because I copied it across VNC, which doesn't like ellipses. (I don't log in to Bugzilla on the PC on which I wrote the patch.)

> AFAIK nsIScriptSecurityManager.STANDARD is the default.
Well that call isn't there any more to avoid code duplication anyway.

(In reply to Philip Chee from comment #6)
> > const kPrivateWindow = 5;
Eek, I forgot to rename this with the others (late untested change, oops)

(In reply to Philip Chee from comment #7)
> > +<!ENTITY newPrivateCmd.accesskey "P">
> This access key clashes with File->New->Composer _P_age.
Oops. How about "W"?
(In reply to Philip Chee from comment #4)
> (From update of attachment 713193 [details] [diff] [review])
> >  function OpenBrowserWindow()
> [Consider supporting the Firefox syntax OpenBrowserWindow({private: true})
> for extension compatibility]
Actually there's only one caller of OpenBrowserWindow, I might as well call openNewPrivateWith directly.
Blocks: 841616
(In reply to comment #9)
> (In reply to Philip Chee from comment #4)
> > (From update of attachment 713193 [details] [diff] [review])
> > >  function OpenBrowserWindow()
> > [Consider supporting the Firefox syntax OpenBrowserWindow({private: true})
> > for extension compatibility]
> Actually there's only one caller of OpenBrowserWindow, I might as well call
> openNewPrivateWith directly.
I mean only one caller of OpenPrivateWindow of course.
>>> +<!ENTITY newPrivateCmd.accesskey "P">
>> This access key clashes with File->New->Composer _P_age.
> Oops. How about "W"?
Sure.

>> Actually there's only one caller of OpenBrowserWindow, I might as well call
>> openNewPrivateWith directly.
> I mean only one caller of OpenPrivateWindow of course.
Sounds good.
Attachment #714347 - Flags: review?(philip.chee)
Comment on attachment 714347 [details] [diff] [review]
Addressed review comments

r=me
Attachment #714347 - Flags: review?(philip.chee) → review+
Attachment #713193 - Flags: review?(iann_bugzilla)
Comment on attachment 714347 [details] [diff] [review]
Addressed review comments

r=me
Attachment #714347 - Flags: review+
Pushed comm-central changeset 98476aa17e13.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.18
Tue Feb 19 2013 22:01:31
Error: ReferenceError: FillInHTMLTooltip is not defined
Source file: chrome://navigator/content/navigator.xul
Line: 1
 ----------
Tue Feb 19 2013 22:01:26
Error: ReferenceError: getBrowserURL is not defined
Source file: chrome://infolister/content/seamonkey-support.js
Line: 2
 ----------
Tue Feb 19 2013 22:01:26
Error: TypeError: popup is null
Source file: chrome://navigator/content/safeBrowsingOverlay.js
Line: 42
 ----------
Tue Feb 19 2013 22:01:26
Error: NS_ERROR_INVALID_POINTER: Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIRDFService.GetDataSourceBlocking]
Source file: chrome://communicator/content/sidebar/sidebarOverlay.js
Line: 709
 ----------
Tue Feb 19 2013 22:01:26
Error: ReferenceError: isElementVisible is not defined
Source file: chrome://navigator/content/navigator.js
Line: 709
 ----------
Tue Feb 19 2013 22:01:26
Error: error in processing external entity reference
Source file: jar:file:///C:/T1/hg/objdir-sm/mozilla/dist/seamonkey/omni.ja!/chrome/comm/content/navigator/urlbarBindings.xml
Line: 9, Column: 3
Source code:
  %textcontextDTD;
 ----------
Tue Feb 19 2013 22:01:26
Error: error in processing external entity reference
Source file: jar:file:///C:/T1/hg/objdir-sm/mozilla/dist/seamonkey/omni.ja!/chrome/comm/content/communicator/search/search.xml
Line: 10, Column: 3
Source code:
  %textcontextDTD;
IanN pointed me at: http://hg.mozilla.org/comm-central/rev/98476aa17e13#l8.12

    8.12 -<!-- LOCALIZATION NOTE : FILE This file contains the global menu items --> 
    8.13 +!-- LOCALIZATION NOTE : FILE This file contains the global menu items --> 

Bustage fix pushed r=me
http://hg.mozilla.org/comm-central/rev/04714c3e9acd
Depends on: 842529
Depends on: 875889, 883577
Depends on: 891081
You need to log in before you can comment on or make changes to this bug.