Use the baseline information exposed by C++ to pipe baseline data through RenderBox.

Use the baseline information exposed by C++ to pipe baseline data through RenderBox.

This also fixes the C++ side to give the right baseline information.
Previously it was giving the baseline distance for the font, but not
for the actual laid-out text.

I considered also providing a "defaultBaseline" accessor that returns
the distance for the actual dominant baseline, but it turns out right
now we never decide the baseline is ideographic. We always use the
alphabetic baseline. We should probably fix that...

R=eseidel@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/134f2e4b8c004d70caca54182064cd945da805d5

[eseidel, 2015-06-23 23:02:34]

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

[eseidel, 2015-06-23 23:02:35]

lgtm

[Hixie, 2015-06-23 23:30:29]

I need to document that the baseline data is only ok after layout. probably add
an assert that the node isn't dirty.

[Hixie, 2015-06-23 23:40:22]

also, the logic I added in Flex is all wrong for horizontal. I need to pick the
min offset from all the children, or something.

[Hixie, 2015-06-23 23:41:38]

not sure what stack should do...

[abarth-chromium, 2015-06-24 01:33:10]

On 2015/06/23 at 23:41:38, ianh wrote:
> not sure what stack should do...

Probably take the min over all its children, adjusted for their position.

[eseidel, 2015-06-24 23:20:35]

https://codereview.chromium.org/1200233002/diff/80001/sky/engine/core/dom/Ele...
File sky/engine/core/dom/Element.cpp (right):

https://codereview.chromium.org/1200233002/diff/80001/sky/engine/core/dom/Ele...
sky/engine/core/dom/Element.cpp:978: return box->firstLineBoxBaseline(false,
AlphabeticBaseline);
Please use an enum instead of a bool in non-setter callsites.

e.g. here:

firstLineBoxBaseline(NonAutoBaseline, ...)

Except with a name which actually describes what you're doing.

https://codereview.chromium.org/1200233002/diff/80001/sky/engine/core/renderi...
File sky/engine/core/rendering/RenderBlock.h (right):

https://codereview.chromium.org/1200233002/diff/80001/sky/engine/core/renderi...
sky/engine/core/rendering/RenderBlock.h:219: virtual int
firstLineBoxBaseline(bool autoBaseline = true, FontBaseline baselineType =
AlphabeticBaseline) const override;
There is no need to make these params optional.  I'm shocked the compiler lets
you do this.  Which method is it supposed to call when you do
firstLineBoxBaseline()!

https://codereview.chromium.org/1200233002/diff/80001/sky/engine/core/renderi...
File sky/engine/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/1200233002/diff/80001/sky/engine/core/renderi...
sky/engine/core/rendering/RenderBox.h:432: // if autoBaseline is true,
baselineType is ignored and determined from the context
Why not just make it a separate method?  firstLineBoxBaselineFromContext()?

[Hixie, 2015-06-24 23:31:47]

On 2015/06/24 at 23:20:35, eseidel wrote:
> Please use an enum instead of a bool in non-setter callsites.
> 
> e.g. here:
> 
> firstLineBoxBaseline(NonAutoBaseline, ...)
> 
> Except with a name which actually describes what you're doing.

k.


> There is no need to make these params optional.  I'm shocked the compiler lets
you do this.  Which method is it supposed to call when you do
firstLineBoxBaseline()!

Turns out there's only one implementation, so it's unambiguous. I agree that
it's weird that the compiler doesn't catch this though.

Anyway I'll replace all this with just one enum.


> // if autoBaseline is true, baselineType is ignored and determined from the
context
> Why not just make it a separate method?  firstLineBoxBaselineFromContext()?

It would duplicate a lot of code. The two methods would be identical everywhere
except in one expression of one of the subclasses.

[Hixie, 2015-06-25 00:01:18]

Committed patchset #7 (id:120001) manually as
134f2e4b8c004d70caca54182064cd945da805d5 (presubmit successful).