[chromium] CCThreadProxy must initialize frameBeginTime to monotonicallyIncreasingTime rather than zero

• Component: New Bugs
• Status: RESOLVED FIXED

[Nat Duca]

[chromium] CCThreadProxy must initialize frameBeginTime to monotonicallyIncreasingTime rather than zero

[Nat Duca]

Created attachment 133054 [details]
Patch

[Nat Duca]

Created attachment 133062 [details]
Use other time

[W. James MacLean]

Why change to currentTime?

When I tried the first patch with monotonic time, slashdot.org work great.

With currentTime I get weird jankiness near the bottom of the page when slashdot.org tries to load more, thus extending the page length.

[Nat Duca]

I'm surprised at this causing jank -- I wonder if its coincidental?

The issue here is that same interface drives the JS requestAnimationFrame which still requires regular timestamps. I experimented with passing both, but because our CCLTH->WVI callback path is certifiably insane, that is hard to make work right.

How about we fix the correctness bug and then come back and do tuning and/or dual timestamps as a followon patch.

Or, we could do this:
https://bugs.webkit.org/show_bug.cgi?id=66683

Which would then allow our updateAnimation flow to use monotonic clock as well.

[W. James MacLean]

(In reply to comment #4)
> I'm surprised at this causing jank -- I wonder if its coincidental?

Not sure, it only seems to occur at that spot on the page when slashdot tries to load more content ... before and after it's fine, but it's goes all weird during the loading.

> The issue here is that same interface drives the JS requestAnimationFrame which still requires regular timestamps. I experimented with passing both, but because our CCLTH->WVI callback path is certifiably insane, that is hard to make work right.
> 
> How about we fix the correctness bug and then come back and do tuning and/or dual timestamps as a followon patch.
> 
> Or, we could do this:
> https://bugs.webkit.org/show_bug.cgi?id=66683
> 
> Which would then allow our updateAnimation flow to use monotonic clock as well.

I'm good with either, as I don't (at the moment) have a clear opinion as to which route is better. We can file a follow-on bug for the jankiness.

[James Robinson]

Comment on attachment 133062 [details]
Use other time

R=me

monotonic is better for animations, but updateAnimations() on WVI currently requires currentTime()

[WebKit Review Bot]

Comment on attachment 133062 [details]
Use other time

Clearing flags on attachment: 133062

Committed r111585: <http://trac.webkit.org/changeset/111585>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Gentilcore]

In the chromium android port (at least) requestAnimationFrame was broken so that the timestamp passed to the callback was always zero. This patch fixed it up.

If this wasn't a platform specific breakage, I just wanted to double check that some upstream layout tests were failing as well as a result of the broken callback param. If not, we should add a test that verifies a non-zero RAF callback param along with this fix.

[Nat Duca]

(In reply to comment #9)
> ... is this tested

No it isn't. Layout tests may not be the correct way to test this, however. Unit test incoming.

[Tony Gentilcore]

(In reply to comment #10)
> (In reply to comment #9)
> > ... is this tested
> 
> No it isn't. Layout tests may not be the correct way to test this, however. Unit test incoming.

Nice.

Just thinking about the params to the RAF callback... since that is exposed to the web platform, can't there be at least one layout test for it?

[Nat Duca]

(In reply to comment #11)
> (In reply to comment #10)
Probs could be! I've got a unit test on the way, maybe I"ll get that filed, you can r- it and say "make a layout test too" and then I'll try to get to one of those opportunistically. :)

[Nat Duca]

(In reply to comment #12)
> (In reply to comment #11)
Woah, such a layout test already exists. ~flail hat~

[Nat Duca]

(In reply to comment #13)
> Woah, such a layout test already exists. ~flail hat~

https://bugs.webkit.org/show_bug.cgi?id=82236

The reason this slipped passed layout tests is because the layout tests run without threaded mode flags. I know jamesr has wanted to fix this for a while.