-
Notifications
You must be signed in to change notification settings - Fork 28.9k
fix duration event of timeline summary #47742
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
fix duration event of timeline summary #47742
Conversation
relation to #47771 |
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.
The PR itself looks good. I'm more concerned if we can have a Dart change like https://dart-review.googlesource.com/c/sdk/+/127921 but none of our framework tests would fail to notify that such Dart change is in some way breaking the Flutter framework. I also didn't notice any significant change in https://flutter-dashboard.appspot.com/benchmarks.html . @goderbauer : do you know if there's any integration test that's supposed to catch such Dart changes?
https://dart-review.googlesource.com/c/sdk/+/127921, it has merged in out own engine. |
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.
@yuanhuihui : ah, you're right! Flutter framework is using a 7-day old engine right before your commit has been merged into the Dart SDK Github repo:
- Flutter framework is using engine flutter/engine@42bb7c9 with a commit date of Dec 21, 2019, 7:27 AM GMT+8
- https://dart-review.googlesource.com/c/sdk/+/127921 has been in Github as dart-lang/sdk@006f611 on Dec 21, 2019, 7:37 AM GMT+8
LGTM to the updated PR.
CC @franciscojma86 : I think the recent post-submit failures of the engine roll that you reverted is related to this PR. The engine roll probably should be paused until the landing of this PR.
@liyuqian yeah I stopped it yesterday. I was trying to figure out how to fix this so happy to see this PR :) |
The build is green again @yuanhuihui, is this ok to land? |
I'll land to unblock the roll. Will revert if needed. |
This seems to break the build
Will revert for now |
This reverts commit e43fd1c.
This reverts commit 50d4212.
This PR needs to include a new hash in |
It looks like flutter/engine@5a730c6 rolled into Engine cleanly so using that specific hash should work to get a clean roll into Framework. |
In my own engine, we have fix flutter_gallery. Because of using a 7-day old engine, this timeline_summary PR failed to merge before. So I plan to submit a fixing flutter_gallery PR after the timeline_summary PR have merged. It seems that you have discovered and solved this problem now. |
Need to modify duration event to beginEnd event, because of PR https://dart-review.googlesource.com/c/sdk/+/127921