Minimal integration with the Skia text shaper module #9556

jason-simmons · 2019-06-27T22:20:53Z

This converts the libtxt Paragraph and ParagraphBuilder classes into
interfaces with Minikin and SkShaper/SkParagraph based implementations.

Use the --enable-skshaper GN flag to select the Skia shaper implementation
at build time.

lib/ui/text/paragraph_builder.cc

gaaclarke · 2019-06-27T22:40:24Z

third_party/txt/src/skia/paragraph_builder_skia.cc

+SkFontStyle MakeSkFontStyle(txt::FontWeight font_weight,
+                            txt::FontStyle font_style) {
+  return SkFontStyle(static_cast<SkFontStyle::Weight>(
+                         static_cast<int>(font_weight) * 100 + 100),


I'd pull out the magic numbers into named variables to better document them.

Added a comment about this

gaaclarke · 2019-06-27T22:41:19Z

third_party/txt/src/skia/paragraph_builder_skia.cc

+  text_style.setLocale(SkString(txt.locale.c_str()));
+  skia.setTextStyle(text_style);
+
+  // TODO: strut


Shouldn't the TODO comment be a full sentence?

removed this

gaaclarke · 2019-06-27T22:41:48Z

third_party/txt/src/skia/paragraph_builder_skia.cc

+
+  // TODO: break strategy?...
+
+  // This is something flutter expect us to do


Removed this (these lines originated from a work in progress branch developed by the Skia team)

gaaclarke · 2019-06-27T22:42:55Z

third_party/txt/src/skia/paragraph_builder_skia.cc

+}
+
+void ParagraphBuilderSkia::AddPlaceholder(PlaceholderRun& span) {
+  // TODO: implement


Should we have an assert(false) or log statement in the meantime?

gaaclarke · 2019-06-27T22:43:21Z

third_party/txt/src/skia/paragraph_builder_skia.h

+
+namespace txt {
+
+class ParagraphBuilderSkia : public ParagraphBuilder {


Add docstring please.

Looks good. Are we using Doxygen for this code? If you should use /// http://www.doxygen.nl/manual/docblocks.html

third_party/txt/src/skia/paragraph_skia.h

gaaclarke · 2019-06-27T22:46:16Z

third_party/txt/src/txt/paragraph_builder_txt.cc

+}
+
+void ParagraphBuilderTxt::Pop() {
+  if (style_stack_.empty())


Google style requires curly braces. From the looks of our code that isn't the rules for chromium?

third_party/txt/src/txt/paragraph_builder_txt.h

third_party/txt/tests/txt_test_utils.cc

gaaclarke · 2019-06-28T00:34:48Z

LGTM from my read through. I'm not really familiar with the code, you might want someone else's opinion.

GaryQian · 2019-06-28T00:39:36Z

third_party/txt/src/txt/paragraph.h

+  virtual ~Paragraph() = default;

-  void SetFontCollection(std::shared_ptr<FontCollection> font_collection);
+  virtual double GetMaxWidth() = 0;


I would prefer if the documentation/comments regarding what each of these methods do exist in the interface definition. Without an implementation to directly back it up, these methods can quickly get ambiguous to people trying to learn this code.

Also, pointing out the existing implementations of this interface can be very helpful.

GaryQian · 2019-06-28T00:42:32Z

third_party/txt/src/txt/paragraph.h

 * limitations under the License.
 */

 #ifndef LIB_TXT_SRC_PARAGRAPH_H_


Should the interface definitions still live in the src/txt directory? I can see arguments for both leaving it or moving it one layer up.

For now I'd like to keep this in src/txt. Most of the contents of src/txt (TextStyle, FontCollection, etc.) will be used in both the Minikin and SkShaper based code paths.

If the code layout becomes too confusing, then in a later change we can move the Minikin-specific classes (ParagraphTxt and ParagraphBuilderTxt, plus a few support classes like PaintRecord) out of src/txt

GaryQian

LGTM!

GaryQian · 2019-07-02T17:58:05Z

cc @Rusino

This converts the libtxt Paragraph and ParagraphBuilder classes into interfaces with Minikin and SkShaper/SkParagraph based implementations. Use the --enable-skshaper GN flag to select the Skia shaper implementation at build time.

…/engine#9556)

flutter/engine@75387db...49445ce git log 75387db..49445ce --no-merges --oneline 49445ce FLEViewController/Engine API changes (flutter/engine#9750) 2a79462 Add Fuchsia build CI presubmit steps (flutter/engine#9736) 67cebdb Roll fuchsia/sdk/core/linux-amd64 from KGmm_RIJoXS19zTm2crjM3RYpmp5Y03-fLUeVdylbTYC to ehWVT9QJbC-vFMM6SkkQM9HJ9oITFCws7FC9JnrFq2gC (flutter/engine#9765) 089c740 Roll fuchsia/sdk/core/mac-amd64 from lCQWEeR_Ert7t_qAbMRycwrRyZC-dIprYPyPJzwPmg4C to EYnRdXFT9l-d8Qkz4zeTRXnqfV3KQzpQhoPs1r0-740C (flutter/engine#9759) b22410e Include SkParagraph headers only when the enable-skshaper flag is on (flutter/engine#9758) 2cd650d Minimal integration with the Skia text shaper module (flutter/engine#9556) f775f5e Re-enable the Wuffs GIF decoder (flutter/engine#9466) aca0482 Make all shell unit tests use the OpenGL rasterizer. (flutter/engine#9746) bc57291 Make FLEViewController's view an internal detail (flutter/engine#9741) 9776043 Synchronize main thread and gpu thread for first render frame (flutter/engine#9506) f600ae8 Use libc++ variant of string view and remove the FML variant. (flutter/engine#9737) 564f53f Revert "Improve caching limits for Skia (#9503)" (flutter/engine#9740) b453d3c libtxt: fix reference counting of SkFontStyleSets held by font asset providers (flutter/engine#9561) fa7627d Fix backspace crash on Chinese devices (flutter/engine#9734) 56885f7 Let pushColorFilter accept all types of ColorFilters (flutter/engine#9641) 6dccb21 Roll src/third_party/skia 96fdfe0fe88e..af4e7b6cf616 (1 commits) (flutter/engine#9735) 8511d9b Roll fuchsia/sdk/core/mac-amd64 from byM-kyxL4bemlTYNqhKUfJfZoIUrCSzS6XzsFr4n9-MC to lCQWEeR_Ert7t_qAbMRycwrRyZC-dIprYPyPJzwPmg4C (flutter/engine#9742) b3bf0a1 Roll fuchsia/sdk/core/linux-amd64 from I2Qe1zxgckzIzMBTztvzeWYsDgcb9Fw-idSI16oIlx8C to KGmm_RIJoXS19zTm2crjM3RYpmp5Y03-fLUeVdylbTYC (flutter/engine#9743) 7e56823 Fix windows test by not attempting to open a directory as a file. (flutter/engine#9745) 6cf0d13 Roll src/third_party/skia a3ffaabcc4f2..96fdfe0fe88e (5 commits) (flutter/engine#9731) 49a00ae Fix Fuchsia build. (flutter/engine#9730) b3bb39b Roll src/third_party/dart 06c3d7ad3a...09fc76bc51 (flutter/engine#9728) 2284210 Make the license script compatible with recently changed Dart I/O stream APIs (flutter/engine#9725) ad582b5 Rework image & texture management to use concurrent message queues. (flutter/engine#9486) 1dcd5f5 Roll src/third_party/skia 6b82cf638682..a3ffaabcc4f2 (24 commits) (flutter/engine#9726) 129979c Revert "Roll src/third_party/dart 06c3d7ad3a..7acecda2cc (12 commits)" (flutter/engine#9724) 8020d7e Roll src/third_party/skia 56065d9b875f..6b82cf638682 (3 commits) (flutter/engine#9718) e24bd78 Roll src/third_party/dart 06c3d7ad3a..7acecda2cc (12 commits) 3d2668c Reland isolate group changes 802bd15 iOS platform view opacity (flutter/engine#9667) 3b6265b Roll src/third_party/dart b5aeaa6796..06c3d7ad3a (44 commits) 887e052 Refactor ColorFilter to have a native wrapper (flutter/engine#9668) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.

flutter/engine@75387db...49445ce git log 75387db..49445ce --no-merges --oneline 49445ce FLEViewController/Engine API changes (flutter/engine#9750) 2a79462 Add Fuchsia build CI presubmit steps (flutter/engine#9736) 67cebdb Roll fuchsia/sdk/core/linux-amd64 from KGmm_RIJoXS19zTm2crjM3RYpmp5Y03-fLUeVdylbTYC to ehWVT9QJbC-vFMM6SkkQM9HJ9oITFCws7FC9JnrFq2gC (flutter/engine#9765) 089c740 Roll fuchsia/sdk/core/mac-amd64 from lCQWEeR_Ert7t_qAbMRycwrRyZC-dIprYPyPJzwPmg4C to EYnRdXFT9l-d8Qkz4zeTRXnqfV3KQzpQhoPs1r0-740C (flutter/engine#9759) b22410e Include SkParagraph headers only when the enable-skshaper flag is on (flutter/engine#9758) 2cd650d Minimal integration with the Skia text shaper module (flutter/engine#9556) f775f5e Re-enable the Wuffs GIF decoder (flutter/engine#9466) aca0482 Make all shell unit tests use the OpenGL rasterizer. (flutter/engine#9746) bc57291 Make FLEViewController&flutter#39;s view an internal detail (flutter/engine#9741) 9776043 Synchronize main thread and gpu thread for first render frame (flutter/engine#9506) f600ae8 Use libc&flutter#43;&flutter#43; variant of string view and remove the FML variant. (flutter/engine#9737) 564f53f Revert &flutter#34;Improve caching limits for Skia (flutter#9503)&flutter#34; (flutter/engine#9740) b453d3c libtxt: fix reference counting of SkFontStyleSets held by font asset providers (flutter/engine#9561) fa7627d Fix backspace crash on Chinese devices (flutter/engine#9734) 56885f7 Let pushColorFilter accept all types of ColorFilters (flutter/engine#9641) 6dccb21 Roll src/third_party/skia 96fdfe0fe88e..af4e7b6cf616 (1 commits) (flutter/engine#9735) 8511d9b Roll fuchsia/sdk/core/mac-amd64 from byM-kyxL4bemlTYNqhKUfJfZoIUrCSzS6XzsFr4n9-MC to lCQWEeR_Ert7t_qAbMRycwrRyZC-dIprYPyPJzwPmg4C (flutter/engine#9742) b3bf0a1 Roll fuchsia/sdk/core/linux-amd64 from I2Qe1zxgckzIzMBTztvzeWYsDgcb9Fw-idSI16oIlx8C to KGmm_RIJoXS19zTm2crjM3RYpmp5Y03-fLUeVdylbTYC (flutter/engine#9743) 7e56823 Fix windows test by not attempting to open a directory as a file. (flutter/engine#9745) 6cf0d13 Roll src/third_party/skia a3ffaabcc4f2..96fdfe0fe88e (5 commits) (flutter/engine#9731) 49a00ae Fix Fuchsia build. (flutter/engine#9730) b3bb39b Roll src/third_party/dart 06c3d7ad3a...09fc76bc51 (flutter/engine#9728) 2284210 Make the license script compatible with recently changed Dart I/O stream APIs (flutter/engine#9725) ad582b5 Rework image & texture management to use concurrent message queues. (flutter/engine#9486) 1dcd5f5 Roll src/third_party/skia 6b82cf638682..a3ffaabcc4f2 (24 commits) (flutter/engine#9726) 129979c Revert &flutter#34;Roll src/third_party/dart 06c3d7ad3a..7acecda2cc (12 commits)&flutter#34; (flutter/engine#9724) 8020d7e Roll src/third_party/skia 56065d9b875f..6b82cf638682 (3 commits) (flutter/engine#9718) e24bd78 Roll src/third_party/dart 06c3d7ad3a..7acecda2cc (12 commits) 3d2668c Reland isolate group changes 802bd15 iOS platform view opacity (flutter/engine#9667) 3b6265b Roll src/third_party/dart b5aeaa6796..06c3d7ad3a (44 commits) 887e052 Refactor ColorFilter to have a native wrapper (flutter/engine#9668) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.

googlebot added the cla: yes label Jun 27, 2019

jason-simmons requested a review from GaryQian June 27, 2019 22:33

jason-simmons force-pushed the exp_skparagraph branch from a2b82bf to 05474be Compare June 27, 2019 22:33

gaaclarke reviewed Jun 27, 2019

View reviewed changes

jason-simmons force-pushed the exp_skparagraph branch 2 times, most recently from ce2c9d0 to 7cbc036 Compare June 28, 2019 00:11

GaryQian reviewed Jun 28, 2019

View reviewed changes

jason-simmons force-pushed the exp_skparagraph branch 2 times, most recently from e9b8ea7 to 372500b Compare June 28, 2019 01:23

GaryQian approved these changes Jun 28, 2019

View reviewed changes

Minimal integration with the Skia text shaper module

aa727ca

This converts the libtxt Paragraph and ParagraphBuilder classes into interfaces with Minikin and SkShaper/SkParagraph based implementations. Use the --enable-skshaper GN flag to select the Skia shaper implementation at build time.

jason-simmons force-pushed the exp_skparagraph branch from 372500b to aa727ca Compare July 10, 2019 20:19

jason-simmons merged commit 2cd650d into flutter:master Jul 10, 2019

engine-flutter-autoroll mentioned this pull request Jul 11, 2019

Roll engine 75387dbc147d..b22410ef6b11 (28 commits) flutter/flutter#35944

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 11, 2019

2cd650d Minimal integration with the Skia text shaper module (flutter…

2b48fd2

…/engine#9556)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 11, 2019

2cd650d Minimal integration with the Skia text shaper module (flutter…

876045d

…/engine#9556)

This was referenced Jul 11, 2019

Roll engine 75387dbc147d..089c740841e8 (29 commits) flutter/flutter#35947

Closed

Roll engine 75387dbc147d..67cebdb926a4 (30 commits) flutter/flutter#35949

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 11, 2019

2cd650d Minimal integration with the Skia text shaper module (flutter…

2346e61

…/engine#9556)

engine-flutter-autoroll mentioned this pull request Jul 11, 2019

Roll engine 75387dbc147d..49445ce4949b (32 commits) flutter/flutter#35952

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 11, 2019

2cd650d Minimal integration with the Skia text shaper module (flutter…

833367f

…/engine#9556)

GaryQian mentioned this pull request Aug 28, 2019

Text shaping backend switch: SkShaper and SkParagraph flutter/flutter#39420

Closed


		// TODO: break strategy?...

		// This is something flutter expect us to do


		namespace txt {

		class ParagraphBuilderSkia : public ParagraphBuilder {

Minimal integration with the Skia text shaper module #9556

Minimal integration with the Skia text shaper module #9556

Uh oh!

Conversation

jason-simmons commented Jun 27, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gaaclarke commented Jun 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaryQian left a comment

Choose a reason for hiding this comment

Uh oh!

GaryQian commented Jul 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants