Last Comment Bug 650280 - Switching from Private Browsing to Normal Browsing keeps search strings while in Panorama
: Switching from Private Browsing to Normal Browsing keeps search strings while...
Status: RESOLVED FIXED
: privacy
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 9
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on: 650573 651643
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-15 07:39 PDT by AndreiD[QA]
Modified: 2016-04-12 14:00 PDT (History)
7 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Screenshot1 (491.65 KB, image/png)
2011-04-18 01:11 PDT, AndreiD[QA]
no flags Details
Screenshot2 (523.60 KB, image/png)
2011-04-18 01:12 PDT, AndreiD[QA]
no flags Details
patch v1 (8.13 KB, patch)
2011-04-18 22:04 PDT, Tim Taubert [:ttaubert]
ehsan: review+
raymond: feedback+
Details | Diff | Review
patch for checkin (do not push before bug 651643 and 650573) (4.86 KB, patch)
2011-04-25 15:09 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
patch v2 (5.15 KB, patch)
2011-07-28 20:44 PDT, Tim Taubert [:ttaubert]
ehsan: review+
Details | Diff | Review

Description AndreiD[QA] 2011-04-15 07:39:55 PDT
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 1 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-15 14:10:02 PDT

*** This bug has been marked as a duplicate of bug 628082 ***
Comment 2 AndreiD[QA] 2011-04-18 00:54:55 PDT
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] 2011-04-18 01:11:21 PDT
Created attachment 526672 [details]
Screenshot1
Comment 4 AndreiD[QA] 2011-04-18 01:12:42 PDT
Created attachment 526673 [details]
Screenshot2
Comment 5 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-18 18:24:53 PDT
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] 2011-04-18 22:04:21 PDT
Created attachment 526931 [details] [diff] [review]
patch v1
Comment 7 Raymond Lee [:raymondlee] 2011-04-18 22:43:08 PDT
Comment on attachment 526931 [details] [diff] [review]
patch v1

Looks good!
Comment 8 Tim Taubert [:ttaubert] 2011-04-19 04:23:55 PDT
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 9 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-19 15:33:33 PDT
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] 2011-04-20 03:06:18 PDT
(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 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-20 11:43:58 PDT
(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] 2011-04-20 15:08:28 PDT
(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] 2011-04-22 09:45:04 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-25 12:50:52 PDT
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] 2011-04-25 15:09:54 PDT
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.
Comment 16 Tim Taubert [:ttaubert] 2011-07-28 20:44:30 PDT
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.
Comment 17 Tim Taubert [:ttaubert] 2011-08-21 10:05:21 PDT
http://hg.mozilla.org/integration/fx-team/rev/1ea40cc5b3f3
Comment 18 Tim Taubert [:ttaubert] 2011-08-21 23:56:29 PDT
http://hg.mozilla.org/mozilla-central/rev/1ea40cc5b3f3

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