-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Skwasm Renderer - initial implementation #39072
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
…e name. Change-Id: I5e3644a904917a375486acd47830f311fe19a5ce Tested: Running unit tests in this repo as well as running against coming web engine changes that use the @Native annotation: flutter/engine#39072 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/279479 Reviewed-by: Ömer Ağacan <omersa@google.com> Commit-Queue: Jackson Gardner <jacksongardner@google.com> Reviewed-by: Aske Simon Christensen <askesc@google.com>
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.
LGTM! But someone else should take a look at the C++ files.
@@ -0,0 +1,63 @@ | |||
# Copyright 2019 The Flutter Authors. All rights reserved. |
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 agree it's orthogonal. It's a bit weird that some of our build files live in web_sdk
at the root of the repo, while others live inside web_ui
.
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.
LGTM with nits
|
||
@override | ||
void saveLayer(ui.Rect? bounds, ui.Paint uiPaint) { | ||
assert(uiPaint is SkwasmPaint); |
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.
nit: unnecessary since the next line will crash anyways if uiPaint is not a SkwasmPaint
SkwasmImage clone() => this; | ||
|
||
@override | ||
bool isCloneOf(ui.Image other) => other == this; |
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.
nit: change to identical(this, other)
. The way it's written now is potentially problematic if later on we add ==
operator to SkwasmImage
in a way where ==
is not necessarily the same as being a clone
@@ -17,160 +18,182 @@ import '../../renderer.dart'; | |||
class SkwasmRenderer implements Renderer { | |||
@override | |||
ui.Path combinePaths(ui.PathOperation op, ui.Path path1, ui.Path path2) { | |||
throw UnimplementedError('Not yet implemented'); | |||
assert(path1 is SkwasmPath); |
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.
nit: here and below, same issue as above re the assertions. since the as
checks do the same thing
{ | ||
source = rebase_path("$root_out_dir/canvaskit_chromium/canvaskit.js") | ||
destination = "flutter_web_sdk/canvaskit_chromium/canvaskit.js" | ||
destination = "canvaskit/chromium/canvaskit.js" | ||
}, | ||
{ | ||
source = rebase_path("$root_out_dir/canvaskit_chromium/canvaskit.wasm") | ||
destination = "flutter_web_sdk/canvaskit_chromium/canvaskit.wasm" | ||
destination = "canvaskit/chromium/canvaskit.wasm" | ||
}, |
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.
Thanks for doing the chromium change as well! I guess I'm not gonna need my restructuring PR after all (#39796).
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.
Actually though, the copy steps from that PR might still be pertinent. I don't have any copy steps here.
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.
You are right. I still need out/wasm_release/canvaskit/chromium/...
so that the flutter web server can find the locally built canvaskit(s) when --local-web-sdk
is specified.
Although the PR will be smaller after your change.
] | ||
} else { | ||
ldflags += [ | ||
"-O1", |
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 use something higher for release builds?
} | ||
|
||
inline SkRRect createRRect(const SkScalar* f) { | ||
const SkScalar* twelveFloats = reinterpret_cast<const SkScalar*>(f); |
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 cast looks like an noop.
} | ||
|
||
SKWASM_EXPORT void paint_destroy(SkPaint* paint) { | ||
delete paint; |
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.
What's the ownership model for SkPaint
? Is it safe to delete the object if it's been used in a picture?
Skwasm Renderer - initial implementation
Implements a handful of renderer APIs, including paths, canvas, picture, paint.