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

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[AndreiD[QA]]

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.

Comment 2

[AndreiD[QA]]

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

Comment 3

[AndreiD[QA]]

Attachment: Screenshot1

Comment 4

[AndreiD[QA]]

Attachment: Screenshot2

Comment 5

[(no longer active)]

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?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 7

[Raymond Lee [:raymondlee]]

Comment on attachment 526931 [details] [diff] [review]
patch v1

Looks good!

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 526931 [details] [diff] [review]
patch v1

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=8729ecb92654

(needs patch from bug 650573)

Comment 9

[(no longer active)]

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.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(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.

Comment 11

[(no longer active)]

(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.  ;-)

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

(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 13

[Ian Gilman [:iangilman]]

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.

Comment 14

[(no longer active)]

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.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 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.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 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.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/fx-team/rev/1ea40cc5b3f3

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/1ea40cc5b3f3