Last Comment Bug 525475 - disable Aero Peek tab preview for Firefox 3.6
: disable Aero Peek tab preview for Firefox 3.6
Status: RESOLVED FIXED
: dev-doc-complete, verified1.9.2
Product: Firefox
Classification: Client Software
Component: Shell Integration (show other bugs)
: 3.6 Branch
: x86 Windows 7
: P2 normal with 2 votes (vote)
: ---
Assigned To: David Dahl :ddahl
:
: Robert Strong [:rstrong] (use needinfo to contact me)
Mentors:
Depends on: 529815
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-30 09:27 PDT by Mike Shaver (:shaver -- probably not reading bugmail closely)
Modified: 2010-12-17 06:56 PST (History)
26 users (show)
shaver: blocking‑firefox3.6+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta3-fixed


Attachments
v 1.0 Disable tab preview pref to false (830 bytes, patch)
2009-11-04 11:38 PST, David Dahl :ddahl
tellrob: review+
Details | Diff | Splinter Review
v 1.0.1 - patch for 1.9.2 branch (579 bytes, patch)
2009-11-04 12:54 PST, David Dahl :ddahl
tellrob: review+
Details | Diff | Splinter Review

Description Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-10-30 09:27:34 PDT
We've decided, based on the current state and recent history of this specific feature, that we won't be shipping tab preview for Aero Peek in Windows 7, so we need to disable it before ship (ideally before the next beta push in 1-2 weeks).
Comment 1 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2009-10-30 10:17:08 PDT
Is it going to be shipped pref'd off or is the code going to be removed?
Comment 2 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-10-30 10:18:24 PDT
I don't think we need pref control for it; would rather do the simplest thing that disables it entirely, which is probably leaving some dead code in place and avoiding the hooking.  (Assuming that the dead code isn't substantial enough to cause impact itself.)
Comment 3 Jim Mathies [:jimm] 2009-10-30 10:35:56 PDT
(In reply to comment #2)
> I don't think we need pref control for it; would rather do the simplest thing
> that disables it entirely, which is probably leaving some dead code in place
> and avoiding the hooking.  (Assuming that the dead code isn't substantial
> enough to cause impact itself.)

FWIW, we already have pref control for it through browser.taskbar.previews.enable. What concerns do we have with leaving the code in and disabling it by default?
Comment 4 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-10-30 10:39:02 PDT
How well-tested is the disabling via the pref?  That's mostly my concern, but if it's already there, then let's just flip the pref on 1.9.2 for now and go, sure.
Comment 5 Tony Chung [:tchung] 2009-10-30 11:29:21 PDT
We should probably jump in here and help qa that pref a bit.  is it simply toggling browser.taskbar.previews.enable to on and off?
Comment 6 Jim Mathies [:jimm] 2009-10-30 11:55:33 PDT
(In reply to comment #5)
> We should probably jump in here and help qa that pref a bit.  is it simply
> toggling browser.taskbar.previews.enable to on and off?

yes, that should do it.
Comment 7 Nochum Sossonko [:Natch] 2009-10-30 14:15:33 PDT
Most of taskbar previews is "disabled-by-default" already since it's platform-specific for Windows only. See http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#136 fe. That might actually help with removing the feature/preffing it off.
Comment 8 [not reading bugmail] 2009-10-30 14:51:12 PDT
(In reply to comment #7)
> Most of taskbar previews is "disabled-by-default" already since it's
> platform-specific for Windows only. See
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#136
> fe. That might actually help with removing the feature/preffing it off.

Or for 1.9.2
http://mxr.mozilla.org/mozilla1.9.2/source/browser/base/content/browser.js#6286
Comment 9 Dietrich Ayala (:dietrich) 2009-11-02 12:39:01 PST
Can someone enumerate the major problems with tab preview? It would help evaluate the level of brokenness, to determine if we should disable the feature entirely, or just pref it off.
Comment 10 [not reading bugmail] 2009-11-02 13:04:38 PST
The discussion I think started with bug 474056 comment 53, creating bug 522262, the main driver I think and a host of many others attached to bug 474056.  The tab preview problem was partly fixed by bug 522416, but we're still see many UX issues using tab previews which I tried to summarize in bug 522262 comment 24.
Comment 11 Dietrich Ayala (:dietrich) 2009-11-04 11:32:07 PST
Thanks Dale, that summary really helped. I think we're ok to just disable the feature by default, but still ship the bits.
Comment 12 David Dahl :ddahl 2009-11-04 11:38:50 PST
Created attachment 410297 [details] [diff] [review]
v 1.0 Disable tab preview pref to false

Just a pref change
Comment 13 Dão Gottwald [:dao] 2009-11-04 11:41:11 PST
Note that there's code in the browser.js startup path that could be skipped if we decided not to support browser.taskbar.previews.enable=true on 1.9.2.
Comment 14 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-11-04 11:42:01 PST
Let's leave it for the power users and others who will help us get lots of coverage here.
Comment 15 Dão Gottwald [:dao] 2009-11-04 12:00:02 PST
I think we'll likely get more coverage by leaving it enabled by default on trunk. Also note that said code in browser.js is nontrivial, Rob or Jim should know if there's a perf impact.
Comment 16 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-11-04 12:16:25 PST
Leaving it on  by default on trunk doesn't conflict with making it preffable in 3.6, so I'm not sure why that's relevant.

If there is a perf impact for that code, we would have seen it, no?  I thought we did, in fact, and resolved it.  (COM/registry cache stuff, etc.)

What are you arguing _for_, and why?  I can't figure it out from your comments.
Comment 17 David Dahl :ddahl 2009-11-04 12:53:10 PST
Comment on attachment 410297 [details] [diff] [review]
v 1.0 Disable tab preview pref to false

This is the trunk patch.
Comment 18 David Dahl :ddahl 2009-11-04 12:54:52 PST
Created attachment 410314 [details] [diff] [review]
v 1.0.1 - patch for 1.9.2 branch
Comment 19 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-11-04 12:55:31 PST
I'm not sure why we need a trunk patch for a bug to disable it in 3.6 -- does anyone think we should disable it on trunk as well?  I haven't heard it suggested by anyone, as far as I can recall, but it may be someone's proposal indeed.
Comment 20 David Dahl :ddahl 2009-11-04 12:56:38 PST
no. I just realized I hadn't been working on 1.9.2. my bad.
Comment 21 Dão Gottwald [:dao] 2009-11-04 14:14:38 PST
(In reply to comment #16)
> Leaving it on  by default on trunk doesn't conflict with making it preffable in
> 3.6, so I'm not sure why that's relevant.

I'm saying that coverage from 3.6 users isn't needed.

> If there is a perf impact for that code, we would have seen it, no?  I thought
> we did, in fact, and resolved it.  (COM/registry cache stuff, etc.)
> 
> What are you arguing _for_, and why?  I can't figure it out from your comments.

I don't think there's a compelling reason to check wintaksbar.available and load the module, so I'm arguing for moving for removing the browser.js code from 1.9.2. I would hope that there's no perf impact anymore, but I'm not sure, so I'm deferring to Rob and Jim.
Comment 22 Jim Mathies [:jimm] 2009-11-04 14:54:39 PST
(In reply to comment #21)
> I don't think there's a compelling reason to check wintaksbar.available and
> load the module, so I'm arguing for moving for removing the browser.js code
> from 1.9.2. I would hope that there's no perf impact anymore, but I'm not sure,
> so I'm deferring to Rob and Jim.

FWIW, all perf regressions were addressed.
Comment 23 Rob Arnold [:robarnold] 2009-11-04 17:19:20 PST
(In reply to comment #22)
> (In reply to comment #21)
> > I don't think there's a compelling reason to check wintaksbar.available and
> > load the module, so I'm arguing for moving for removing the browser.js code
> > from 1.9.2. I would hope that there's no perf impact anymore, but I'm not sure,
> > so I'm deferring to Rob and Jim.
> 
> FWIW, all perf regressions were addressed.

We have no coverage on Windows 7 which is the only platform where the module is imported. There could be a regression (it's extra disk IO after all) but we don't have a way to measure it.
Comment 24 Almeida 2009-11-07 04:13:46 PST
I'm using Windows 7 right now, can I help you? Please let me know if so.
Comment 25 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2009-11-09 12:20:12 PST
According to:
https://bugzilla.mozilla.org/show_bug.cgi?id=313462#c43

AP is being removed for the 3.6 release.  Shaver's original comments guesstimate a timeframe of the beta-push 1-2 weeks after his comment on Oct 30th.  Well, we are currently neck deep into that beta-push.  I was just wondering if, when, and how deep the AP cuts will go.  Can QA get an update on this?  Thanks.
Comment 26 Theodore 2009-11-10 18:57:46 PST
This issue of spinning circles on Windows 7 seems to be fixed in Firefox 3.6 Beta 2. Tab previews now seem to work as good as in IE8.
Comment 27 Almeida 2009-11-10 19:14:30 PST
I'm already testing Firefox 3.6b2 and the spinning circles bug keeps going. I closed it, started with a new blank profile to test it again and the bug wasn't happening.

I'll keep testing it with the blank new profile, specially under Litmus' test specs, until it happens the same with that blank profile.

If not, it only happens with my own personal profile, then I'll try to find out which profile's file is causing it and attach it here.
Comment 28 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2009-11-10 19:38:16 PST
I concur with comment 27.  I still see the spinning circles on tab previews sometimes.  Granted it doesn't happen as often, but it still does happen.  I'd say we are closer to IE8 performance but not quite there yet.
Comment 29 Rob Arnold [:robarnold] 2009-11-10 20:53:42 PST
(In reply to comment #28)
> I concur with comment 27.  I still see the spinning circles on tab previews
> sometimes.  Granted it doesn't happen as often, but it still does happen.  I'd
> say we are closer to IE8 performance but not quite there yet.

But nothing related to aero peek changed between betas, right?
Comment 30 Almeida 2009-11-11 04:42:20 PST
In fact, I do. I've been the last 6 hours testing it thoroughly and the "spinning circles" still happen, even with completely loaded pages.

In this new beta, I notice something I believe is a new bug: the thumbnail previews' order isn't the same as the tabs' real order. And sometimes, if you have, i.e., 8 tabs open, the first 4 may be correct, but the last 4 are completely random.
Comment 31 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2009-11-11 05:32:52 PST
(In reply to comment #30)
> In fact, I do. I've been the last 6 hours testing it thoroughly and the
> "spinning circles" still happen, even with completely loaded pages.
> 
> In this new beta, I notice something I believe is a new bug: the thumbnail
> previews' order isn't the same as the tabs' real order. And sometimes, if you
> have, i.e., 8 tabs open, the first 4 may be correct, but the last 4 are
> completely random.

Don't have the number handy, but that's been filed.
Comment 32 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2009-11-11 05:35:52 PST
(In reply to comment #29)
> (In reply to comment #28)
> > I concur with comment 27.  I still see the spinning circles on tab previews
> > sometimes.  Granted it doesn't happen as often, but it still does happen.  I'd
> > say we are closer to IE8 performance but not quite there yet.
> But nothing related to aero peek changed between betas, right?

AFAIK nothing has changed.  Aero Peek just seems faster in B2 than it did in B1.
Comment 33 Almeida 2009-11-11 07:28:39 PST
I'm truly sorry, Kyle, didn't get what you meant.
Comment 34 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2009-11-11 07:43:38 PST
(In reply to comment #33)
> I'm truly sorry, Kyle, didn't get what you meant.

I thought the behavior you mentioned is bug 522305 and/or bug 522610.
Comment 35 Almeida 2009-11-11 07:49:27 PST
Indeed, Kyle, you're right, but as the spinning circles' issue was mentioned, I found important to point that out, as it could be a related issue. I've already commited to those issues to help fix those as well.
Comment 36 [not reading bugmail] 2009-11-11 09:03:50 PST
(In reply to comment #29)
> (In reply to comment #28)
> > I concur with comment 27.  I still see the spinning circles on tab previews
> > sometimes.  Granted it doesn't happen as often, but it still does happen.  I'd
> > say we are closer to IE8 performance but not quite there yet.
> 
> But nothing related to aero peek changed between betas, right?

I think the only thing noticed by 1.9.2B2 users was the effect of bug 521668.

(In reply to comment #26)
> This issue of spinning circles on Windows 7 seems to be fixed in Firefox 3.6
> Beta 2. Tab previews now seem to work as good as in IE8.

Bug 522262 is still valid for progress circles as well as all other UX bugs.
Comment 37 Dão Gottwald [:dao] 2009-11-11 10:20:42 PST
Can you please move your conversation somewhere else?
Comment 38 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2009-11-11 13:01:04 PST
Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f79f13a933d6
Comment 39 Tony Chung [:tchung] 2009-11-16 13:20:08 PST
Verified fix on windows 7, Mozilla/5.0 (Windows; U; Windows NT 6.1; fr; rv:1.9.2b3) Gecko/20091115 Firefox/3.6b3
Comment 40 Howie 2009-11-18 05:33:46 PST
I think disabling it is a mistake. Since installing Windows 7, Aero Peek has become very important for me. I made the switch from using Chrome to 3.6b2 because it supported, although imperfectly, previewing individual tabs. Seeing the content of the tabs isn't as important as being able to navigate directly to the tab I want, so the spinning circles are a non-issue for me and, I imagine, for anybody else who relies on Peek Heavily. At the very least, leave the code in so I can continue to enable it with each new update.
Comment 41 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2009-11-18 05:44:20 PST
It is still in the code.  You can enable it by changing the pref browser.taskbar.previews.enable in about:config from false to true.
Comment 42 Eric Shepherd [:sheppy] 2009-11-18 06:42:15 PST
For the purposes of documentation, I'm going to just add a note to the tab preview content that it's disabled by default in 3.6. Does this affect nsITaskbarWindowPreview in any way?
Comment 43 Rob Arnold [:robarnold] 2009-11-18 07:27:29 PST
(In reply to comment #42)
> For the purposes of documentation, I'm going to just add a note to the tab
> preview content that it's disabled by default in 3.6. Does this affect
> nsITaskbarWindowPreview in any way?

No, the pref only controls the frontend part of the feature. The nsITaskbar* interfaces don't check any prefs.
Comment 44 Eric Shepherd [:sheppy] 2009-11-24 13:03:30 PST
This sounds like it has no bearing at all on developer documentation then, if this is just a matter of a feature being disabled by default from the user's perspective.

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