Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clamp the presentation time to a usable range #21514

Merged
merged 1 commit into from
Jan 31, 2020

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Jan 30, 2020

If the time container is seeked to a large enough value, we will end up
truncating it to SMILTime::Latest(), which is the largest value that
isn't one of the two special values ("indefinite" and "unresolved").
When trying to derive other values from this value - like if we have an
interval begin at it - we can end up in a loop since any the result of
any additions will yield the same value, leading to the element being
rescheduled at the same point in time, hanging UpdateIntervals().
This mechanism can also be used to implement the "once" animation-policy
in a slightly nicer way. This will be done as a follow-up.

Bug: 1039886
Change-Id: If13d7d7d3c44c4f586d15852eb05105879f44918
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2030885
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#737260}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review process for this patch is being conducted in the Chromium project.

@stephenmcgruer
Copy link
Contributor

The Firefox runs in TaskCluster failed due to #21529.

However, weirdly, neither Chrome nor Safari appear to have seen this test when looking at the change. Safari reports not supporting them:

ERROR Unsupported test type crashtest for product safari

But Chrome just reports no affected tests?

 0:15.17 INFO Identified 0 affected tests
...
 0:37.97 INFO Running crashtest tests
 0:37.97 INFO No crashtest tests to run

@stephenmcgruer
Copy link
Contributor

Figured it out; #21531 should fix the logic to detect when crashtests are changed and run them

If the time container is seeked to a large enough value, we will end up
truncating it to SMILTime::Latest(), which is the largest value that
isn't one of the two special values ("indefinite" and "unresolved").
When trying to derive other values from this value - like if we have an
interval begin at it - we can end up in a loop since any the result of
any additions will yield the same value, leading to the element being
rescheduled at the same point in time, hanging UpdateIntervals().
This mechanism can also be used to implement the "once" animation-policy
in a slightly nicer way. This will be done as a follow-up.

Bug: 1039886
Change-Id: If13d7d7d3c44c4f586d15852eb05105879f44918
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2030885
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#737260}
@stephenmcgruer stephenmcgruer force-pushed the chromium-export-cl-2030885 branch from 06ffc72 to 0432f23 Compare January 31, 2020 19:31
@chromium-wpt-export-bot chromium-wpt-export-bot merged commit 0db6154 into master Jan 31, 2020
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-cl-2030885 branch January 31, 2020 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants