Initial integration of libtxt with Flutter alongside Blink. #3771

GaryQian · 2017-06-14T00:49:48Z

No description provided.

abarth · 2017-06-14T01:06:47Z

BUILD.gn

      "//lib/ftl:ftl_unittests",
+      "//lib/txt/examples:txt_example($host_toolchain)",
+      "//lib/txt/tests/txt($host_toolchain)",  # txt_unittests
+      "//lib/txt/tests/unittest($host_toolchain)",  # minikin_unittest


It would be cleaner if these could be referenced as //lib/txt/examples and //lib/txt/tests, respectively. That will give us more freedom to add more examples and tests over time without changing both repos.

abarth · 2017-06-14T01:07:08Z

DEPS

   Var('fuchsia_git') + '/zip' + '@' + '92dc87ca645fe8e9f5151ef6dac86d8311a7222f',

+  'src/lib/txt':
+   Var('fuchsia_git') + '/txt' + '@' + 'Id71461bd96e08eae0e24ab35a040874d960af8e0',


Is the "I" extra here?

abarth · 2017-06-14T01:08:00Z

lib/ui/text.dart

+
+  // Toggles between Blink text shaping backend and libtxt backend. This allows
+  // for debugging and development and is not intended to be a final feature.
+  void toggleTxt() native "Paragraph_toggleTxt";


We shouldn't expose this to Dart. These APIs are meant to be stable. Instead, we should use a command line flag to enable libtxt.

I have removed this function for now and started with the flag. I'll finish up the command line flag in a later patch. This pr is getting really big!

We can't land this patch with this function here.

abarth · 2017-06-14T01:08:31Z

lib/ui/text/paragraph.cc


-Paragraph::Paragraph(PassOwnPtr<RenderView> renderView)
-    : m_renderView(renderView) {}
+bool Paragraph::m_usingBlink = true;


I'd move this bool to the Settings object.

abarth · 2017-06-14T01:10:13Z

lib/ui/text/paragraph.h

 #define FLUTTER_LIB_UI_TEXT_PARAGRAPH_H_

 #include "flutter/lib/ui/painting/canvas.h"
+#include "flutter/lib/ui/text/paragraph_builder.h"


Why do we need knowledge of the paragraph builder?

Oops, left over from before!

abarth · 2017-06-14T01:11:23Z

lib/ui/text/paragraph_builder.cc

+      style.ellipsis = ellipsis;
+
+    m_paragraphBuilder.SetParagraphStyle(style);
+  }


I'd put this in an "else". We shouldn't do the blink-related work in the non-blink case.

Moved them all to elses

abarth · 2017-06-14T01:11:41Z

lib/ui/text/paragraph_builder.cc

+    }
+
+    m_paragraphBuilder.PushStyle(tstyle);
+  }


abarth · 2017-06-14T01:12:18Z

lib/ui/text/paragraph_impl.h

+
+#include "flutter/lib/ui/painting/canvas.h"
+#include "flutter/lib/ui/text/text_box.h"
+#include "flutter/sky/engine/core/rendering/RenderView.h"


The base class shouldn't know about RenderView.

abarth · 2017-06-14T01:12:32Z

lib/ui/text/paragraph_impl.h

+class ParagraphImpl {
+ public:
+  virtual void setRenderView(PassOwnPtr<RenderView> renderView,
+                             std::unique_ptr<txt::Paragraph>& paragraph) = 0;


This function should move to ParagraphImplBlink.

Restructured the inheritance to be better. No longer need this function!

abarth · 2017-06-14T01:13:27Z

lib/ui/text/paragraph_impl_blink.cc

+
+namespace blink {
+
+IMPLEMENT_WRAPPERTYPEINFO(ui, ParagraphImplBlink);


This shouldn't need a WRAPPERTYPEINFO. The Paragraph is the object with the Dart wrapper, not this object.

abarth · 2017-06-14T01:13:49Z

lib/ui/text/paragraph_impl_blink.h

+  ParagraphImplBlink();
+
+  void setRenderView(PassOwnPtr<RenderView> renderView,
+                     std::unique_ptr<txt::Paragraph>& paragraph);


This shouldn't be here.

abarth · 2017-06-14T01:14:00Z

lib/ui/text/paragraph_impl_blink.h

+class ParagraphImplBlink : public ParagraphImpl,
+                           public ftl::RefCountedThreadSafe<ParagraphImplBlink>,
+                           public tonic::DartWrappable {
+  DEFINE_WRAPPERTYPEINFO();


No need for this start wrapper stuff.

abarth · 2017-06-14T01:14:26Z

lib/ui/text/paragraph_impl_blink.h

+namespace blink {
+
+class ParagraphImplBlink : public ParagraphImpl,
+                           public ftl::RefCountedThreadSafe<ParagraphImplBlink>,


Why is this object ref counted? It seems like it should just be owned by the Paragraph object.

abarth · 2017-06-14T01:15:42Z

lib/ui/text/paragraph.h

 private:
+  friend class ParagraphBuilder;
+
+  std::unique_ptr<ParagraphImpl> m_paragraphImpl;


Yes, you're holding the ParagraphImpl with a unique_ptr, which means they shouldn't inherit from refcounted or do any of that sort of stuff.

abarth · 2017-06-16T02:53:38Z

lib/ui/text/paragraph_impl_txt.cc

+}
+
+void ParagraphImplTxt::paint(Canvas* canvas, double x, double y) {
+  SkCanvas* skCanvas = canvas->canvas();


The style guys says to use names_like_this for local variables. That means this variable should be called sk_canvas.

abarth · 2017-06-16T02:53:48Z

lib/ui/text/paragraph_impl_txt.cc

+  SkCanvas* skCanvas = canvas->canvas();
+  if (!skCanvas)
+    return;
+  txt::ParagraphStyle pStyle = m_paragraph->GetParagraphStyle();


s/pStyle/style/

abarth · 2017-06-16T02:54:06Z

lib/ui/text/paragraph_impl_txt.cc

+
+Dart_Handle ParagraphImplTxt::getPositionForOffset(double dx,
+                                                   double dy) {  // TODO.
+  return NULL;


abarth · 2017-06-16T02:54:10Z

lib/ui/text/paragraph_impl_txt.cc

+}
+
+Dart_Handle ParagraphImplTxt::getWordBoundary(unsigned offset) {  // TODO.
+  return NULL;


GaryQian · 2017-06-16T18:53:10Z

@abarth @chinmaygarde Should be ready for review again.

abarth · 2017-06-16T19:10:03Z

lib/ui/text/paragraph.cc

+  m_paragraphImpl = std::make_unique<ParagraphImplBlink>(renderView);
+}
+
+Paragraph::Paragraph(std::unique_ptr<txt::Paragraph>* paragraph) {


This should be a std::unique_ptrtxt::Paragraph not a std::unique_ptrtxt::Paragraph*

abarth · 2017-06-16T19:58:38Z

lib/ui/text/paragraph.cc

-Paragraph::Paragraph(PassOwnPtr<RenderView> renderView)
-    : m_renderView(renderView) {}
+Paragraph::Paragraph(PassOwnPtr<RenderView> renderView) {
+  m_paragraphImpl = std::make_unique<ParagraphImplBlink>(renderView);


I'd move this to the initializer list.

abarth · 2017-06-16T19:58:44Z

lib/ui/text/paragraph.cc

+}
+
+Paragraph::Paragraph(std::unique_ptr<txt::Paragraph> paragraph) {
+  m_paragraphImpl = std::make_unique<ParagraphImplTxt>(std::move(paragraph));


This one too.

abarth · 2017-06-16T20:00:13Z

lib/ui/text/paragraph_impl_blink.cc

+namespace blink {
+
+ParagraphImplBlink::ParagraphImplBlink(PassOwnPtr<RenderView> renderView) {
+  m_renderView = renderView;


I'd move this to the intializer list.

abarth · 2017-06-16T20:00:36Z

lib/ui/text/paragraph_impl_blink.h

+
+class ParagraphImplBlink : public ParagraphImpl {
+ public:
+  ~ParagraphImplBlink();


abarth · 2017-06-16T20:00:57Z

lib/ui/text/paragraph_impl.h

+
+namespace blink {
+
+class ParagraphImpl {


Needs a virtual destructor.

abarth · 2017-06-16T20:01:19Z

lib/ui/text/paragraph_impl_blink.h

+  double maxIntrinsicWidth();
+  double alphabeticBaseline();
+  double ideographicBaseline();
+  bool didExceedMaxLines();


These should all be marked override

abarth · 2017-06-16T20:02:03Z

lib/ui/text/paragraph_impl_txt.cc

+}
+
+double ParagraphImplTxt::minIntrinsicWidth() {
+  // TODO(garyq): Implement in the library.


I'd probably remove these TODOs from here. They seem to belong in libtxt given that's where the code is missing.

abarth · 2017-06-16T20:02:23Z

lib/ui/text/paragraph_impl_txt.cc

+}
+
+Dart_Handle ParagraphImplTxt::getPositionForOffset(double dx, double dy) {
+  // TODO(garyq): Implement in the library.


These I'd keep here because the code is missing from here.

abarth · 2017-06-16T20:02:34Z

lib/ui/text/paragraph_impl_txt.h

+
+class ParagraphImplTxt : public ParagraphImpl {
+ public:
+  ~ParagraphImplTxt();


Also missing a bunch of overrides here

abarth · 2017-06-16T20:02:56Z

…3771)" This reverts commit c548c65.

* Revert "Initial integration of libtxt with Flutter alongside Blink. (#3771)" This reverts commit c548c65. * Revert "Call Selection.removeSelection if the framework has cleared the selection (#3782)" This reverts commit e5b79ba. * Revert "Update switches to use StringView. (#3781)" This reverts commit 07d4357.

…3771)" This reverts commit c548c65.

…3785) * Revert "Enable line join styles and miter limit. (#3777)" This reverts commit 5403f65. * Revert "Revert "Update switches to use StringView." (#3784)" This reverts commit 80f039f. * Revert "Initial integration of libtxt with Flutter alongside Blink. (#3771)" This reverts commit c548c65.

eseidelGoogle · 2017-06-22T15:02:43Z

Part of flutter/flutter#10476.

GaryQian added 4 commits June 9, 2017 16:47

Transition to Hybrid lib/txt and blink text system.

8f94084

Switchable text shaping engines.

22910bf

Add txt to DEPS and default to Blink

b217ceb

Slgihtly more efficient ParagraphBuilder.

eff2968

googlebot added the cla: yes label Jun 14, 2017

GaryQian requested review from abarth, chinmaygarde and jason-simmons June 14, 2017 00:50

abarth reviewed Jun 14, 2017

View reviewed changes

lib/ui/text/paragraph_builder.cc Outdated

}

m_paragraphBuilder.PushStyle(tstyle);

}

Copy link

Contributor

abarth Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

abarth reviewed Jun 14, 2017

View reviewed changes

GaryQian and others added 8 commits June 13, 2017 18:30

Remove extraneous refcounted and wrapper stuff

cbcee22

Fully isolate Blink/txt versions of ParagraphBuilder.

ef19bfe

Remove extra "I" in DEPS

751c545

Restructure inheritance and slay some zombie code.

64b781a

Restructure building of txt tests.

9fc1491

New version of txt

b60fcde

Remove exposed toggleTxt().

2f086a0

Fix licenses and cleanup.

5a1be21

abarth reviewed Jun 16, 2017

View reviewed changes

GaryQian added 2 commits June 16, 2017 11:49

Fix suggestions in comments.

5da00ab

Missed a spot!

d9601df

abarth reviewed Jun 16, 2017

View reviewed changes

GaryQian added 4 commits June 16, 2017 12:15

Update DEPS to support latest methods.

967660f

Fix moving pointers around.

4fa1dc1

Make Travis happy.

ac4bfcf

Remove toggleTxt()

aa82f5a

abarth reviewed Jun 16, 2017

View reviewed changes

lib/ui/text/paragraph_impl.h

namespace blink {

class ParagraphImpl {

Copy link

Contributor

abarth Jun 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a virtual destructor.

abarth reviewed Jun 16, 2017

View reviewed changes

Nits and whatnot. Virtual destructor.

25d8462

GaryQian merged commit c548c65 into flutter:master Jun 16, 2017

pylaligand added a commit that referenced this pull request Jun 16, 2017

Revert "Initial integration of libtxt with Flutter alongside Blink. (#…

dfa487d

…3771)" This reverts commit c548c65.

ianloic added a commit that referenced this pull request Jun 16, 2017

Revert "Initial integration of libtxt with Flutter alongside Blink. (#…

ba110f0

…3771)" This reverts commit c548c65.

ianloic mentioned this pull request Jun 16, 2017

Revert "Initial integration of libtxt with Flutter alongside Blink." #3785

Merged


		namespace blink {

		IMPLEMENT_WRAPPERTYPEINFO(ui, ParagraphImplBlink);

Initial integration of libtxt with Flutter alongside Blink. #3771

Initial integration of libtxt with Flutter alongside Blink. #3771

Uh oh!

Conversation

GaryQian commented Jun 14, 2017

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!

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!

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!

GaryQian commented Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

GaryQian commented Jun 16, 2017 •

edited

Loading