[chromium] CCThreadProxy must initialize frameBeginTime to monotonicallyIncreasingTime rather than zero
Created attachment 133054 [details] Patch
Created attachment 133062 [details] Use other time
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.
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.
(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.
Comment on attachment 133062 [details] Use other time R=me monotonic is better for animations, but updateAnimations() on WVI currently requires currentTime()
Comment on attachment 133062 [details] Use other time Clearing flags on attachment: 133062 Committed r111585: <http://trac.webkit.org/changeset/111585>
All reviewed patches have been landed. Closing bug.
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.
(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.
(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?
(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. :)
(In reply to comment #12) > (In reply to comment #11) Woah, such a layout test already exists. ~flail hat~
(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.