Use client ICU data with skwasm. #42018

eyebrowsoffire · 2023-05-13T17:40:03Z

For now, I am keeping this implementation simple without any extra caching. I may add caching later based on profiling results.

flutter-dashboard · 2023-05-13T17:40:06Z

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.

mdebbar · 2023-05-15T17:24:05Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/paragraph.dart

@@ -583,6 +584,7 @@ class SkwasmParagraphBuilder implements ui.ParagraphBuilder {

  @override
  void addText(String text) {
+    _allText += text;


The reason we didn't do this with CanvasKit is that SkParagraphBuilder can add other parts of text that your _allText won't capture. E.g:

https://github.com/google/skia/blob/4a348e2e59ffcc5fb47b362d52e83467efaee74c/modules/skparagraph/src/ParagraphBuilderImpl.cpp#L139

So the line/word/grapheme breaks that you calculate will be off.

Gah, you're right. I actually looked through the implementation to see if anything else affected the text, but I guess I missed that. I'll fix it

I believe StringBuffer would be significantly more efficient, too. (unless browsers already implement zero-copy concatenation?)

mdebbar · 2023-05-15T17:28:15Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/paragraph.dart

  @override
  ui.Paragraph build() {
+    _addSegmenterData(_allText);


This is where you would call ParagraphBuilder::getText()

Suggested change

_addSegmenterData(_allText);

_addSegmenterData(paragraphBuilderGetText(handle));

Keep in mind that getText() returns the text in UTF8 format which isn't compatible with Dart strings. So it needs to be converted to UTF16. See here:

engine/lib/web_ui/lib/src/engine/canvaskit/canvaskit_api.dart

Lines 2650 to 2655 in dffc480

@JS('getText')

external JSString _getTextUtf8();

String getTextUtf8() => _getTextUtf8().toDart;

// SkParagraphBuilder.getText() returns a utf8 string, we need to decode it

// into a utf16 string.

String getText() => utf8.decode(getTextUtf8().codeUnits);

mdebbar · 2023-05-15T17:36:24Z

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/paragraph.dart

+    for (int i = 0; i < lineBreaks.length; i++) {
+      final LineBreakFragment fragment = lineBreaks[i];
+      lineBreakPointer[i + 1].position = fragment.end;
+      lineBreakPointer[i + 1].lineBreakType = fragment.type == LineBreakType.mandatory ? 100 : 0;


Consider using constants instead of 0 and 100 literals.

mdebbar · 2023-05-15T18:07:19Z

third_party/canvaskit/BUILD.gn

+    # In Chromium browsers, we can use the browser's APIs to get the necessary
+    # ICU data.
+    skia_use_icu = false
+    skia_use_client_icu = true


So Skwasm is supposed to only run in Chromium? If yes, can we throw a meaningful error that says "Skwasm is only supported on Chromium"?

This is basically covered by flutter/flutter#122978 and is something that would have to be implemented at the flutter.js/framework/flutter tool level.

mdebbar · 2023-05-15T18:14:22Z

lib/web_ui/skwasm/text/paragraph_builder.cpp

+SKWASM_EXPORT std::vector<SkUnicode::LineBreakBefore>* lineBreakBuffer_create(
+    size_t length) {
+  return new std::vector<SkUnicode::LineBreakBefore>(
+      length, {0, SkUnicode::LineBreakType::kSoftLineBreak});


Can we do this initialization of the first two items in _addLineBreakData so it's all in one place? I don't think there's a significant performance difference either way, right?

This is basically just zero-initialization of the vector. I have to provide some value here anyway in the C++ code to populate each element in the vector in the first place. I could add something that sets the first line break to 0, 0 in _addLineBreakData, but it's basically pointless since it's already 0, 0.

Hixie · 2023-05-15T19:45:03Z

That's a lot of scary looking code to not have any tests, but

test-exempt: this code will be covered by existing tests once it is turned on

mdebbar

LGTM!

…126968) flutter/engine@1c775e3...87a03e1 2023-05-16 bdero@google.com [Impeller] Use 32 bit Gaussian function in the 2-pass blur (flutter/engine#42069) 2023-05-16 skia-flutter-autoroll@skia.org Roll Skia from 9b0e912a1cb9 to 88d7a68694d9 (13 revisions) (flutter/engine#42081) 2023-05-16 jacksongardner@google.com Use client ICU data with skwasm. (flutter/engine#42018) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#126968) flutter/engine@1c775e3...87a03e1 2023-05-16 bdero@google.com [Impeller] Use 32 bit Gaussian function in the 2-pass blur (flutter/engine#42069) 2023-05-16 skia-flutter-autoroll@skia.org Roll Skia from 9b0e912a1cb9 to 88d7a68694d9 (13 revisions) (flutter/engine#42081) 2023-05-16 jacksongardner@google.com Use client ICU data with skwasm. (flutter/engine#42018) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Use client ICU data with skwasm.

0068c17

flutter-dashboard bot added the platform-web Code specifically for the web engine label May 13, 2023

eyebrowsoffire requested a review from mdebbar May 13, 2023 17:40

flutter-dashboard bot added the needs tests label May 13, 2023

Use single quotes.

95dd05c

mdebbar reviewed May 15, 2023

View reviewed changes

eyebrowsoffire added 3 commits May 15, 2023 17:11

Address Mouad's comments, and use the getText API.

50d0b43

Merge branch 'main' into skwasm_client_icu

d273be0

Merge branch 'main' into skwasm_client_icu

c6002d6

mdebbar approved these changes May 16, 2023

View reviewed changes

eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label May 16, 2023

auto-submit bot merged commit 2cf50e7 into flutter:main May 16, 2023
26 checks passed

engine-flutter-autoroll mentioned this pull request May 16, 2023

Roll Flutter Engine from 1c775e34e2d5 to e931e820680e (2 revisions) flutter/flutter#126965

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 16, 2023

2cf50e78b Use client ICU data with skwasm. (flutter/engine#42018)

3ab0319

engine-flutter-autoroll mentioned this pull request May 17, 2023

Roll Flutter Engine from 1c775e34e2d5 to 87a03e107df7 (3 revisions) flutter/flutter#126968

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 17, 2023

2cf50e78b Use client ICU data with skwasm. (flutter/engine#42018)

b5c3261

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use client ICU data with skwasm. #42018

Use client ICU data with skwasm. #42018

eyebrowsoffire commented May 13, 2023

flutter-dashboard bot commented May 13, 2023

mdebbar May 15, 2023

eyebrowsoffire May 15, 2023

Hixie May 15, 2023

mdebbar May 15, 2023

mdebbar May 15, 2023

mdebbar May 15, 2023

eyebrowsoffire May 15, 2023

mdebbar May 15, 2023

eyebrowsoffire May 15, 2023

Hixie commented May 15, 2023

mdebbar left a comment

	_addSegmenterData(_allText);
	_addSegmenterData(paragraphBuilderGetText(handle));

	@JS('getText')
	external JSString _getTextUtf8();
	String getTextUtf8() => _getTextUtf8().toDart;
	// SkParagraphBuilder.getText() returns a utf8 string, we need to decode it
	// into a utf16 string.
	String getText() => utf8.decode(getTextUtf8().codeUnits);

Use client ICU data with skwasm. #42018

Use client ICU data with skwasm. #42018

Conversation

eyebrowsoffire commented May 13, 2023

flutter-dashboard bot commented May 13, 2023

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

Hixie commented May 15, 2023

mdebbar left a comment

Choose a reason for hiding this comment