Last Comment Bug 837510 - Add File - New - Private Window menuitem
: Add File - New - Private Window menuitem
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: unspecified
: All All
: -- normal (vote)
: seamonkey2.18
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on: 837492 837493 842529 875889 883577 891081
Blocks: 460895 841230 841616
  Show dependency treegraph
 
Reported: 2013-02-03 09:37 PST by neil@parkwaycc.co.uk
Modified: 2013-07-09 14:21 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (18.77 KB, patch)
2013-02-03 09:55 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Revised patch (25.54 KB, patch)
2013-02-12 16:59 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Addressed review comments (25.65 KB, patch)
2013-02-15 06:08 PST, neil@parkwaycc.co.uk
philip.chee: review+
iann_bugzilla: review+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2013-02-03 09:37:58 PST
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 neil@parkwaycc.co.uk 2013-02-03 09:55:35 PST
Created attachment 709499 [details] [diff] [review]
Proposed patch
Comment 2 neil@parkwaycc.co.uk 2013-02-08 16:50:42 PST
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 neil@parkwaycc.co.uk 2013-02-12 16:59:14 PST
Created attachment 713193 [details] [diff] [review]
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 2013-02-14 09:14:35 PST
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 2013-02-14 09:18:06 PST
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 2013-02-14 09:19:23 PST
> const kPrivateWindow = 5;
How about kNewPrivateWindow ?
Comment 7 Philip Chee 2013-02-14 09:37:08 PST
> +<!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 neil@parkwaycc.co.uk 2013-02-14 12:56:46 PST
(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 neil@parkwaycc.co.uk 2013-02-14 16:34:50 PST
(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 neil@parkwaycc.co.uk 2013-02-15 05:26:42 PST
(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 2013-02-15 05:47:51 PST
>>> +<!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 neil@parkwaycc.co.uk 2013-02-15 06:08:27 PST
Created attachment 714347 [details] [diff] [review]
Addressed review comments
Comment 13 Philip Chee 2013-02-15 08:56:32 PST
Comment on attachment 714347 [details] [diff] [review]
Addressed review comments

r=me
Comment 14 Ian Neal 2013-02-17 15:56:12 PST
Comment on attachment 714347 [details] [diff] [review]
Addressed review comments

r=me
Comment 15 neil@parkwaycc.co.uk 2013-02-18 16:31:55 PST
Pushed comm-central changeset 98476aa17e13.
Comment 16 Philip Chee 2013-02-19 06:31:46 PST
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 2013-02-19 06:36:52 PST
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

Note You need to log in before you can comment on or make changes to this bug.