From abdd22ccd7b9c9a6abb4210eb8496799498f5a7e Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Mon, 2 Mar 2020 13:42:42 -0800 Subject: [PATCH 1/6] [web] Introduce platform message to enable experiment flags on web --- lib/web_ui/lib/src/engine.dart | 1 + .../src/engine/compositor/initialization.dart | 3 +- .../lib/src/engine/text/measurement.dart | 9 +-- .../lib/src/engine/web_experiments.dart | 55 ++++++++++++++ lib/web_ui/lib/src/engine/window.dart | 10 +++ .../test/engine/web_experiments_test.dart | 74 +++++++++++++++++++ .../test/golden_tests/engine/scuba.dart | 8 +- .../engine/text_overflow_golden_test.dart | 6 +- lib/web_ui/test/paragraph_test.dart | 24 +++--- 9 files changed, 163 insertions(+), 27 deletions(-) create mode 100644 lib/web_ui/lib/src/engine/web_experiments.dart create mode 100644 lib/web_ui/test/engine/web_experiments_test.dart diff --git a/lib/web_ui/lib/src/engine.dart b/lib/web_ui/lib/src/engine.dart index 00b90d9bb4b9d..c954ff8b84243 100644 --- a/lib/web_ui/lib/src/engine.dart +++ b/lib/web_ui/lib/src/engine.dart @@ -117,6 +117,7 @@ part 'engine/text_editing/text_editing.dart'; part 'engine/util.dart'; part 'engine/validators.dart'; part 'engine/vector_math.dart'; +part 'engine/web_experiments.dart'; part 'engine/window.dart'; bool _engineInitialized = false; diff --git a/lib/web_ui/lib/src/engine/compositor/initialization.dart b/lib/web_ui/lib/src/engine/compositor/initialization.dart index a4074ba3d7c5f..7ea4053fa5c20 100644 --- a/lib/web_ui/lib/src/engine/compositor/initialization.dart +++ b/lib/web_ui/lib/src/engine/compositor/initialization.dart @@ -6,8 +6,7 @@ part of engine; /// EXPERIMENTAL: Enable the Skia-based rendering backend. -const bool experimentalUseSkia = - bool.fromEnvironment('FLUTTER_WEB_USE_SKIA', defaultValue: false); +bool get experimentalUseSkia => webExperiments.useSkia; /// The URL to use when downloading the CanvasKit script and associated wasm. /// diff --git a/lib/web_ui/lib/src/engine/text/measurement.dart b/lib/web_ui/lib/src/engine/text/measurement.dart index 3781ff6412ef5..814a7884c7d6c 100644 --- a/lib/web_ui/lib/src/engine/text/measurement.dart +++ b/lib/web_ui/lib/src/engine/text/measurement.dart @@ -187,13 +187,6 @@ abstract class TextMeasurementService { static TextMeasurementService get canvasInstance => CanvasTextMeasurementService.instance; - /// Whether the new experimental implementation of canvas-based text - /// measurement is enabled or not. - /// - /// This is only used for testing at the moment. Once the implementation is - /// complete and production-ready, we'll get rid of this flag. - static bool enableExperimentalCanvasImplementation = const bool.fromEnvironment('FLUTTER_WEB_USE_EXPERIMENTAL_CANVAS_TEXT', defaultValue: false); - /// Gets the appropriate [TextMeasurementService] instance for the given /// [paragraph]. static TextMeasurementService forParagraph(ui.Paragraph paragraph) { @@ -206,7 +199,7 @@ abstract class TextMeasurementService { // Skip using canvas measurements until the iframe becomes visible. // see: https://github.com/flutter/flutter/issues/36341 if (!window.physicalSize.isEmpty && - enableExperimentalCanvasImplementation && + webExperiments.useCanvasText && _canUseCanvasMeasurement(paragraph)) { return canvasInstance; } diff --git a/lib/web_ui/lib/src/engine/web_experiments.dart b/lib/web_ui/lib/src/engine/web_experiments.dart new file mode 100644 index 0000000000000..96a3a571e1b39 --- /dev/null +++ b/lib/web_ui/lib/src/engine/web_experiments.dart @@ -0,0 +1,55 @@ +// 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. + +// @dart = 2.6 +part of engine; + +const MethodCodec codec = JSONMethodCodec(); + +final WebExperiments webExperiments = WebExperiments._(); + +/// A bag of all experiment flags in the web engine. +/// +/// This class also handles platform messages that can be sent to enable/disable +/// certain experiments at runtime without the need to access engine internals. +class WebExperiments { + WebExperiments._(); + + /// Experiment flag for using the Skia-based rendering backend. + bool get useSkia => _useSkia ?? false; + set useSkia(bool enabled) { + _useSkia = enabled; + } + bool _useSkia = const bool.fromEnvironment('FLUTTER_WEB_USE_SKIA'); + + /// Experiment flag for using canvas-based text measurement. + bool get useCanvasText => _useCanvasText ?? false; + set useCanvasText(bool enabled) { + _useCanvasText = enabled; + } + bool _useCanvasText = + const bool.fromEnvironment('FLUTTER_WEB_USE_EXPERIMENTAL_CANVAS_TEXT'); + + /// Reset all experimental flags to their default values. + void reset() { + _useSkia = null; + _useCanvasText = null; + } + + /// Handles the platform message used to enable/disable web experiments in the + /// web engine. + void enableWebExperiments(Map message) { + for (final String name in message.keys) { + final bool enabled = message[name]; + switch (name) { + case 'useSkia': + _useSkia = enabled; + break; + case 'useCanvasText': + _useCanvasText = enabled; + break; + } + } + } +} diff --git a/lib/web_ui/lib/src/engine/window.dart b/lib/web_ui/lib/src/engine/window.dart index e2004cda29286..c0b3bc50cd509 100644 --- a/lib/web_ui/lib/src/engine/window.dart +++ b/lib/web_ui/lib/src/engine/window.dart @@ -210,6 +210,16 @@ class EngineWindow extends ui.Window { break; } return; + + case 'flutter/experiments': + const MethodCodec codec = JSONMethodCodec(); + final MethodCall decoded = codec.decodeMethodCall(data); + final Map arguments = decoded.arguments; + switch (decoded.method) { + case 'enableWebExperiments': + webExperiments.enableWebExperiments(arguments); + return; + } } if (pluginMessageCallHandler != null) { diff --git a/lib/web_ui/test/engine/web_experiments_test.dart b/lib/web_ui/test/engine/web_experiments_test.dart new file mode 100644 index 0000000000000..0ff7a1348ae00 --- /dev/null +++ b/lib/web_ui/test/engine/web_experiments_test.dart @@ -0,0 +1,74 @@ +// 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. + +// @dart = 2.6 +import 'package:test/test.dart'; +import 'package:ui/src/engine.dart'; + +void main() { + tearDown(() { + webExperiments.reset(); + }); + + test('default web experiment values', () { + expect(webExperiments.useSkia, false); + expect(webExperiments.useCanvasText, false); + }); + + test('can turn on/off web experiments', () { + webExperiments.enableWebExperiments({ + 'useSkia': true, + }); + expect(webExperiments.useSkia, true); + expect(webExperiments.useCanvasText, false); + + webExperiments.enableWebExperiments({ + 'useSkia': false, + 'useCanvasText': true, + }); + expect(webExperiments.useSkia, false); + expect(webExperiments.useCanvasText, true); + + webExperiments.enableWebExperiments({ + 'useSkia': true, + 'useCanvasText': null, + }); + expect(webExperiments.useSkia, true); + // Goes back to default value. + expect(webExperiments.useCanvasText, false); + + webExperiments.enableWebExperiments({ + 'useSkia': null, + }); + // Goes back to default value. + expect(webExperiments.useSkia, false); + expect(webExperiments.useCanvasText, false); + }); + + test('can reset web experiments', () { + webExperiments.enableWebExperiments({ + 'useSkia': true, + 'useCanvasText': false, + }); + webExperiments.reset(); + expect(webExperiments.useSkia, false); + expect(webExperiments.useCanvasText, false); + + webExperiments.enableWebExperiments({ + 'useSkia': false, + 'useCanvasText': true, + }); + webExperiments.reset(); + expect(webExperiments.useSkia, false); + expect(webExperiments.useCanvasText, false); + + webExperiments.enableWebExperiments({ + 'useSkia': true, + 'useCanvasText': true, + }); + webExperiments.reset(); + expect(webExperiments.useSkia, false); + expect(webExperiments.useCanvasText, false); + }); +} diff --git a/lib/web_ui/test/golden_tests/engine/scuba.dart b/lib/web_ui/test/golden_tests/engine/scuba.dart index 421f4ae758be6..f334a2b0b91bf 100644 --- a/lib/web_ui/test/golden_tests/engine/scuba.dart +++ b/lib/web_ui/test/golden_tests/engine/scuba.dart @@ -71,7 +71,7 @@ class EngineScubaTester { sceneElement.append(canvas.rootElement); html.document.body.append(sceneElement); String screenshotName = '${fileName}_${canvas.runtimeType}'; - if (TextMeasurementService.enableExperimentalCanvasImplementation) { + if (webExperiments.useCanvasText) { screenshotName += '+canvas_measurement'; } await diffScreenshot( @@ -96,18 +96,20 @@ void testEachCanvas(String description, CanvasTest body, test('$description (bitmap)', () { try { TextMeasurementService.initialize(rulerCacheCapacity: 2); + webExperiments.useCanvasText = false; return body(BitmapCanvas(bounds)); } finally { + webExperiments.useCanvasText = null; TextMeasurementService.clearCache(); } }); test('$description (bitmap + canvas measurement)', () async { try { TextMeasurementService.initialize(rulerCacheCapacity: 2); - TextMeasurementService.enableExperimentalCanvasImplementation = true; + webExperiments.useCanvasText = true; await body(BitmapCanvas(bounds)); } finally { - TextMeasurementService.enableExperimentalCanvasImplementation = false; + webExperiments.useCanvasText = null; TextMeasurementService.clearCache(); } }); diff --git a/lib/web_ui/test/golden_tests/engine/text_overflow_golden_test.dart b/lib/web_ui/test/golden_tests/engine/text_overflow_golden_test.dart index 167921f337f5b..978c53bb6e813 100644 --- a/lib/web_ui/test/golden_tests/engine/text_overflow_golden_test.dart +++ b/lib/web_ui/test/golden_tests/engine/text_overflow_golden_test.dart @@ -5,7 +5,7 @@ // @dart = 2.6 import 'dart:async'; -import 'package:ui/ui.dart'; +import 'package:ui/ui.dart' hide window; import 'package:ui/src/engine.dart'; import 'scuba.dart'; @@ -89,7 +89,7 @@ void main() async { offset = offset.translate(0, p.height + 10); // Only the first line is rendered with an ellipsis. - if (!TextMeasurementService.enableExperimentalCanvasImplementation) { + if (!webExperiments.useCanvasText) { // This is now correct with the canvas-based measurement, so we shouldn't // print the "(wrong)" warning. p = warning('(wrong)'); @@ -106,7 +106,7 @@ void main() async { // Only the first two lines are rendered and the ellipsis appears on the 2nd // line. - if (!TextMeasurementService.enableExperimentalCanvasImplementation) { + if (!webExperiments.useCanvasText) { // This is now correct with the canvas-based measurement, so we shouldn't // print the "(wrong)" warning. p = warning('(wrong)'); diff --git a/lib/web_ui/test/paragraph_test.dart b/lib/web_ui/test/paragraph_test.dart index 29d52b830218f..beb82bca20ea5 100644 --- a/lib/web_ui/test/paragraph_test.dart +++ b/lib/web_ui/test/paragraph_test.dart @@ -4,7 +4,7 @@ // @dart = 2.6 import 'package:ui/src/engine.dart'; -import 'package:ui/ui.dart'; +import 'package:ui/ui.dart' hide window; import 'package:test/test.dart'; @@ -12,18 +12,20 @@ void testEachMeasurement(String description, VoidCallback body, {bool skip}) { test('$description (dom measurement)', () async { try { TextMeasurementService.initialize(rulerCacheCapacity: 2); + webExperiments.useCanvasText = false; return body(); } finally { + webExperiments.useCanvasText = null; TextMeasurementService.clearCache(); } }, skip: skip); test('$description (canvas measurement)', () async { try { TextMeasurementService.initialize(rulerCacheCapacity: 2); - TextMeasurementService.enableExperimentalCanvasImplementation = true; + webExperiments.useCanvasText = true; return body(); } finally { - TextMeasurementService.enableExperimentalCanvasImplementation = false; + webExperiments.useCanvasText = null; TextMeasurementService.clearCache(); } }, skip: skip); @@ -184,7 +186,7 @@ void main() async { test('getPositionForOffset multi-line', () { // [Paragraph.getPositionForOffset] for multi-line text doesn't work well // with dom-based measurement. - TextMeasurementService.enableExperimentalCanvasImplementation = true; + webExperiments.useCanvasText = true; TextMeasurementService.initialize(rulerCacheCapacity: 2); final ParagraphBuilder builder = ParagraphBuilder(ParagraphStyle( @@ -280,11 +282,11 @@ void main() async { ); TextMeasurementService.clearCache(); - TextMeasurementService.enableExperimentalCanvasImplementation = false; + webExperiments.useCanvasText = null; }); test('getPositionForOffset multi-line centered', () { - TextMeasurementService.enableExperimentalCanvasImplementation = true; + webExperiments.useCanvasText = true; TextMeasurementService.initialize(rulerCacheCapacity: 2); final ParagraphBuilder builder = ParagraphBuilder(ParagraphStyle( @@ -387,7 +389,7 @@ void main() async { ); TextMeasurementService.clearCache(); - TextMeasurementService.enableExperimentalCanvasImplementation = false; + webExperiments.useCanvasText = null; }); testEachMeasurement('getBoxesForRange returns a box', () { @@ -782,7 +784,7 @@ void main() async { test('longestLine', () { // [Paragraph.longestLine] is only supported by canvas-based measurement. - TextMeasurementService.enableExperimentalCanvasImplementation = true; + webExperiments.useCanvasText = true; TextMeasurementService.initialize(rulerCacheCapacity: 2); final ParagraphBuilder builder = ParagraphBuilder(ParagraphStyle( @@ -797,7 +799,7 @@ void main() async { expect(paragraph.longestLine, 50.0); TextMeasurementService.clearCache(); - TextMeasurementService.enableExperimentalCanvasImplementation = false; + webExperiments.useCanvasText = null; }); testEachMeasurement('getLineBoundary (single-line)', () { @@ -824,7 +826,7 @@ void main() async { test('getLineBoundary (multi-line)', () { // [Paragraph.getLineBoundary] for multi-line paragraphs is only supported // by canvas-based measurement. - TextMeasurementService.enableExperimentalCanvasImplementation = true; + webExperiments.useCanvasText = true; TextMeasurementService.initialize(rulerCacheCapacity: 2); final ParagraphBuilder builder = ParagraphBuilder(ParagraphStyle( @@ -867,7 +869,7 @@ void main() async { } TextMeasurementService.clearCache(); - TextMeasurementService.enableExperimentalCanvasImplementation = false; + webExperiments.useCanvasText = null; }); testEachMeasurement('width should be a whole integer', () { From d73750f12716fb4364b1455185ae7e4d804a136c Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Fri, 13 Mar 2020 10:56:06 -0700 Subject: [PATCH 2/6] revert canvaskit changes + use js interop instead of platform messages --- lib/web_ui/lib/src/engine.dart | 3 + .../src/engine/compositor/initialization.dart | 3 +- .../lib/src/engine/web_experiments.dart | 38 ++++------ lib/web_ui/lib/src/engine/window.dart | 10 --- .../test/engine/web_experiments_test.dart | 71 +++++++++---------- 5 files changed, 51 insertions(+), 74 deletions(-) diff --git a/lib/web_ui/lib/src/engine.dart b/lib/web_ui/lib/src/engine.dart index c954ff8b84243..4f4df2c8a0944 100644 --- a/lib/web_ui/lib/src/engine.dart +++ b/lib/web_ui/lib/src/engine.dart @@ -162,6 +162,9 @@ void webOnlyInitializeEngine() { // initialize framework bindings. domRenderer; + // Calling this getter to force the [WebExperiments] instance to be created. + webExperiments; + bool waitingForAnimation = false; ui.webOnlyScheduleFrameCallback = () { // We're asked to schedule a frame and call `frameHandler` when the frame diff --git a/lib/web_ui/lib/src/engine/compositor/initialization.dart b/lib/web_ui/lib/src/engine/compositor/initialization.dart index 7ea4053fa5c20..a4074ba3d7c5f 100644 --- a/lib/web_ui/lib/src/engine/compositor/initialization.dart +++ b/lib/web_ui/lib/src/engine/compositor/initialization.dart @@ -6,7 +6,8 @@ part of engine; /// EXPERIMENTAL: Enable the Skia-based rendering backend. -bool get experimentalUseSkia => webExperiments.useSkia; +const bool experimentalUseSkia = + bool.fromEnvironment('FLUTTER_WEB_USE_SKIA', defaultValue: false); /// The URL to use when downloading the CanvasKit script and associated wasm. /// diff --git a/lib/web_ui/lib/src/engine/web_experiments.dart b/lib/web_ui/lib/src/engine/web_experiments.dart index 96a3a571e1b39..ee60469eb6a53 100644 --- a/lib/web_ui/lib/src/engine/web_experiments.dart +++ b/lib/web_ui/lib/src/engine/web_experiments.dart @@ -5,8 +5,6 @@ // @dart = 2.6 part of engine; -const MethodCodec codec = JSONMethodCodec(); - final WebExperiments webExperiments = WebExperiments._(); /// A bag of all experiment flags in the web engine. @@ -14,42 +12,32 @@ final WebExperiments webExperiments = WebExperiments._(); /// This class also handles platform messages that can be sent to enable/disable /// certain experiments at runtime without the need to access engine internals. class WebExperiments { - WebExperiments._(); - - /// Experiment flag for using the Skia-based rendering backend. - bool get useSkia => _useSkia ?? false; - set useSkia(bool enabled) { - _useSkia = enabled; + WebExperiments._() { + js.context['_flutter_internal_update_experiment'] = updateExperiment; } - bool _useSkia = const bool.fromEnvironment('FLUTTER_WEB_USE_SKIA'); /// Experiment flag for using canvas-based text measurement. bool get useCanvasText => _useCanvasText ?? false; set useCanvasText(bool enabled) { _useCanvasText = enabled; } - bool _useCanvasText = - const bool.fromEnvironment('FLUTTER_WEB_USE_EXPERIMENTAL_CANVAS_TEXT'); + + bool _useCanvasText = const bool.fromEnvironment( + 'FLUTTER_WEB_USE_EXPERIMENTAL_CANVAS_TEXT', + defaultValue: null, + ); /// Reset all experimental flags to their default values. void reset() { - _useSkia = null; _useCanvasText = null; } - /// Handles the platform message used to enable/disable web experiments in the - /// web engine. - void enableWebExperiments(Map message) { - for (final String name in message.keys) { - final bool enabled = message[name]; - switch (name) { - case 'useSkia': - _useSkia = enabled; - break; - case 'useCanvasText': - _useCanvasText = enabled; - break; - } + /// Used to enable/disable experimental flags in the web engine. + void updateExperiment(String name, bool enabled) { + switch (name) { + case 'useCanvasText': + _useCanvasText = enabled; + break; } } } diff --git a/lib/web_ui/lib/src/engine/window.dart b/lib/web_ui/lib/src/engine/window.dart index c0b3bc50cd509..e2004cda29286 100644 --- a/lib/web_ui/lib/src/engine/window.dart +++ b/lib/web_ui/lib/src/engine/window.dart @@ -210,16 +210,6 @@ class EngineWindow extends ui.Window { break; } return; - - case 'flutter/experiments': - const MethodCodec codec = JSONMethodCodec(); - final MethodCall decoded = codec.decodeMethodCall(data); - final Map arguments = decoded.arguments; - switch (decoded.method) { - case 'enableWebExperiments': - webExperiments.enableWebExperiments(arguments); - return; - } } if (pluginMessageCallHandler != null) { diff --git a/lib/web_ui/test/engine/web_experiments_test.dart b/lib/web_ui/test/engine/web_experiments_test.dart index 0ff7a1348ae00..96ba9539b2c75 100644 --- a/lib/web_ui/test/engine/web_experiments_test.dart +++ b/lib/web_ui/test/engine/web_experiments_test.dart @@ -3,6 +3,9 @@ // found in the LICENSE file. // @dart = 2.6 +import 'dart:html' as html; +import 'dart:js_util' as js_util; + import 'package:test/test.dart'; import 'package:ui/src/engine.dart'; @@ -12,63 +15,55 @@ void main() { }); test('default web experiment values', () { - expect(webExperiments.useSkia, false); expect(webExperiments.useCanvasText, false); }); test('can turn on/off web experiments', () { - webExperiments.enableWebExperiments({ - 'useSkia': true, - }); - expect(webExperiments.useSkia, true); - expect(webExperiments.useCanvasText, false); - - webExperiments.enableWebExperiments({ - 'useSkia': false, - 'useCanvasText': true, - }); - expect(webExperiments.useSkia, false); + webExperiments.updateExperiment('useCanvasText', true); expect(webExperiments.useCanvasText, true); - webExperiments.enableWebExperiments({ - 'useSkia': true, - 'useCanvasText': null, - }); - expect(webExperiments.useSkia, true); - // Goes back to default value. + webExperiments.updateExperiment('useCanvasText', false); expect(webExperiments.useCanvasText, false); - webExperiments.enableWebExperiments({ - 'useSkia': null, - }); + webExperiments.updateExperiment('useCanvasText', null); // Goes back to default value. - expect(webExperiments.useSkia, false); + expect(webExperiments.useCanvasText, false); + }); + + test('ignores unknown experiments', () { + expect(webExperiments.useCanvasText, false); + webExperiments.updateExperiment('foobarbazqux', true); + expect(webExperiments.useCanvasText, false); + webExperiments.updateExperiment('foobarbazqux', false); expect(webExperiments.useCanvasText, false); }); test('can reset web experiments', () { - webExperiments.enableWebExperiments({ - 'useSkia': true, - 'useCanvasText': false, - }); + webExperiments.updateExperiment('useCanvasText', true); webExperiments.reset(); - expect(webExperiments.useSkia, false); expect(webExperiments.useCanvasText, false); - webExperiments.enableWebExperiments({ - 'useSkia': false, - 'useCanvasText': true, - }); + webExperiments.updateExperiment('useCanvasText', true); + webExperiments.updateExperiment('foobarbazqux', true); webExperiments.reset(); - expect(webExperiments.useSkia, false); expect(webExperiments.useCanvasText, false); + }); - webExperiments.enableWebExperiments({ - 'useSkia': true, - 'useCanvasText': true, - }); - webExperiments.reset(); - expect(webExperiments.useSkia, false); + test('js interop also works', () { + expect(webExperiments.useCanvasText, false); + + js_util.callMethod( + html.window, + '_flutter_internal_update_experiment', + ['useCanvasText', true], + ); + expect(webExperiments.useCanvasText, true); + + js_util.callMethod( + html.window, + '_flutter_internal_update_experiment', + ['useCanvasText', null], + ); expect(webExperiments.useCanvasText, false); }); } From 23cf2d868827ae7f3f3a7e4bbfb60b051ae11d71 Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Mon, 16 Mar 2020 10:18:13 -0700 Subject: [PATCH 3/6] more tests --- .../test/engine/web_experiments_test.dart | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/lib/web_ui/test/engine/web_experiments_test.dart b/lib/web_ui/test/engine/web_experiments_test.dart index 96ba9539b2c75..8318bcd2d92f3 100644 --- a/lib/web_ui/test/engine/web_experiments_test.dart +++ b/lib/web_ui/test/engine/web_experiments_test.dart @@ -52,18 +52,25 @@ void main() { test('js interop also works', () { expect(webExperiments.useCanvasText, false); - js_util.callMethod( - html.window, - '_flutter_internal_update_experiment', - ['useCanvasText', true], - ); + expect(() => jsUpdateExperiment('useCanvasText', true), returnsNormally); expect(webExperiments.useCanvasText, true); - js_util.callMethod( - html.window, - '_flutter_internal_update_experiment', - ['useCanvasText', null], - ); + expect(() => jsUpdateExperiment('useCanvasText', null), returnsNormally); expect(webExperiments.useCanvasText, false); }); + + test('js interop throws on wrong type', () { + expect(() => jsUpdateExperiment(123, true), throwsA(anything)); + expect(() => jsUpdateExperiment('foo', 123), throwsA(anything)); + expect(() => jsUpdateExperiment('foo', 'bar'), throwsA(anything)); + expect(() => jsUpdateExperiment(false, 'foo'), throwsA(anything)); + }); +} + +void jsUpdateExperiment(dynamic name, dynamic enabled) { + js_util.callMethod( + html.window, + '_flutter_internal_update_experiment', + [name, enabled], + ); } From a5cd13e9f46aabc1da36f6a9f9c3bbcc5b86fc59 Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Mon, 16 Mar 2020 10:37:34 -0700 Subject: [PATCH 4/6] fix missing license --- ci/licenses_golden/licenses_flutter | 1 + 1 file changed, 1 insertion(+) diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index c2a8ee725502c..1f029bb647d73 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -495,6 +495,7 @@ FILE: ../../../flutter/lib/web_ui/lib/src/engine/text_editing/text_editing.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/util.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/validators.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/vector_math.dart +FILE: ../../../flutter/lib/web_ui/lib/src/engine/web_experiments.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/window.dart FILE: ../../../flutter/lib/web_ui/lib/src/ui/annotations.dart FILE: ../../../flutter/lib/web_ui/lib/src/ui/canvas.dart From 351bda9f1d00830381a322d3d35ccfe460980ac1 Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Fri, 20 Mar 2020 13:35:02 -0700 Subject: [PATCH 5/6] address review comments --- lib/web_ui/lib/src/engine.dart | 3 +- .../lib/src/engine/text/measurement.dart | 2 +- .../lib/src/engine/web_experiments.dart | 14 ++++- .../test/engine/web_experiments_test.dart | 52 ++++++++++--------- .../test/golden_tests/engine/scuba.dart | 10 ++-- .../engine/text_overflow_golden_test.dart | 4 +- lib/web_ui/test/paragraph_test.dart | 24 ++++----- 7 files changed, 61 insertions(+), 48 deletions(-) diff --git a/lib/web_ui/lib/src/engine.dart b/lib/web_ui/lib/src/engine.dart index 4f4df2c8a0944..04e688ff090e7 100644 --- a/lib/web_ui/lib/src/engine.dart +++ b/lib/web_ui/lib/src/engine.dart @@ -162,8 +162,7 @@ void webOnlyInitializeEngine() { // initialize framework bindings. domRenderer; - // Calling this getter to force the [WebExperiments] instance to be created. - webExperiments; + WebExperiments.ensureInitialized(); bool waitingForAnimation = false; ui.webOnlyScheduleFrameCallback = () { diff --git a/lib/web_ui/lib/src/engine/text/measurement.dart b/lib/web_ui/lib/src/engine/text/measurement.dart index 814a7884c7d6c..30ae994538c1a 100644 --- a/lib/web_ui/lib/src/engine/text/measurement.dart +++ b/lib/web_ui/lib/src/engine/text/measurement.dart @@ -199,7 +199,7 @@ abstract class TextMeasurementService { // Skip using canvas measurements until the iframe becomes visible. // see: https://github.com/flutter/flutter/issues/36341 if (!window.physicalSize.isEmpty && - webExperiments.useCanvasText && + WebExperiments.instance.useCanvasText && _canUseCanvasMeasurement(paragraph)) { return canvasInstance; } diff --git a/lib/web_ui/lib/src/engine/web_experiments.dart b/lib/web_ui/lib/src/engine/web_experiments.dart index ee60469eb6a53..2e2050938e8df 100644 --- a/lib/web_ui/lib/src/engine/web_experiments.dart +++ b/lib/web_ui/lib/src/engine/web_experiments.dart @@ -5,8 +5,6 @@ // @dart = 2.6 part of engine; -final WebExperiments webExperiments = WebExperiments._(); - /// A bag of all experiment flags in the web engine. /// /// This class also handles platform messages that can be sent to enable/disable @@ -14,8 +12,20 @@ final WebExperiments webExperiments = WebExperiments._(); class WebExperiments { WebExperiments._() { js.context['_flutter_internal_update_experiment'] = updateExperiment; + registerHotRestartListener(() { + js.context['_flutter_internal_update_experiment'] = null; + }); } + static WebExperiments ensureInitialized() { + if (WebExperiments.instance == null) { + WebExperiments.instance = WebExperiments._(); + } + return WebExperiments.instance; + } + + static WebExperiments instance; + /// Experiment flag for using canvas-based text measurement. bool get useCanvasText => _useCanvasText ?? false; set useCanvasText(bool enabled) { diff --git a/lib/web_ui/test/engine/web_experiments_test.dart b/lib/web_ui/test/engine/web_experiments_test.dart index 8318bcd2d92f3..034cbba04b945 100644 --- a/lib/web_ui/test/engine/web_experiments_test.dart +++ b/lib/web_ui/test/engine/web_experiments_test.dart @@ -10,53 +10,57 @@ import 'package:test/test.dart'; import 'package:ui/src/engine.dart'; void main() { + setUp(() { + WebExperiments.ensureInitialized(); + }); + tearDown(() { - webExperiments.reset(); + WebExperiments.instance.reset(); }); test('default web experiment values', () { - expect(webExperiments.useCanvasText, false); + expect(WebExperiments.instance.useCanvasText, false); }); test('can turn on/off web experiments', () { - webExperiments.updateExperiment('useCanvasText', true); - expect(webExperiments.useCanvasText, true); + WebExperiments.instance.updateExperiment('useCanvasText', true); + expect(WebExperiments.instance.useCanvasText, true); - webExperiments.updateExperiment('useCanvasText', false); - expect(webExperiments.useCanvasText, false); + WebExperiments.instance.updateExperiment('useCanvasText', false); + expect(WebExperiments.instance.useCanvasText, false); - webExperiments.updateExperiment('useCanvasText', null); + WebExperiments.instance.updateExperiment('useCanvasText', null); // Goes back to default value. - expect(webExperiments.useCanvasText, false); + expect(WebExperiments.instance.useCanvasText, false); }); test('ignores unknown experiments', () { - expect(webExperiments.useCanvasText, false); - webExperiments.updateExperiment('foobarbazqux', true); - expect(webExperiments.useCanvasText, false); - webExperiments.updateExperiment('foobarbazqux', false); - expect(webExperiments.useCanvasText, false); + expect(WebExperiments.instance.useCanvasText, false); + WebExperiments.instance.updateExperiment('foobarbazqux', true); + expect(WebExperiments.instance.useCanvasText, false); + WebExperiments.instance.updateExperiment('foobarbazqux', false); + expect(WebExperiments.instance.useCanvasText, false); }); test('can reset web experiments', () { - webExperiments.updateExperiment('useCanvasText', true); - webExperiments.reset(); - expect(webExperiments.useCanvasText, false); - - webExperiments.updateExperiment('useCanvasText', true); - webExperiments.updateExperiment('foobarbazqux', true); - webExperiments.reset(); - expect(webExperiments.useCanvasText, false); + WebExperiments.instance.updateExperiment('useCanvasText', true); + WebExperiments.instance.reset(); + expect(WebExperiments.instance.useCanvasText, false); + + WebExperiments.instance.updateExperiment('useCanvasText', true); + WebExperiments.instance.updateExperiment('foobarbazqux', true); + WebExperiments.instance.reset(); + expect(WebExperiments.instance.useCanvasText, false); }); test('js interop also works', () { - expect(webExperiments.useCanvasText, false); + expect(WebExperiments.instance.useCanvasText, false); expect(() => jsUpdateExperiment('useCanvasText', true), returnsNormally); - expect(webExperiments.useCanvasText, true); + expect(WebExperiments.instance.useCanvasText, true); expect(() => jsUpdateExperiment('useCanvasText', null), returnsNormally); - expect(webExperiments.useCanvasText, false); + expect(WebExperiments.instance.useCanvasText, false); }); test('js interop throws on wrong type', () { diff --git a/lib/web_ui/test/golden_tests/engine/scuba.dart b/lib/web_ui/test/golden_tests/engine/scuba.dart index f334a2b0b91bf..6ae5739577be5 100644 --- a/lib/web_ui/test/golden_tests/engine/scuba.dart +++ b/lib/web_ui/test/golden_tests/engine/scuba.dart @@ -71,7 +71,7 @@ class EngineScubaTester { sceneElement.append(canvas.rootElement); html.document.body.append(sceneElement); String screenshotName = '${fileName}_${canvas.runtimeType}'; - if (webExperiments.useCanvasText) { + if (WebExperiments.instance.useCanvasText) { screenshotName += '+canvas_measurement'; } await diffScreenshot( @@ -96,20 +96,20 @@ void testEachCanvas(String description, CanvasTest body, test('$description (bitmap)', () { try { TextMeasurementService.initialize(rulerCacheCapacity: 2); - webExperiments.useCanvasText = false; + WebExperiments.instance.useCanvasText = false; return body(BitmapCanvas(bounds)); } finally { - webExperiments.useCanvasText = null; + WebExperiments.instance.useCanvasText = null; TextMeasurementService.clearCache(); } }); test('$description (bitmap + canvas measurement)', () async { try { TextMeasurementService.initialize(rulerCacheCapacity: 2); - webExperiments.useCanvasText = true; + WebExperiments.instance.useCanvasText = true; await body(BitmapCanvas(bounds)); } finally { - webExperiments.useCanvasText = null; + WebExperiments.instance.useCanvasText = null; TextMeasurementService.clearCache(); } }); diff --git a/lib/web_ui/test/golden_tests/engine/text_overflow_golden_test.dart b/lib/web_ui/test/golden_tests/engine/text_overflow_golden_test.dart index 978c53bb6e813..507be4fd33df9 100644 --- a/lib/web_ui/test/golden_tests/engine/text_overflow_golden_test.dart +++ b/lib/web_ui/test/golden_tests/engine/text_overflow_golden_test.dart @@ -89,7 +89,7 @@ void main() async { offset = offset.translate(0, p.height + 10); // Only the first line is rendered with an ellipsis. - if (!webExperiments.useCanvasText) { + if (!WebExperiments.instance.useCanvasText) { // This is now correct with the canvas-based measurement, so we shouldn't // print the "(wrong)" warning. p = warning('(wrong)'); @@ -106,7 +106,7 @@ void main() async { // Only the first two lines are rendered and the ellipsis appears on the 2nd // line. - if (!webExperiments.useCanvasText) { + if (!WebExperiments.instance.useCanvasText) { // This is now correct with the canvas-based measurement, so we shouldn't // print the "(wrong)" warning. p = warning('(wrong)'); diff --git a/lib/web_ui/test/paragraph_test.dart b/lib/web_ui/test/paragraph_test.dart index beb82bca20ea5..c3432a00bed1f 100644 --- a/lib/web_ui/test/paragraph_test.dart +++ b/lib/web_ui/test/paragraph_test.dart @@ -12,20 +12,20 @@ void testEachMeasurement(String description, VoidCallback body, {bool skip}) { test('$description (dom measurement)', () async { try { TextMeasurementService.initialize(rulerCacheCapacity: 2); - webExperiments.useCanvasText = false; + WebExperiments.instance.useCanvasText = false; return body(); } finally { - webExperiments.useCanvasText = null; + WebExperiments.instance.useCanvasText = null; TextMeasurementService.clearCache(); } }, skip: skip); test('$description (canvas measurement)', () async { try { TextMeasurementService.initialize(rulerCacheCapacity: 2); - webExperiments.useCanvasText = true; + WebExperiments.instance.useCanvasText = true; return body(); } finally { - webExperiments.useCanvasText = null; + WebExperiments.instance.useCanvasText = null; TextMeasurementService.clearCache(); } }, skip: skip); @@ -186,7 +186,7 @@ void main() async { test('getPositionForOffset multi-line', () { // [Paragraph.getPositionForOffset] for multi-line text doesn't work well // with dom-based measurement. - webExperiments.useCanvasText = true; + WebExperiments.instance.useCanvasText = true; TextMeasurementService.initialize(rulerCacheCapacity: 2); final ParagraphBuilder builder = ParagraphBuilder(ParagraphStyle( @@ -282,11 +282,11 @@ void main() async { ); TextMeasurementService.clearCache(); - webExperiments.useCanvasText = null; + WebExperiments.instance.useCanvasText = null; }); test('getPositionForOffset multi-line centered', () { - webExperiments.useCanvasText = true; + WebExperiments.instance.useCanvasText = true; TextMeasurementService.initialize(rulerCacheCapacity: 2); final ParagraphBuilder builder = ParagraphBuilder(ParagraphStyle( @@ -389,7 +389,7 @@ void main() async { ); TextMeasurementService.clearCache(); - webExperiments.useCanvasText = null; + WebExperiments.instance.useCanvasText = null; }); testEachMeasurement('getBoxesForRange returns a box', () { @@ -784,7 +784,7 @@ void main() async { test('longestLine', () { // [Paragraph.longestLine] is only supported by canvas-based measurement. - webExperiments.useCanvasText = true; + WebExperiments.instance.useCanvasText = true; TextMeasurementService.initialize(rulerCacheCapacity: 2); final ParagraphBuilder builder = ParagraphBuilder(ParagraphStyle( @@ -799,7 +799,7 @@ void main() async { expect(paragraph.longestLine, 50.0); TextMeasurementService.clearCache(); - webExperiments.useCanvasText = null; + WebExperiments.instance.useCanvasText = null; }); testEachMeasurement('getLineBoundary (single-line)', () { @@ -826,7 +826,7 @@ void main() async { test('getLineBoundary (multi-line)', () { // [Paragraph.getLineBoundary] for multi-line paragraphs is only supported // by canvas-based measurement. - webExperiments.useCanvasText = true; + WebExperiments.instance.useCanvasText = true; TextMeasurementService.initialize(rulerCacheCapacity: 2); final ParagraphBuilder builder = ParagraphBuilder(ParagraphStyle( @@ -869,7 +869,7 @@ void main() async { } TextMeasurementService.clearCache(); - webExperiments.useCanvasText = null; + WebExperiments.instance.useCanvasText = null; }); testEachMeasurement('width should be a whole integer', () { From c2114c43d44e6327de3e0a4b28ff0210d52f442b Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Mon, 23 Mar 2020 11:51:21 -0700 Subject: [PATCH 6/6] fix tests --- lib/web_ui/test/canvas_test.dart | 4 ++++ lib/web_ui/test/paragraph_builder_test.dart | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/lib/web_ui/test/canvas_test.dart b/lib/web_ui/test/canvas_test.dart index 363204a120d6e..3a63ee87085d7 100644 --- a/lib/web_ui/test/canvas_test.dart +++ b/lib/web_ui/test/canvas_test.dart @@ -11,6 +11,10 @@ import 'package:test/test.dart'; import 'mock_engine_canvas.dart'; void main() { + setUpAll(() { + WebExperiments.ensureInitialized(); + }); + group('EngineCanvas', () { MockEngineCanvas mockCanvas; ui.Paragraph paragraph; diff --git a/lib/web_ui/test/paragraph_builder_test.dart b/lib/web_ui/test/paragraph_builder_test.dart index 08ac0d778fbf2..54a3bcf53b046 100644 --- a/lib/web_ui/test/paragraph_builder_test.dart +++ b/lib/web_ui/test/paragraph_builder_test.dart @@ -3,11 +3,16 @@ // found in the LICENSE file. // @dart = 2.6 +import 'package:ui/src/engine.dart'; import 'package:ui/ui.dart'; import 'package:test/test.dart'; void main() { + setUpAll(() { + WebExperiments.ensureInitialized(); + }); + test('Should be able to build and layout a paragraph', () { final ParagraphBuilder builder = ParagraphBuilder(ParagraphStyle()); builder.addText('Hello');