Thanks for updating. Now this looks much better. What I was in fact suggesting is, something normalization of scroll timeline's time duration.

So for example, given that there are two different scrollers, one is 100px scrollable height and 10px scroll port, the other is 100px scrollable height and 50px height. With these scrollers, scrollTop=10px in the first one is at 1/10 position, scrollTop=5px in the second one is also at 1/10, can we represent both as a single same value?I believe we can.

Say, if we have a fake TimingParams::mDuration for all scroll timelines, say if the duration is 100, Then GetCurrentTimeAsDuration for scroll timelines for both cases would return same 10? Then GetCurrentTimeAsDouble would return 0.1.

So a question is what the reasonable fake TimingParam::mDuration is. Maybe INT64_MAX - 1, I am less sure though? (Given that INT64_MAX is a special value).

In D129101#4247970, @hiro wrote:

So for example, given that there are two different scrollers, one is 100px scrollable height and 10px scroll port, the other is 100px scrollable height and 50px height. With these scrollers, scrollTop=10px in the first one is at 1/10 position, scrollTop=5px in the second one is also at 1/10, can we represent both as a single same value?I believe we can.

The formula of layout scroll range is: scrollable height - scroll port height, so let me update the first case to use 110px scrollable height.

Let's calculate this again:
Case 1:
scrollable rect height: 110px;
scroll port height: 10px;
so it's the scroll range height is: 110 - 10 = 100px;
With position: 10px, the progress is 10/100 = 0.1

Case 2:
scrollable rect height: 100px;
scroll port height: 50px;
so it's the scroll range height is: 100 - 50 = 50px;
With position: 5px, the progress is 5/50 = 0.1

Say, if we have a fake TimingParams::mDuration for all scroll timelines, say if the duration is 100, Then GetCurrentTimeAsDuration for scroll timelines for both cases would return same 10? Then GetCurrentTimeAsDouble would return 0.1.

So a question is what the reasonable fake TimingParam::mDuration is. Maybe INT64_MAX - 1, I am less sure though? (Given that INT64_MAX is a special value).

So, we keep TimingParam::mDuration be always a fixed value for scroll-timeline (e.g. let's use INT64_MAX - 1 now),
and let GetCurrentTimeAsDuration() return 0.1 * (INT64_MAX - 1), for Case 1 and Case 2. Therefore, we don't have to update AnimationEffect::GetLocalTime() because GetCurrentTimeAsDuration() work properly for scroll-timeline.

INT64_MAX represents the forever timestamp, so using INT64_MAX - 1 is ok to me now because we don't have the progress which is more than 100% for scroll-timeline. Therefore, we don't let GetCurrentTimeAsDuration() overflow.

Let's try this normalization. Thanks.

In D129101#4247970, @hiro wrote:

Say, if we have a fake TimingParams::mDuration for all scroll timelines, say if the duration is 100, Then GetCurrentTimeAsDuration for scroll timelines for both cases would return same 10? Then GetCurrentTimeAsDouble would return 0.1.

Well, GetCurrentTimeAsDouble() just converts the returned value of GetCurrentTimeAsDuration() into a double value. Why is it 0.1 in this case?

In D129101#4256289, @boris wrote:

INT64_MAX represents the forever timestamp, so using INT64_MAX - 1 is ok to me now because we don't have the progress which is more than 100% for scroll-timeline. Therefore, we don't let GetCurrentTimeAsDuration() overflow.

Oops. Need to make sure the animation playback rate shouldn't be more than 1.0. Otherwise, we may overflow. It seems only web animation APIs can update the playback rate, so it's fine for now.

In D129101#4256297, @boris wrote:

In D129101#4247970, @hiro wrote:

Say, if we have a fake TimingParams::mDuration for all scroll timelines, say if the duration is 100, Then GetCurrentTimeAsDuration for scroll timelines for both cases would return same 10? Then GetCurrentTimeAsDouble would return 0.1.

Well, GetCurrentTimeAsDouble() just converts the returned value of GetCurrentTimeAsDuration() into a double value. Why is it 0.1 in this case?

I am sorry for the confusion. I meant 0.1 portion of the entire duration. Anyway my point is it's a normalized duration.

In D129101#4256297, @boris wrote:

In D129101#4247970, @hiro wrote:

Say, if we have a fake TimingParams::mDuration for all scroll timelines, say if the duration is 100, Then GetCurrentTimeAsDuration for scroll timelines for both cases would return same 10? Then GetCurrentTimeAsDouble would return 0.1.

Well, GetCurrentTimeAsDouble() just converts the returned value of GetCurrentTimeAsDuration() into a double value. Why is it 0.1 in this case?

It seems GetCurrentTimeAsDouble() converts the millisecond to microsecond, so it may be 100000 in this example. I will double-check this. If it's possible to get a very large value, we may use a different fixed duration (e.g. 1000).

Code analysis found 1 defect in the diff 508051:

• 1 defect found by clang-tidy

You can run this analysis locally with:

• ./mach static-analysis check --outgoing (C/C++)

For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply -p0).

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

I've been assuming if we tweak AnimationEffect::SpecifiedTiming for scroll linked animations, we don't need to tweak in each functions in KeyframeEffect?

In D129101#4265179, @hiro wrote:

I've been assuming if we tweak AnimationEffect::SpecifiedTiming for scroll linked animations, we don't need to tweak in each functions in KeyframeEffect?

So something like:

AnimationEffect::SpecifiedTiming() {
  if (mAnimation->UsingScrollTimeline()) {
    return mAnimation->GetTimeline()->GenerateTiming();
  }
  return mTiming;
}

It's worth to try. :)

In D129101#4265662, @boris wrote:

In D129101#4265179, @hiro wrote:

I've been assuming if we tweak AnimationEffect::SpecifiedTiming for scroll linked animations, we don't need to tweak in each functions in KeyframeEffect?

So something like:

AnimationEffect::SpecifiedTiming() {
  if (mAnimation->UsingScrollTimeline()) {
    return mAnimation->GetTimeline()->GenerateTiming();
  }
  return mTiming;
}

It's worth to try. :)

I am assuming the GenerateTiming() could be a constexpr and the name could be simpler something like ComputedTiming or some such.

Basically this looks good. I'd like to review again with the change that GetCurrentTimeAsDuration returns 1 in unscrollable scroller cases. Thanks!

This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other. Tip: this Firefox add-on makes it easy!

In D129101#4277430, @hiro wrote:

r+ for now, this change should have at least one RTL test case.

I added the test (using vertical-rl) into the last patch already.

tests are in part 8