Closed Bug 650280 Opened 9 years ago Closed 8 years ago

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

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 9

People

(Reporter: andrei.domuta, Assigned: ttaubert)

References

Details

(Keywords: privacy)

Attachments

(3 files, 2 obsolete files)

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.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 628082
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
Attached image Screenshot1
Attached image 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 → ---
OS: Windows XP → All
Hardware: x86 → All
Assignee: nobody → tim.taubert
Status: REOPENED → ASSIGNED
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #526931 - Flags: feedback?(raymond)
Comment on attachment 526931 [details] [diff] [review]
patch v1

Looks good!
Attachment #526931 - Flags: feedback?(raymond) → feedback+
Attachment #526931 - Flags: review?(ian)
Depends on: 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.
(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.  ;-)
(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+
Depends on: 651643
(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
Attachment #528190 - Attachment description: patch for checkin (do not push before bug 651643) → patch for checkin (do not push before bug 651643 and 650573)
Attached patch patch v2Splinter Review
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)
Attachment #549296 - Flags: review?(ehsan) → review+
http://hg.mozilla.org/mozilla-central/rev/1ea40cc5b3f3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago8 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.