-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[web_benchmarks] Make package compatible with Chromium v89+ #518
Conversation
Thanks for the contribution!
This PR will need tests, as this isn't a test-exempt change. |
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Are you still planning on updating this PR with tests? |
Thank you for your contribution. I'm going to close this PR for now since there are outstanding comments, just to get this off our PR review queue. Please don't hesitate to submit a new PR if you have the time to address the review comments. Thanks! |
@amanv8060 I'm attempting to bring this up to speed with the latest main and have it deployed. Could you edit this PR and allow permissions to the maintainers so I can push the changes I did? Thanks! |
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
(Never mind, my changes were disallowed because the PR was closed, not because it was misconfigured!) |
Revisited this. We want to update the version of Chrome used by tests to see if it fixes this problem: flutter/flutter#98278 I've refreshed this PR, and added tests so this can land. PTAL @stuartmorgan, and @yjbanov to see if this does what we want! Note: To fix the problem linked above, this code needs to be back-ported to the copy in flutter/flutter. |
packages/web_benchmarks/test/src/browser_test_json_samples.dart
Outdated
Show resolved
Hide resolved
This isn't working currently On gallery: Stdout
Inside the package : `flutter test testing/web_benchmarks_test.dart` or `dart test testing/web_benchmarks_test.dart --chain-stack-traces` Stdout
flutter doctor -v
|
- Update changelog - Change WebFrameWidgetImpl::UpdateLifecycle to WebFrameWidgetImpl::updateAllLifecyclePhases based upon https://github.com/chromium/chromium/blob/master/third_party/blink/public/web/web_widget.h#L113-L117 Signed-off-by: Aman Verma <[email protected]>
@amanv8060 very interesting. I'm looking at the implementation of the code that you sent, and it seems that the "WebFrameWidgetImpl::UpdateLifecycle" method is the only leaving traces behind. However, It's not logging the 'requested_update' passed in! See code of the implementation. Passing @yjbanov does this mean that we can't reliably detect the UpdateAllLifecyclePhases event? Do we need something else? Do we need to ask chromium to log the |
@amanv8060 I've ran the tests both in this repo and in flutter gallery, and they both seem to work? (I'm running on Linux + Chrome 98) flutter/packages: web_benchmarks/testing$ flutter test testing
00:03 +0: Can run a web benchmark
Launching Chrome.
Launching Google Chrome 98.0.4758.102
Waiting for the benchmark to report benchmark profile.
[CHROME]: [0215/133905.676259:ERROR:socket_posix.cc(150)] bind() failed: Address already in use (98)
[CHROME]:
[CHROME]: DevTools listening on ws://[::1]:10000/devtools/browser/7d98943f-6aa3-48c0-9c79-4ed15696cb57
Connecting to DevTools: ws://localhost:10000/devtools/page/CE0BFB50A4F770847AB7EF8A7E80F659
Connected to Chrome tab: (http://localhost:9999/index.html)
Launching benchmark "scroll"
Extracted 299 measured frames.
Skipped 1 non-measured frames.
Launching benchmark "page"
[APP] Testing round 0...
[APP] Testing round 1...
[APP] Testing round 2...
[APP] Testing round 3...
[APP] Testing round 4...
[APP] Testing round 5...
[APP] Testing round 6...
[APP] Testing round 7...
[APP] Testing round 8...
[APP] Testing round 9...
Extracted 474 measured frames.
Skipped 0 non-measured frames.
Launching benchmark "tap"
[APP] Testing round 0...
[APP] Testing round 1...
[APP] Testing round 2...
[APP] Testing round 3...
[APP] Testing round 4...
[APP] Testing round 5...
[APP] Testing round 6...
[APP] Testing round 7...
[APP] Testing round 8...
[APP] Testing round 9...
Extracted 299 measured frames.
Skipped 0 non-measured frames.
Received profile data
00:26 +1: All tests passed! flutter/gallery: test_benchmarks$ git remote -v
origin [email protected]:flutter/gallery.git (fetch)
origin [email protected]:flutter/gallery.git (push)
$ flutter test test_benchmarks
Warning: You are using these overridden dependencies:
! web_benchmarks 0.0.7 from path ../packages/packages/web_benchmarks
Running "flutter pub get" in flutter_gallery... 1,265ms
00:12 +0: /usr/local/google/home/dit/github/flutter_gallery/test_benchmarks/benchmarks_test.dart: Can run a web benchmark
Shell: Starting web benchmark tests ...
01:56 +1: /usr/local/google/home/dit/github/flutter_gallery/test_benchmarks/web_bundle_size_test.dart: Web Compile bundle size
Shell:
Shell: Building without sound null safety
Shell: For more information see https://dart.dev/null-safety/unsound-null-safety
Shell:
Shell: Compiling lib/main.dart for the Web... 103.2s
01:57 +1: /usr/local/google/home/dit/github/flutter_gallery/test_benchmarks/web_bundle_size_test.dart: Web Compile bundle size
Shell: 4100 /usr/local/google/home/dit/github/flutter_gallery/build/web/main.dart.js
Shell: 1008 /usr/local/google/home/dit/github/flutter_gallery/build/web/main.dart.js.gz
01:57 +2: All tests passed! Can you please validate if the code works for you or not? You seem more familiar with these packages than I am; I might be missing some setup or something. |
This comment was marked as resolved.
This comment was marked as resolved.
This passes locally for me with Chrome 98.0.4758.102. |
(Following instructions here) Base position: 950365
I've updated the Cirrus yaml so "Can run a web benchmark" test runs both for Chrome 84 and Chrome 98. We can remove the v84 from the test matrix once we remove support for v84 on the code. At least for now, we still have a smoke test for the older version of the code. |
currently, benchmark tests are being skipped in the gallery, looks like only the bundle size test was actually that was run. ( inferring from time difference in the log). |
@amanv8060 ah! I didn't realize that there was a
The problem seems to be that some
I am not sure if the results make sense, but at least the gallery benchmarks code seems to be running. I can send a PR to flutter/gallery after this one lands. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Just to be sure, tested on Chromium 97, which passed here. (I added, then removed 97 from the test matrix in the last couple of commits). Landing this as soon as it goes green! |
This pr Updates the event names with changes introduced in chromium v89
updateAllLifecyclePhases calls UpdateLifecycle under the hood
https://github.com/chromium/chromium/blob/c4d3c31083a2e1481253ff2d24298a1dfe19c754/third_party/blink/public/web/web_widget.h#L109
List which issues are fixed by this PR. You must list at least one issue.
flutter/flutter#94027
Another issue that relies on this: flutter/gallery#463
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.