Closed Bug 823091 Opened 7 years ago Closed 7 years ago

2nd tab in a window opens without animation

Categories

(Core :: XPCOM, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: avih, Assigned: vlad)

References

Details

Attachments

(1 file, 1 obsolete file)

When a window has only one tab, opening a second one sometimes happen instantly (without animation). The 3rd tab, 4th etc always open correctly with animation.

This is possibly a regression from bug 731974. I noticed it when I applied vlad's patch on its own (though didn't compare it to the pre-applied tree), and also compared m-c official nightlies from 2012-12-11 and 2012-12-19 - it happens on the latter but not the former (vlad's patch landed 2012-12-12).

A quick visual analysis suggests that when opening the 2nd tab, it animates to a deadline of (last-closed-tab-timestamp + tab-open-animation-duration). So if the 2nd tab is opened instantly after close, we'll see the full open animation, if opened quickly after close, we'll see a quicker tab open animation, and if opened a second or more after last tab close, it'll open instantly.
Status: NEW → ASSIGNED
Just noticed:
- The bug happens when using the KB to open a new tab (ctrl-t).
- The bug doesn't happen when clicking the new tab |+| button at the tabstrip.
With some logging:

0[5824e0]: [641d518] AddRefreshDriver 66fd290
0[5824e0]: [641d518] StartTimer
0[5824e0]: [641d518] scheduling callback for 16 ms (1)
------- TAB OPEN START @ 1355942374015
0[5824e0]: [641d518] precise timer last tick late by 46.738539 ms, next tick in 3 ms
0[5824e0]: [641d518] scheduling callback for 3 ms (2)
0[5824e0]: [641d518] ticking drivers...
0[5824e0]: >> TickDriver: 66fd290 (jsnow: 1355942374017725)
------- FX_TAB_ANIM_OPEN_MS 3

That's just weird.  That first tick took forever to occur (72ms!), but that wasn't the relevant one.  I'm suspecting that something isn't reporting the right values to layout for current time/animation time, because it thinks the animation needs to be over even though only 3ms have really elapsed.
Indeed, adding some more printfs, the ontransitionend event that's received tells us that we took 250ms, when we really only took 3.

Looking at some code, in nsTransitionManager: http://hg.mozilla.org/mozilla-central/annotate/1b6ab3a080d8/layout/style/nsTransitionManager.cpp#l770 the starttime of the event is set to the mostRecentRefresh from the refresh driver.  I don't really understand why -- if we're just starting a new transition, there may not have been any refreshes in a long while.  Shouldn't this be TimeStamp::Now() + delay ?

This matches what Avi is seeing in comment #0; if we had a very recent refresh, then mostRecentReferesh is accurate enough.  If I change that start time to TimeStamp::Now(), this problem goes away.
(Forgot to Cc dbaron; he's the original author.)

Note that when I think about this, TimeStamp::Now() is also probably not correct; we really want some kind of shared "now" value for all animations started during the current JS execution so that they will correctly execute in lockstep, right?
As I said to vlad on IRC:

 (1) There's value in using the refresh timestamp if people are chaining transitions by responding to transitionend events.  If we use the MostRecentRefresh(), then such chaining won't introduce extra delay.

 (2) At the time that code was originally written, the semantics of MostRecentRefresh() were different -- if the refresh driver wasn't running, it would make the refresh driver start running within the call, which would set the timestamp to Now().  I'm not sure what the semantics are today, but I suspect this may not be the only place that expects MostRecentRefresh to return a time that's actually recent (i.e., only lagging by the frame rate that we're hitting... though potentially lagging because we're actually throttling the frame rate in background tabs).
Here's a patch that's running through the tryserver now (https://tbpl.mozilla.org/?tree=Try&rev=fadac9dbe2a8).  It brings back the old behaviour of making sure the timer is started whenenver we query for the most recent refresh.  In the case of the "precise" (foreground tab/chrome) timer, this means that the most recent refresh becomes "now" if we're just starting the timer, and we schedule a new one for 16ms later.

This fixes this bug.
Comment on attachment 694026 [details] [diff] [review]
start timer when querying recent refresh time

(Ugh, ignore the tabbrowser.xml hunk in here.)

The rest seems to be happy on the try server and fixes the issue here.
Attachment #694026 - Flags: review?(dbaron)
Comment on attachment 694026 [details] [diff] [review]
start timer when querying recent refresh time

(Trying bz and roc just in case one of them is able to get to this before the holidays; ignore the tabbrowser.xml changes!)
Attachment #694026 - Flags: review?(roc)
Attachment #694026 - Flags: review?(bzbarsky)
I'm a little worried this patch might be a little too big of a hammer (and might regress things that have been fixed, e.g., stuff related to refresh driver not running while bfcached).  I think bzbarsky is probably more on top of that stuff than I am and would be a better reviewer than I am, but if he's not around I could give it a try.
Comment on attachment 694026 [details] [diff] [review]
start timer when querying recent refresh time

The bfcache bits should be fine; we check mFrozen in EnsureTimerStarted.

What I'm confused about is why we tick the inactive timer ASAP but delay the precise timer by one mRateDuration....  Why not delay both?
Fewer things to change from the current code, but you're right, I should just delay the first tick of both appropriately.  I'll update.
Attached patch updated patcSplinter Review
Updated with above comment.  Makes me want to refactor StartTimer so that it's one function with the TimerTick/TimerTickOne as a parameter...
https://hg.mozilla.org/mozilla-central/rev/aa8b7e9cc8e5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment on attachment 694026 [details] [diff] [review]
start timer when querying recent refresh time

This patch (and its review requests) are obsolete, right?
Attachment #694026 - Attachment is obsolete: true
Attachment #694026 - Flags: review?(dbaron)
Attachment #694026 - Flags: review?(bzbarsky)
You need to log in before you can comment on or make changes to this bug.