Create LineLayout api

Create LineLayout api

This is a first step in having the line layout code not talk directly to
the LayoutObject hierarchy. Eventually, the plan is to fully isolate the
LayoutObject hierarchy from the rest of the codebase (editing, DOM,
paint, etc) and use include guards to enforce that. In this way, we can
be explicit about not relying on implementation details outside of the
box layout code.

This is implemented by creating a POD wrapper around the equivalent
LayoutObject class and passing them around by value. This is similar to
what the web/ classes do (e.g. WebNode) and we believe this will be
extremely low overhead performance wise and strictly equivalent memory
wise. In this patch, there happens to be a LayoutObject class for each
LineLayout* wrapper class, but that's not required (or even desired) in
the end. The first step here is to isolate all the code to make it
explicit which parts of the box layout code line layout current needs
and then in a followup step to take a step back and figure out what the
desired end result is.

Not all files in the layout/line directory have been converted yet. Will
occur in following patches.

BUG=499321
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198631

[pilgrim_google, 2015-06-08 14:03:19]

pilgrim@chromium.org changed reviewers:
+ eae@chromium.org, ikilpatrick@chromium.org, leviw@chromium.org,
ojan@chromium.org

[pilgrim_google, 2015-06-08 14:03:21]

forked from https://codereview.chromium.org/1149953007/ and rebased. now with
MOAR NULLPTR

[leviw_travelin_and_unemployed, 2015-06-08 18:14:28]

https://codereview.chromium.org/1164933006/diff/1/Source/core/layout/api/Line...
File Source/core/layout/api/LineLayoutBox.h (right):

https://codereview.chromium.org/1164933006/diff/1/Source/core/layout/api/Line...
Source/core/layout/api/LineLayoutBox.h:25: LayoutBox* box();
For consistency, this should be toBox().

https://codereview.chromium.org/1164933006/diff/1/Source/core/layout/api/Line...
File Source/core/layout/api/LineLayoutItem.h (right):

https://codereview.chromium.org/1164933006/diff/1/Source/core/layout/api/Line...
Source/core/layout/api/LineLayoutItem.h:28: // operator UnspecifiedBoolType()
const { return m_layoutObject; }
We shouldn't land this commented out code. I assume putting this back in gets us
a lot more compiler failures?

https://codereview.chromium.org/1164933006/diff/1/Source/core/layout/api/Line...
File Source/core/layout/api/LineLayoutText.h (right):

https://codereview.chromium.org/1164933006/diff/1/Source/core/layout/api/Line...
Source/core/layout/api/LineLayoutText.h:32: LayoutText* text() { return
toLayoutText(layoutObject()); }
toText()

[leviw_travelin_and_unemployed, 2015-06-08 20:05:00]

https://codereview.chromium.org/1164933006/diff/20001/Source/core/layout/api/...
File Source/core/layout/api/LineLayoutItem.h (right):

https://codereview.chromium.org/1164933006/diff/20001/Source/core/layout/api/...
Source/core/layout/api/LineLayoutItem.h:22: operator LayoutObject*() const {
return m_layoutObject; }
You should have a TODO here about removing this. This method is the only reason
the world is compiling right now, but ultimately it should go away.

[pilgrim_google, 2015-06-10 01:34:03]

Please re-review.

[leviw_travelin_and_unemployed, 2015-06-10 21:11:27]

I think we need a tracking bug for this (and other) API -- something that
explains what's happening and what's left to do. For instance, we should
eventually have include guards to keep line layout from using Layout______
headers.

Also: can you verify locally or with the perf trybots that this isn't going to
be a big perf regression?

https://codereview.chromium.org/1164933006/diff/60001/Source/core/layout/api/...
File Source/core/layout/api/LineLayoutItem.h (right):

https://codereview.chromium.org/1164933006/diff/60001/Source/core/layout/api/...
Source/core/layout/api/LineLayoutItem.h:23: // switching all of core/layout/line
to using the API.
Bug link.

[pilgrim_google, 2015-06-11 14:51:07]

On 2015/06/10 at 21:11:27, leviw wrote:
> I think we need a tracking bug for this (and other) API -- something that
explains what's happening and what's left to do. For instance, we should
eventually have include guards to keep line layout from using Layout______
headers.

https://crbug.com/499321 can be filled in with all the necessary information

>
https://codereview.chromium.org/1164933006/diff/60001/Source/core/layout/api/...
> File Source/core/layout/api/LineLayoutItem.h (right):
> 
>
https://codereview.chromium.org/1164933006/diff/60001/Source/core/layout/api/...
> Source/core/layout/api/LineLayoutItem.h:23: // switching all of
core/layout/line to using the API.
> Bug link.

Added.

> Also: can you verify locally or with the perf trybots that this isn't going to
be a big perf regression?

At the moment it's not hooked into anything, so there's no perf regression. I
will monitor it as we move things over.

[pilgrim_google, 2015-06-11 14:53:19]

rebased, addressed feedback. pls re-review

[leviw_travelin_and_unemployed, 2015-06-11 20:40:27]

On 2015/06/11 at 14:51:07, pilgrim wrote:
> On 2015/06/10 at 21:11:27, leviw wrote:
> > I think we need a tracking bug for this (and other) API -- something that
explains what's happening and what's left to do. For instance, we should
eventually have include guards to keep line layout from using Layout______
headers.
> 
> https://crbug.com/499321 can be filled in with all the necessary information
> 
> >
https://codereview.chromium.org/1164933006/diff/60001/Source/core/layout/api/...
> > File Source/core/layout/api/LineLayoutItem.h (right):
> > 
> >
https://codereview.chromium.org/1164933006/diff/60001/Source/core/layout/api/...
> > Source/core/layout/api/LineLayoutItem.h:23: // switching all of
core/layout/line to using the API.
> > Bug link.
> 
> Added.
> 
> > Also: can you verify locally or with the perf trybots that this isn't going
to be a big perf regression?
> 
> At the moment it's not hooked into anything, so there's no perf regression. I
will monitor it as we move things over.

I don't think this is correct. We're adding an extra function call between
potentially hot methods in the (notoriously sensitive) line layout code, are we
not? If we made this all inline, we wouldn't be paying the cost, but it was a
conscious decision to try and avoid making them all inline to begin with.

[pilgrim_google, 2015-06-11 21:49:27]

On 2015/06/11 at 20:40:27, leviw wrote:
> On 2015/06/11 at 14:51:07, pilgrim wrote:
> > On 2015/06/10 at 21:11:27, leviw wrote:
> > > I think we need a tracking bug for this (and other) API -- something that
explains what's happening and what's left to do. For instance, we should
eventually have include guards to keep line layout from using Layout______
headers.
> > 
> > https://crbug.com/499321 can be filled in with all the necessary information
> > 
> > >
https://codereview.chromium.org/1164933006/diff/60001/Source/core/layout/api/...
> > > File Source/core/layout/api/LineLayoutItem.h (right):
> > > 
> > >
https://codereview.chromium.org/1164933006/diff/60001/Source/core/layout/api/...
> > > Source/core/layout/api/LineLayoutItem.h:23: // switching all of
core/layout/line to using the API.
> > > Bug link.
> > 
> > Added.
> > 
> > > Also: can you verify locally or with the perf trybots that this isn't
going to be a big perf regression?
> > 
> > At the moment it's not hooked into anything, so there's no perf regression.
I will monitor it as we move things over.
> 
> I don't think this is correct. We're adding an extra function call between
potentially hot methods in the (notoriously sensitive) line layout code, are we
not? If we made this all inline, we wouldn't be paying the cost, but it was a
conscious decision to try and avoid making them all inline to begin with.

OK, I've never had the opportunity to use the perf trybots. Reading
https://www.chromium.org/developers/telemetry/performance-try-bots it looks
pretty easy once you do it once. Which perf trybot(s) should I be testing with?

[leviw_travelin_and_unemployed, 2015-06-11 23:00:29]

I'd give Linux a try.

Just running the Blink perf tests (run-perf-tests) over the line layout
(PerformanceTests/Layout/line-layout*) tests locally should give you a pretty
good idea.

[pilgrim_google, 2015-06-12 00:19:55]

On 2015/06/11 at 23:00:29, leviw wrote:
> I'd give Linux a try.
> 
> Just running the Blink perf tests (run-perf-tests) over the line layout
(PerformanceTests/Layout/line-layout*) tests locally should give you a pretty
good idea.

pilgrim@z620:~/work/src/third_party/WebKit% Tools/Scripts/run-perf-tests --debug
PerformanceTests/Layout/line-layout*

Running 2 tests
Running Layout/line-layout-line-height.html (1 of 2)
DESCRIPTION: Measures performance of line-height calculation in layout.
RESULT Layout: line-layout-line-height: Time= 10478.8980999 ms
median= 10463.729999 ms, stdev= 57.7233024372 ms, min= 10409.8160006 ms, max=
10622.4929988 ms
RESULT Layout: line-layout-line-height: JSHeap= 2555648.0 bytes
median= 2555648.0 bytes, stdev= 0.0 bytes, min= 2555648.0 bytes, max= 2555648.0
bytes
Finished: 266.325133 s

Running Layout/line-layout.html (2 of 2)
DESCRIPTION: Measures performance of line breaking in layout.
RESULT Layout: line-layout: Runs= 8.06382841398 runs/s
median= 8.55942379737 runs/s, stdev= 0.768796287553 runs/s, min= 6.52766226387
runs/s, max= 8.60667730398 runs/s
RESULT Layout: line-layout: JSHeap= 2171058.0 bytes
median= 2150340.0 bytes, stdev= 36857.1429912 bytes, min= 2150272.0 bytes, max=
2233280.0 bytes
Finished: 26.791088 s



pilgrim@z620:~/work/src/third_party/WebKit% Tools/Scripts/run-perf-tests --debug
PerformanceTests/Layout/line-layout*

Running 2 tests
Running Layout/line-layout-line-height.html (1 of 2)
DESCRIPTION: Measures performance of line-height calculation in layout.
RESULT Layout: line-layout-line-height: Time= 11162.2208002 ms
median= 11112.2625005 ms, stdev= 105.243266765 ms, min= 11080.1559985 ms, max=
11410.9719992 ms
RESULT Layout: line-layout-line-height: JSHeap= 2555646.0 bytes
median= 2555648.0 bytes, stdev= 3.55409326645 bytes, min= 2555640.0 bytes, max=
2555648.0 bytes
Finished: 283.416413 s

Running Layout/line-layout.html (2 of 2)
DESCRIPTION: Measures performance of line breaking in layout.
RESULT Layout: line-layout: Runs= 7.12988842278 runs/s
median= 7.36517140728 runs/s, stdev= 0.864304539656 runs/s, min= 6.04072657759
runs/s, max= 7.9727120569 runs/s
RESULT Layout: line-layout: JSHeap= 2150246.0 bytes
median= 2150244.0 bytes, stdev= 30.9158450263 bytes, min= 2150216.0 bytes, max=
2150280.0 bytes
Finished: 26.854254 s

[leviw_travelin_and_unemployed, 2015-06-12 00:24:16]

On 2015/06/12 at 00:19:55, pilgrim wrote:
> On 2015/06/11 at 23:00:29, leviw wrote:
> > I'd give Linux a try.
> > 
> > Just running the Blink perf tests (run-perf-tests) over the line layout
(PerformanceTests/Layout/line-layout*) tests locally should give you a pretty
good idea.
> 
> pilgrim@z620:~/work/src/third_party/WebKit% Tools/Scripts/run-perf-tests
--debug PerformanceTests/Layout/line-layout*
> 
> Running 2 tests
> Running Layout/line-layout-line-height.html (1 of 2)
> DESCRIPTION: Measures performance of line-height calculation in layout.
> RESULT Layout: line-layout-line-height: Time= 10478.8980999 ms
> median= 10463.729999 ms, stdev= 57.7233024372 ms, min= 10409.8160006 ms, max=
10622.4929988 ms
> RESULT Layout: line-layout-line-height: JSHeap= 2555648.0 bytes
> median= 2555648.0 bytes, stdev= 0.0 bytes, min= 2555648.0 bytes, max=
2555648.0 bytes
> Finished: 266.325133 s
> 
> Running Layout/line-layout.html (2 of 2)
> DESCRIPTION: Measures performance of line breaking in layout.
> RESULT Layout: line-layout: Runs= 8.06382841398 runs/s
> median= 8.55942379737 runs/s, stdev= 0.768796287553 runs/s, min= 6.52766226387
runs/s, max= 8.60667730398 runs/s
> RESULT Layout: line-layout: JSHeap= 2171058.0 bytes
> median= 2150340.0 bytes, stdev= 36857.1429912 bytes, min= 2150272.0 bytes,
max= 2233280.0 bytes
> Finished: 26.791088 s
> 
> 
> 
> pilgrim@z620:~/work/src/third_party/WebKit% Tools/Scripts/run-perf-tests
--debug PerformanceTests/Layout/line-layout*
> 
> Running 2 tests
> Running Layout/line-layout-line-height.html (1 of 2)
> DESCRIPTION: Measures performance of line-height calculation in layout.
> RESULT Layout: line-layout-line-height: Time= 11162.2208002 ms
> median= 11112.2625005 ms, stdev= 105.243266765 ms, min= 11080.1559985 ms, max=
11410.9719992 ms
> RESULT Layout: line-layout-line-height: JSHeap= 2555646.0 bytes
> median= 2555648.0 bytes, stdev= 3.55409326645 bytes, min= 2555640.0 bytes,
max= 2555648.0 bytes
> Finished: 283.416413 s
> 
> Running Layout/line-layout.html (2 of 2)
> DESCRIPTION: Measures performance of line breaking in layout.
> RESULT Layout: line-layout: Runs= 7.12988842278 runs/s
> median= 7.36517140728 runs/s, stdev= 0.864304539656 runs/s, min= 6.04072657759
runs/s, max= 7.9727120569 runs/s
> RESULT Layout: line-layout: JSHeap= 2150246.0 bytes
> median= 2150244.0 bytes, stdev= 30.9158450263 bytes, min= 2150216.0 bytes,
max= 2150280.0 bytes
> Finished: 26.854254 s

Two things:
1) You need to compare your results with your patch and without.
2) Always run perf tests in release mode. Debug won't give you reasonable perf
numbers...

[pilgrim_google, 2015-06-12 00:56:19]

On 2015/06/12 at 00:24:16, leviw wrote:
> Two things:
> 1) You need to compare your results with your patch and without.
> 2) Always run perf tests in release mode. Debug won't give you reasonable perf
numbers...

The first set was without the patch, the second set was with the patch. Sorry I
didn't make that clear.

I will re-run in release mode and report back.

[pilgrim_google, 2015-06-12 01:41:35]

On 2015/06/12 at 00:56:19, pilgrim_google wrote:
> On 2015/06/12 at 00:24:16, leviw wrote:
> > Two things:
> > 1) You need to compare your results with your patch and without.
> > 2) Always run perf tests in release mode. Debug won't give you reasonable
perf numbers...
> 
> The first set was without the patch, the second set was with the patch. Sorry
I didn't make that clear.
> 
> I will re-run in release mode and report back.

release mode results. First set is without patch, second set is with patch.

pilgrim@z620:~/work/src% build/gyp_chromium -Dcomponent=shared_library && ninja
-C out/Release chrome blink_tests
...
pilgrim@z620:~/work/src/third_party/WebKit% Tools/Scripts/run-perf-tests
PerformanceTests/Layout/line-layout*

Running 2 tests
Running Layout/line-layout-line-height.html (1 of 2)
DESCRIPTION: Measures performance of line-height calculation in layout.
RESULT Layout: line-layout-line-height: Time= 889.815500192 ms
median= 824.879501015 ms, stdev= 109.549253194 ms, min= 810.661001131 ms, max=
1071.87999971 ms
RESULT Layout: line-layout-line-height: JSHeap= 2537260.0 bytes
median= 2537260.0 bytes, stdev= 4.10391340843 bytes, min= 2537256.0 bytes, max=
2537264.0 bytes
Finished: 26.253400 s

Running Layout/line-layout.html (2 of 2)
DESCRIPTION: Measures performance of line breaking in layout.
RESULT Layout: line-layout: Runs= 102.721229473 runs/s
median= 106.559976625 runs/s, stdev= 10.0065422958 runs/s, min= 84.5275874078
runs/s, max= 110.844264318 runs/s
RESULT Layout: line-layout: JSHeap= 2147660.0 bytes
median= 2147676.0 bytes, stdev= 30.9838667699 bytes, min= 2147608.0 bytes, max=
2147680.0 bytes
Finished: 23.670795 s

pilgrim@z620:~/work/src/third_party/WebKit% git cl patch 1164933006
Committed patch locally.

pilgrim@z620:~/work/src% build/gyp_chromium -Dcomponent=shared_library && ninja
-C out/Release chrome blink_tests
...
pilgrim@z620:~/work/src/third_party/WebKit% Tools/Scripts/run-perf-tests
PerformanceTests/Layout/line-layout*

Running 2 tests
Running Layout/line-layout-line-height.html (1 of 2)
DESCRIPTION: Measures performance of line-height calculation in layout.
RESULT Layout: line-layout-line-height: Time= 917.975600157 ms
median= 874.408000149 ms, stdev= 95.7158139603 ms, min= 863.724000752 ms, max=
1140.55999927 ms
RESULT Layout: line-layout-line-height: JSHeap= 2537262.0 bytes
median= 2537264.0 bytes, stdev= 3.55409326645 bytes, min= 2537256.0 bytes, max=
2537264.0 bytes
Finished: 26.897781 s

Running Layout/line-layout.html (2 of 2)
DESCRIPTION: Measures performance of line breaking in layout.
RESULT Layout: line-layout: Runs= 91.3217548802 runs/s
median= 94.8573715159 runs/s, stdev= 8.79036172712 runs/s, min= 76.4639988089
runs/s, max= 100.844954161 runs/s
RESULT Layout: line-layout: JSHeap= 2137910.0 bytes
median= 2134688.0 bytes, stdev= 5749.40948913 bytes, min= 2134648.0 bytes, max=
2147616.0 bytes
Finished: 23.727566 s

[leviw_travelin_and_unemployed, 2015-06-12 18:12:28]

On 2015/06/12 at 01:41:35, pilgrim wrote:
> On 2015/06/12 at 00:56:19, pilgrim_google wrote:
> > On 2015/06/12 at 00:24:16, leviw wrote:
> > > Two things:
> > > 1) You need to compare your results with your patch and without.
> > > 2) Always run perf tests in release mode. Debug won't give you reasonable
perf numbers...
> > 
> > The first set was without the patch, the second set was with the patch.
Sorry I didn't make that clear.
> > 
> > I will re-run in release mode and report back.
> 
> release mode results. First set is without patch, second set is with patch.
> 
> pilgrim@z620:~/work/src% build/gyp_chromium -Dcomponent=shared_library &&
ninja -C out/Release chrome blink_tests
> ...
> pilgrim@z620:~/work/src/third_party/WebKit% Tools/Scripts/run-perf-tests
PerformanceTests/Layout/line-layout*
> 
> Running 2 tests
> Running Layout/line-layout-line-height.html (1 of 2)
> DESCRIPTION: Measures performance of line-height calculation in layout.
> RESULT Layout: line-layout-line-height: Time= 889.815500192 ms
> median= 824.879501015 ms, stdev= 109.549253194 ms, min= 810.661001131 ms, max=
1071.87999971 ms
> RESULT Layout: line-layout-line-height: JSHeap= 2537260.0 bytes
> median= 2537260.0 bytes, stdev= 4.10391340843 bytes, min= 2537256.0 bytes,
max= 2537264.0 bytes
> Finished: 26.253400 s
> 
> Running Layout/line-layout.html (2 of 2)
> DESCRIPTION: Measures performance of line breaking in layout.
> RESULT Layout: line-layout: Runs= 102.721229473 runs/s
> median= 106.559976625 runs/s, stdev= 10.0065422958 runs/s, min= 84.5275874078
runs/s, max= 110.844264318 runs/s
> RESULT Layout: line-layout: JSHeap= 2147660.0 bytes
> median= 2147676.0 bytes, stdev= 30.9838667699 bytes, min= 2147608.0 bytes,
max= 2147680.0 bytes
> Finished: 23.670795 s
> 
> pilgrim@z620:~/work/src/third_party/WebKit% git cl patch 1164933006
> Committed patch locally.
> 
> pilgrim@z620:~/work/src% build/gyp_chromium -Dcomponent=shared_library &&
ninja -C out/Release chrome blink_tests
> ...
> pilgrim@z620:~/work/src/third_party/WebKit% Tools/Scripts/run-perf-tests
PerformanceTests/Layout/line-layout*
> 
> Running 2 tests
> Running Layout/line-layout-line-height.html (1 of 2)
> DESCRIPTION: Measures performance of line-height calculation in layout.
> RESULT Layout: line-layout-line-height: Time= 917.975600157 ms
> median= 874.408000149 ms, stdev= 95.7158139603 ms, min= 863.724000752 ms, max=
1140.55999927 ms
> RESULT Layout: line-layout-line-height: JSHeap= 2537262.0 bytes
> median= 2537264.0 bytes, stdev= 3.55409326645 bytes, min= 2537256.0 bytes,
max= 2537264.0 bytes
> Finished: 26.897781 s
> 
> Running Layout/line-layout.html (2 of 2)
> DESCRIPTION: Measures performance of line breaking in layout.
> RESULT Layout: line-layout: Runs= 91.3217548802 runs/s
> median= 94.8573715159 runs/s, stdev= 8.79036172712 runs/s, min= 76.4639988089
runs/s, max= 100.844954161 runs/s
> RESULT Layout: line-layout: JSHeap= 2137910.0 bytes
> median= 2134688.0 bytes, stdev= 5749.40948913 bytes, min= 2134648.0 bytes,
max= 2147616.0 bytes
> Finished: 23.727566 s


It does appear to have a decent-sized negative impact in its current form.

line-layout median goes from 106.6->94.8, which is greater than the StdDev.
line-layout-line-height is effected less, and has a regression that's within the
StdDev.

Our current approach needs to be tweaked.

This (almost surely) is due to the extra function call happening. For all the
straight-through calls (ones without any logic -- should be all of them now) we
should implement them in the header so they can be inlined. This should mitigate
the performance issues... on Desktop. Go ahead and make that change and try the
perf tests again. I suspect we'll be happy with them.

We compile Android with inlining turned off. This means there's a possibility
that this will still be a big perf issue on Android. If this is the case, I
believe we can use ALWAYS_INLINE as a further mitigation (it shouldn't have any
code bloat cost if I understand things properly), but we should avoid jumping
through this hoop until we have data that we need it.

[leviw_travelin_and_unemployed, 2015-06-12 18:14:43]

leviw@chromium.org changed reviewers:
+ jyasskin@chromium.org

[leviw_travelin_and_unemployed, 2015-06-12 18:14:44]

+jyasskin@ to sanity check our approach.

Jeffrey, I'm happy to chat with you about this patch to maybe save you some time
if you're interested/willing.

[pilgrim_google, 2015-06-16 14:22:29]

On 2015/06/12 at 18:12:28, leviw wrote:
> This (almost surely) is due to the extra function call happening. For all the
straight-through calls (ones without any logic -- should be all of them now) we
should implement them in the header so they can be inlined. This should mitigate
the performance issues... on Desktop. Go ahead and make that change and try the
perf tests again. I suspect we'll be happy with them.

Current technical blocker: moving "#include "core/layout/LayoutBlockFlow.h""
from LineLayoutBlockFlow.cpp to LineLayoutBlockFlow.h (as required in order to
move the implementation of toBlockFlow() to LineLayoutBlockFlow.h) results in
strange compile errors elsewhere.

FAILED: ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/third_party/WebKit/Source/core/html/shadow/webcore_html.SpinButtonElement.o.d
-DV8_DEP$
In file included from
../../third_party/WebKit/Source/core/html/shadow/SpinButtonElement.cpp:36:
In file included from
../../third_party/WebKit/Source/core/layout/LayoutBox.h:29:
In file included from
../../third_party/WebKit/Source/core/layout/shapes/ShapeOutsideInfo.h:33:
In file included from
../../third_party/WebKit/Source/core/layout/api/LineLayoutBlockFlow.h:10:
In file included from
../../third_party/WebKit/Source/core/layout/LayoutBlockFlow.h:41:
../../third_party/WebKit/Source/core/layout/LayoutBlock.h:55:40: error: base
class has incomplete type
class CORE_EXPORT LayoutBlock : public LayoutBox {
                                ~~~~~~~^~~~~~~~~
../../third_party/WebKit/Source/core/dom/Node.h:79:7: note: forward declaration
of 'blink::LayoutBox'
class LayoutBox;
      ^

[pilgrim_google, 2015-06-16 20:46:34]

move LineLayoutBlockFlow implementation to header

[pilgrim_google, 2015-06-16 20:53:03]

move LineLayoutBox implementation to header

[pilgrim_google, 2015-06-16 21:33:53]

all line API implementations now in headers

[pilgrim_google, 2015-06-16 22:01:43]

On 2015/06/12 at 18:12:28, leviw wrote:
> This (almost surely) is due to the extra function call happening. For all the
straight-through calls (ones without any logic -- should be all of them now) we
should implement them in the header so they can be inlined. This should mitigate
the performance issues... on Desktop. Go ahead and make that change and try the
perf tests again. I suspect we'll be happy with them.

Baseline (no patch):

pilgrim@z620:~/work/src/third_party/WebKit%     Tools/Scripts/run-perf-tests
PerformanceTests/Layout/line-layout*

Running 2 tests
Running Layout/line-layout-line-height.html (1 of 2)
DESCRIPTION: Measures performance of line-height calculation in layout.
RESULT Layout: line-layout-line-height: Time= 880.944750085 ms
median= 832.790500484 ms, stdev= 99.7091081986 ms, min= 804.804001004 ms, max=
1062.73099966 ms
RESULT Layout: line-layout-line-height: JSHeap= 2537000.0 bytes
median= 2537000.0 bytes, stdev= 0.0 bytes, min= 2537000.0 bytes, max= 2537000.0
bytes
Finished: 25.891227 s

Running Layout/line-layout.html (2 of 2)
DESCRIPTION: Measures performance of line breaking in layout.
RESULT Layout: line-layout: Runs= 106.924699356 runs/s
median= 109.631196495 runs/s, stdev= 6.06371448555 runs/s, min= 84.1959409842
runs/s, max= 110.468213941 runs/s
RESULT Layout: line-layout: JSHeap= 2145520.0 bytes
median= 2147384.0 bytes, stdev= 3331.47793977 bytes, min= 2139896.0 bytes, max=
2147416.0 bytes
Finished: 23.744508 s

Update (with patch):

pilgrim@z620:~/work/src/third_party/WebKit% Tools/Scripts/run-perf-tests
PerformanceTests/Layout/line-layout*

Running 2 tests
Running Layout/line-layout-line-height.html (1 of 2)
DESCRIPTION: Measures performance of line-height calculation in layout.
RESULT Layout: line-layout-line-height: Time= 984.457700048 ms
median= 934.707500041 ms, stdev= 117.148397483 ms, min= 887.358000502 ms, max=
1172.29699902 ms
RESULT Layout: line-layout-line-height: JSHeap= 2536998.0 bytes
median= 2537000.0 bytes, stdev= 3.55409326645 bytes, min= 2536992.0 bytes, max=
2537000.0 bytes
Finished: 28.676935 s

Running Layout/line-layout.html (2 of 2)
DESCRIPTION: Measures performance of line breaking in layout.
RESULT Layout: line-layout: Runs= 94.8243541665 runs/s
median= 100.185783883 runs/s, stdev= 8.76594921242 runs/s, min= 76.6621307567
runs/s, max= 101.321870665 runs/s
RESULT Layout: line-layout: JSHeap= 2144144.0 bytes
median= 2147384.0 bytes, stdev= 5776.64845463 bytes, min= 2134392.0 bytes, max=
2147416.0 bytes
Finished: 23.427405 s

[pilgrim_google, 2015-06-16 22:02:27]

^^ those performance tests were comparing ToT to ToT+patch 11

[esprehn, 2015-06-17 20:16:25]

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

[esprehn, 2015-06-17 20:16:27]

https://codereview.chromium.org/1164933006/diff/200001/Source/core/layout/Lay...
File Source/core/layout/LayoutBlockFlow.h (right):

https://codereview.chromium.org/1164933006/diff/200001/Source/core/layout/Lay...
Source/core/layout/LayoutBlockFlow.h:500: friend class LineLayoutBlockFlow;
Please no friends, expose API instead.

https://codereview.chromium.org/1164933006/diff/200001/Source/core/layout/api...
File Source/core/layout/api/LineLayoutBlockFlow.h (right):

https://codereview.chromium.org/1164933006/diff/200001/Source/core/layout/api...
Source/core/layout/api/LineLayoutBlockFlow.h:112: return
toBlockFlow()->containsFloats() ?
toBlockFlow()->m_floatingObjects->set().last().get() : 0;
Add a method to layoutBlockFlow for this.

[esprehn, 2015-06-17 20:29:53]

https://codereview.chromium.org/1164933006/diff/200001/Source/core/layout/api...
File Source/core/layout/api/LineLayoutBox.cpp (right):

https://codereview.chromium.org/1164933006/diff/200001/Source/core/layout/api...
Source/core/layout/api/LineLayoutBox.cpp:13: : LineLayoutItem(layoutBox)
All the constructors and destructors need to be inline, otherwise there's
function calls hiding in all the wrapping and unwrapping logic.

https://codereview.chromium.org/1164933006/diff/200001/Source/core/layout/api...
File Source/core/layout/api/LineLayoutInline.cpp (right):

https://codereview.chromium.org/1164933006/diff/200001/Source/core/layout/api...
Source/core/layout/api/LineLayoutInline.cpp:10:
LineLayoutInline::LineLayoutInline(LayoutInline* layoutInline)
ditto

https://codereview.chromium.org/1164933006/diff/200001/Source/core/layout/api...
File Source/core/layout/api/LineLayoutItem.h (right):

https://codereview.chromium.org/1164933006/diff/200001/Source/core/layout/api...
Source/core/layout/api/LineLayoutItem.h:150: friend class LineLayoutBlockFlow;
// for layoutObject
You shouldn't need to make any of these friends.

https://codereview.chromium.org/1164933006/diff/200001/Source/core/layout/api...
File Source/core/layout/api/LineLayoutText.cpp (right):

https://codereview.chromium.org/1164933006/diff/200001/Source/core/layout/api...
Source/core/layout/api/LineLayoutText.cpp:10:
LineLayoutText::LineLayoutText(const LineLayoutItem& item)
ditto

[pilgrim_google, 2015-06-18 01:52:35]

Baseline (no patch):

pilgrim@z620:~/work/src/third_party/WebKit% Tools/Scripts/run-perf-tests
PerformanceTests/Layout/line-layout*

Running 2 tests
Running Layout/line-layout-line-height.html (1 of 2)
DESCRIPTION: Measures performance of line-height calculation in layout.
RESULT Layout: line-layout-line-height: Time= 819.35979994 ms
median= 807.1049992 ms, stdev= 38.2792213125 ms, min= 791.407000273 ms, max=
959.149001166 ms
RESULT Layout: line-layout-line-height: JSHeap= 2545396.0 bytes
median= 2545396.0 bytes, stdev= 4.10391340843 bytes, min= 2545392.0 bytes, max=
2545400.0 bytes
Finished: 24.781140 s

Running Layout/line-layout.html (2 of 2)
DESCRIPTION: Measures performance of line breaking in layout.
RESULT Layout: line-layout: Runs= 105.648164074 runs/s
median= 109.903436163 runs/s, stdev= 7.56691324822 runs/s, min= 84.690191439
runs/s, max= 110.857780497 runs/s
RESULT Layout: line-layout: JSHeap= 2153920.0 bytes
median= 2155784.0 bytes, stdev= 3331.47793977 bytes, min= 2148296.0 bytes, max=
2155816.0 bytes
Finished: 23.608581 s

With patch 12:

pilgrim@z620:~/work/src/third_party/WebKit% Tools/Scripts/run-perf-tests
PerformanceTests/Layout/line-layout*

Running 2 tests
Running Layout/line-layout-line-height.html (1 of 2)
DESCRIPTION: Measures performance of line-height calculation in layout.
RESULT Layout: line-layout-line-height: Time= 828.582849912 ms
median= 801.460499875 ms, stdev= 62.2778542262 ms, min= 795.220999047 ms, max=
1053.37899923 ms
RESULT Layout: line-layout-line-height: JSHeap= 2545400.0 bytes
median= 2545400.0 bytes, stdev= 0.0 bytes, min= 2545400.0 bytes, max= 2545400.0
bytes
Finished: 24.680239 s

Running Layout/line-layout.html (2 of 2)
DESCRIPTION: Measures performance of line breaking in layout.
RESULT Layout: line-layout: Runs= 107.040009567 runs/s
median= 110.189070746 runs/s, stdev= 6.38070129621 runs/s, min= 84.6215963713
runs/s, max= 111.035305866 runs/s
RESULT Layout: line-layout: JSHeap= 2155848.0 bytes
median= 2155848.0 bytes, stdev= 32.8313072666 bytes, min= 2155816.0 bytes, max=
2155880.0 bytes
Finished: 23.614958 s

[pilgrim_google, 2015-06-18 01:53:54]

Will get rid of all my friends tomorrow.

[leviw_travelin_and_unemployed, 2015-06-18 22:45:54]

On 2015/06/18 at 01:52:35, pilgrim wrote:
> Baseline (no patch):
> 
> pilgrim@z620:~/work/src/third_party/WebKit% Tools/Scripts/run-perf-tests
PerformanceTests/Layout/line-layout*

Looks like our perf issues are gone?

\o/!

[pilgrim_google, 2015-06-22 14:07:50]

On 2015/06/18 at 22:45:54, leviw wrote:
> On 2015/06/18 at 01:52:35, pilgrim wrote:
> > Baseline (no patch):
> > 
> > pilgrim@z620:~/work/src/third_party/WebKit% Tools/Scripts/run-perf-tests
PerformanceTests/Layout/line-layout*
> 
> Looks like our perf issues are gone?
> 
> \o/!

Yes, and other feedback has also been addressed. Please re-review.

[leviw_travelin_and_unemployed, 2015-06-23 18:39:19]

https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/Flo...
File Source/core/layout/FloatingObjects.cpp (right):

https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/Flo...
Source/core/layout/FloatingObjects.cpp:519: ShapeOutsideDeltas shapeDeltas =
shapeOutside->computeDeltasForContainingBlockLine(const_cast<LayoutBlockFlow*>(m_layoutObject),
floatingObject, m_lineTop, m_lineBottom - m_lineTop);
Is the const_cast necessary because you took the const out of the definition of
computeDeltasForContainingBlockLine?

https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/Lay...
File Source/core/layout/LayoutBlockFlow.h (right):

https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/Lay...
Source/core/layout/LayoutBlockFlow.h:500: friend class LineLayoutBlockFlow;
I thought we got rid of the friend classes.

https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/sha...
File Source/core/layout/shapes/ShapeOutsideInfo.cpp (right):

https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/sha...
Source/core/layout/shapes/ShapeOutsideInfo.cpp:286: ShapeOutsideDeltas
ShapeOutsideInfo::computeDeltasForContainingBlockLine(LineLayoutBlockFlow
containingBlock, const FloatingObject& floatingObject, LayoutUnit lineTop,
LayoutUnit lineHeight)
Why remove the const here?

[pilgrim_google, 2015-06-23 18:43:15]

On 2015/06/23 at 18:39:19, leviw wrote:
>
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/Flo...
> File Source/core/layout/FloatingObjects.cpp (right):
> 
>
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/Flo...
> Source/core/layout/FloatingObjects.cpp:519: ShapeOutsideDeltas shapeDeltas =
shapeOutside->computeDeltasForContainingBlockLine(const_cast<LayoutBlockFlow*>(m_layoutObject),
floatingObject, m_lineTop, m_lineBottom - m_lineTop);
> Is the const_cast necessary because you took the const out of the definition
of computeDeltasForContainingBlockLine?
> 
>
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/Lay...
> File Source/core/layout/LayoutBlockFlow.h (right):
> 
>
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/sha...
> File Source/core/layout/shapes/ShapeOutsideInfo.cpp (right):
> 
>
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/sha...
> Source/core/layout/shapes/ShapeOutsideInfo.cpp:286: ShapeOutsideDeltas
ShapeOutsideInfo::computeDeltasForContainingBlockLine(LineLayoutBlockFlow
containingBlock, const FloatingObject& floatingObject, LayoutUnit lineTop,
LayoutUnit lineHeight)
> Why remove the const here?

This was from a merge of someone else's refactoring of FloatingObject. It's
possible I messed something up; I had to merge it manually.

>
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/Lay...
> Source/core/layout/LayoutBlockFlow.h:500: friend class LineLayoutBlockFlow;
> I thought we got rid of the friend classes.

I got rid of all the friend classes in the api/ directory. I didn't realize this
one was also new. Will take a look.

[pilgrim_google, 2015-06-23 19:59:32]

On 2015/06/23 at 18:43:15, pilgrim_google wrote:
> On 2015/06/23 at 18:39:19, leviw wrote:
> >
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/Flo...
> > File Source/core/layout/FloatingObjects.cpp (right):
> > 
> >
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/Flo...
> > Source/core/layout/FloatingObjects.cpp:519: ShapeOutsideDeltas shapeDeltas =
shapeOutside->computeDeltasForContainingBlockLine(const_cast<LayoutBlockFlow*>(m_layoutObject),
floatingObject, m_lineTop, m_lineBottom - m_lineTop);
> > Is the const_cast necessary because you took the const out of the definition
of computeDeltasForContainingBlockLine?

No, it's necessary because there is no implicit conversion between LayoutBox*
and LineLayoutBox*. Please don't ask me for any further details; that's the
compiler error I got when I removed the cast. (The cast was just like that since
patch 1, which I didn't write, so I don't pretend to fully understand it.)

> > 
> >
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/Lay...
> > File Source/core/layout/LayoutBlockFlow.h (right):
> > 
> >
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/sha...
> > File Source/core/layout/shapes/ShapeOutsideInfo.cpp (right):
> > 
> >
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/sha...
> > Source/core/layout/shapes/ShapeOutsideInfo.cpp:286: ShapeOutsideDeltas
ShapeOutsideInfo::computeDeltasForContainingBlockLine(LineLayoutBlockFlow
containingBlock, const FloatingObject& floatingObject, LayoutUnit lineTop,
LayoutUnit lineHeight)
> > Why remove the const here?
> 
> This was from a merge of someone else's refactoring of FloatingObject. It's
possible I messed something up; I had to merge it manually.

This is fixed now.

> >
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/Lay...
> > Source/core/layout/LayoutBlockFlow.h:500: friend class LineLayoutBlockFlow;
> > I thought we got rid of the friend classes.
> 
> I got rid of all the friend classes in the api/ directory. I didn't realize
this one was also new. Will take a look.

This is fixed now.

[leviw_travelin_and_unemployed, 2015-06-23 20:07:12]

On 2015/06/23 at 19:59:32, pilgrim wrote:
> On 2015/06/23 at 18:43:15, pilgrim_google wrote:
> > On 2015/06/23 at 18:39:19, leviw wrote:
> > >
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/Flo...
> > > File Source/core/layout/FloatingObjects.cpp (right):
> > > 
> > >
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/Flo...
> > > Source/core/layout/FloatingObjects.cpp:519: ShapeOutsideDeltas shapeDeltas
=
shapeOutside->computeDeltasForContainingBlockLine(const_cast<LayoutBlockFlow*>(m_layoutObject),
floatingObject, m_lineTop, m_lineBottom - m_lineTop);
> > > Is the const_cast necessary because you took the const out of the
definition of computeDeltasForContainingBlockLine?
> 
> No, it's necessary because there is no implicit conversion between LayoutBox*
and LineLayoutBox*. Please don't ask me for any further details; that's the
compiler error I got when I removed the cast. (The cast was just like that since
patch 1, which I didn't write, so I don't pretend to fully understand it.)

We definitely don't want the implicit conversion... but are you sure you mean
LineLayout_Box_ and not LineLayoutBlockFlow?

Can you past the compiler error here?

> 
> > > 
> > >
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/Lay...
> > > File Source/core/layout/LayoutBlockFlow.h (right):
> > > 
> > >
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/sha...
> > > File Source/core/layout/shapes/ShapeOutsideInfo.cpp (right):
> > > 
> > >
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/sha...
> > > Source/core/layout/shapes/ShapeOutsideInfo.cpp:286: ShapeOutsideDeltas
ShapeOutsideInfo::computeDeltasForContainingBlockLine(LineLayoutBlockFlow
containingBlock, const FloatingObject& floatingObject, LayoutUnit lineTop,
LayoutUnit lineHeight)
> > > Why remove the const here?
> > 
> > This was from a merge of someone else's refactoring of FloatingObject. It's
possible I messed something up; I had to merge it manually.
> 
> This is fixed now.
> 
> > >
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/Lay...
> > > Source/core/layout/LayoutBlockFlow.h:500: friend class
LineLayoutBlockFlow;
> > > I thought we got rid of the friend classes.
> > 
> > I got rid of all the friend classes in the api/ directory. I didn't realize
this one was also new. Will take a look.
> 
> This is fixed now.

[pilgrim_google, 2015-06-23 20:19:06]

On 2015/06/23 at 20:07:12, leviw wrote:
> On 2015/06/23 at 19:59:32, pilgrim wrote:
> > On 2015/06/23 at 18:43:15, pilgrim_google wrote:
> > > On 2015/06/23 at 18:39:19, leviw wrote:
> > > >
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/Flo...
> > > > File Source/core/layout/FloatingObjects.cpp (right):
> > > > 
> > > >
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/Flo...
> > > > Source/core/layout/FloatingObjects.cpp:519: ShapeOutsideDeltas
shapeDeltas =
shapeOutside->computeDeltasForContainingBlockLine(const_cast<LayoutBlockFlow*>(m_layoutObject),
floatingObject, m_lineTop, m_lineBottom - m_lineTop);
> > > > Is the const_cast necessary because you took the const out of the
definition of computeDeltasForContainingBlockLine?
> > 
> > No, it's necessary because there is no implicit conversion between
LayoutBox* and LineLayoutBox*. Please don't ask me for any further details;
that's the compiler error I got when I removed the cast. (The cast was just like
that since patch 1, which I didn't write, so I don't pretend to fully understand
it.)
> 
> We definitely don't want the implicit conversion... but are you sure you mean
LineLayout_Box_ and not LineLayoutBlockFlow?
> 
> Can you past the compiler error here?

Oops, yes, I meant LineLayoutBlockFlow. Patch 16 took out the const_cast if you
want to see what the trybots do with it. Here is the error I get locally:

../../third_party/WebKit/Source/core/layout/FloatingObjects.cpp:519:92: error:
no viable conversion from 'const blink::LayoutBlockFlow' to 'const
blink::LineLayoutBlockFlow'
        ShapeOutsideDeltas shapeDeltas =
shapeOutside->computeDeltasForContainingBlockLine(*m_layoutObject,
floatingObject, m_lineTop, m_lineBottom - m_lineTop);
                                                                                
          ^~~~~~~~~~~~~~~
../../third_party/WebKit/Source/core/layout/api/LineLayoutBlockFlow.h:20:7:
note: candidate constructor (the implicit copy constructor) not viable: no known
conversion from 'const blink::LayoutBlockFlow' to 'const
blink::LineLayoutBlockFlow &' for 1st argument
class LineLayoutBlockFlow : public LineLayoutBox {
      ^
../../third_party/WebKit/Source/core/layout/api/LineLayoutBlockFlow.h:20:7:
note: candidate constructor (the implicit move constructor) not viable: no known
conversion from 'const blink::LayoutBlockFlow' to 'blink::LineLayoutBlockFlow
&&' for 1st argument
class LineLayoutBlockFlow : public LineLayoutBox {
      ^
../../third_party/WebKit/Source/core/layout/api/LineLayoutBlockFlow.h:22:5:
note: candidate constructor not viable: no known conversion from 'const
blink::LayoutBlockFlow' to 'blink::LayoutBlockFlow *' for 1st argument
    LineLayoutBlockFlow(LayoutBlockFlow* blockFlow)
    ^
../../third_party/WebKit/Source/core/layout/api/LineLayoutBlockFlow.h:27:5:
note: candidate constructor not viable: no known conversion from 'const
blink::LayoutBlockFlow' to 'const blink::LineLayoutItem &' for 1st argument
    LineLayoutBlockFlow(const LineLayoutItem& item)
    ^
../../third_party/WebKit/Source/core/layout/shapes/ShapeOutsideInfo.h:99:86:
note: passing argument to parameter here
    ShapeOutsideDeltas computeDeltasForContainingBlockLine(const
LineLayoutBlockFlow&, const FloatingObject&, LayoutUnit lineTop, LayoutUnit
lineHeight);
                                                                                
    ^
1 error generated.

[leviw_travelin_and_unemployed, 2015-06-23 20:28:43]

On 2015/06/23 at 20:19:06, pilgrim wrote:
> On 2015/06/23 at 20:07:12, leviw wrote:
> > On 2015/06/23 at 19:59:32, pilgrim wrote:
> > > On 2015/06/23 at 18:43:15, pilgrim_google wrote:
> > > > On 2015/06/23 at 18:39:19, leviw wrote:
> > > > >
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/Flo...
> > > > > File Source/core/layout/FloatingObjects.cpp (right):
> > > > > 
> > > > >
https://codereview.chromium.org/1164933006/diff/260001/Source/core/layout/Flo...
> > > > > Source/core/layout/FloatingObjects.cpp:519: ShapeOutsideDeltas
shapeDeltas =
shapeOutside->computeDeltasForContainingBlockLine(const_cast<LayoutBlockFlow*>(m_layoutObject),
floatingObject, m_lineTop, m_lineBottom - m_lineTop);
> > > > > Is the const_cast necessary because you took the const out of the
definition of computeDeltasForContainingBlockLine?
> > > 
> > > No, it's necessary because there is no implicit conversion between
LayoutBox* and LineLayoutBox*. Please don't ask me for any further details;
that's the compiler error I got when I removed the cast. (The cast was just like
that since patch 1, which I didn't write, so I don't pretend to fully understand
it.)
> > 
> > We definitely don't want the implicit conversion... but are you sure you
mean LineLayout_Box_ and not LineLayoutBlockFlow?
> > 
> > Can you past the compiler error here?
> 
> Oops, yes, I meant LineLayoutBlockFlow. Patch 16 took out the const_cast if
you want to see what the trybots do with it. Here is the error I get locally:
> 
> ../../third_party/WebKit/Source/core/layout/FloatingObjects.cpp:519:92: error:
no viable conversion from 'const blink::LayoutBlockFlow' to 'const
blink::LineLayoutBlockFlow'
>         ShapeOutsideDeltas shapeDeltas =
shapeOutside->computeDeltasForContainingBlockLine(*m_layoutObject,
floatingObject, m_lineTop, m_lineBottom - m_lineTop);

Don't you just need to wrap that in a LineLayoutBlockFlow object?

ShapeOutsideDeltas shapeDeltas =
shapeOutside->computeDeltasForContainingBlockLine(LineLayoutBlockFlow(m_layoutObject),
floatingObject, m_lineTop, m_lineBottom - m_lineTop);

>                                                                               
            ^~~~~~~~~~~~~~~
> ../../third_party/WebKit/Source/core/layout/api/LineLayoutBlockFlow.h:20:7:
note: candidate constructor (the implicit copy constructor) not viable: no known
conversion from 'const blink::LayoutBlockFlow' to 'const
blink::LineLayoutBlockFlow &' for 1st argument
> class LineLayoutBlockFlow : public LineLayoutBox {
>       ^
> ../../third_party/WebKit/Source/core/layout/api/LineLayoutBlockFlow.h:20:7:
note: candidate constructor (the implicit move constructor) not viable: no known
conversion from 'const blink::LayoutBlockFlow' to 'blink::LineLayoutBlockFlow
&&' for 1st argument
> class LineLayoutBlockFlow : public LineLayoutBox {
>       ^
> ../../third_party/WebKit/Source/core/layout/api/LineLayoutBlockFlow.h:22:5:
note: candidate constructor not viable: no known conversion from 'const
blink::LayoutBlockFlow' to 'blink::LayoutBlockFlow *' for 1st argument
>     LineLayoutBlockFlow(LayoutBlockFlow* blockFlow)
>     ^
> ../../third_party/WebKit/Source/core/layout/api/LineLayoutBlockFlow.h:27:5:
note: candidate constructor not viable: no known conversion from 'const
blink::LayoutBlockFlow' to 'const blink::LineLayoutItem &' for 1st argument
>     LineLayoutBlockFlow(const LineLayoutItem& item)
>     ^
> ../../third_party/WebKit/Source/core/layout/shapes/ShapeOutsideInfo.h:99:86:
note: passing argument to parameter here
>     ShapeOutsideDeltas computeDeltasForContainingBlockLine(const
LineLayoutBlockFlow&, const FloatingObject&, LayoutUnit lineTop, LayoutUnit
lineHeight);
>                                                                               
      ^
> 1 error generated.

[leviw_travelin_and_unemployed, 2015-06-23 20:29:10]

https://codereview.chromium.org/1164933006/diff/300001/Source/core/layout/api...
File Source/core/layout/api/LineLayoutBlockFlow.h (right):

https://codereview.chromium.org/1164933006/diff/300001/Source/core/layout/api...
Source/core/layout/api/LineLayoutBlockFlow.h:22:
LineLayoutBlockFlow(LayoutBlockFlow* blockFlow)
We should make all these constructors explicit.

[leviw_travelin_and_unemployed, 2015-06-23 21:51:46]

Perhaps some other folks can understand why this doesn't work. I'm confused.

https://codereview.chromium.org/1164933006/diff/320001/Source/core/layout/api...
File Source/core/layout/api/LineLayoutItem.h (right):

https://codereview.chromium.org/1164933006/diff/320001/Source/core/layout/api...
Source/core/layout/api/LineLayoutItem.h:26: explicit LineLayoutItem() :
m_layoutObject(0) { }
You don't need the explicit here... maybe try removing that to see if it fixes
the issue?

https://codereview.chromium.org/1164933006/diff/320001/Source/core/layout/api...
Source/core/layout/api/LineLayoutItem.h:42: return
LineLayoutItem(m_layoutObject->parent());
../../third_party/WebKit/Source/core/layout/api/LineLayoutItem.h:42:16: error:
no matching constructor for initialization of 'blink::LineLayoutItem'
        return LineLayoutItem(m_layoutObject->parent());
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I'm actually confused about why this doesn't work.

LineLayoutItem has a constructor that takes a LayoutObject*.
m_layoutObject->parent() returns a LayoutObject*.

I need a life line. I think jyasskin is OOO today, but I'm gonna bother him
about it tomorrow.

[esprehn, 2015-06-23 21:57:18]

https://codereview.chromium.org/1164933006/diff/320001/Source/core/layout/api...
File Source/core/layout/api/LineLayoutItem.h (right):

https://codereview.chromium.org/1164933006/diff/320001/Source/core/layout/api...
Source/core/layout/api/LineLayoutItem.h:16: class LayoutObject;
You don't need this forward decl, you include LayoutObject.h

https://codereview.chromium.org/1164933006/diff/320001/Source/core/layout/api...
Source/core/layout/api/LineLayoutItem.h:35: bool hasLayoutObject() const
isNull(), but I think you want to just use the UnspecifiedBool pattern and allow
implicit conversion of LineLayoutItem to a boolean.

[leviw_travelin_and_unemployed, 2015-06-24 19:10:51]

https://codereview.chromium.org/1164933006/diff/320001/Source/core/layout/api...
File Source/core/layout/api/LineLayoutItem.h (right):

https://codereview.chromium.org/1164933006/diff/320001/Source/core/layout/api...
Source/core/layout/api/LineLayoutItem.h:25: explicit LineLayoutItem(const
LineLayoutItem& item) : m_layoutObject(item.m_layoutObject) { }
The jyasskin took a look and this is the problem: you don't actually want an
explicit copy constructor. This is preventing the code from being able to return
a valid value, as that would require a copy.

The only constructors that need the explicit are the ones that take Layout____*
types.

[pilgrim_google, 2015-06-25 13:04:31]

On 2015/06/24 at 19:10:51, leviw wrote:
>
https://codereview.chromium.org/1164933006/diff/320001/Source/core/layout/api...
> File Source/core/layout/api/LineLayoutItem.h (right):
> 
>
https://codereview.chromium.org/1164933006/diff/320001/Source/core/layout/api...
> Source/core/layout/api/LineLayoutItem.h:25: explicit LineLayoutItem(const
LineLayoutItem& item) : m_layoutObject(item.m_layoutObject) { }
> The jyasskin took a look and this is the problem: you don't actually want an
explicit copy constructor. This is preventing the code from being able to return
a valid value, as that would require a copy.
> 
> The only constructors that need the explicit are the ones that take
Layout____* types.

This works now.

[pilgrim_google, 2015-06-25 13:05:03]

On 2015/06/23 at 21:57:18, esprehn wrote:
>
https://codereview.chromium.org/1164933006/diff/320001/Source/core/layout/api...
> File Source/core/layout/api/LineLayoutItem.h (right):
> 
>
https://codereview.chromium.org/1164933006/diff/320001/Source/core/layout/api...
> Source/core/layout/api/LineLayoutItem.h:16: class LayoutObject;
> You don't need this forward decl, you include LayoutObject.h

Fixed.

>
https://codereview.chromium.org/1164933006/diff/320001/Source/core/layout/api...
> Source/core/layout/api/LineLayoutItem.h:35: bool hasLayoutObject() const
> isNull(), but I think you want to just use the UnspecifiedBool pattern and
allow implicit conversion of LineLayoutItem to a boolean.

I don't know how to do this.

[esprehn, 2015-06-25 15:58:39]

On 2015/06/25 at 13:05:03, pilgrim wrote:
> ...
> 
> >
https://codereview.chromium.org/1164933006/diff/320001/Source/core/layout/api...
> > Source/core/layout/api/LineLayoutItem.h:35: bool hasLayoutObject() const
> > isNull(), but I think you want to just use the UnspecifiedBool pattern and
allow implicit conversion of LineLayoutItem to a boolean.
> 
> I don't know how to do this.

https://code.google.com/p/chromium/codesearch#search/&q=UnspecifiedBool&sq=pa...

You should try using code search more often. :)

[pilgrim_google, 2015-06-25 16:06:06]

On 2015/06/25 at 15:58:39, esprehn wrote:
> On 2015/06/25 at 13:05:03, pilgrim wrote:
> > ...
> > 
> > >
https://codereview.chromium.org/1164933006/diff/320001/Source/core/layout/api...
> > > Source/core/layout/api/LineLayoutItem.h:35: bool hasLayoutObject() const
> > > isNull(), but I think you want to just use the UnspecifiedBool pattern and
allow implicit conversion of LineLayoutItem to a boolean.
> > 
> > I don't know how to do this.
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=UnspecifiedBool&sq=pa...
> 
> You should try using code search more often. :)

Sorry, I should have been more specific. I have already searched the entire
codebase, examined every single instance of "UnspecifiedBool", read every
comment (even the ones that just tell you to read other comments), and tried my
best to implement the pattern in LineLayoutItem, and failed, and now I'm
admitting that I failed and asking for help.

[esprehn, 2015-06-25 16:24:17]

On 2015/06/25 at 16:06:06, pilgrim wrote:
> ...
> > 
> > You should try using code search more often. :)
> 
> Sorry, I should have been more specific. I have already searched the entire
codebase, examined every single instance of "UnspecifiedBool", read every
comment (even the ones that just tell you to read other comments), and tried my
best to implement the pattern in LineLayoutItem, and failed, and now I'm
admitting that I failed and asking for help.

Copy pasta:

// This conversion operator allows implicit conversion to bool but not to other
integer types.
typedef void* LineLayoutItem::*UnspecifiedBoolType;
operator UnspecifiedBoolType() const { return m_layoutObject ? &m_layoutObject :
nullptr; }

[pilgrim_google, 2015-06-25 17:55:39]

On 2015/06/25 at 16:24:17, esprehn wrote:
> Copy pasta:
> 
> // This conversion operator allows implicit conversion to bool but not to
other integer types.
> typedef void* LineLayoutItem::*UnspecifiedBoolType;
> operator UnspecifiedBoolType() const { return m_layoutObject ? &m_layoutObject
: nullptr; }

FAILED: ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/third_party/WebKit/Source/core/layout/line/webcore_rendering.LineBreaker.o.d
-DV8_DEPRECATION_WARNINGS -D_FILE_OFFSET_BITS=64 -DCHROMIUM_BUILD
-DCR_CLANG_REVISION=239765-1 -DCOMPONENT_BUILD -DUI_COMPOSITOR_IMAGE_TRANSPORT
-DUSE_AURA=1 -DUSE_ASH=1 -DUSE_PANGO=1 -DUSE_CAIRO=1
-DUSE_DEFAULT_RENDER_THEME=1 -DUSE_LIBJPEG_TURBO=1 -DUSE_X11=1
-DUSE_CLIPBOARD_AURAX11=1 -DENABLE_ONE_CLICK_SIGNIN -DENABLE_PRE_SYNC_BACKUP
-DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_MEDIA_ROUTER=1
-DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS
-DENABLE_HIDPI=1 -DUSE_UDEV -DDONT_EMBED_BUILD_METADATA -DENABLE_TASK_MANAGER=1
-DENABLE_EXTENSIONS=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1
-DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_BACKGROUND=1
-DENABLE_GOOGLE_NOW=1 -DCLD_VERSION=2 -DENABLE_PRINTING=1
-DENABLE_BASIC_PRINTING=1 -DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1
-DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1
-DENABLE_SUPERVISED_USERS=1 -DENABLE_MDNS=1 -DENABLE_SERVICE_DISCOVERY=1
-DV8_USE_EXTERNAL_STARTUP_DATA -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD
-DSAFE_BROWSING_DB_LOCAL -DSAFE_BROWSING_SERVICE -DGL_GLEXT_PROTOTYPES
-DBLINK_IMPLEMENTATION=1 -DINSIDE_BLINK -DENABLE_LAYOUT_UNIT_IN_INLINE_BOXES=0
-DWTF_USE_CONCATENATED_IMPULSE_RESPONSES=1 -DENABLE_INPUT_MULTIPLE_FIELDS_UI=1
-DENABLE_WEB_AUDIO=1 -DWTF_USE_WEBAUDIO_FFMPEG=1
-DWTF_USE_DEFAULT_RENDER_THEME=1 -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0
-DSKIA_DLL -DGR_GL_IGNORE_ES3_MSAA=0 -DSK_SUPPORT_GPU=1
-DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DCHROME_PNG_WRITE_SUPPORT
-DPNG_USER_CONFIG -DLIBXML_STATIC -DLIBXSLT_STATIC -DV8_SHARED -DUSING_V8_SHARED
-DUSE_LIBPCI=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -D__STDC_CONSTANT_MACROS
-D__STDC_FORMAT_MACROS -DDYNAMIC_ANNOTATIONS_ENABLED=1
-DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_DEBUG -D_GLIBCXX_DEBUG=1 -Igen
-I../../third_party/WebKit/Source -I../.. -I../../skia/config
-I../../third_party/khronos -I../../gpu -Igen/blink
-I../../third_party/angle/include -I../../third_party/ffmpeg
-I../../third_party/icu/source/i18n -I../../third_party/icu/source/common
-I../../third_party/WebKit -I../../third_party/skia/include/core
-I../../third_party/skia/include/effects -I../../third_party/skia/include/pdf
-I../../third_party/skia/include/gpu -I../../third_party/skia/include/lazy
-I../../third_party/skia/include/pathops -I../../third_party/skia/include/pipe
-I../../third_party/skia/include/ports -I../../third_party/skia/include/utils
-I../../skia/ext -I../../third_party/iccjpeg -I../../third_party/libpng
-I../../third_party/libwebp -I../../third_party/libxml/linux/include
-I../../third_party/libxml/src/include -I../../third_party/libxslt
-I../../third_party/npapi -I../../third_party/npapi/bindings
-I../../third_party/ots/include -I../../third_party/qcms/src
-I../../third_party/sqlite -I../../third_party/zlib -I../../v8/include
-fstack-protector --param=ssp-buffer-size=4 -Werror -pthread
-fno-strict-aliasing -Wall -Wno-unused-parameter -Wno-missing-field-initializers
-fvisibility=hidden -pipe -fPIC -Wno-reserved-user-defined-literal -Xclang -load
-Xclang
/home/pilgrim/work/src/third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so
-Xclang -add-plugin -Xclang find-bad-constructs -Xclang
-plugin-arg-find-bad-constructs -Xclang check-templates -fcolor-diagnostics
-B/home/pilgrim/work/src/third_party/binutils/Linux_x64/Release/bin
-Wheader-hygiene -Wno-char-subscripts -Wno-unneeded-internal-declaration
-Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing
-Wno-deprecated-register -Wno-inconsistent-missing-override
-Wglobal-constructors -Wexit-time-destructors -fno-strict-aliasing -Xclang -load
-Xclang
/home/pilgrim/work/src/third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.so
-Xclang -add-plugin -Xclang blink-gc-plugin -I/usr/include/freetype2 -m64
-march=x86-64 -O0 -g -funwind-tables -gsplit-dwarf
-Wno-undefined-bool-conversion -Wno-tautological-undefined-compare
-fno-exceptions -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden
-Wsign-compare -std=gnu++11 -Wno-c++0x-compat  -c
../../third_party/WebKit/Source/core/layout/line/LineBreaker.cpp -o
obj/third_party/WebKit/Source/core/layout/line/webcore_rendering.LineBreaker.o
In file included from
../../third_party/WebKit/Source/core/layout/line/LineBreaker.cpp:24:
In file included from
../../third_party/WebKit/Source/core/layout/line/LineBreaker.h:26:
In file included from
../../third_party/WebKit/Source/core/layout/api/LineLayoutBlockFlow.h:10:
In file included from
../../third_party/WebKit/Source/core/layout/api/LineLayoutBox.h:9:
../../third_party/WebKit/Source/core/layout/api/LineLayoutItem.h:43:51: error:
cannot initialize return object of type 'UnspecifiedBoolType' (aka 'void
*blink::LineLayoutItem::*') with an rvalue of type 'blink::LayoutObject *const
*'
    operator UnspecifiedBoolType() const { return m_layoutObject ?
&m_layoutObject : nullptr; }
                                                 
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from
../../third_party/WebKit/Source/core/layout/line/LineBreaker.cpp:24:
In file included from
../../third_party/WebKit/Source/core/layout/line/LineBreaker.h:27:
../../third_party/WebKit/Source/core/layout/line/InlineIterator.h:118:57: error:
use of overloaded operator '==' is ambiguous (with operand types
'blink::LineLayoutItem' and 'blink::LineLayoutItem')
    return it1.offset() == it2.offset() && it1.object() == it2.object();
                                           ~~~~~~~~~~~~ ^  ~~~~~~~~~~~~
../../third_party/WebKit/Source/core/layout/line/InlineIterator.h:118:57: note:
built-in candidate operator==(void *class blink::LineLayoutItem::*, void *class
blink::LineLayoutItem::*)
../../third_party/WebKit/Source/core/layout/line/InlineIterator.h:118:57: note:
built-in candidate operator==(void *const class blink::LineLayoutItem::*, void
*const class blink::LineLayoutItem::*)
../../third_party/WebKit/Source/core/layout/line/InlineIterator.h:118:57: note:
built-in candidate operator==(void *__restrict class blink::LineLayoutItem::*,
void *__restrict class blink::LineLayoutItem::*)
../../third_party/WebKit/Source/core/layout/line/InlineIterator.h:118:57: note:
built-in candidate operator==(void *const __restrict class
blink::LineLayoutItem::*, void *const __restrict class blink::LineLayoutItem::*)
../../third_party/WebKit/Source/core/layout/line/InlineIterator.h:118:57: note:
built-in candidate operator==(void *volatile class blink::LineLayoutItem::*,
void *volatile class blink::LineLayoutItem::*)
../../third_party/WebKit/Source/core/layout/line/InlineIterator.h:118:57: note:
built-in candidate operator==(void *const volatile class
blink::LineLayoutItem::*, void *const volatile class blink::LineLayoutItem::*)
../../third_party/WebKit/Source/core/layout/line/InlineIterator.h:118:57: note:
built-in candidate operator==(void *volatile __restrict class
blink::LineLayoutItem::*, void *volatile __restrict class
blink::LineLayoutItem::*)
../../third_party/WebKit/Source/core/layout/line/InlineIterator.h:118:57: note:
built-in candidate operator==(void *const volatile __restrict class
blink::LineLayoutItem::*, void *const volatile __restrict class
blink::LineLayoutItem::*)
../../third_party/WebKit/Source/core/layout/line/InlineIterator.h:118:57: note:
built-in candidate operator==(class blink::LayoutObject *, class
blink::LayoutObject *)
../../third_party/WebKit/Source/core/layout/line/InlineIterator.h:118:57: note:
built-in candidate operator==(const class blink::LayoutObject *, const class
blink::LayoutObject *)
../../third_party/WebKit/Source/core/layout/line/InlineIterator.h:123:57: error:
use of overloaded operator '!=' is ambiguous (with operand types
'blink::LineLayoutItem' and 'blink::LineLayoutItem')
    return it1.offset() != it2.offset() || it1.object() != it2.object();
                                           ~~~~~~~~~~~~ ^  ~~~~~~~~~~~~

[Jeffrey Yasskin, 2015-06-25 20:37:01]

On 2015/06/25 16:24:17, esprehn wrote:
> On 2015/06/25 at 16:06:06, pilgrim wrote:
> > ...
> > > 
> > > You should try using code search more often. :)
> > 
> > Sorry, I should have been more specific. I have already searched the entire
> codebase, examined every single instance of "UnspecifiedBool", read every
> comment (even the ones that just tell you to read other comments), and tried
my
> best to implement the pattern in LineLayoutItem, and failed, and now I'm
> admitting that I failed and asking for help.
> 
> Copy pasta:
> 
> // This conversion operator allows implicit conversion to bool but not to
other
> integer types.
> typedef void* LineLayoutItem::*UnspecifiedBoolType;
> operator UnspecifiedBoolType() const { return m_layoutObject ? &m_layoutObject
:
> nullptr; }

The existing versions, like
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...,
look more like:

typedef LayoutObject* LineLayoutItem::*UnspecifiedBoolType;
operator UnspecifiedBoolType() const { return m_layoutObject ?
&LineLayoutItem::m_layoutObject : nullptr; }

You need a pointer-to-member, and you need the typedef to have the right type.
That's what 

error: cannot initialize return object of type 'UnspecifiedBoolType' (aka 'void
*blink::LineLayoutItem::*') with an rvalue of type 'blink::LayoutObject *const
*'
    operator UnspecifiedBoolType() const { return m_layoutObject ?
&m_layoutObject : nullptr; }

is saying: you've declared the return type to be "void
*blink::LineLayoutItem::*", but "&m_layoutObject" is a "blink::LayoutObject
*const *" instead.

[pilgrim_google, 2015-06-26 12:58:14]

On 2015/06/25 at 20:37:01, jyasskin wrote:
> On 2015/06/25 16:24:17, esprehn wrote:
> > On 2015/06/25 at 16:06:06, pilgrim wrote:
> > > ...
> > > > 
> > > > You should try using code search more often. :)
> > > 
> > > Sorry, I should have been more specific. I have already searched the
entire
> > codebase, examined every single instance of "UnspecifiedBool", read every
> > comment (even the ones that just tell you to read other comments), and tried
my
> > best to implement the pattern in LineLayoutItem, and failed, and now I'm
> > admitting that I failed and asking for help.
> > 
> > Copy pasta:
> > 
> > // This conversion operator allows implicit conversion to bool but not to
other
> > integer types.
> > typedef void* LineLayoutItem::*UnspecifiedBoolType;
> > operator UnspecifiedBoolType() const { return m_layoutObject ?
&m_layoutObject :
> > nullptr; }
> 
> The existing versions, like
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...,
look more like:
> 
> typedef LayoutObject* LineLayoutItem::*UnspecifiedBoolType;
> operator UnspecifiedBoolType() const { return m_layoutObject ?
&LineLayoutItem::m_layoutObject : nullptr; }
> 
> You need a pointer-to-member, and you need the typedef to have the right type.
That's what 
> 
> error: cannot initialize return object of type 'UnspecifiedBoolType' (aka
'void *blink::LineLayoutItem::*') with an rvalue of type 'blink::LayoutObject
*const *'
>     operator UnspecifiedBoolType() const { return m_layoutObject ?
&m_layoutObject : nullptr; }
> 
> is saying: you've declared the return type to be "void
*blink::LineLayoutItem::*", but "&m_layoutObject" is a "blink::LayoutObject
*const *" instead.

Thanks, that makes (a little bit of) sense. But it still causes multiple "error:
use of overloaded operator '==' is ambiguous (with operand types
'blink::LineLayoutItem' and 'blink::LineLayoutItem')" and "error: use of
overloaded operator '!=' is ambiguous (with operand types
'blink::LineLayoutItem' and 'blink::LineLayoutItem')" errors, as seen in patch
19.

[jsbell, 2015-06-26 20:29:01]

Drive-by: I think you can just add this to LineLayoutItem:

+    bool operator==(const LineLayoutItem& other) const { return m_layoutObject
== other.m_layoutObject; }
+    bool operator!=(const LineLayoutItem& other) const { return m_layoutObject
!= other.m_layoutObject; }

Other uses of the UnspecifiedBoolType pattern offer equality operators (e.g. in
wtf/).

I believe the error is because previously the compiler would see "a == b" and
use `operator LayoutObject*()` on both the lhs/rhs to infer operator==. With
UnspecifiedBoolType it's ambiguous whether to use `operator LayoutObject*()` or
`operator UnspecifiedBoolType()` to infer operator==.

[pilgrim_google, 2015-06-29 14:24:25]

Explicit constructors and UnspecifiedBool work now. Please re-review

[jsbell, 2015-06-29 16:56:03]

Bleah - msvc and gcc might need an explicit operator!

[Jeffrey Yasskin, 2015-07-01 17:54:34]

jyasskin@chromium.org changed reviewers:
- jyasskin@chromium.org

[pilgrim_google, 2015-07-09 15:09:21]

Fancy operators cause crashes in ~dozen layout tests for reasons that are beyond
my debugging capability. Please re-review this latest patch as-is and let me
know if there are any other requirements or if we can finally commit this and
move on.

[leviw_travelin_and_unemployed, 2015-07-09 19:36:52]

LGTM!

Nice job! Thanks for sticking with this!

https://codereview.chromium.org/1164933006/diff/500001/Source/core/layout/api...
File Source/core/layout/api/LineLayoutItem.h (right):

https://codereview.chromium.org/1164933006/diff/500001/Source/core/layout/api...
Source/core/layout/api/LineLayoutItem.h:34: // TODO: Remove this. It's only here
to make things compile before
Nit: TODO(pilgrim)

[pilgrim_google, 2015-07-09 20:24:47]

The CQ bit was checked by pilgrim@chromium.org

[pilgrim_google, 2015-07-09 20:24:47]

The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org
Link to the patchset: https://codereview.chromium.org/1164933006/#ps520001
(title: "rebase + nits")

[commit-bot: I haz the power, 2015-07-09 20:25:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1164933006/520001

[commit-bot: I haz the power, 2015-07-09 21:40:45]

Committed patchset #27 (id:520001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198631