837492 - Add basic private browsing support

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[[e-mail]]

Let's begin our private browsing support by changing the title of private windows and also the window type and also turning off some stuff that private windows probably don't need.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[Stefan [:stefanh]]

Hmm, it's obvious that something hasn't been working for a while... I don't see any differences between private/non-private window titles when debugQA is enabled... ("PageTitle - SeaMonkey{Build ID: XXXXXXXXXXXXXX}"

Without DebugQA the window title is as expected:
- Non-private window: "PageTitle"
- Private window: "PageTitle - Private Browsing"

Comment 3

[[e-mail]]

(In reply to Stefan from comment #2)
> Hmm, it's obvious that something hasn't been working for a while... I don't
> see any differences between private/non-private window titles when debugQA
> is enabled... ("PageTitle - SeaMonkey{Build ID: XXXXXXXXXXXXXX}"
Yes, debugQA clobbers the title, but my change was supposed to address that.
Maybe you didn't pick it up in your build for some reason?

Comment 4

[Stefan [:stefanh]]

(In reply to [e-mail] from comment #3)
> Yes, debugQA clobbers the title, but my change was supposed to address that.
> Maybe you didn't pick it up in your build for some reason?

Ah, sorry - a new profile fixed it. I now have "foo - Private Browsing {Build ID}"

Comment 5

[Stefan [:stefanh]]

OK, so I just noticed another thing. If you open a blank page in private browsing mode, the window title becomes "Private Browsing" and not "Untitled - Private Browsing".

See http://mxr.mozilla.org/comm-central/source/suite/browser/tabbrowser.xml#884

Comment 6

[Ian Neal]

URLs typed into a private browsing window's address bar seem to go into the urlbar history and are accessible in non-private browsing window's urlbar history.

Comment 7

[Stefan [:stefanh]]

Another thing: you will always get a new window if you, for instance, try to open a bookmark from the Personal Toolbar.

Comment 8

[Stefan [:stefanh]]

(In reply to Stefan [:stefanh] from comment #7)
> Another thing: you will always get a new window if you, for instance, try to
> open a bookmark from the Personal Toolbar.

From a private browsing window, of course.

Comment 9

[[e-mail]]

(In reply to Stefan from comment #5)
> OK, so I just noticed another thing. If you open a blank page in private
> browsing mode, the window title becomes "Private Browsing" and not "Untitled
> - Private Browsing".

That's because the behaviour in a Windows build is for "SeaMonkey" and not "Untitled - SeaMonkey". I don't know an easy way to differentiate the two. (If you turn on debugQA you will probably get "SeaMonkey {Build ID: NNNNNNNNNNNNNN}" and not "Untitled - SeaMonkey {Build ID: NNNNNNNNNNNNNN}".)

(In reply to Ian Neal from comment #6)
> URLs typed into a private browsing window's address bar seem to go into the
> urlbar history and are accessible in non-private browsing window's urlbar
> history.

Indeed, there are probably all sorts of places where private browsing doesn't work properly yet. But without a few starting patches we can't really hope to track them down ;-)

(In reply to Stefan from comment #7)
> Another thing: you will always get a new window if you, for instance, try to
> open a bookmark from the Personal Toolbar.

Yes, I need to think about the best way to handle this. (Currently it thinks you're in a Mac-style zero-window configuration.) For now I noticed that drag-n-drop works.

Comment 10

[Ian Neal]

Comment on attachment 709472 [details] [diff] [review]
Proposed patch

I'd like to have the urlbar history issue fixed before I r= this but so far so good.

Comment 11

[[e-mail]]

diff --git a/suite/common/contentAreaClick.js b/suite/common/contentAreaClick
--- a/suite/common/contentAreaClick.js
+++ b/suite/common/contentAreaClick.js
@@ -193,16 +193,19 @@
       }
       loadURI(url);
     }
     event.stopPropagation();
   }

   function addToUrlbarHistory(aUrlToAdd)
   {
+    if (gPrivate)
+      return;
+
     // Remove leading and trailing spaces first
     aUrlToAdd = aUrlToAdd.trim();

     if (!aUrlToAdd)
       return;
     if (aUrlToAdd.search(/[\x00-\x1F]/) != -1) // don't store bad URLs
       return;

Comment 12

[Stefan [:stefanh]]

Before we move on, can you fix so that new windows/tabs opened from a private window are private?

Comment 13

[[e-mail]]

(In reply to Stefan from comment #12)
> Before we move on, can you fix so that new windows/tabs opened from a
> private window are private?

This bug is just about supporting windows created privately.
Actually creating private windows is the subject of other bugs.

Comment 14

[Stefan [:stefanh]]

Comment on attachment 709472 [details] [diff] [review]
Proposed patch

OK. I have only skimmed the code, but I'm happy with the mac window titles. I do think IanN:s comment should be addressed, though.

Comment 15

[[e-mail]]

Attachment: Addressed review comments

Note: gPrivate is now null or window, this will make getTopWin() easier.

Comment 16

[[e-mail]]

Pushed comm-central changeset d9f8f39bf0fa.

I forgot to credit stefanh in the checkin comment though :-(

Comment 17

[Philip Chee]

Comment on attachment 709472 [details] [diff] [review]
Proposed patch

> +  var lc = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> +                 .getInterface(Components.interfaces.nsIWebNavigation)
> +                 .QueryInterface(Components.interfaces.nsILoadContext);
> +  if (lc.usePrivateBrowsing) {
I presume you don't have any plans to use the toolkit PrivateBrowsingUtils.jsm?

> +    gPrivate = window;
> +    document.documentElement.setAttribute("windowtype", "navigator:private");
> +    var titlemodifier = document.documentElement.getAttribute("titlemodifier");
> +    if (titlemodifier)
> +      titlemodifier += " ";
> +    titlemodifier += document.documentElement.getAttribute("titleprivate");
> +    document.documentElement.setAttribute("titlemodifier", titlemodifier);
> +    document.title = titlemodifier;
> +  }
Firefox distinguishes betweeen "permanent" and "temporary" private browsing. Is this something that you plan to do in a later bug/patch?

> -  if (REMOTESERVICE_CONTRACTID in Components.classes) {
> +  if (!gPrivate && REMOTESERVICE_CONTRACTID in Components.classes) {
>      var remoteService =
>        Components.classes[REMOTESERVICE_CONTRACTID]
>                  .getService(Components.interfaces.nsIRemoteService);
>      remoteService.registerWindow(window);
[What does this do? nothing else in m-c/c-c calls registerWindow()]

>    function addToUrlbarHistory(aUrlToAdd)
>    {
> +    if (gPrivate)
> +      return;
> +
>      // Remove leading and trailing spaces first
>      aUrlToAdd = aUrlToAdd.trim();
>  
>      if (!aUrlToAdd)
[firefox also tests for |!aUrlToAdd.contains(" ")| I wonder why]
[Blame says Blake Ross added this with an empty commit message :P ]
 
> +<!ENTITY mainWindow.titleprivate "Private Browsing">
A localization note might be appropriate e.g.
<!-- LOCALIZATION NOTE (mainWindow.titleprivate):
     This will be appended to the window's title inside the private browsing mode -->
Also consider adding parentheses "(Private Browsing)"

Comment 18

[[e-mail]]

(In reply to Philip Chee from comment #17)
> > +  var lc = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> > +                 .getInterface(Components.interfaces.nsIWebNavigation)
> > +                 .QueryInterface(Components.interfaces.nsILoadContext);
> > +  if (lc.usePrivateBrowsing) {
> I presume you don't have any plans to use the toolkit
> PrivateBrowsingUtils.jsm?
It felt like overkill.

> Firefox distinguishes betweeen "permanent" and "temporary" private browsing.
> Is this something that you plan to do in a later bug/patch?
No; wasn't that for backward compatibility with the old private browsing?

> [What does this do? nothing else in m-c/c-c calls registerWindow()]
[Dunno, but it's been there for 8 years...]

> >    function addToUrlbarHistory(aUrlToAdd)
> [firefox also tests for |!aUrlToAdd.contains(" ")| I wonder why]
[Their URLBar history works differently, it only accepts actual URLs.
 Ours accepts arbitrary strings.]

> > +<!ENTITY mainWindow.titleprivate "Private Browsing">
> A localization note might be appropriate e.g.
> <!-- LOCALIZATION NOTE (mainWindow.titleprivate):
>      This will be appended to the window's title inside the private browsing
> mode -->
> Also consider adding parentheses "(Private Browsing)"
Good points, I'll land a followup.