Closed Bug 631932 Opened 9 years ago Closed 9 years ago

Aero Peek tab previews are always enabled after entering and exiting Panorama

Categories

(Firefox Graveyard :: Panorama, defect)

x86
Windows 7
defect
Not set

Tracking

(blocking2.0 final+)

VERIFIED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: rain1, Assigned: jimm)

References

Details

(Whiteboard: [hardblocker])

Attachments

(1 file)

STR:
1. Disable Aero Peek tab previews.
2. Enter Panorama.
3. Exit Panorama.

The tab previews always remain enabled.

Pretty sure the bug is at https://hg.mozilla.org/mozilla-central/rev/953be21166fe#l1.57 -- you shouldn't be changing enabled and should probably instead have a separate _tempEnabled bool.
blocking2.0: --- → ?
Oops, I'll get a fix in this week. Thanks Sid. Drivers, this should block final.
Assignee: nobody → jmathies
Attached patch patchSplinter Review
This seems to fix it.
Attachment #510263 - Flags: review?(dao)
blocking2.0: ? → final+
Whiteboard: [hardblocker]
Comment on attachment 510263 [details] [diff] [review]
patch

What if checkPreviewCount is called while in panorama? What if panorama is shown/hidden in two windows?
(In reply to comment #3)
> Comment on attachment 510263 [details] [diff] [review]
> patch
> 
> What if checkPreviewCount is called while in panorama? What if panorama is
> shown/hidden in two windows?

I didn't see any issues testing this, with the pref off, tab previews stay off, with it on, we get per window tab previews disabled when panorama is enabled. Enabling and disabling panorama in multiple windows doesn't break anything.  

checkPreviewCount: function () {
    if (this.previews.length > this.maxpreviews)
      this.enabled = false;
    else
      this.enabled = this._prefenabled;
  },

If _prefenabled is false, enabled will always be false.
(In reply to comment #4)
> If _prefenabled is false, enabled will always be false.

And if _prefenabled is true?
(In reply to comment #5)
> (In reply to comment #4)
> > If _prefenabled is false, enabled will always be false.
> 
> And if _prefenabled is true?

Doesn't appear to be an issue. That sets AeroPeeks's this.enabled to this._prefenabled, which calls the enabled setter. The setter then does this:

    if (this._enabled == enable)
      return;


So nothing changes. Is there some specific STR case you see you would like me to test?
Comment on attachment 510263 [details] [diff] [review]
patch

I was confusing TabWindow.prototype.enabled with AeroPeek.enabled.
Attachment #510263 - Flags: review?(dao) → review+
Whiteboard: [hardblocker] → [hardblocker][has patch]
http://hg.mozilla.org/mozilla-central/rev/49fde7346226
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [hardblocker][has patch] → [hardblocker]
Duplicate of this bug: 631985
Duplicate of this bug: 631935
Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110215 Firefox/4.0b12pre

Verified issue and it's no longer reproducible.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.