Add basic private browsing support

RESOLVED FIXED in seamonkey2.18

Status

SeaMonkey
UI Design
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

(Blocks: 1 bug)

unspecified
seamonkey2.18
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 709472 [details] [diff] [review]
Proposed patch
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #709472 - Flags: review?(stefanh)
Attachment #709472 - Flags: review?(philip.chee)
Attachment #709472 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

5 years ago
Blocks: 837496
(Assignee)

Updated

5 years ago
Blocks: 837510

Comment 2

5 years ago
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"
(Assignee)

Comment 3

5 years ago
(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

5 years ago
(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

5 years ago
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

5 years ago
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

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

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
(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

5 years ago
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.
Attachment #709472 - Flags: review?(iann_bugzilla) → feedback+
(Assignee)

Comment 11

5 years ago
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

5 years ago
Before we move on, can you fix so that new windows/tabs opened from a private window are private?
(Assignee)

Comment 13

5 years ago
(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

5 years ago
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.
Attachment #709472 - Flags: review?(stefanh) → review+
(Assignee)

Comment 15

5 years ago
Created attachment 712265 [details] [diff] [review]
Addressed review comments

Note: gPrivate is now null or window, this will make getTopWin() easier.
Attachment #712265 - Flags: review?(iann_bugzilla)

Updated

5 years ago
Attachment #712265 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 16

5 years ago
Pushed comm-central changeset d9f8f39bf0fa.

I forgot to credit stefanh in the checkin comment though :-(
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Target Milestone: --- → seamonkey2.18

Comment 17

5 years ago
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)"
Attachment #709472 - Flags: review?(philip.chee) → review+
(Assignee)

Comment 18

5 years ago
(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.
(Assignee)

Updated

5 years ago
Blocks: 846762
You need to log in before you can comment on or make changes to this bug.