Closed Bug 525475 Opened 15 years ago Closed 15 years ago

disable Aero Peek tab preview for Firefox 3.6

Categories

(Firefox :: Shell Integration, defect, P2)

Product:
Firefox
Component:
Shell Integration
Version:
3.6 Branch
Platform:
x86
Windows 7
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
status1.9.2 --- beta3-fixed

People

(Reporter: shaver, Assigned: ddahl)

References

Details

(Keywords: dev-doc-complete, verified1.9.2)

Attachments

(2 files)

v 1.0 Disable tab preview pref to false
830 bytes, patch
robarnold
: review+
Details | Diff | Splinter Review
v 1.0.1 - patch for 1.9.2 branch
579 bytes, patch
robarnold
: review+
Details | Diff | Splinter Review 
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).
Flags: blocking-firefox3.6+
Is it going to be shipped pref'd off or is the code going to be removed?
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.)
(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?
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.
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?
(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.
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.
(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
Assignee: nobody → ddahl
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.
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.
Thanks Dale, that summary really helped. I think we're ok to just disable the feature by default, but still ship the bits.
Just a pref change
Attachment #410297 - Flags: review?(tellrob)
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.
Attachment #410297 - Flags: review?(tellrob) → review+
Let's leave it for the power users and others who will help us get lots of coverage here.
Keywords: checkin-needed
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.
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 on attachment 410297 [details] [diff] [review]
v 1.0 Disable tab preview pref to false

This is the trunk patch.

        Attachment #410314 -
        Flags: review?(tellrob)

    
    

    
  
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.

    
    

    
  
no. I just realized I hadn't been working on 1.9.2. my bad.

    
  
Keywords: checkin-needed

    
    

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

    
    

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

    
    

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

        Attachment #410314 -
        Flags: review?(tellrob) → review+
Whiteboard: [can land]

    
    

    
  
I'm using Windows 7 right now, can I help you? Please let me know if so.

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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.

    
    

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

    
    

    
  
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.

    
    

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

    
    

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

    
    

    
  
I'm truly sorry, Kyle, didn't get what you meant.

    
    

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

    
    

    
  
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.

    
    

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

    
    

    
  
Can you please move your conversation somewhere else?

    
    

    
  
Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f79f13a933d6
Status: NEW → RESOLVED
Closed: 15 years ago

          status1.9.2:
          --- → final-fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [can land]
Version: unspecified → 3.6 Branch

    
    

    
  
Verified fix on windows 7, Mozilla/5.0 (Windows; U; Windows NT 6.1; fr; rv:1.9.2b3) Gecko/20091115 Firefox/3.6b3
Keywords: verified1.9.2

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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?
Keywords: dev-doc-needed

    
    

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

    
    

    
  
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.

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: