This repository was archived by the owner on Feb 25, 2025. It is now read-only.
[fuchsia] set vsync_offset based on config file#18006
Merged
iskakaushik merged 13 commits intoflutter:masterfrom May 1, 2020
Merged
[fuchsia] set vsync_offset based on config file#18006iskakaushik merged 13 commits intoflutter:masterfrom
iskakaushik merged 13 commits intoflutter:masterfrom
Conversation
In order to better support different products on Fuchsia, we should change performance-sensitive attributes based on config files passed in. This change does so for `vsync_offset`.
dreveman
reviewed
Apr 28, 2020
iskakaushik
reviewed
Apr 28, 2020
iskakaushik
reviewed
Apr 30, 2020
Contributor
iskakaushik
left a comment
iskakaushik
left a comment
There was a problem hiding this comment.
Looks good overall left a couple of comments regarding names, style and additional tests.
|
|
||
| class ProductConfigurationTest : public testing::Test {}; | ||
|
|
||
| TEST_F(ProductConfigurationTest, ValidVsyncOffset) { |
Contributor
There was a problem hiding this comment.
can you add the following tests:
- Invalid json string.
- Negative vsync offset.
- Unexpected vsync offset (larger than 1 second for example deserves a warning).
| zx_handle_t session_present_handle, | ||
| flutter::TaskRunners task_runners) | ||
| flutter::TaskRunners task_runners, | ||
| ProductConfiguration product_config) |
Contributor
There was a problem hiding this comment.
I would take a float/double in this constructor. Its generally a good dependency injection practice for consumers of a configuration to take specific parameters.
Contributor
Author
There was a problem hiding this comment.
Sounds good, but why a floating point number and not an integer/fml::TimeDelta?
Contributor
Author
There was a problem hiding this comment.
Changed it to take an fml::TimeDelta since that's the type of vsync_offset_
arbreng
approved these changes
Apr 30, 2020
Contributor
arbreng
left a comment
arbreng
left a comment
There was a problem hiding this comment.
LGTM after kaushiks comments are addressed
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
May 1, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
May 1, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
May 1, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
May 1, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
May 1, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
May 1, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
May 2, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
May 5, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In order to better support different products on Fuchsia, we should
change performance-sensitive attributes based on config files passed in.
This change does so for
vsync_offset. If the file does not exist, or we cannotsuccessfully parse it, there is no change in behavior.