The default bug view has changed. See this FAQ.

Switching from Private Browsing to Normal Browsing keeps search strings while in Panorama

RESOLVED FIXED in Firefox 9

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: AndreiD[QA], Assigned: ttaubert)

Tracking

({privacy})

Trunk
Firefox 9
privacy
Dependency tree / graph

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Build:
Mozilla/5.0 (Windows NT 5.1; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

Steps to reproduce:
1. Launch FFx on a new profile
2. Navigate to some websites (example: mozilla, google, live)
3. Enter private browsing 
4. Navigate to some websites (example: mozilla, facebook, cnn)
5. Enter panorama and search for (example: "mozilla") without hitting ENTER 
6. Exit Private Browsing (using the ctrl+shift+p shortcut)

Actual result:
6. In Normal Mode search is active in Panorama, highlighting the "mozilla" string

Expected result:
6. The search should be kept private.

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 628082
(Reporter)

Comment 2

6 years ago
In reply to Comment 1, I'm adding some screenshots of the problem, as for me it is unclear whether it's the same thing that causes the issue (as in bug 628082). Please have a look at them. Thanks
(Reporter)

Comment 3

6 years ago
Created attachment 526672 [details]
Screenshot1
(Reporter)

Comment 4

6 years ago
Created attachment 526673 [details]
Screenshot2
Oh, I was wrong, this is not the same issue.  Thanks for correcting me!  :-)

Tim, is this something that you can take a look at?
Status: RESOLVED → REOPENED
Component: Private Browsing → Panorama
Keywords: privacy
QA Contact: private.browsing → panorama
Resolution: DUPLICATE → ---
(Assignee)

Updated

6 years ago
OS: Windows XP → All
Hardware: x86 → All
(Assignee)

Updated

6 years ago
Assignee: nobody → tim.taubert
Status: REOPENED → ASSIGNED
(Assignee)

Comment 6

6 years ago
Created attachment 526931 [details] [diff] [review]
patch v1
Attachment #526931 - Flags: feedback?(raymond)
Comment on attachment 526931 [details] [diff] [review]
patch v1

Looks good!
Attachment #526931 - Flags: feedback?(raymond) → feedback+
(Assignee)

Updated

6 years ago
Attachment #526931 - Flags: review?(ian)
(Assignee)

Updated

6 years ago
Depends on: 650573
(Assignee)

Comment 8

6 years ago
Comment on attachment 526931 [details] [diff] [review]
patch v1

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=8729ecb92654

(needs patch from bug 650573)
Comment on attachment 526931 [details] [diff] [review]
patch v1

>   // register a clean up for private browsing just in case
>   registerCleanupFunction(function() {
>-    pb.privateBrowsingEnabled = false;
>+    if (pb.privateBrowsingEnabled)
>+      pb.privateBrowsingEnabled = false;
>   });

This is not really needed, as the private browsing service will ignore this if privateBrowsingEnabled is false.
(Assignee)

Comment 10

6 years ago
(In reply to comment #9)
> >   // register a clean up for private browsing just in case
> >   registerCleanupFunction(function() {
> >-    pb.privateBrowsingEnabled = false;
> >+    if (pb.privateBrowsingEnabled)
> >+      pb.privateBrowsingEnabled = false;
> >   });
> 
> This is not really needed, as the private browsing service will ignore this if
> privateBrowsingEnabled is false.

The private browsing service will do nothing but for some reason starts a transition:

http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#507

So if I remove the line the tests fail because there is an ongoing transition while the next test is started.
(In reply to comment #10)
> (In reply to comment #9)
> > >   // register a clean up for private browsing just in case
> > >   registerCleanupFunction(function() {
> > >-    pb.privateBrowsingEnabled = false;
> > >+    if (pb.privateBrowsingEnabled)
> > >+      pb.privateBrowsingEnabled = false;
> > >   });
> > 
> > This is not really needed, as the private browsing service will ignore this if
> > privateBrowsingEnabled is false.
> 
> The private browsing service will do nothing but for some reason starts a
> transition:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#507
> 
> So if I remove the line the tests fail because there is an ongoing transition
> while the next test is started.

Oh, that's a bug.  We should be bailing out of the setter method there early (before the try block).  Can you please file a bug?  I promise a speedy review if you also attach a patch.  ;-)
(Assignee)

Comment 12

6 years ago
(In reply to comment #11)
> Oh, that's a bug.  We should be bailing out of the setter method there early
> (before the try block).  Can you please file a bug?  I promise a speedy review
> if you also attach a patch.  ;-)

Filed bug 651643 and it has a patch :)
Comment on attachment 526931 [details] [diff] [review]
patch v1

Ehsan, can you review this? I'm phasing out my review duties, and would like to spread the Panorama code knowledge around.
Attachment #526931 - Flags: review?(ian) → review?(ehsan)
Comment on attachment 526931 [details] [diff] [review]
patch v1

Review of attachment 526931 [details] [diff] [review]:

The patch looks good in general.  r=me with the nits below fixed.

::: browser/base/content/test/tabview/browser_tabview_bug650280.js
@@ +9,5 @@
+
+  waitForExplicitFinish();
+
+  registerCleanupFunction(function() {
+    cw && cw.hideSearch();

Nit: Please change this to the more conventional if syntax.

@@ +12,5 @@
+  registerCleanupFunction(function() {
+    cw && cw.hideSearch();
+    TabView.hide();
+    if (pb.privateBrowsingEnabled)
+      pb.privateBrowsingEnabled = false;

This should no longer be necessary with the patch in bug 651643?  (I'd like to ask you to land this once bug 651643 has been fixed).

::: browser/base/content/test/tabview/browser_tabview_dragdrop.js
@@ +2,5 @@
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 function test() {
   waitForExplicitFinish();
+  showTabView(onTabViewWindowLoaded);

Is this change (and the other changes to this test) related to this bug?  If not, I'd rather for you to move them into another bug.

As a general rule, I prefer the "one bug per issue" model, as it would make it far easier to come back to a bug in the future and follow along the rationale for the changes made in that bug.

::: browser/base/content/test/tabview/browser_tabview_privatebrowsing.js
@@ +27,5 @@
   
   // register a clean up for private browsing just in case
   registerCleanupFunction(function() {
+    if (pb.privateBrowsingEnabled)
+      pb.privateBrowsingEnabled = false;

Same as above.
Attachment #526931 - Flags: review?(ehsan) → review+
(Assignee)

Updated

6 years ago
Depends on: 651643
(Assignee)

Comment 15

6 years ago
Created attachment 528190 [details] [diff] [review]
patch for checkin (do not push before bug 651643 and 650573)

(In reply to comment #14)
> +  registerCleanupFunction(function() {
> +    cw && cw.hideSearch();

Fixed.

> +    if (pb.privateBrowsingEnabled)
> +      pb.privateBrowsingEnabled = false;
> 
> This should no longer be necessary with the patch in bug 651643?  (I'd like to
> ask you to land this once bug 651643 has been fixed).

Alright, let's first fix bug 651643.

> Is this change (and the other changes to this test) related to this bug?  If
> not, I'd rather for you to move them into another bug.
> 
> As a general rule, I prefer the "one bug per issue" model, as it would make it
> far easier to come back to a bug in the future and follow along the rationale
> for the changes made in that bug.

Removed other changes as they're only cleanup and unrelated.
Attachment #526931 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #528190 - Attachment description: patch for checkin (do not push before bug 651643) → patch for checkin (do not push before bug 651643 and 650573)
(Assignee)

Comment 16

6 years ago
Created attachment 549296 [details] [diff] [review]
patch v2

I moved the hideSearch() call to a place that is called when entering/leaving pb mode. So we don't need that call in two different places. Also the second call was erroneous because it was only called when (self._privateBrowsing.wasInTabView) - when we started PB mode while in Panorama.
Attachment #528190 - Attachment is obsolete: true
Attachment #549296 - Flags: review?(ehsan)

Updated

6 years ago
Attachment #549296 - Flags: review?(ehsan) → review+
(Assignee)

Comment 17

6 years ago
http://hg.mozilla.org/integration/fx-team/rev/1ea40cc5b3f3
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 18

6 years ago
http://hg.mozilla.org/mozilla-central/rev/1ea40cc5b3f3
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 9
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.