From cd052a56cba67bbc96fdf9a3ef940c3b3ca17a55 Mon Sep 17 00:00:00 2001 From: Kate Lovett Date: Mon, 2 Dec 2019 15:08:35 -0800 Subject: [PATCH] Revert "Re-land "Add option to delay rendering the first frame (#45135)" (#45588)" (#45939) This reverts commit c59151b1cdb70c77368c04aaed39ca4791cad539. --- .../flutter/lib/src/rendering/binding.dart | 63 +-------- packages/flutter/lib/src/widgets/binding.dart | 52 ++++---- .../lib/src/widgets/localizations.dart | 17 ++- .../binding_deferred_first_frame_test.dart | 120 ------------------ .../test/widgets/localizations_test.dart | 73 ----------- packages/flutter_test/lib/src/binding.dart | 31 +---- 6 files changed, 37 insertions(+), 319 deletions(-) delete mode 100644 packages/flutter/test/widgets/binding_deferred_first_frame_test.dart delete mode 100644 packages/flutter/test/widgets/localizations_test.dart diff --git a/packages/flutter/lib/src/rendering/binding.dart b/packages/flutter/lib/src/rendering/binding.dart index 0ad6a7a610a..4b4343a1f0b 100644 --- a/packages/flutter/lib/src/rendering/binding.dart +++ b/packages/flutter/lib/src/rendering/binding.dart @@ -284,62 +284,6 @@ mixin RendererBinding on BindingBase, ServicesBinding, SchedulerBinding, Gesture _mouseTracker.schedulePostFrameCheck(); } - int _firstFrameDeferredCount = 0; - bool _firstFrameSent = false; - - /// Whether frames produced by [drawFrame] are sent to the engine. - /// - /// If false the framework will do all the work to produce a frame, - /// but the frame is never send to the engine to actually appear on screen. - /// - /// See also: - /// - /// * [deferFirstFrame], which defers when the first frame is send to the - /// engine. - bool get sendFramesToEngine => _firstFrameSent || _firstFrameDeferredCount == 0; - - /// Tell the framework to not send the first frames to the engine until there - /// is a corresponding call to [allowFirstFrame]. - /// - /// Call this to perform asynchronous initialisation work before the first - /// frame is rendered (which takes down the splash screen). The framework - /// will still do all the work to produce frames, but those frames are never - /// send to the engine and will not appear on screen. - /// - /// Calling this has no effect after the first frame has been send to the - /// engine. - void deferFirstFrame() { - assert(_firstFrameDeferredCount >= 0); - _firstFrameDeferredCount += 1; - } - - /// Called after [deferFirstFrame] to tell the framework that it is ok to - /// send the first frame to the engine now. - /// - /// For best performance, this method should only be called while the - /// [schedulerPhase] is [SchedulerPhase.idle]. - /// - /// This method may only be called once for each corresponding call - /// to [deferFirstFrame]. - void allowFirstFrame() { - assert(_firstFrameDeferredCount > 0); - _firstFrameDeferredCount -= 1; - // Always schedule a warm up frame even if the deferral count is not down to - // zero yet since the removal of a deferral may uncover new deferrals that - // are lower in the widget tree. - if (!_firstFrameSent) - scheduleWarmUpFrame(); - } - - /// Call this to pretend that no frames have been sent to the engine yet. - /// - /// This is useful for tests that want to call [deferFirstFrame] and - /// [allowFirstFrame] since those methods only have an effect if no frames - /// have been sent to the engine yet. - void resetFirstFrameSent() { - _firstFrameSent = false; - } - /// Pump the rendering pipeline to generate a frame. /// /// This method is called by [handleDrawFrame], which itself is called @@ -401,11 +345,8 @@ mixin RendererBinding on BindingBase, ServicesBinding, SchedulerBinding, Gesture pipelineOwner.flushLayout(); pipelineOwner.flushCompositingBits(); pipelineOwner.flushPaint(); - if (sendFramesToEngine) { - renderView.compositeFrame(); // this sends the bits to the GPU - pipelineOwner.flushSemantics(); // this also sends the semantics to the OS. - _firstFrameSent = true; - } + renderView.compositeFrame(); // this sends the bits to the GPU + pipelineOwner.flushSemantics(); // this also sends the semantics to the OS. } @override diff --git a/packages/flutter/lib/src/widgets/binding.dart b/packages/flutter/lib/src/widgets/binding.dart index 80f1b14ed91..8cae6c137b3 100644 --- a/packages/flutter/lib/src/widgets/binding.dart +++ b/packages/flutter/lib/src/widgets/binding.dart @@ -574,6 +574,9 @@ mixin WidgetsBinding on BindingBase, ServicesBinding, SchedulerBinding, GestureB } bool _needToReportFirstFrame = true; + int _deferFirstFrameReportCount = 0; + bool get _reportFirstFrame => _deferFirstFrameReportCount == 0; + final Completer _firstFrameCompleter = Completer(); @@ -599,6 +602,11 @@ mixin WidgetsBinding on BindingBase, ServicesBinding, SchedulerBinding, GestureB /// Whether the first frame has finished building. /// + /// Only useful in profile and debug builds; in release builds, this always + /// return false. This can be deferred using [deferFirstFrameReport] and + /// [allowFirstFrameReport]. The value is set at the end of the call to + /// [drawFrame]. + /// /// This value can also be obtained over the VM service protocol as /// `ext.flutter.didSendFirstFrameEvent`. /// @@ -610,30 +618,27 @@ mixin WidgetsBinding on BindingBase, ServicesBinding, SchedulerBinding, GestureB /// Tell the framework not to report the frame it is building as a "useful" /// first frame until there is a corresponding call to [allowFirstFrameReport]. /// - /// Deprecated. Use [deferFirstFrame]/[allowFirstFrame] to delay rendering the - /// first frame. - @Deprecated( - 'Use deferFirstFrame/allowFirstFrame to delay rendering the first frame. ' - 'This feature was deprecated after v1.12.4.' - ) + /// This is used by [WidgetsApp] to avoid reporting frames that aren't useful + /// during startup as the "first frame". void deferFirstFrameReport() { if (!kReleaseMode) { - deferFirstFrame(); + assert(_deferFirstFrameReportCount >= 0); + _deferFirstFrameReportCount += 1; } } /// When called after [deferFirstFrameReport]: tell the framework to report /// the frame it is building as a "useful" first frame. /// - /// Deprecated. Use [deferFirstFrame]/[allowFirstFrame] to delay rendering the - /// first frame. - @Deprecated( - 'Use deferFirstFrame/allowFirstFrame to delay rendering the first frame. ' - 'This feature was deprecated after v1.12.4.' - ) + /// This method may only be called once for each corresponding call + /// to [deferFirstFrameReport]. + /// + /// This is used by [WidgetsApp] to report when the first useful frame is + /// painted. void allowFirstFrameReport() { if (!kReleaseMode) { - allowFirstFrame(); + assert(_deferFirstFrameReportCount >= 1); + _deferFirstFrameReportCount -= 1; } } @@ -750,23 +755,18 @@ mixin WidgetsBinding on BindingBase, ServicesBinding, SchedulerBinding, GestureB return true; }()); - TimingsCallback firstFrameCallback; - if (_needToReportFirstFrame) { + if (_needToReportFirstFrame && _reportFirstFrame) { assert(!_firstFrameCompleter.isCompleted); + TimingsCallback firstFrameCallback; firstFrameCallback = (List timings) { - assert(sendFramesToEngine); if (!kReleaseMode) { developer.Timeline.instantSync('Rasterized first useful frame'); developer.postEvent('Flutter.FirstFrame', {}); } SchedulerBinding.instance.removeTimingsCallback(firstFrameCallback); - firstFrameCallback = null; _firstFrameCompleter.complete(); }; - // Callback is only invoked when [Window.render] is called. When - // [sendFramesToEngine] is set to false during the frame, it will not - // be called and we need to remove the callback (see below). SchedulerBinding.instance.addTimingsCallback(firstFrameCallback); } @@ -782,14 +782,11 @@ mixin WidgetsBinding on BindingBase, ServicesBinding, SchedulerBinding, GestureB }()); } if (!kReleaseMode) { - if (_needToReportFirstFrame && sendFramesToEngine) { + if (_needToReportFirstFrame && _reportFirstFrame) { developer.Timeline.instantSync('Widgets built first useful frame'); } } _needToReportFirstFrame = false; - if (firstFrameCallback != null && !sendFramesToEngine) { - SchedulerBinding.instance.removeTimingsCallback(firstFrameCallback); - } } /// The [Element] that is at the root of the hierarchy (and which wraps the @@ -837,9 +834,12 @@ mixin WidgetsBinding on BindingBase, ServicesBinding, SchedulerBinding, GestureB return true; }()); + deferFirstFrameReport(); if (renderViewElement != null) buildOwner.reassemble(renderViewElement); - return super.performReassemble(); + return super.performReassemble().then((void value) { + allowFirstFrameReport(); + }); } } diff --git a/packages/flutter/lib/src/widgets/localizations.dart b/packages/flutter/lib/src/widgets/localizations.dart index 2161222dbe2..775fb46eb96 100644 --- a/packages/flutter/lib/src/widgets/localizations.dart +++ b/packages/flutter/lib/src/widgets/localizations.dart @@ -6,7 +6,6 @@ import 'dart:async'; import 'dart:ui' show Locale; import 'package:flutter/foundation.dart'; -import 'package:flutter/rendering.dart'; import 'basic.dart'; import 'binding.dart'; @@ -520,15 +519,15 @@ class _LocalizationsState extends State { // have finished loading. Until then the old locale will continue to be used. // - If we're running at app startup time then defer reporting the first // "useful" frame until after the async load has completed. - RendererBinding.instance.deferFirstFrame(); + WidgetsBinding.instance.deferFirstFrameReport(); typeToResourcesFuture.then((Map value) { - if (mounted) { - setState(() { - _typeToResources = value; - _locale = locale; - }); - } - RendererBinding.instance.allowFirstFrame(); + WidgetsBinding.instance.allowFirstFrameReport(); + if (!mounted) + return; + setState(() { + _typeToResources = value; + _locale = locale; + }); }); } } diff --git a/packages/flutter/test/widgets/binding_deferred_first_frame_test.dart b/packages/flutter/test/widgets/binding_deferred_first_frame_test.dart deleted file mode 100644 index 27e37845815..00000000000 --- a/packages/flutter/test/widgets/binding_deferred_first_frame_test.dart +++ /dev/null @@ -1,120 +0,0 @@ -// Copyright 2019 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -import 'dart:async'; - -import 'package:flutter/rendering.dart'; -import 'package:flutter_test/flutter_test.dart'; -import 'package:flutter/material.dart'; -import 'package:flutter/widgets.dart'; - -const String _actualContent = 'Actual Content'; -const String _loading = 'Loading...'; - -void main() { - testWidgets('deferFirstFrame/allowFirstFrame stops sending frames to engine', (WidgetTester tester) async { - expect(RendererBinding.instance.sendFramesToEngine, isTrue); - - final Completer completer = Completer(); - await tester.pumpWidget( - Directionality( - textDirection: TextDirection.ltr, - child: _DeferringWidget( - key: UniqueKey(), - loader: completer.future, - ), - ), - ); - final _DeferringWidgetState state = tester.state<_DeferringWidgetState>(find.byType(_DeferringWidget)); - - expect(find.text(_loading), findsOneWidget); - expect(find.text(_actualContent), findsNothing); - expect(RendererBinding.instance.sendFramesToEngine, isFalse); - - await tester.pump(); - expect(find.text(_loading), findsOneWidget); - expect(find.text(_actualContent), findsNothing); - expect(RendererBinding.instance.sendFramesToEngine, isFalse); - expect(state.doneLoading, isFalse); - - // Complete the future to start sending frames. - completer.complete(); - await tester.idle(); - expect(state.doneLoading, isTrue); - expect(RendererBinding.instance.sendFramesToEngine, isTrue); - - await tester.pump(); - expect(find.text(_loading), findsNothing); - expect(find.text(_actualContent), findsOneWidget); - expect(RendererBinding.instance.sendFramesToEngine, isTrue); - }); - - testWidgets('Two widgets can defer frames', (WidgetTester tester) async { - expect(RendererBinding.instance.sendFramesToEngine, isTrue); - - final Completer completer1 = Completer(); - final Completer completer2 = Completer(); - await tester.pumpWidget( - Directionality( - textDirection: TextDirection.ltr, - child: Row( - children: [ - _DeferringWidget( - key: UniqueKey(), - loader: completer1.future, - ), - _DeferringWidget( - key: UniqueKey(), - loader: completer2.future, - ), - ], - ), - ), - ); - expect(find.text(_loading), findsNWidgets(2)); - expect(find.text(_actualContent), findsNothing); - expect(RendererBinding.instance.sendFramesToEngine, isFalse); - - completer1.complete(); - completer2.complete(); - await tester.idle(); - - await tester.pump(); - expect(find.text(_loading), findsNothing); - expect(find.text(_actualContent), findsNWidgets(2)); - expect(RendererBinding.instance.sendFramesToEngine, isTrue); - }); -} - -class _DeferringWidget extends StatefulWidget { - const _DeferringWidget({Key key, this.loader}) : super(key: key); - - final Future loader; - - @override - State<_DeferringWidget> createState() => _DeferringWidgetState(); -} - -class _DeferringWidgetState extends State<_DeferringWidget> { - bool doneLoading = false; - - @override - void initState() { - super.initState(); - RendererBinding.instance.deferFirstFrame(); - widget.loader.then((_) { - setState(() { - doneLoading = true; - RendererBinding.instance.allowFirstFrame(); - }); - }); - } - - @override - Widget build(BuildContext context) { - return doneLoading - ? const Text(_actualContent) - : const Text(_loading); - } -} diff --git a/packages/flutter/test/widgets/localizations_test.dart b/packages/flutter/test/widgets/localizations_test.dart deleted file mode 100644 index 535eb6fa197..00000000000 --- a/packages/flutter/test/widgets/localizations_test.dart +++ /dev/null @@ -1,73 +0,0 @@ -// Copyright 2013 The Flutter Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -import 'dart:async'; - -import 'package:flutter_test/flutter_test.dart'; -import 'package:flutter/widgets.dart'; - -void main() { - final TestAutomatedTestWidgetsFlutterBinding binding = TestAutomatedTestWidgetsFlutterBinding(); - - testWidgets('Locale is available when Localizations widget stops defering frames', (WidgetTester tester) async { - final FakeLocalizationsDelegate delegate = FakeLocalizationsDelegate(); - await tester.pumpWidget(Localizations( - locale: const Locale('fo'), - delegates: >[ - WidgetsLocalizationsDelegate(), - delegate, - ], - child: const Text('loaded') - )); - final dynamic state = tester.state(find.byType(Localizations)); - expect(state.locale, isNull); - expect(find.text('loaded'), findsNothing); - - Locale locale; - binding.onAllowFrame = () { - locale = state.locale; - }; - delegate.completer.complete('foo'); - await tester.idle(); - expect(locale, const Locale('fo')); - await tester.pump(); - expect(find.text('loaded'), findsOneWidget); - }); -} - -class FakeLocalizationsDelegate extends LocalizationsDelegate { - final Completer completer = Completer(); - - @override - bool isSupported(Locale locale) => true; - - @override - Future load(Locale locale) => completer.future; - - @override - bool shouldReload(LocalizationsDelegate old) => false; -} - -class TestAutomatedTestWidgetsFlutterBinding extends AutomatedTestWidgetsFlutterBinding { - - VoidCallback onAllowFrame; - - @override - void allowFirstFrame() { - if (onAllowFrame != null) - onAllowFrame(); - super.allowFirstFrame(); - } -} - -class WidgetsLocalizationsDelegate extends LocalizationsDelegate { - @override - bool isSupported(Locale locale) => true; - - @override - Future load(Locale locale) => DefaultWidgetsLocalizations.load(locale); - - @override - bool shouldReload(WidgetsLocalizationsDelegate old) => false; -} diff --git a/packages/flutter_test/lib/src/binding.dart b/packages/flutter_test/lib/src/binding.dart index 32c91c085ec..8b78b6831a8 100644 --- a/packages/flutter_test/lib/src/binding.dart +++ b/packages/flutter_test/lib/src/binding.dart @@ -687,9 +687,6 @@ abstract class TestWidgetsFlutterBinding extends BindingBase runApp(Container(key: UniqueKey(), child: _preTestMessage)); // Reset the tree to a known state. await pump(); - // Pretend that the first frame produced in the test body is the first frame - // sent to the engine. - resetFirstFrameSent(); final bool autoUpdateGoldensBeforeTest = autoUpdateGoldenFiles && !isBrowser; final TestExceptionReporter reportTestExceptionBeforeTest = reportTestException; @@ -966,31 +963,6 @@ class AutomatedTestWidgetsFlutterBinding extends TestWidgetsFlutterBinding { return result; } - int _firstFrameDeferredCount = 0; - bool _firstFrameSent = false; - - @override - bool get sendFramesToEngine => _firstFrameSent || _firstFrameDeferredCount == 0; - - @override - void deferFirstFrame() { - assert(_firstFrameDeferredCount >= 0); - _firstFrameDeferredCount += 1; - } - - @override - void allowFirstFrame() { - assert(_firstFrameDeferredCount > 0); - _firstFrameDeferredCount -= 1; - // Unlike in RendererBinding.allowFirstFrame we do not force a frame her - // to give the test full control over frame scheduling. - } - - @override - void resetFirstFrameSent() { - _firstFrameSent = false; - } - EnginePhase _phase = EnginePhase.sendSemanticsUpdate; // Cloned from RendererBinding.drawFrame() but with early-exit semantics. @@ -1007,8 +979,7 @@ class AutomatedTestWidgetsFlutterBinding extends TestWidgetsFlutterBinding { pipelineOwner.flushCompositingBits(); if (_phase != EnginePhase.compositingBits) { pipelineOwner.flushPaint(); - if (_phase != EnginePhase.paint && sendFramesToEngine) { - _firstFrameSent = true; + if (_phase != EnginePhase.paint) { renderView.compositeFrame(); // this sends the bits to the GPU if (_phase != EnginePhase.composite) { pipelineOwner.flushSemantics();