From 01c28667802ebd82db875d84599f99695524a413 Mon Sep 17 00:00:00 2001 From: Jacob MacDonald Date: Mon, 10 Feb 2025 11:58:22 -0800 Subject: [PATCH] Return more eagerly when toggling service extensions (#162774) Fixes https://github.com/flutter/devtools/issues/8497 When handling requests to enable or disable service extensions, we currently wait until the next frame is rendered before responding to the request. If the app is hidden this will not happen until the app is unhidden, or a frame is scheduled for some other reason. This fix does not await the next frame before responding - which I think is reasonable - but feel free to push back if there is a reason to not do it that way. Updated tests to expect the response before actually rendering the next frame. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --- .../flutter/lib/src/rendering/binding.dart | 53 ++++++++++++------- .../foundation/service_extensions_test.dart | 14 ++--- 2 files changed, 40 insertions(+), 27 deletions(-) diff --git a/packages/flutter/lib/src/rendering/binding.dart b/packages/flutter/lib/src/rendering/binding.dart index c59267961a..3e6877186f 100644 --- a/packages/flutter/lib/src/rendering/binding.dart +++ b/packages/flutter/lib/src/rendering/binding.dart @@ -9,6 +9,7 @@ /// @docImport 'layer.dart'; library; +import 'dart:async'; import 'dart:ui' as ui show PictureRecorder, SceneBuilder, SemanticsUpdate; import 'package:flutter/foundation.dart'; @@ -85,43 +86,49 @@ mixin RendererBinding setter: (bool value) async { if (debugInvertOversizedImages != value) { debugInvertOversizedImages = value; - return _forceRepaint(); + // We don't want to block the vm service response on the frame + // actually rendering, just schedule it and return; + unawaited(_forceRepaint()); } - return Future.value(); }, ); registerBoolServiceExtension( name: RenderingServiceExtensions.debugPaint.name, getter: () async => debugPaintSizeEnabled, - setter: (bool value) { + setter: (bool value) async { if (debugPaintSizeEnabled == value) { - return Future.value(); + return; } debugPaintSizeEnabled = value; - return _forceRepaint(); + // We don't want to block the vm service response on the frame + // actually rendering, just schedule it and return; + unawaited(_forceRepaint()); }, ); registerBoolServiceExtension( name: RenderingServiceExtensions.debugPaintBaselinesEnabled.name, getter: () async => debugPaintBaselinesEnabled, - setter: (bool value) { + setter: (bool value) async { if (debugPaintBaselinesEnabled == value) { - return Future.value(); + return; } debugPaintBaselinesEnabled = value; - return _forceRepaint(); + // We don't want to block the vm service response on the frame + // actually rendering, just schedule it and return; + unawaited(_forceRepaint()); }, ); registerBoolServiceExtension( name: RenderingServiceExtensions.repaintRainbow.name, getter: () async => debugRepaintRainbowEnabled, - setter: (bool value) { + setter: (bool value) async { final bool repaint = debugRepaintRainbowEnabled && !value; debugRepaintRainbowEnabled = value; if (repaint) { - return _forceRepaint(); + // We don't want to block the vm service response on the frame + // actually rendering, just schedule it and return; + unawaited(_forceRepaint()); } - return Future.value(); }, ); registerServiceExtension( @@ -133,34 +140,40 @@ mixin RendererBinding registerBoolServiceExtension( name: RenderingServiceExtensions.debugDisableClipLayers.name, getter: () async => debugDisableClipLayers, - setter: (bool value) { + setter: (bool value) async { if (debugDisableClipLayers == value) { - return Future.value(); + return; } debugDisableClipLayers = value; - return _forceRepaint(); + // We don't want to block the vm service response on the frame + // actually rendering, just schedule it and return; + unawaited(_forceRepaint()); }, ); registerBoolServiceExtension( name: RenderingServiceExtensions.debugDisablePhysicalShapeLayers.name, getter: () async => debugDisablePhysicalShapeLayers, - setter: (bool value) { + setter: (bool value) async { if (debugDisablePhysicalShapeLayers == value) { - return Future.value(); + return; } debugDisablePhysicalShapeLayers = value; - return _forceRepaint(); + // We don't want to block the vm service response on the frame + // actually rendering, just schedule it and return; + unawaited(_forceRepaint()); }, ); registerBoolServiceExtension( name: RenderingServiceExtensions.debugDisableOpacityLayers.name, getter: () async => debugDisableOpacityLayers, - setter: (bool value) { + setter: (bool value) async { if (debugDisableOpacityLayers == value) { - return Future.value(); + return; } debugDisableOpacityLayers = value; - return _forceRepaint(); + // We don't want to block the vm service response on the frame + // actually rendering, just schedule it and return; + unawaited(_forceRepaint()); }, ); return true; diff --git a/packages/flutter/test/foundation/service_extensions_test.dart b/packages/flutter/test/foundation/service_extensions_test.dart index 177c2493c6..3f4421cd9a 100644 --- a/packages/flutter/test/foundation/service_extensions_test.dart +++ b/packages/flutter/test/foundation/service_extensions_test.dart @@ -413,7 +413,7 @@ void main() { }); await binding.flushMicrotasks(); expect(binding.frameScheduled, isTrue); - expect(completed, isFalse); + expect(completed, isTrue); await binding.doFrame(); await binding.flushMicrotasks(); expect(completed, isTrue); @@ -484,7 +484,7 @@ void main() { }); await binding.flushMicrotasks(); expect(binding.frameScheduled, isTrue); - expect(completed, isFalse); + expect(completed, isTrue); await binding.doFrame(); await binding.flushMicrotasks(); expect(completed, isTrue); @@ -546,7 +546,7 @@ void main() { }); await binding.flushMicrotasks(); expect(binding.frameScheduled, isTrue); - expect(completed, isFalse); + expect(completed, isTrue); await binding.doFrame(); await binding.flushMicrotasks(); expect(completed, isTrue); @@ -1038,7 +1038,7 @@ void main() { completed = true; }); await binding.flushMicrotasks(); - expect(completed, false); + expect(completed, isTrue); expect(binding.frameScheduled, isTrue); await binding.doFrame(); await binding.flushMicrotasks(); @@ -1082,7 +1082,7 @@ void main() { }); await binding.flushMicrotasks(); expect(binding.frameScheduled, isTrue); - expect(completed, isFalse); + expect(completed, isTrue); await binding.doFrame(); await binding.flushMicrotasks(); expect(completed, isTrue); @@ -1143,7 +1143,7 @@ void main() { }); await binding.flushMicrotasks(); expect(binding.frameScheduled, isTrue); - expect(completed, isFalse); + expect(completed, isTrue); await binding.doFrame(); await binding.flushMicrotasks(); expect(completed, isTrue); @@ -1204,7 +1204,7 @@ void main() { }); await binding.flushMicrotasks(); expect(binding.frameScheduled, isTrue); - expect(completed, isFalse); + expect(completed, isTrue); await binding.doFrame(); await binding.flushMicrotasks(); expect(completed, isTrue);