[web][refactor] Split html and canvaskit classes more #14320
Conversation
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
I didn't realize this was a refactor until I've already added some comments on the code. Feel free to consider them at a later PR. The refactoring looks good to me, didn't notice anything out of place. I'll let someone who knows the web code make the call if it's kosher.
|
|
||
| static const ui.Color _defaultPaintColor = ui.Color(0xFF000000); | ||
|
|
||
| ui.BlendMode get blendMode => _blendMode; |
There was a problem hiding this comment.
The linter doesn't complain that all public components should have a docstring?
There was a problem hiding this comment.
The code in engine is in a private dart library dart:_engine
There was a problem hiding this comment.
Thanks! This reminded me to add @override annotations on these.
|
|
||
| skPaint.callMethod('setAntiAlias', <bool>[isAntiAlias]); | ||
|
|
||
| if (strokeWidth != 0.0) { |
| final ui.BlurStyle blurStyle = maskFilter.webOnlyBlurStyle; | ||
| final double sigma = maskFilter.webOnlySigma; | ||
|
|
||
| js.JsObject skBlurStyle; |
There was a problem hiding this comment.
Couldn't all these switch statements be condensed to 2 expressions?
_dict = {ui.BlurStyle.normal:'Normal', ui.BlurStyle.solid:'Solid',...};
js.JsObject skBlurStyle = canvasKit['BlurStyle'][_dict[blurStyle]];
There was a problem hiding this comment.
The long-term solution is to canonicalize these values in a static location so we don't have to look them up every time we need them. There are several places in the code that use this pattern. I'm going to leave it as is right now. I moved this code from another file.
|
|
||
| part of engine; | ||
|
|
||
| /// The implementatin of [ui.Paint] used by the CanvasKit backend. |
There was a problem hiding this comment.
implementatin -> implementation
| fillType = ui.PathFillType.nonZero; | ||
| } | ||
|
|
||
| SkPath.from(SkPath other) { |
There was a problem hiding this comment.
Can you add a TODO to add this
harryterkelsen
left a comment
harryterkelsen
left a comment
There was a problem hiding this comment.
LGTM with a couple nits
yjbanov
force-pushed
the
cache-sk-paint
branch
from
45a2bcd to
fa0f868
Compare
| part of engine; | ||
|
|
||
| /// The implementation of [ui.Paint] used by the CanvasKit backend. | ||
| class SkPaint implements ui.Paint { |
There was a problem hiding this comment.
fwiw, having multiple implementations of Paint is going to hurt performance as it means everywhere that calls into a Paint now has to do a dynamic lookup.
There was a problem hiding this comment.
Not necessarily. Since experimentalUseSkia is const, dart2js should be able to figure out that only one non-abstract subclass of Paint is live, and avoid dynamic dispatch. I will double check this
There was a problem hiding this comment.
Just built the gallery with this change and it looks like dart2js has inlined the SkPaint constructor into every new Paint() call, so dynamic dispatch is avoided. Here's a sample function that creates a Paint:
* Split surface and canvaskit classes more * fix licenses * address comments
5 participants
Refactor the HTML vs CanvasKit a bit more:
surface/so they don't pollute the canvaskit API.SkPaint.There should be no functional changes. This prepares code for a future change that will allow managing Skia objects more efficiently (caching and deleting reusable objects, such as Paint, Path, and Paragraph).