Closed Bug 607482 Opened 9 years ago Closed 9 years ago

When in Private Mode, opening a new window causes FF button to flash orange and then turn purple

Categories

(Firefox :: Private Browsing, defect, trivial)

All
Windows 7
defect
Not set
trivial

Tracking

()

RESOLVED FIXED
Firefox 4.0b8
Tracking Status
blocking2.0 --- -

People

(Reporter: imradyurrad, Assigned: ehsan)

References

Details

(Keywords: polish)

Attachments

(3 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101026 Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101026 Firefox/4.0b8pre

The button's orange for about a quarter of a second before it turns purple.

Reproducible: Always
Component: General → Private Browsing
Hardware: x86 → All
Version: unspecified → Trunk
Blocks: 588655
This happens because the privatebrowsingmode attribute is set from delayedStartup.  The easy solution would be to init it inside BrowserStartup.  What do you think, Dao?
Assignee: nobody → ehsan
blocking2.0: --- → ?
Keywords: regression
QA Contact: general → private.browsing
Yeah. I think there are also a few tests that could stop messing with delayedStartup.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regressionpolish
(In reply to comment #2)
> Yeah. I think there are also a few tests that could stop messing with
> delayedStartup.

Hmm, do you mean that we should just call gPrivateBrowsingUI.init() from BrowserStartup instead of delayedStartup?  I'm worried that it might have some perf impact...
The code directly in browser.js looks cheap enough. Is getting the service potentially expensive?
(In reply to comment #4)
> The code directly in browser.js looks cheap enough. Is getting the service
> potentially expensive?

It shouldn't be.  In fact, the service is already initialized when we show a window, so we just grab a reference to the existing object (which involves some XPConnect stuff, obviously).  Let's try it.
Attached patch Patch (v1) (obsolete) — Splinter Review
Attachment #486707 - Flags: review?(dao)
Comment on attachment 486707 [details] [diff] [review]
Patch (v1)

>@@ -1248,16 +1248,19 @@ function BrowserStartup() {
> 
>   allTabs.readPref();
> 
>   TabsOnTop.syncCommand();
> 
>   BookmarksMenuButton.init();
> 
>   setTimeout(delayedStartup, 0, isLoadingBlank, mustLoadSidebar);
>+
>+  // initialize the private browsing UI
>+  gPrivateBrowsingUI.init();
> }

setTimeout should remain at the very end of that function.

I don't really see the point of browser_privatebrowsing_initonwindowload.js. It's very much tied to the current implementation without actually verifying that the button doesn't flash orange. Tests aren't for free but need to be maintained from time to time (as you know very well :-)). I wouldn't add this one.
Attachment #486707 - Flags: review?(dao) → review+
Attached patch For check-inSplinter Review
(In reply to comment #7)
> Comment on attachment 486707 [details] [diff] [review]
> Patch (v1)
> 
> >@@ -1248,16 +1248,19 @@ function BrowserStartup() {
> > 
> >   allTabs.readPref();
> > 
> >   TabsOnTop.syncCommand();
> > 
> >   BookmarksMenuButton.init();
> > 
> >   setTimeout(delayedStartup, 0, isLoadingBlank, mustLoadSidebar);
> >+
> >+  // initialize the private browsing UI
> >+  gPrivateBrowsingUI.init();
> > }
> 
> setTimeout should remain at the very end of that function.

Oops, right.  Fixed that.

> I don't really see the point of browser_privatebrowsing_initonwindowload.js.
> It's very much tied to the current implementation without actually verifying
> that the button doesn't flash orange. Tests aren't for free but need to be
> maintained from time to time (as you know very well :-)). I wouldn't add this
> one.

Fair enough!  I removed the test.
Attachment #486707 - Attachment is obsolete: true
Attachment #486737 - Flags: approval2.0?
Can someone set this to blocking final?
I don't think this blocks. Nice polish for a non-default case, so will a+ the patch instead.
blocking2.0: ? → -
Attachment #486737 - Flags: approval2.0? → approval2.0+
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/119ed9cbd3c6
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → Firefox 4.0b8
Depends on: 609793
This patch fixes the test which required the removeAttribute call to be made asynchronously.  With this, the crash in bug 609793 doesn't happen, so we can land this patch while someone comes up with a solution to bug 609793.
Attachment #490446 - Flags: review?(dao)
Status: REOPENED → ASSIGNED
Comment on attachment 490446 [details] [diff] [review]
Part 2: Adjust the test so that the code doesn't need to unset the attribute asynchronously

Looks like this would be more robust if the observer was added in onEnterPrivateBrowsing and onExitPrivateBrowsing and always removed itself (not just for the second pass).
Attachment #490446 - Flags: review?(dao) → review+
(In reply to comment #14)
> Comment on attachment 490446 [details] [diff] [review]
> Part 2: Adjust the test so that the code doesn't need to unset the attribute
> asynchronously
> 
> Looks like this would be more robust if the observer was added in
> onEnterPrivateBrowsing and onExitPrivateBrowsing and always removed itself (not
> just for the second pass).

Fair enough.
Attachment #490605 - Flags: approval2.0?
Attachment #490605 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/364409371144
http://hg.mozilla.org/mozilla-central/rev/4fea98061fae
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Flags: in-litmus?
Resolution: --- → FIXED
I had to back out because of oranges on m-c (which I had not been getting on the try server).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Relanded again:

http://hg.mozilla.org/mozilla-central/rev/8515f6bd4306
http://hg.mozilla.org/mozilla-central/rev/23262313e08b
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Seems like this patch stuck this time!  :-)
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.