ASSERTION FAILED: !m_overflow

ASSERTION FAILED: !m_overflow

InlineFlowBox doesn't expect m_overflow if knownToHaveNoOverflow is
set. Unfortunately LayoutListItem was bypassing the normal overflow
computation and setting m_overflow without knowledge of InlineFlowBox,
yielding to an ASSERT triggering.

This change refactors the API to prevent manipulations of m_overflow
without the knowledge of InlineFlowBox, fixing the ASSERT. However it
is not the right long term fix. LayoutListItem should be fixed to not
do that as it seems very wrong (https://crbug.com/554160).

BUG=546792
Committed: https://crrev.com/ca1054043bbb319a687bf7214c8d206de53a07c1
Cr-Commit-Position: refs/heads/master@{#359601}

[Julien - ping for review, 2015-11-10 20:00:48]

jchaffraix@chromium.org changed reviewers:
+ eae@chromium.org, leviw@chromium.org

[Julien - ping for review, 2015-11-10 20:00:48]

+Levi / Emil as the masters of InlineFlowBox. Input really appreciated on this
patch!

[leviw_travelin_and_unemployed, 2015-11-10 20:23:22]

Can you explain the pixel diff in
LayoutTests/platform/linux/fast/lists/009-vertical-expected.png?

https://codereview.chromium.org/1415493008/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/1415493008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/InlineBox.h:355: // This boolean
tracks whether we know this box to have no overflow.
"whether we know this box has no overflow"

https://codereview.chromium.org/1415493008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/InlineBox.h:357: // Note that this
boolean shouldn't be set if we could have overflow.
I don't feel that the middle 2 lines of this comment are useful.

https://codereview.chromium.org/1415493008/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/line/InlineFlowBox.h (right):

https://codereview.chromium.org/1415493008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/InlineFlowBox.h:282: // This is the
public API to do so.
I'm not sure we need the 2nd line of this comment.

https://codereview.chromium.org/1415493008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/InlineFlowBox.h:283: void
overrideOverflowFromLogicalRects(const LayoutRect& logicalLayoutOverflow, const
LayoutRect& logicalVisualOverflow, LayoutUnit lineTop, LayoutUnit lineBottom)
This seems like a good change to the API :)

[Julien - ping for review, 2015-11-10 23:52:47]

> Can you explain the pixel diff in
LayoutTests/platform/linux/fast/lists/009-vertical-expected.png?

An unrelated run of --reset-test-results that should have never made it to this
patch!
(https://s-media-cache-ak0.pinimg.com/736x/f3/40/07/f340078a3961fb3dd5fc1a1299...)

https://codereview.chromium.org/1415493008/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/1415493008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/InlineBox.h:355: // This boolean
tracks whether we know this box to have no overflow.
On 2015/11/10 at 20:23:22, leviw wrote:
> "whether we know this box has no overflow"

Changed.

https://codereview.chromium.org/1415493008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/InlineBox.h:357: // Note that this
boolean shouldn't be set if we could have overflow.
On 2015/11/10 at 20:23:22, leviw wrote:
> I don't feel that the middle 2 lines of this comment are useful.

I strongly think they are (or at least something similar). When looking at the
boolean's lifecycle, it downed on me that it's never reset if it's ever cleared.
Those lines are meant to clarify this: it's a _conservative_ estimation and we
do allow some type of errors.

https://codereview.chromium.org/1415493008/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/line/InlineFlowBox.h (right):

https://codereview.chromium.org/1415493008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/InlineFlowBox.h:282: // This is the
public API to do so.
On 2015/11/10 at 20:23:22, leviw wrote:
> I'm not sure we need the 2nd line of this comment.

Fair, I won't fight for it, removed!

[leviw_travelin_and_unemployed, 2015-11-12 22:57:52]

https://codereview.chromium.org/1415493008/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/1415493008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/InlineBox.h:357: // Note that this
boolean shouldn't be set if we could have overflow.
On 2015/11/10 at 23:52:47, Julien Chaffraix - PST wrote:
> On 2015/11/10 at 20:23:22, leviw wrote:
> > I don't feel that the middle 2 lines of this comment are useful.
> 
> I strongly think they are (or at least something similar). When looking at the
boolean's lifecycle, it downed on me that it's never reset if it's ever cleared.
Those lines are meant to clarify this: it's a _conservative_ estimation and we
do allow some type of errors.

Can you explain to me how "tracks whether we know this box has no overflow" and
that it "shouldn't be set if we could have overflow" add different information?

[Julien - ping for review, 2015-11-13 00:31:33]

Updated patch for the taker!

https://codereview.chromium.org/1415493008/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/1415493008/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/line/InlineBox.h:357: // Note that this
boolean shouldn't be set if we could have overflow.
On 2015/11/12 at 22:57:51, leviw wrote:
> On 2015/11/10 at 23:52:47, Julien Chaffraix - PST wrote:
> > On 2015/11/10 at 20:23:22, leviw wrote:
> > > I don't feel that the middle 2 lines of this comment are useful.
> > 
> > I strongly think they are (or at least something similar). When looking at
the boolean's lifecycle, it downed on me that it's never reset if it's ever
cleared. Those lines are meant to clarify this: it's a _conservative_ estimation
and we do allow some type of errors.
> 
> Can you explain to me how "tracks whether we know this box has no overflow"
and that it "shouldn't be set if we could have overflow" add different
information?

Fair point, amended the comment to avoid this duplication:

// This boolean will never be set if there is potential for overflow,
// but it will be eagerly cleared in the opposite case. As such, it's
// a conservative tracking of the absence of overflow.

[leviw_travelin_and_unemployed, 2015-11-13 00:40:33]

LGTM

https://codereview.chromium.org/1415493008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/1415493008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBox.h:355: // This boolean
tracks whether we know that this box has no overflow.
Nit: I wouldn't even bother with this first line now that the rest of your
comment does a good job or wrapping up what this means.

[Julien - ping for review, 2015-11-13 02:08:25]

The CQ bit was checked by jchaffraix@chromium.org

[Julien - ping for review, 2015-11-13 02:08:26]

Thanks Senor Levi!

https://codereview.chromium.org/1415493008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineBox.h (right):

https://codereview.chromium.org/1415493008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineBox.h:355: // This boolean
tracks whether we know that this box has no overflow.
On 2015/11/13 at 00:40:33, leviw wrote:
> Nit: I wouldn't even bother with this first line now that the rest of your
comment does a good job or wrapping up what this means.

Fair, removed.

[Julien - ping for review, 2015-11-13 02:08:26]

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/1415493008/#ps60001
(title: "nittified!")

[commit-bot: I haz the power, 2015-11-13 02:08:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415493008/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415493008/60001

[commit-bot: I haz the power, 2015-11-13 02:58:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-13 02:58:04]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Julien - ping for review, 2015-11-13 18:23:44]

The CQ bit was checked by jchaffraix@chromium.org

[commit-bot: I haz the power, 2015-11-13 18:24:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415493008/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415493008/60001

[commit-bot: I haz the power, 2015-11-13 20:12:34]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-13 20:13:28]

Patchset 4 (id:??) landed as
https://crrev.com/ca1054043bbb319a687bf7214c8d206de53a07c1
Cr-Commit-Position: refs/heads/master@{#359601}