Styling for text fragments

Styling for text fragments

This is a completion of Eric's WIP patch:
https://codereview.chromium.org/1179663005/

Low level support for creating a paragraph that contains
runs of styled text. The styles may be nested.

The Paragraph and RenderParagraph classes have been
replaced by Inline and RenderInline. Styled text is defined
with a tree of InlineText and InlineStyle objects.

R=abarth@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/315ac84aea78952e3f569ae84362d9e231143bcf

[hansmuller, 2015-06-16 18:24:30]

hansmuller@chromium.org changed reviewers:
+ abarth@chromium.org, ianh@google.com

[hansmuller, 2015-06-16 18:28:14]

Patchset #1 (id:1) has been deleted

[hansmuller, 2015-06-16 18:31:59]

hansmuller@chromium.org changed reviewers:
+ hansmuller@chromium.org

[hansmuller, 2015-06-16 18:32:00]

PTAL.

Note also: assuming this all passes muster, perhaps paragraph.dart should be
text.dart?

I'd like to add an easier to use RichText class in a separate CL.

[Hixie, 2015-06-16 19:20:07]

https://codereview.chromium.org/1177833012/diff/20001/sky/sdk/lib/rendering/p...
File sky/sdk/lib/rendering/paragraph.dart (right):

https://codereview.chromium.org/1177833012/diff/20001/sky/sdk/lib/rendering/p...
sky/sdk/lib/rendering/paragraph.dart:111: class TextNode {
why is this one class with two mutually exclusive constructors rather than two
classes that inherit from a common abstract class?

https://codereview.chromium.org/1177833012/diff/20001/sky/sdk/lib/rendering/p...
sky/sdk/lib/rendering/paragraph.dart:235: // TODO(hansmuller): restore this
method.
please do this before checking in

https://codereview.chromium.org/1177833012/diff/20001/sky/sdk/lib/widgets/bas...
File sky/sdk/lib/widgets/basic.dart (right):

https://codereview.chromium.org/1177833012/diff/20001/sky/sdk/lib/widgets/bas...
sky/sdk/lib/widgets/basic.dart:348: class TextNodeContainer extends
RenderObjectWrapper {
why Container?

[hansmuller, 2015-06-16 19:30:13]

https://codereview.chromium.org/1177833012/diff/20001/sky/sdk/lib/rendering/p...
File sky/sdk/lib/rendering/paragraph.dart (right):

https://codereview.chromium.org/1177833012/diff/20001/sky/sdk/lib/rendering/p...
sky/sdk/lib/rendering/paragraph.dart:111: class TextNode {
On 2015/06/16 19:20:06, Hixie wrote:
> why is this one class with two mutually exclusive constructors rather than two
> classes that inherit from a common abstract class?

It seemed simpler since the class (or classes) do so little. I could write
TextNodeBase, TextNode, and StyleTextNode (because TextNode, TextTextNode and
StyleTextNode would be horrible) and then users of the API would have look up
the concrete subclasses. Since the Component API would be in terms of
TextNodeBase.

https://codereview.chromium.org/1177833012/diff/20001/sky/sdk/lib/rendering/p...
sky/sdk/lib/rendering/paragraph.dart:235: // TODO(hansmuller): restore this
method.
On 2015/06/16 19:20:06, Hixie wrote:
> please do this before checking in

OK.

https://codereview.chromium.org/1177833012/diff/20001/sky/sdk/lib/widgets/bas...
File sky/sdk/lib/widgets/basic.dart (right):

https://codereview.chromium.org/1177833012/diff/20001/sky/sdk/lib/widgets/bas...
sky/sdk/lib/widgets/basic.dart:348: class TextNodeContainer extends
RenderObjectWrapper {
On 2015/06/16 19:20:06, Hixie wrote:
> why Container?

TextNodeWrapper OK?

[hansmuller, 2015-06-16 21:10:04]

Updated per Ian's feedback. PTAL.

[abarth-chromium, 2015-06-16 22:54:17]

https://codereview.chromium.org/1177833012/diff/40001/sky/sdk/lib/rendering/p...
File sky/sdk/lib/rendering/paragraph.dart (right):

https://codereview.chromium.org/1177833012/diff/40001/sky/sdk/lib/rendering/p...
sky/sdk/lib/rendering/paragraph.dart:110: return
owner.createText(inlineText.text);
too much indent

Why not have InlineText override this function?

https://codereview.chromium.org/1177833012/diff/40001/sky/sdk/lib/rendering/p...
sky/sdk/lib/rendering/paragraph.dart:112: assert(this is InlineStyle);
It seems like _toDOM should be abstract (not have an implementation) and the two
subclasses should implement the function themselves.

https://codereview.chromium.org/1177833012/diff/40001/sky/sdk/lib/rendering/p...
sky/sdk/lib/rendering/paragraph.dart:116:
inlineStyle.children.forEach((InlineBase child) {
for (InlineBase child in inlineStyle.children) { ... }

https://codereview.chromium.org/1177833012/diff/40001/sky/sdk/lib/rendering/p...
sky/sdk/lib/rendering/paragraph.dart:128: } else if (this is InlineStyle) {
Again, this should be done with overrides.

https://codereview.chromium.org/1177833012/diff/40001/sky/sdk/lib/rendering/p...
sky/sdk/lib/rendering/paragraph.dart:131:
inlineStyle.children.forEach((InlineBase child) {
Please use for ( ... in ... ) instead of forEach.  It's much cheaper because you
don't need to allocate a stack frame for each entry in the list.

https://codereview.chromium.org/1177833012/diff/40001/sky/sdk/lib/rendering/p...
sky/sdk/lib/rendering/paragraph.dart:135: result.add("-unrecognized Inline
type-");
You won't need this if you use overrides.

https://codereview.chromium.org/1177833012/diff/40001/sky/sdk/lib/rendering/p...
sky/sdk/lib/rendering/paragraph.dart:169: class RenderInline extends RenderBox {
Why did you change the name of this class from RenderParagraph?  In the past,
we've used the term "RenderInline" to refer to something that isn't necessarily
box-shaped.  For example, a run of text that might break over a line that's part
of a larger paragraph.  We've been using the term RenderParagraph to refer to a
box-shaped thing that contains various inlines.

[abarth-chromium, 2015-06-16 22:54:32]

This looks great.  Just some minor nits about naming and style.

[hansmuller, 2015-06-16 23:34:36]

Excellent feedback about InlineBase et al.

https://codereview.chromium.org/1177833012/diff/40001/sky/sdk/lib/rendering/p...
File sky/sdk/lib/rendering/paragraph.dart (right):

https://codereview.chromium.org/1177833012/diff/40001/sky/sdk/lib/rendering/p...
sky/sdk/lib/rendering/paragraph.dart:110: return
owner.createText(inlineText.text);
On 2015/06/16 22:54:17, abarth wrote:
> too much indent
> 
> Why not have InlineText override this function?

GACK. Sorry about that. Either I'm lazy or tired :-). Both.

https://codereview.chromium.org/1177833012/diff/40001/sky/sdk/lib/rendering/p...
sky/sdk/lib/rendering/paragraph.dart:112: assert(this is InlineStyle);
On 2015/06/16 22:54:17, abarth wrote:
> It seems like _toDOM should be abstract (not have an implementation) and the
two
> subclasses should implement the function themselves.

YES. That's much better.

https://codereview.chromium.org/1177833012/diff/40001/sky/sdk/lib/rendering/p...
sky/sdk/lib/rendering/paragraph.dart:116:
inlineStyle.children.forEach((InlineBase child) {
On 2015/06/16 22:54:17, abarth wrote:
> for (InlineBase child in inlineStyle.children) { ... }

Done.

https://codereview.chromium.org/1177833012/diff/40001/sky/sdk/lib/rendering/p...
sky/sdk/lib/rendering/paragraph.dart:128: } else if (this is InlineStyle) {
On 2015/06/16 22:54:17, abarth wrote:
> Again, this should be done with overrides.

Done.

https://codereview.chromium.org/1177833012/diff/40001/sky/sdk/lib/rendering/p...
sky/sdk/lib/rendering/paragraph.dart:135: result.add("-unrecognized Inline
type-");
On 2015/06/16 22:54:17, abarth wrote:
> You won't need this if you use overrides.

Yes I do not.

https://codereview.chromium.org/1177833012/diff/40001/sky/sdk/lib/rendering/p...
sky/sdk/lib/rendering/paragraph.dart:169: class RenderInline extends RenderBox {
On 2015/06/16 22:54:17, abarth wrote:
> Why did you change the name of this class from RenderParagraph?  In the past,
> we've used the term "RenderInline" to refer to something that isn't
necessarily
> box-shaped.  For example, a run of text that might break over a line that's
part
> of a larger paragraph.  We've been using the term RenderParagraph to refer to
a
> box-shaped thing that contains various inlines.

OK, I've renamed the class to be RenderParagraph.

[abarth-chromium, 2015-06-16 23:41:30]

LGTM

https://codereview.chromium.org/1177833012/diff/60001/sky/tests/framework/sto...
File sky/tests/framework/stocks-expected.txt (right):

https://codereview.chromium.org/1177833012/diff/60001/sky/tests/framework/sto...
sky/tests/framework/stocks-expected.txt:36: 2 |  |  |  |  |  |  | paintChild
RenderInline at Point(24.0, 0.0)
Is this expected.txt file still accurate?

[hansmuller, 2015-06-16 23:42:38]

On 2015/06/16 23:41:30, abarth wrote:
> LGTM
> 
>
https://codereview.chromium.org/1177833012/diff/60001/sky/tests/framework/sto...
> File sky/tests/framework/stocks-expected.txt (right):
> 
>
https://codereview.chromium.org/1177833012/diff/60001/sky/tests/framework/sto...
> sky/tests/framework/stocks-expected.txt:36: 2 |  |  |  |  |  |  | paintChild
> RenderInline at Point(24.0, 0.0)
> Is this expected.txt file still accurate?

No, but I've updated it.  Thanks again for the careful review.

[hansmuller1, 2015-06-16 23:48:31]

Committed patchset #5 (id:100001) manually as
315ac84aea78952e3f569ae84362d9e231143bcf (presubmit successful).