Last Comment Bug 837492 - Add basic private browsing support
: Add basic private browsing support
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: seamonkey2.18
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on:
Blocks: 460895 837496 837510 846762
  Show dependency treegraph
 
Reported: 2013-02-03 07:03 PST by neil@parkwaycc.co.uk
Modified: 2013-03-01 07:21 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (8.10 KB, patch)
2013-02-03 07:13 PST, neil@parkwaycc.co.uk
stefanh: review+
philip.chee: review+
iann_bugzilla: feedback+
Details | Diff | Review
Addressed review comments (8.64 KB, patch)
2013-02-10 09:04 PST, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Review

Description neil@parkwaycc.co.uk 2013-02-03 07:03:09 PST
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 neil@parkwaycc.co.uk 2013-02-03 07:13:45 PST
Created attachment 709472 [details] [diff] [review]
Proposed patch
Comment 2 Stefan [:stefanh] 2013-02-03 12:34:41 PST
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 neil@parkwaycc.co.uk 2013-02-03 13:10:42 PST
(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] 2013-02-03 13:49:44 PST
(In reply to neil@parkwaycc.co.uk 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] 2013-02-03 13:59:22 PST
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 2013-02-03 14:04:29 PST
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] 2013-02-03 14:33:42 PST
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] 2013-02-03 14:34:40 PST
(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 neil@parkwaycc.co.uk 2013-02-03 16:12:03 PST
(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 2013-02-04 05:29:06 PST
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 neil@parkwaycc.co.uk 2013-02-04 16:33:10 PST
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] 2013-02-09 08:39:01 PST
Before we move on, can you fix so that new windows/tabs opened from a private window are private?
Comment 13 neil@parkwaycc.co.uk 2013-02-09 09:54:42 PST
(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] 2013-02-09 10:00:16 PST
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 neil@parkwaycc.co.uk 2013-02-10 09:04:43 PST
Created attachment 712265 [details] [diff] [review]
Addressed review comments

Note: gPrivate is now null or window, this will make getTopWin() easier.
Comment 16 neil@parkwaycc.co.uk 2013-02-12 11:26:06 PST
Pushed comm-central changeset d9f8f39bf0fa.

I forgot to credit stefanh in the checkin comment though :-(
Comment 17 Philip Chee 2013-02-14 06:37:24 PST
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 neil@parkwaycc.co.uk 2013-02-14 06:49:19 PST
(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.

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