-
Notifications
You must be signed in to change notification settings - Fork 6k
migrate web engine implementation to null-safe Dart #19172
Conversation
74a6197 to
def757f
Compare
ferhatb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First batch of comments.
ferhatb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments Batch2
| /// | ||
| /// Painting outside the bounds of this rectangle is cropped. | ||
| final ui.Rect bounds; | ||
| final ui.Rect? bounds; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why nullable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we kill HoudiniCanvas until we're ready for a proper investigation? Not sure if it's worth maintaining it with no tests and users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please go ahead.
mdebbar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great effort! Thank you so much for working on it.
I'm still not done with the review, but wanted to submit the comments I have so far. All my comments are things we can iterate on later, not blockers.
| /// This is used to implement operating system specific behavior such as | ||
| /// soft keyboards. | ||
| OperatingSystem get operatingSystem { | ||
| OperatingSystem? get operatingSystem { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, this should never return null. debugOperatingSystemOverride is returned only if it's non-null. And _detectOperatingSystem() returns a non-nullable.
Maybe we need the same trick you did in line 45?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| return debugBrowserEngineOverride; | ||
| } | ||
| return _browserEngine ??= _detectBrowserEngine(); | ||
| return debugBrowserEngineOverride ?? (_browserEngine ??= _detectBrowserEngine()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this required to make the analyzer happy? I think it's possible to infer from the old code that the result is always non-nullable. Is this a known issue or should we file one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is to satisfy the type inference. It's a known issue, but not easily solvable. Maybe sealed classes will help one day.
|
|
||
| /// The active path in the browser history. | ||
| String get path; | ||
| String? get path; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this is nullable? In the HashLocationStrategy it always returns a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
|
||
| @override | ||
| void pushState(dynamic state, String title, String url) { | ||
| void pushState(dynamic state, String title, String? url) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nullable url here is probably what's causing multiple other things to be nullable (e.g. String? get path, etc). Let's make it non-nullable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
|
||
| @override | ||
| void replaceState(dynamic state, String title, String url) { | ||
| void replaceState(dynamic state, String title, String? url) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| static PointerBinding? _instance; | ||
|
|
||
| static void initInstance(html.Element glassPaneElement) { | ||
| static void initInstance(html.Element? glassPaneElement) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
glassPaneElement should never be null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| final html.Element? glassPaneElement; | ||
| _PointerDataCallback _callback; | ||
| PointerDataConverter _pointerDataConverter; | ||
| PointerDataConverter? _pointerDataConverter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find a situation where this can be null. It's always initialized in the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| } | ||
|
|
||
| static Profiler get instance { | ||
| static Profiler? get instance { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This never returns null. If _instance is null, it throws.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| @visibleForTesting | ||
| ParagraphRuler findOrCreateRuler(ParagraphGeometricStyle style) { | ||
| ParagraphRuler ruler = _rulers[style]; | ||
| ParagraphRuler? findOrCreateRuler(ParagraphGeometricStyle style) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In all cases, this function returns a ruler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| final String? _ellipsis; | ||
| final ui.Locale? _locale; | ||
|
|
||
| String? get _effectiveFontFamily { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is never null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
d9771b6 to
56ea6bb
Compare
|
|
||
| /// Given a [domElement], set attributes that are specific to this input type. | ||
| void configureInputMode(html.HtmlElement domElement) { | ||
| void configureInputMode(html.HtmlElement? domElement) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When calling this method, dom_element should not be null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| if (operatingSystem == OperatingSystem.iOs || | ||
| operatingSystem == OperatingSystem.android) { | ||
| domElement.setAttribute('inputmode', inputmodeAttribute); | ||
| domElement!.setAttribute('inputmode', inputmodeAttribute!); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dom_element should not be null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| const EngineInputType(); | ||
|
|
||
| static EngineInputType fromName(String name) { | ||
| static EngineInputType fromName(String? name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name shouldn't be null. It always get defaulted to TextInputType.text on the framework side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
I think responded to all the comments (at least those I was able to find). PTAL! This is no longer a draft. While I'm still testing manually, this is good to go. |
231bb98 to
a21cf7e
Compare
| } | ||
|
|
||
| html.Element prepareAccesibilityPlaceholder() { | ||
| html.Element? prepareAccesibilityPlaceholder() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prepareAccesibilityPlaceholder always returns a value so this is never null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| @override | ||
| html.Element prepareAccesibilityPlaceholder() { | ||
| _semanticsPlaceholder = html.Element.tag('flt-semantics-placeholder'); | ||
| html.Element? prepareAccesibilityPlaceholder() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is never null (related to the previous comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| AutofillInfo.fromFrameworkMessage(focusedElementAutofill); | ||
| final Map<String, html.HtmlElement> elements = <String, html.HtmlElement>{}; | ||
| final Map<String, AutofillInfo> items = <String, AutofillInfo>{}; | ||
| final Map<String?, html.HtmlElement> elements = <String?, html.HtmlElement>{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something, I don't understand the usage of String? as a map key.
Does that mean the key is nullable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Most of the conversion is syntactic. Prior to null safety everything was nullable. The goal of this PR is to convert to the new syntax (i.e. add ? to what's nullable). Converting to non-null is nice-to-have, but it's not realistic to clean up everything in one PR. I expect most of the actual refactoring will happen in many small PRs following this one.
| <StreamSubscription<html.Event>>[]; | ||
| keys.forEach((String key) { | ||
| final html.Element element = elements[key]; | ||
| keys.forEach((String? key) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question. Why map can have null keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| /// | ||
| /// Used as id of the text field. | ||
| final String uniqueIdentifier; | ||
| final String? uniqueIdentifier; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is autofill info, it means this value is always sent. Therefore this should not be null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| /// in the field. | ||
| /// See: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/autocomplete | ||
| final String hint; | ||
| final String? hint; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above comment: "If there is autofill info, it means this value is always sent. Therefore this should not be null."
a21cf7e to
8bb4e61
Compare
| @override | ||
| set strokeMiterLimit(double value) { | ||
| assert(value != null); | ||
| throw UnsupportedError('SurfacePaint.strokeMiterLimit'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keeps assert and add TODO, there is already an issue i believe. Otherwise it would be breaking change (some apps using it today would start failing).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| (t1 * t1 * y2); | ||
| // Expand bounds. | ||
| minX = math.min(minX, extremaX); | ||
| minX = math.min(minX, extremaX as double); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion change tprime/extremaX/Y to final double and remove as. Here and below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, although it seems as double is a noop in dart2js. It compiles to:
extremaX = t13 * curX + t14 * cpX + t12 * x2;
extremaY = t13 * curY + t14 * cpY + t12 * y2;
minX = Math.min(minX, extremaX);
maxX = Math.max(maxX, extremaX);
minY = Math.min(minY, extremaY);
maxY = Math.max(maxY, extremaY);
| ); | ||
|
|
||
| if (frameSize.isEmpty) { | ||
| if (layerTree.frameSize.isEmpty) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this never sets the frameSize on layerTree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to de-null frameSize I changed the initialization sequence a little. frameSize is now final non-null field on LayerTree, so it's guaranteed to be available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also removed frameSize from Rasterizer as it is available in the LayerTree, which Rasterizer has access to already.
| } | ||
|
|
||
| @override | ||
| double get devicePixelRatio => _debugDevicePixelRatio != null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this be double get devicePixelRatio => _debugDevicePixelRatio ?? browserDevicePixelRatio;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
d78ee38 to
43882d0
Compare
* 1ae95f7 Roll Skia from 318afe66e699 to 63a0a758ce14 (4 revisions) (flutter/engine#19335) * b1b9469 Manual roll of Dart e24733ebd1...63cf56d925 (flutter/engine#19337) * de68a7f Roll Skia from 63a0a758ce14 to f123f06aabd6 (9 revisions) (flutter/engine#19336) * 220a831 Move fuchsia/scenic integration behind #define (flutter/engine#19003) * de74f8a migrate web engine implementation to null-safe Dart (flutter/engine#19172) * 4cd3ec0 Fix broken mac/fuchsia compiles (flutter/engine#19339) * 77e9f94 Roll Dart SDK from e24733ebd16c to 243ad5427564 (5 revisions) (flutter/engine#19338) * 4333fc3 Roll Dart SDK from 243ad5427564 to 7fc61e77e225 (5 revisions) (flutter/engine#19342) * b3cfbd2 Roll Fuchsia Mac SDK from YGK_M... to l3tHO... (flutter/engine#19343) * a28b7f0 Implement onDisplayPlatformView (flutter/engine#19344) * 1a4f38f Roll Dart SDK from 7fc61e77e225 to 871f0ee31eb0 (4 revisions) (flutter/engine#19346) * 3d61564 Roll Fuchsia Linux SDK from _d0dW... to Y_dK2... (flutter/engine#19347) * 727a38d Roll Fuchsia Mac SDK from l3tHO... to SuveI... (flutter/engine#19348) * ccfa0e7 Roll Fuchsia Linux SDK from Y_dK2... to lgSTC... (flutter/engine#19349) * 2ff740e Roll Dart SDK from 871f0ee31eb0 to f91547d6dd45 (1 revision) (flutter/engine#19350) * 692dfb3 update compilation rules for null-safety (flutter/engine#19386) * 729ca5e Roll Fuchsia Linux SDK from lgSTC... to ScRia... (flutter/engine#19389)
* 1ae95f7 Roll Skia from 318afe66e699 to 63a0a758ce14 (4 revisions) (flutter/engine#19335) * b1b9469 Manual roll of Dart e24733ebd1...63cf56d925 (flutter/engine#19337) * de68a7f Roll Skia from 63a0a758ce14 to f123f06aabd6 (9 revisions) (flutter/engine#19336) * 220a831 Move fuchsia/scenic integration behind #define (flutter/engine#19003) * de74f8a migrate web engine implementation to null-safe Dart (flutter/engine#19172) * 4cd3ec0 Fix broken mac/fuchsia compiles (flutter/engine#19339) * 77e9f94 Roll Dart SDK from e24733ebd16c to 243ad5427564 (5 revisions) (flutter/engine#19338) * 4333fc3 Roll Dart SDK from 243ad5427564 to 7fc61e77e225 (5 revisions) (flutter/engine#19342) * b3cfbd2 Roll Fuchsia Mac SDK from YGK_M... to l3tHO... (flutter/engine#19343) * a28b7f0 Implement onDisplayPlatformView (flutter/engine#19344) * 1a4f38f Roll Dart SDK from 7fc61e77e225 to 871f0ee31eb0 (4 revisions) (flutter/engine#19346) * 3d61564 Roll Fuchsia Linux SDK from _d0dW... to Y_dK2... (flutter/engine#19347) * 727a38d Roll Fuchsia Mac SDK from l3tHO... to SuveI... (flutter/engine#19348) * ccfa0e7 Roll Fuchsia Linux SDK from Y_dK2... to lgSTC... (flutter/engine#19349) * 2ff740e Roll Dart SDK from 871f0ee31eb0 to f91547d6dd45 (1 revision) (flutter/engine#19350) * 692dfb3 update compilation rules for null-safety (flutter/engine#19386) * 729ca5e Roll Fuchsia Linux SDK from lgSTC... to ScRia... (flutter/engine#19389)
Description
This upgrades the
dart:_enginecode to null-safe Dart.Notable changes:
?,!, etc)html.window.navigator.clipboard?.writeText != nullandhtml.window.navigator.clipboard?.readText != nullto justhtml.window.navigator.clipboard != nullbecause the tear-offs are never null.nullnow throwsUnsupportedError.HtmlViewEmbedderupon construction so that it can befinaland non-null.List<Conic>with_ConicPairas temporary storage of results (NNBD does not support fixed-capacity lists).finalfields can be immediately initialized to non-null values as opposed to lazy initializing them (e.g.WriteBuffer,SurfaceCanvas)EngineLineMetrics.withTextuses bogus values for unsupported fieldsfactory AutofillInfo.fromFrameworkMessage). This is a new language requirement.getBoxesForRangein RTL mode (see paragraph_test.dart)How to review this PR
dart:_engine. It is not practical for one person to carefully review all of it. Instead, each reviewer is assigned a subset of files (although do feel free to comment on anything). The assignments are tracked in this spreadsheet: https://docs.google.com/spreadsheets/d/1Efar1J6eDARoeDlDI4qXdoldJuxUcMUeLpXpu3gQDMA/edit?pli=1#gid=180643056Related Issues
Fixes flutter/flutter#58811