Skip to content

Roll engine to f3ff83a5db71262d240aa5337a2a9a22c73c4749. (dart roll).#21143

Merged
aam merged 4 commits intoflutter:masterfrom
aam:roll-20180828
Aug 29, 2018
Merged

Roll engine to f3ff83a5db71262d240aa5337a2a9a22c73c4749. (dart roll).#21143
aam merged 4 commits intoflutter:masterfrom
aam:roll-20180828

Conversation

@aam
Copy link
Member

@aam aam commented Aug 29, 2018

No description provided.

aam added 2 commits August 28, 2018 19:47
Changes since last roll:
```
5613939 Roll src/third_party/skia 7ba1d64f0706..5f0726b01019 (12 commits) (flutter#6104)
47a1ce0 Allow embedders to set the root surface transformation. (flutter#6085)
```
@aam aam requested a review from a-siva August 29, 2018 03:52
@aam aam merged commit abc1723 into flutter:master Aug 29, 2018
@aam aam deleted the roll-20180828 branch August 29, 2018 04:30
@aam
Copy link
Member Author

aam commented Aug 29, 2018

https://dart-review.googlesource.com/c/sdk/+/70160 (cc @jcollins-g ) caused analyzer warnings reported in https://github.com/flutter/flutter/runs/14496912:

   info • Prefer declare const constructors on `@immutable` classes • packages/flutter/lib/src/widgets/platform_view.dart:53:3 • prefer_const_constructors_in_immutables
   info • Prefer declare const constructors on `@immutable` classes • packages/flutter/lib/src/widgets/scroll_view.dart:53:3 • prefer_const_constructors_in_immutables
   info • Prefer declare const constructors on `@immutable` classes • packages/flutter/lib/src/widgets/single_child_scroll_view.dart:186:3 • prefer_const_constructors_in_immutables

Those were incorrect lints which required adding appropriate //ignore directives.

@devoncarew
Copy link
Contributor

@danrubel, looks like an issue related to the move to Fasta. Also cc'ing @a14n, who authored the lint.

From the docs here: http://dart-lang.github.io/linter/lints/prefer_const_constructors_in_immutables.html, it looks like we need an @immutable annotation on the definition of the class in order for this lint to apply. I don't see that annotation on these classes, so I'm not sure why the lint would be firing.

@devoncarew
Copy link
Contributor

I filed dart-lang/sdk#34297 to track this.

aam added a commit to aam/flutter that referenced this pull request Aug 29, 2018
@a14n
Copy link
Contributor

a14n commented Aug 29, 2018

I don't see that annotation on these classes, so I'm not sure why the lint would be firing.

@immutable applies to child classes. Widget (which is a parent of AndroidView) is annotated with @immutable.

@devoncarew
Copy link
Contributor

It looks like the lint is correct then, and we should change the constructors over to const?

@aam
Copy link
Member Author

aam commented Aug 29, 2018

It looks like the lint is correct then, and we should change the constructors over to const?

It doesn't work as there are other constraints that prevent those constructors to be const.

aam added a commit that referenced this pull request Aug 29, 2018
@devoncarew
Copy link
Contributor

OK, sounds like there are false positives with the lint, or issues where we don't actually want the constructor to be const even for immutable classes.

@a14n
Copy link
Contributor

a14n commented Aug 29, 2018

It doesn't work as there are other constraints that prevent those constructors to be const.

Once dart-lang/sdk#33408 has landed all constructors pointed out in the log could be const. I think we are in a transition state where CFE and Analyzer differ. /cc @bwilkerson

@a14n
Copy link
Contributor

a14n commented Aug 29, 2018

For instance:

  AndroidView({ // ignore: prefer_const_constructors_in_immutables
    Key key,
    @required this.viewType,
    this.onPlatformViewCreated,
    this.hitTestBehavior = PlatformViewHitTestBehavior.opaque,
    this.layoutDirection,
    this.gestureRecognizers = const <OneSequenceGestureRecognizer> [],
    this.creationParams,
    this.creationParamsCodec
  }) : assert(viewType != null),
       assert(hitTestBehavior != null),
       assert(gestureRecognizers != null),
       assert(creationParams == null || creationParamsCodec != null),
       super(key: key);

creationParams == null prevents const for now but will be const after the landing of dart-lang/sdk#33408

@aam
Copy link
Member Author

aam commented Aug 29, 2018

Thank you, @a14n . Makes sense.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants