take web_ui to null safety #19027

yjbanov · 2020-06-12T21:21:28Z

Description

Convert the web version of dart:ui to null safety syntax.

Related Issues

flutter/flutter#53661

vsmenon

Nice!

vsmenon · 2020-06-12T22:18:59Z

lib/web_ui/lib/src/engine/platform_views.dart

+// TODO(yjbanov): The code in this file was temporarily moved to lib/web_ui/lib/ui.dart
+//                during the NNBD migration so that `dart:ui` does not have to export
+//                `dart:_engine`. NNBD does not allow exported non-migrated libraries
+//                from migrated libraries.


@leafpetersen - is this a bug or an intended restriction?

This was intentional. We could remove the restriction, but I was aiming to have the property that opted in packages had no legacy types in their public interface, so that if you migrated relative to an opted in package you could be confident that you were done.

vsmenon · 2020-06-12T22:19:11Z

lib/web_ui/lib/src/engine/semantics/image.dart


  void _setLabel(html.Element element) {
    if (semanticsObject.hasLabel) {
+      print('>>> semanticsObject.label = ${semanticsObject.label}');


And a few more below.

This is just a draft PR. I exported it early to help reproduce dart-lang/sdk#42324. Sorry if it wasn't clear.

Cleaned it up.

vsmenon · 2020-06-12T22:22:58Z

lib/web_ui/lib/src/engine/util.dart

 /// Return value should be null on success, and a string error message on
 /// failure.
-typedef Callbacker<T> = String Function(Callback<T> callback);
+typedef Callbacker<T> = String/*?*/ Function(Callback<T> callback);


Do you mean to keep comment syntax here?

For now. This PR only migrates dart:ui. dart:_engine will follow next.

vsmenon · 2020-06-12T22:32:06Z

lib/web_ui/lib/src/ui/channel_buffers.dart

      final T item = _queue.removeFirst();
      if (_dropItemCallback != null) {
-        _dropItemCallback(item);
+        _dropItemCallback!(item);


Could replace if block with:

_dropItemCallback?.call(item);

vsmenon · 2020-06-12T22:34:11Z

lib/web_ui/lib/src/ui/initialization.dart

-engine.AssetManager/*?*/ _assetManager;
-engine.FontCollection _fontCollection;
+engine.AssetManager? _assetManager;
+engine.FontCollection? _fontCollection;


Should this be a late?

Haven't been lucky with late yet, but maybe I'm not trying hard enough. The issue is most of our initialization patterns rely on null checks, e.g. if (foo == null) initializeFoo();. However, late non-null variables cannot be null checked.

It's OK for this PR to be incomplete. The goal is to move to the new syntax and to have a more or less correct public API surface. Internal parts can be revisited later.

Filed flutter/flutter#59371 to revisit this.

Looks to me like it could be late final engine.FontCollection _fontCollection = engine.FontCollection();

leafpetersen · 2020-06-12T22:43:38Z

lib/web_ui/lib/src/engine/compositor/initialization.dart

 SkiaFontCollection skiaFontCollection;

+/// Initializes [skiaFontCollection].
+void ensureSkiaFontCollectionInitialized() {


Once this file is opted in, can this become late SkiaFontCollection = skiaFontCollection? Maybe final if it doesn't get reset?

Hopefully. Filed flutter/flutter#59371 to revisit this.

leafpetersen · 2020-06-12T22:49:50Z

lib/web_ui/lib/src/ui/canvas.dart

-  void clipRect(Rect/*!*/ rect,
-      {ClipOp clipOp/*!*/ = ClipOp.intersect, bool/*!*/ doAntiAlias = true});
+  void clipRect(Rect rect,
+      {ClipOp clipOp/*!*/ = ClipOp.intersect, bool doAntiAlias = true});


Leftover comment syntax?

Good catch. The comment syntax was on the variable, not on the type, so the migration tool missed it. Fixed.

jonahwilliams · 2020-06-12T23:24:54Z

lib/web_ui/lib/src/ui/canvas.dart

  ///  * [BlendMode], which discusses the use of [Paint.blendMode] with
  ///    [saveLayer].
-  void saveLayer(Rect/*?*/ bounds, Paint/*!*/ paint);
+  void saveLayer(Rect? bounds, Paint paint);


How are you validating that these APIs migrated to the same types as the other dart:ui?

Fixed api_confrom_test.dart, but I'm not sure how to validate parameters. Can't find the right fields in the AST nodes 🤔 (maybe just tired at the end of the week)

jonahwilliams · 2020-06-12T23:37:17Z

lib/web_ui/pubspec.yaml


 dependencies:
-  meta: 1.1.8
+  meta: ^1.1.8


do you still need meta?

Yeah, we have tests and dev tools using it.

jonahwilliams · 2020-06-12T23:37:35Z

lib/web_ui/test/engine/semantics/semantics_test.dart

    }
    expect(semantics().semanticsEnabled, true);
  });



Assuming that if tests pass they were migrated correctly

Of course 😎

jonahwilliams

LGTM

jonahwilliams · 2020-06-12T23:38:14Z

(But not to CI)

leafpetersen · 2020-06-13T00:36:39Z

lib/web_ui/lib/src/ui/geometry.dart

-  static Rect/*?*/ lerp(Rect/*?*/ a, Rect/*?*/ b, double/*!*/ t) {
+  static Rect? lerp(Rect? a, Rect? b, double t) {
    assert(t != null);
    if (b == null) {


Ok, I just have to say - how cool is it that this code just worked with no changes?

I did change this function in a previous PR to assist with type inference: f581f42#diff-3afaf9265618b9fee97b10a7de58cc76

But it's still pretty cool that despite both a and b being nullable, there are no ! in the body of the function.

Ah, I almost asked why it wasn't written the way it was pre-refactor... :)

If avoiding the extra instructions is important, you could write it as:

if (a == null) return (b == null) ? null : b * t; if (b == null) return (a == null) ? null : a * (1.0 - t) return ....

I also noticed that there were places where this was called with what looked like definitely non-nullable arguments, which made me wonder whether it was worth having a specialized version. But I have no idea which pieces of this are perf sensitive, so...

It would be nice to have an annotation instructing the compiler to specialize automatically, similar to Scala's @specialized.

Consider filing a feature request for @specialized.

Done: dart-lang/sdk#42361

leafpetersen · 2020-06-13T00:46:06Z

lib/web_ui/lib/src/ui/lerp.dart

  a ??= 0.0;
  b ??= 0.0;
-  return a + (b - a) * t;
+  return a + (b - a) * t as double?;


just as double. The result can't be null.

@stereotype441 good feedback for the tool?

leafpetersen · 2020-06-13T01:12:18Z

lib/web_ui/lib/ui.dart

+  callback(codec.encodeSuccessEnvelope(null));
+}
+
+// TODO(yjbanov): remove _Callback, _Callbacker, and _futurize. They are here only


I think this bug just got fixed in master, but probably hasn't rolled to you yet.

yjbanov · 2020-06-13T02:16:23Z

(But not to CI)

I keep forgetting that non-nullable experiment in analysis_options.yaml is not enough. You also have to pass it to dartanalyzer via command-line options.

zanderso · 2020-06-16T04:28:44Z

lib/web_ui/dev/integration_tests_manager.dart

-    await chromeDriverInstaller.install(alwaysInstall: true);
+    // TODO(yjbanov): remove this dynamic hack when chromeDriverInstaller.install returns Future<void>
+    //                https://github.com/flutter/flutter/issues/59376
+    dynamic installationFuture = chromeDriverInstaller.install(alwaysInstall: true) as dynamic;


zanderso · 2020-06-16T04:37:23Z

lib/web_ui/lib/src/ui/geometry.dart

  ///
  /// The `center` argument is assumed to be an offset from the origin.
-  Rect.fromCenter({ Offset center/*!*/, double/*!*/ width, double/*!*/ height }) : this.fromLTRB(
+  Rect.fromCenter({ required Offset center/*!*/, required double width, required double height }) : this.fromLTRB(


Stray /*!*/ ?

zanderso · 2020-06-16T04:41:15Z

lib/web_ui/lib/src/ui/geometry.dart

-  static Rect/*?*/ lerp(Rect/*?*/ a, Rect/*?*/ b, double/*!*/ t) {
+  static Rect? lerp(Rect? a, Rect? b, double t) {
    assert(t != null);
    if (b == null) {


Consider filing a feature request for @specialized.

zanderso · 2020-06-16T04:57:56Z

lib/web_ui/lib/src/ui/window.dart

  ///    [Locale] objects.
-  String/*!*/ get languageCode => _deprecatedLanguageSubtagMap[_languageCode] ?? _languageCode;
-  final String/*!*/ _languageCode;
+  String get languageCode => _deprecatedLanguageSubtagMap[_languageCode] ?? _languageCode;


Should the value type of _deprecatedLanguageSubtagMap be String?? Or does the ? get tacked onto e.g. the return type of operator[]?

Yes, operator[] always returns nullable type, no matter what type arguments the map has.

yjbanov · 2020-06-16T18:51:33Z

@zanderso

Consider filing a feature request for @specialized.

dart-lang/sdk#42361

googlebot added the cla: yes label Jun 12, 2020

yjbanov mentioned this pull request Jun 12, 2020

nnbd: dart2js leaks _Required placeholder values dart-lang/sdk#42324

Closed

yjbanov requested review from leafpetersen and vsmenon June 12, 2020 21:29

vsmenon reviewed Jun 12, 2020

View reviewed changes

leafpetersen reviewed Jun 12, 2020

View reviewed changes

yjbanov marked this pull request as ready for review June 12, 2020 23:19

yjbanov requested review from jonahwilliams and zanderso June 12, 2020 23:20

jonahwilliams reviewed Jun 12, 2020

View reviewed changes

jonahwilliams approved these changes Jun 12, 2020

View reviewed changes

leafpetersen reviewed Jun 13, 2020

View reviewed changes

yjbanov force-pushed the nnbd-migrate-apply-web-ui branch from 8822517 to f0259c0 Compare June 15, 2020 16:34

zanderso reviewed Jun 16, 2020

View reviewed changes

zanderso approved these changes Jun 16, 2020

View reviewed changes

yjbanov added 7 commits June 16, 2020 11:58

take web_ui to null safety

243a130

nullability fixes

c0a0f1f

address comments

a36fa30

ignore unnecessary_null_comparison

ac05da8

restore api_conform_test.dart

3f47f96

non-nullable analysis options

55456d7

cast expression to non-null double

1e96807

engine-flutter-autoroll mentioned this pull request Jun 17, 2020

Roll Engine from 965fbbed1776 to 43ad8347e148 (89 revisions) flutter/flutter#59652

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020

00aa0bc take web_ui to null safety (flutter/engine#19027)

f8b3fbb

engine-flutter-autoroll mentioned this pull request Jun 17, 2020

Roll Engine from 965fbbed1776 to 137320b5c7a4 (90 revisions) flutter/flutter#59656

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020

00aa0bc take web_ui to null safety (flutter/engine#19027)

4ca69d5

engine-flutter-autoroll mentioned this pull request Jun 17, 2020

Roll Engine from 965fbbed1776 to 9dc08393bedd (91 revisions) flutter/flutter#59660

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020

00aa0bc take web_ui to null safety (flutter/engine#19027)

8bd2155

engine-flutter-autoroll mentioned this pull request Jun 17, 2020

Roll Engine from 965fbbed1776 to ef29ce68aa24 (92 revisions) flutter/flutter#59664

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020

00aa0bc take web_ui to null safety (flutter/engine#19027)

b0ae4cc

engine-flutter-autoroll mentioned this pull request Jun 17, 2020

Roll Engine from 965fbbed1776 to f4a5f9f4486d (93 revisions) flutter/flutter#59665

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020

00aa0bc take web_ui to null safety (flutter/engine#19027)

325a203

engine-flutter-autoroll mentioned this pull request Jun 17, 2020

Roll Engine from 965fbbed1776 to c9c80011bb1f (94 revisions) flutter/flutter#59671

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020

00aa0bc take web_ui to null safety (flutter/engine#19027)

1a4248e

engine-flutter-autoroll mentioned this pull request Jun 17, 2020

Roll Engine from 965fbbed1776 to 237b5f32eff8 (95 revisions) flutter/flutter#59681

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020

00aa0bc take web_ui to null safety (flutter/engine#19027)

ba9ddb4

engine-flutter-autoroll mentioned this pull request Jun 17, 2020

Roll Engine from 965fbbed1776 to d14958ea4db2 (103 revisions) flutter/flutter#59713

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020

00aa0bc take web_ui to null safety (flutter/engine#19027)

1c92594

engine-flutter-autoroll mentioned this pull request Jun 17, 2020

Roll Engine from 965fbbed1776 to e9688f8e362a (106 revisions) flutter/flutter#59722

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2020

00aa0bc take web_ui to null safety (flutter/engine#19027)

ecf8c15

guidezpl mentioned this pull request Jun 18, 2020

Flutter Web Hot Refresh Broken - :59527/dart_sdk.js:133084 Uncaught (in promise) TypeError: Cannot read property 'ensureFontsLoaded' of null flutter/flutter#59718

Closed

engine-flutter-autoroll mentioned this pull request Jun 18, 2020

Roll Engine from 965fbbed1776 to e7ffcb2970f2 (119 revisions) flutter/flutter#59776

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 18, 2020

00aa0bc take web_ui to null safety (flutter/engine#19027)

040e7a7

engine-flutter-autoroll mentioned this pull request Jun 18, 2020

Roll Engine from 965fbbed1776 to 0266e60a0834 (121 revisions) flutter/flutter#59783

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 18, 2020

00aa0bc take web_ui to null safety (flutter/engine#19027)

6bd8105

engine-flutter-autoroll mentioned this pull request Jun 18, 2020

Roll Engine from 965fbbed1776 to 5157a6b2107d (122 revisions) flutter/flutter#59790

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 18, 2020

00aa0bc take web_ui to null safety (flutter/engine#19027)

0d57837

engine-flutter-autoroll mentioned this pull request Jun 18, 2020

Roll Engine from 965fbbed1776 to b7f8d2d11040 (125 revisions) flutter/flutter#59795

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 18, 2020

00aa0bc take web_ui to null safety (flutter/engine#19027)

e6e98ee

engine-flutter-autoroll mentioned this pull request Jun 18, 2020

Roll Engine from 965fbbed1776 to b5f5e6332cb4 (127 revisions) flutter/flutter#59806

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 18, 2020

00aa0bc take web_ui to null safety (flutter/engine#19027)

3b03e1d

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 18, 2020

00aa0bc take web_ui to null safety (flutter/engine#19027)

06db9b0

take web_ui to null safety #19027

take web_ui to null safety #19027

Uh oh!

Conversation

yjbanov commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Uh oh!

vsmenon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Jun 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

yjbanov commented Jun 12, 2020 •

edited

Loading