migrate rendering to nullsafety #64621

Hixie · 2020-08-26T07:36:25Z

This is a continuation of #64456, the nnbd migration of rendering/.

googlebot · 2020-08-26T07:36:30Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

mit-mit · 2020-08-26T12:29:54Z

Related to dart-lang/sdk#40146

a14n · 2020-08-26T13:11:34Z

packages/flutter/lib/src/rendering/box.dart

+  ///    [addWithPaintOffset].
+  ///
+  ///  * `paintTransform` has the semantics of the `transform` passed to
+  ///    [addWithPaintTransform], except thit it must be invertible; it


packages/flutter/lib/src/rendering/list_wheel_viewport.dart

a14n · 2020-08-26T13:25:01Z

packages/flutter/lib/src/rendering/viewport.dart

@@ -165,6 +165,11 @@ abstract class RenderViewportBase<ParentDataClass extends ContainerParentDataMix
    extends RenderBox with ContainerRenderObjectMixin<RenderSliver, ParentDataClass>
    implements RenderAbstractViewport {
  /// Initializes fields for subclasses.
+  /// Initializes fields for subclasses.


Duplicated line

a14n · 2020-08-26T13:28:02Z

Looks good but CI is failing

googlebot · 2020-08-26T21:51:22Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

Hixie · 2020-08-26T21:51:46Z

(overriding clabot status because @a14n and I are working together on this)

googlebot · 2020-08-26T22:15:20Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2020-08-27T00:21:39Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

a14n

LGTM

fluttergithubbot · 2020-08-27T10:18:04Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite testonly_devicelab_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

goderbauer

LGTM after comment is resolved

goderbauer · 2020-08-27T16:30:11Z

packages/flutter/lib/src/widgets/table.dart

-  final Map<int, TableColumnWidth> columnWidths;
+  ///
+  /// If this is set to null, then an empty map is assumed.
+  final Map<int, TableColumnWidth>/*?*/ columnWidths;


Why is this still specified as an annotation?

because this file is still opted-out (in widgets)

Ah, that makes sense! Thanks.

we haven't migrated widgets/ yet. This is just making sure we make the right call when we get to it.

jonahwilliams · 2020-08-27T16:34:48Z

packages/flutter/lib/src/foundation/diagnostics.dart

    String joiner = ', ',
    DiagnosticLevel minLevel = DiagnosticLevel.debug,
  }) {
-    if (kReleaseMode) {


IIRC the reason these were added is that dart2js could not optimize the function otherwise. If that is still the case I would expect it to show up on the web size benchmarks

Based on the history, @dnfield added the original if (kReleaseMode) return logic, then @ferhatb added the assert() around the body. This removes the if condition and leaves the assert so that the method can be non-nullable. I think it was ferhat's change that improved size, and this keeps that logic. Let's keep an eye on the numbers though, certainly...

Ahh, yeah - I think I was remembering it backwards

The problem with making this an assert is that profile mode will no longer show as much helpful information. But I guess we don't have any tests covering that and no one is complaining about it so oh well.

My general opinion on this is release and profile should be as close to identical as possible except for the minimum required to actually enable collecting metrics.

jonahwilliams · 2020-08-27T16:35:51Z

packages/flutter/lib/src/rendering/custom_paint.dart

@@ -830,52 +827,52 @@ class RenderCustomPaint extends RenderProxyBox {
      config.isChecked = properties.checked;
    }
    if (properties.selected != null) {


This function makes me sad

jonahwilliams

LGTM

googlebot · 2020-08-27T17:41:26Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2020-08-27T17:42:51Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

ferhatb · 2020-08-28T17:00:38Z

Yes profile mode info is reduced but closer to release build. This also allowed us to write an integration test to detect if treeshaking regresses.

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Aug 26, 2020

googlebot added the cla: no label Aug 26, 2020

Hixie marked this pull request as draft August 26, 2020 07:36

Hixie mentioned this pull request Aug 26, 2020

migrate rendering to nullsafety #64456

Closed

13 tasks

a14n reviewed Aug 26, 2020

View reviewed changes

packages/flutter/lib/src/rendering/list_wheel_viewport.dart Show resolved Hide resolved

a14n reviewed Aug 26, 2020

View reviewed changes

Hixie force-pushed the nnbd-rendering branch 4 times, most recently from e812f9a to cd4421c Compare August 26, 2020 21:49

Hixie added cla: yes and removed cla: no labels Aug 26, 2020

Hixie force-pushed the nnbd-rendering branch from cd4421c to 0ac9dce Compare August 26, 2020 22:15

googlebot added cla: no and removed cla: yes labels Aug 26, 2020

Hixie force-pushed the nnbd-rendering branch 2 times, most recently from 93e5b51 to 06dea30 Compare August 26, 2020 23:53

Hixie marked this pull request as ready for review August 27, 2020 00:21

Hixie force-pushed the nnbd-rendering branch from 06dea30 to 795921d Compare August 27, 2020 00:21

flutter-dashboard bot added the a: tests "flutter test", flutter_test, or one of our tests label Aug 27, 2020

Hixie added cla: yes and removed cla: no labels Aug 27, 2020

a14n approved these changes Aug 27, 2020

View reviewed changes

Hixie added the waiting for tree to go green label Aug 27, 2020

fluttergithubbot removed the waiting for tree to go green label Aug 27, 2020

goderbauer approved these changes Aug 27, 2020

View reviewed changes

jonahwilliams reviewed Aug 27, 2020

View reviewed changes

jonahwilliams approved these changes Aug 27, 2020

View reviewed changes

a14n and others added 4 commits August 27, 2020 10:41

migrate rendering to nullsafety

bef8fd6

Migrate newly merged code

1a6846a

Apply more review comments

8857c2c

More fixes

ca8ddb4

Hixie force-pushed the nnbd-rendering branch from 795921d to ca8ddb4 Compare August 27, 2020 17:41

googlebot added cla: no and removed cla: yes labels Aug 27, 2020

Hixie added cla: yes and removed cla: no labels Aug 27, 2020

Hixie added the waiting for tree to go green label Aug 27, 2020

fluttergithubbot merged commit 8a6a76a into flutter:master Aug 27, 2020

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020

migrate rendering to nullsafety (flutter#64621)

1a21f73

xu-baolin mentioned this pull request Dec 24, 2020

fix a RenderSliverFixedExtentBoxAdaptor Exception #72922

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate rendering to nullsafety #64621

migrate rendering to nullsafety #64621

Hixie commented Aug 26, 2020

googlebot commented Aug 26, 2020

mit-mit commented Aug 26, 2020

a14n Aug 26, 2020

a14n Aug 26, 2020

a14n commented Aug 26, 2020

googlebot commented Aug 26, 2020

Hixie commented Aug 26, 2020

googlebot commented Aug 26, 2020

googlebot commented Aug 27, 2020

a14n left a comment

fluttergithubbot commented Aug 27, 2020

goderbauer left a comment

goderbauer Aug 27, 2020

a14n Aug 27, 2020

goderbauer Aug 27, 2020

Hixie Aug 27, 2020

jonahwilliams Aug 27, 2020

Hixie Aug 27, 2020

jonahwilliams Aug 27, 2020

dnfield Aug 27, 2020

Hixie Aug 27, 2020

jonahwilliams Aug 27, 2020

Hixie Aug 27, 2020

jonahwilliams left a comment

googlebot commented Aug 27, 2020

googlebot commented Aug 27, 2020

ferhatb commented Aug 28, 2020

migrate rendering to nullsafety #64621

migrate rendering to nullsafety #64621

Conversation

Hixie commented Aug 26, 2020

googlebot commented Aug 26, 2020

mit-mit commented Aug 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a14n commented Aug 26, 2020

googlebot commented Aug 26, 2020

Hixie commented Aug 26, 2020

googlebot commented Aug 26, 2020

googlebot commented Aug 27, 2020

a14n left a comment

Choose a reason for hiding this comment

fluttergithubbot commented Aug 27, 2020

goderbauer 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

jonahwilliams left a comment

Choose a reason for hiding this comment

googlebot commented Aug 27, 2020

googlebot commented Aug 27, 2020

ferhatb commented Aug 28, 2020