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

Enable hot mode by default. --no-hot disables it #5794

Merged
merged 1 commit into from
Sep 13, 2016
Merged

Enable hot mode by default. --no-hot disables it #5794

merged 1 commit into from
Sep 13, 2016

Conversation

johnmccutchan
Copy link
Contributor

@sethladd
Copy link
Contributor

sethladd commented Sep 9, 2016

How does this impact our internal build system? (if at all)

@yjbanov
Copy link
Contributor

yjbanov commented Sep 9, 2016

Could you please test dev/benchmarks/complex_layout with the following commands?

# This command should exit automatically immediately after the app starts and
# it should print startup metrics to the console. If it runs in the resident mode
# then something's wrong
flutter run --profile --trace-startup

# This command just checks that driver tests still function after this change
flutter drive --profile --trace-startup -t test_driver/scroll_perf.dart

Thanks.

@sethladd
Copy link
Contributor

sethladd commented Sep 9, 2016

Closes #5280

@eseidelGoogle
Copy link
Contributor

I don't know how to evaluate this change. I'm suuuuuper excited to see it. Just no clue how to weigh yes/no.

@Hixie
Copy link
Contributor

Hixie commented Sep 9, 2016

I support this change but we should make sure we test hot reload so it doesn't break again. (Ahem.)

@eseidelGoogle
Copy link
Contributor

eseidelGoogle commented Sep 9, 2016

Does the Atom plugin need to know about this change? (to remove the checkbox it has now). @devoncarew

Related: Will current atom break if it keeps specifying --hot, or will that be silently ignored?

@devoncarew
Copy link
Member

I'll double check, but I believe the logic is written to go by what the daemon protocol returns to us when we launch an app, not by what we send in as the setting.

@johnmccutchan
Copy link
Contributor Author

@yjbanov --profile mode doesn't support hot mode so those two commands will still use the existing runner.

@Hixie I have a benchmark that can act as an integration test of hot reload and hot restart.

@eseidelGoogle I'll hold off on pulling the switch until you give me the OK. I created this CL to kick start this conversation.

@eseidelGoogle
Copy link
Contributor

I suspect we should land this on a Monday AM, instead of a Friday PM. But
honestly I'm hugely in favor of this idea. You have my support. I'd like
us to try and fail and rollout if needed. So lgtm, just don't let that
override others.

Please communicate this to at least the internal list once you've landed.
Thanks!

On Fri, Sep 9, 2016 at 2:51 PM, John McCutchan notifications@github.com
wrote:

@yjbanov https://github.com/yjbanov --profile mode doesn't support hot
mode so those two commands will still use the existing runner.

@Hixie https://github.com/Hixie I have a benchmark that can act as an
integration test of hot reload and hot restart.

@eseidelGoogle https://github.com/eseidelGoogle I'll hold off on
pulling the switch until you give me the OK. I created this CL to kick
start this conversation.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5794 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ALTvi8VrzdA3LZv8le2lUltsKh7BhFDkks5qodTmgaJpZM4J5ZSz
.

@johnmccutchan
Copy link
Contributor Author

PTAL. I'd like to land this today and see what starts to burn.

@sethladd
Copy link
Contributor

Yes, let's turn this on.

@dragostis
Copy link
Contributor

LGTM.

@johnmccutchan
Copy link
Contributor Author

Just finishing the benchmark / integration test CL before landing this.

@johnmccutchan johnmccutchan merged commit fd50ccd into flutter:master Sep 13, 2016
rmacnak-google added a commit that referenced this pull request Jul 19, 2018
55b423f Ensure assistiveTechnologyEnabled is initialized when the android view is set up (#5793)
3054f31 Add touch events to the platform views method channel API. (#5796)
91c16af Revert "Roll Dart to 937ee2e8ca4b76499e24cd463f07bfb736bccd74. (#5745)" (#5799)
1a66f89 Roll Dart to 937ee2e8ca4b76499e24cd463f07bfb736bccd74. (#5745)
8d046a7 Revert "Roll src/third_party/skia 9e0d7e4072e4..8f8bf8880d9d (32 commits) (#5794)" (#5797)
e8d172c Revert "Fix Dart 2 reload when running from a snapshot instead of platform.dill. (#5792)" (#5795)
624cf7e Fix Dart 2 reload when running from a snapshot instead of platform.dill. (#5792)
16da471 Roll src/third_party/skia 9e0d7e4072e4..8f8bf8880d9d (32 commits) (#5794)
3fe63fd Add presubmit check for engine to flutter roll (#5790)
DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request Jul 19, 2018
55b423f Ensure assistiveTechnologyEnabled is initialized when the android view is set up (flutter#5793)
3054f31 Add touch events to the platform views method channel API. (flutter#5796)
91c16af Revert "Roll Dart to 937ee2e8ca4b76499e24cd463f07bfb736bccd74. (flutter#5745)" (flutter#5799)
1a66f89 Roll Dart to 937ee2e8ca4b76499e24cd463f07bfb736bccd74. (flutter#5745)
8d046a7 Revert "Roll src/third_party/skia 9e0d7e4072e4..8f8bf8880d9d (32 commits) (flutter#5794)" (flutter#5797)
e8d172c Revert "Fix Dart 2 reload when running from a snapshot instead of platform.dill. (flutter#5792)" (flutter#5795)
624cf7e Fix Dart 2 reload when running from a snapshot instead of platform.dill. (flutter#5792)
16da471 Roll src/third_party/skia 9e0d7e4072e4..8f8bf8880d9d (32 commits) (flutter#5794)
3fe63fd Add presubmit check for engine to flutter roll (flutter#5790)
@coveralls
Copy link

Coverage Status

Coverage remained the same at 71.675% when pulling d6f9afc on johnmccutchan:enable_hot_mode_by_default into 157ffaa on flutter:master.

@eseidelGoogle
Copy link
Contributor

I'm very confused as to why @coveralls is commenting on a 2 year old PR.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants