[webview_benchmarks] Migrate to null safety #879

amanv8060 · 2022-02-17T09:56:51Z

Migrated package to null safety

List which issues are fixed by this PR. You must list at least one issue.
Fixes: #98647

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

amanv8060 · 2022-02-18T06:31:56Z

@yjbanov / @stuartmorgan . this is ready to be reviewed.
Thanks.

stuartmorgan

I've left specific notes inline; in general this is using far too much force unwrapping rather than making things actually type-safe.

packages/web_benchmarks/lib/client.dart

packages/web_benchmarks/lib/src/browser.dart

stuartmorgan · 2022-02-22T21:32:46Z

packages/web_benchmarks/lib/src/runner.dart

@@ -156,7 +155,7 @@ class BenchmarkServer {
          // Trace data is null when the benchmark is not frame-based, such as RawRecorder.
          if (latestPerformanceTrace != null) {
            final BlinkTraceSummary traceSummary =
-                BlinkTraceSummary.fromJson(latestPerformanceTrace);
+                BlinkTraceSummary.fromJson(latestPerformanceTrace!)!;


Throughout this function, look for opportunities to use locals so that the checks don't require force unwrapping.

BlinkTraceSummary.fromJson has not many things that can be replaced with locals, it returns null only in one case.

packages/web_benchmarks/pubspec.yaml

amanv8060 · 2022-03-13T22:22:22Z

@stuartmorgan should I also update dependencies in pubspec.yaml? Since the versions used were to support non-null safe code with +2 to allow null safe.

stuartmorgan · 2022-03-25T19:55:59Z

@stuartmorgan should I also update dependencies in pubspec.yaml? Since the versions used were to support non-null safe code with +2 to allow null safe.

Yes, a null-safe package should require null-safe dependencies.

stuartmorgan · 2022-03-25T19:56:37Z

@yjbanov should do the primary review on this.

stuartmorgan · 2022-04-26T19:55:40Z

@yjbanov Ping on this review?

mdebbar

I have a few suggestions, but otherwise it looks good!

Thanks for migrating the package!

packages/web_benchmarks/lib/client.dart

packages/web_benchmarks/lib/src/browser.dart

mdebbar · 2022-05-11T15:20:15Z

packages/web_benchmarks/lib/src/browser.dart

-    WipConnection debugConnection;
-    if (withDebugging) {
+    WipConnection? debugConnection;
+    if (options.debugPort != null) {


Same here, use a local variable.

sure, made it in sync with this comment as well.

packages/web_benchmarks/lib/src/recorder.dart

amanv8060 · 2022-05-18T07:24:09Z

@mdebbar I have done the changes.

packages/web_benchmarks/lib/src/recorder.dart

mdebbar

stuartmorgan · 2022-05-18T18:04:56Z

packages/web_benchmarks/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.0.8


Are there really no breaking changes here? That's very unusual for an NNBD transition, and I see public API that takes non-nullable values which looks breaking to me (since previously, e.g., int was equivalent to int? in NNBD code, not int).

Can you please direct me to it. None of the changes look breaking to me.recorder.dart#L66 is one , but it was expected to be non null previously as well.

Can you please direct me to it.

https://github.com/flutter/packages/pull/879/files#diff-97332f3a9c3e68b393026c3f408eb96ec266dd389c858623f87ad97096207a63R43-R45 for example, is changing three public parameters to non-nullable types.

None of the changes look breaking to me.recorder.dart#L66 is one , but it was expected to be non null previously as well.

I haven't looked at all of the logic, which is why it's a question. Maybe in practice nothing changes, it's just unusual in an NNBD migration.

I agree with @stuartmorgan. I think this needs to be version 0.1.0. The dart docs recommend indicating the NNBD migration as breaking change:

Update the version of the package to indicate a breaking change:

If your package is already at 1.0.0 or greater, increase the major version. For example, if the previous version is 2.3.2, the new version is 3.0.0.

If your package hasn’t reached 1.0.0 yet, either increase the minor version or update the version to 1.0.0. For example, if the previous version is 0.3.2, the new version is either 0.4.0 or 1.0.0.

https://dart.dev/null-safety/migration-guide#package-version

Note that there are (unsurprisingly) 0 published packages depending on this, which means the ecosystem cost of making this a breaking version change even if it doesn't turn out to be strictly necessary is essentially zero.

@stuartmorgan since there are multiple such cases, i have added a generic line regarding breaking change. Also updated the version constraint

packages/web_benchmarks/testing/test_app/lib/benchmarks/runner.dart

stuartmorgan · 2022-06-07T20:41:43Z

packages/web_benchmarks/lib/src/browser.dart

        if (value is! List) {
          throw FormatException(
              '"Tracing.dataCollected" returned malformed data. '
              'Expected a List but got: ${value.runtimeType}');
        }
-        _tracingData.addAll(event.params['value'].cast<Map<String, dynamic>>());
+        _tracingData!


There's enough separation here and for _tracingCompleter that this is fragile; it would be safer to make new non-nullable locals for both, and then set the fields to the local variable.

stuartmorgan · 2022-06-07T20:44:19Z

packages/web_benchmarks/lib/src/recorder.dart

-  Profile profile;
-  Completer<void> _runCompleter;
+  late Profile profile;
+  Completer<void>? _runCompleter;


This is force-unwrapped a lot; please add a comment explaining which methods it's guaranteed to be non-null in and why.

stuartmorgan · 2022-06-07T20:46:55Z

packages/web_benchmarks/lib/src/recorder.dart


  /// The number of frames ignored as warm-up frames.
-  int get warmUpFrameCount =>
+  int? get warmUpFrameCount =>


If _warmUpFrameCount is expected to be non-null whenever useCustomWarmUp is true, then shouldn't this return be non-nullable, and _warmUpFrameCount be force-unwrapped in the next line?

stuartmorgan · 2022-06-07T20:47:36Z

packages/web_benchmarks/lib/src/recorder.dart

    if (value < 0.0) {
      throw StateError(
        'Timeseries $name: negative metric values are not supported. Got: $value',
      );
    }
    _allValues.add(value);
    if (useCustomWarmUp && isWarmUpValue) {
-      _warmUpFrameCount += 1;
+      _warmUpFrameCount = _warmUpFrameCount! + 1;


In which case the RHS here can use the getter, and not need to force-unwrap.

stuartmorgan · 2022-06-07T20:48:46Z

packages/web_benchmarks/lib/src/recorder.dart

      _RecordingWidgetsBinding();
    }
    return _RecordingWidgetsBinding.instance;
  }

-  FrameRecorder _recorder;
+  FrameRecorder? _recorder;


Similar to my earlier comment, this needs a comment explaining when and why it's safe to force-unwrap.

stuartmorgan · 2022-06-07T20:49:28Z

packages/web_benchmarks/lib/src/runner.dart

@@ -117,16 +116,16 @@ class BenchmarkServer {
        Completer<List<Map<String, dynamic>>>();
    final List<Map<String, dynamic>> collectedProfiles =
        <Map<String, dynamic>>[];
-    List<String> benchmarks;
-    Iterator<String> benchmarkIterator;
+    List<String>? benchmarks;


Why is this nullable? We strongly prefer non-nullable collections to avoid having two ways of expressing empty, which is very error-prone to use.

If it is null, we populate it here.

yjbanov

LGTM if this is still passing tests. Let's rebase and see?

yjbanov · 2022-08-11T22:10:10Z

packages/web_benchmarks/lib/src/browser.dart

+        io.Platform.environment['PROGRAMFILES'],
+        io.Platform.environment['PROGRAMFILES(X86)'],
+      ])
+        if (item != null) item


You can also do ...array.whereNotNull()

https://api.flutter.dev/flutter/package-collection_collection/IterableNullableExtension/whereNotNull.html

this got off my radar, will update this today.

...<String?>[ io.Platform.environment['LOCALAPPDATA'], io.Platform.environment['PROGRAMFILES'], io.Platform.environment['PROGRAMFILES(X86)'], ].whereType<String>()

could we do this ?

This reverts commit 490ca5f.

github-actions bot added the p: web_benchmarks label Feb 17, 2022

amanv8060 marked this pull request as ready for review February 17, 2022 18:58

stuartmorgan requested review from yjbanov and stuartmorgan February 22, 2022 21:04

stuartmorgan requested changes Feb 22, 2022

View reviewed changes

amanv8060 force-pushed the migrate_webview_benchmarks branch from 68a65d7 to 9ab6bea Compare March 13, 2022 20:00

amanv8060 requested a review from stuartmorgan March 13, 2022 22:32

amanv8060 force-pushed the migrate_webview_benchmarks branch from 6f3b4f5 to 4fdfbd6 Compare March 26, 2022 13:00

amanv8060 mentioned this pull request Mar 30, 2022

refactor: migrate test , test_driver , test_goldens flutter/gallery#627

Merged

8 tasks

amanv8060 mentioned this pull request Apr 28, 2022

Migrate to null safety flutter/gallery#527

Closed

mdebbar self-requested a review May 11, 2022 15:06

mdebbar reviewed May 11, 2022

View reviewed changes

amanv8060 force-pushed the migrate_webview_benchmarks branch from 4fdfbd6 to 0ad647c Compare May 12, 2022 20:16

amanv8060 requested a review from mdebbar May 14, 2022 08:23

mdebbar reviewed May 18, 2022

View reviewed changes

packages/web_benchmarks/lib/src/recorder.dart Outdated Show resolved Hide resolved

packages/web_benchmarks/lib/src/recorder.dart Outdated Show resolved Hide resolved

packages/web_benchmarks/lib/src/recorder.dart Outdated Show resolved Hide resolved

mdebbar approved these changes May 18, 2022

View reviewed changes

stuartmorgan reviewed May 18, 2022

View reviewed changes

amanv8060 force-pushed the migrate_webview_benchmarks branch 2 times, most recently from fc65553 to 319ef00 Compare May 26, 2022 09:45

amanv8060 requested a review from stuartmorgan May 26, 2022 13:00

stuartmorgan requested changes Jun 7, 2022

View reviewed changes

amanv8060 marked this pull request as draft June 15, 2022 07:13

yjbanov approved these changes Aug 11, 2022

View reviewed changes

Initial Migration

31b4987

amanv8060 added 15 commits August 14, 2022 00:29

preliminary migration of remaining files

e47ec7c

Format

1ca133a

Final migration check

14454d2

Make analyzer happy

bd27ed3

remove lint and use ambiguate

4335c40

fix some force unwrappings

a0a6973

fix more null safety issues

9e1bb08

more fixes

16c79d6

fix: upgrade dependeny versions

a73c4d5

apply suggestions

b2af9f8

fix format and a issue, other not fixed yet

fd3c721

fix failing test

486f35e

test remove nullable

0fa3264

remove ambiguate

8a5927c

Update pubspec

0e1362b

amanv8060 force-pushed the migrate_webview_benchmarks branch from 319ef00 to 0e1362b Compare August 13, 2022 18:59

address comments

56974bf

amanv8060 marked this pull request as ready for review August 14, 2022 04:20

update packages and where method

490ca5f

amanv8060 force-pushed the migrate_webview_benchmarks branch from ec682d1 to 490ca5f Compare August 14, 2022 04:34

amanv8060 added 2 commits August 14, 2022 14:26

empty commit to retrigger test

e48ba5b

Revert "update packages and where method"

f634ba5

This reverts commit 490ca5f.

amanv8060 requested a review from stuartmorgan August 23, 2022 19:33

stuartmorgan approved these changes Aug 24, 2022

View reviewed changes

Merge branch 'main' into migrate_webview_benchmarks

45febab

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 24, 2022

auto-submit bot merged commit 2e17cff into flutter:main Aug 24, 2022

amanv8060 deleted the migrate_webview_benchmarks branch August 24, 2022 20:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[webview_benchmarks] Migrate to null safety #879

[webview_benchmarks] Migrate to null safety #879

amanv8060 commented Feb 17, 2022 •

edited

amanv8060 commented Feb 18, 2022

stuartmorgan left a comment

stuartmorgan Feb 22, 2022

amanv8060 Mar 13, 2022

amanv8060 commented Mar 13, 2022

stuartmorgan commented Mar 25, 2022

stuartmorgan commented Mar 25, 2022

stuartmorgan commented Apr 26, 2022

mdebbar left a comment

mdebbar May 11, 2022

amanv8060 May 12, 2022

amanv8060 commented May 18, 2022

mdebbar left a comment

stuartmorgan May 18, 2022

amanv8060 May 25, 2022

stuartmorgan May 25, 2022

nilsreichardt May 25, 2022

stuartmorgan May 25, 2022

amanv8060 May 26, 2022

stuartmorgan Jun 7, 2022

stuartmorgan Jun 7, 2022

stuartmorgan Jun 7, 2022

stuartmorgan Jun 7, 2022

stuartmorgan Jun 7, 2022

stuartmorgan Jun 7, 2022

amanv8060 Jun 11, 2022

yjbanov left a comment

yjbanov Aug 11, 2022

amanv8060 Aug 12, 2022

amanv8060 Aug 14, 2022

[webview_benchmarks] Migrate to null safety #879

[webview_benchmarks] Migrate to null safety #879

Conversation

amanv8060 commented Feb 17, 2022 • edited

Pre-launch Checklist

amanv8060 commented Feb 18, 2022

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanv8060 commented Mar 13, 2022

stuartmorgan commented Mar 25, 2022

stuartmorgan commented Mar 25, 2022

stuartmorgan commented Apr 26, 2022

mdebbar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanv8060 commented May 18, 2022

mdebbar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yjbanov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanv8060 commented Feb 17, 2022 •

edited