837510 - Add File - New - Private Window menuitem

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[[e-mail]]

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.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[[e-mail]]

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);
}

Comment 3

[[e-mail]]

Attachment: Revised patch

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).

Comment 4

[Philip Chee]

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.

Comment 5

[Philip Chee]

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

Comment 6

[Philip Chee]

> const kPrivateWindow = 5;
How about kNewPrivateWindow ?

Comment 7

[Philip Chee]

> +<!ENTITY newPrivateCmd.label "Private Window">
> +<!ENTITY newPrivateCmd.key "P">
> +<!ENTITY newPrivateCmd.accesskey "P">
This access key clashes with File->New->Composer _P_age.

Comment 8

[[e-mail]]

(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"?

Comment 9

[[e-mail]]

(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.

Comment 10

[[e-mail]]

(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.

Comment 11

[Philip Chee]

>>> +<!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.

Comment 12

[[e-mail]]

Attachment: Addressed review comments

Comment 13

[Philip Chee]

Comment on attachment 714347 [details] [diff] [review]
Addressed review comments

r=me

Comment 14

[Ian Neal]

Comment on attachment 714347 [details] [diff] [review]
Addressed review comments

r=me

Comment 15

[[e-mail]]

Pushed comm-central changeset 98476aa17e13.

Comment 16

[Philip Chee]

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;

Comment 17

[Philip Chee]

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