Split list marker updating into two steps

Split list marker updating into two steps

This patch reverts r164210[1] and fixes it in a different way. A list
marker can be parented on different renderers and changing a list
marker to being "inside" can change the parent. This patch splits the
reparent and update steps from invalidating list marker parent's width.
This refactoring has a few advantages such as removing the
preferredLogicalWidthsDirty check and not dirtying the marker's child
lines since these steps happen naturally as part of reparenting the
marker renderer.

Detailed changes:
1) updateMarkerLocation is now a private implementation detail of
RenderListItem.
2) The reparent and update steps have been refactored out of
updateMarkerLocation. updateMarkerLocation now reparents and updates
the marker while updateMarkerLocationAndInvalidateWidth adds a width
invalidation step that's necessary only during layout (before layout it
wouldn't do anything).
3) The comment "Sanity check the location of our marker." has been
removed--there is no sanity in the render list marker code.
4) HTMLLIElement::attach now just does the reparent and update steps
instead of also invalidating the marker's width.

[1] https://src.chromium.org/viewvc/blink?view=rev&revision=164210

BUG=301196, 390569
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177428

[esprehn, 2014-07-02 00:11:00]

Your failure looks real:

fast/lists/list-item-without-list-reparented-crash.html

[pdr., 2014-07-02 04:29:15]

Ready for an initial review.

[skobes, 2014-07-02 05:59:43]

lgtm

https://codereview.chromium.org/368733002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderListItem.h (right):

https://codereview.chromium.org/368733002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderListItem.h:72: bool updateMarker();
Suggest a comment explaining the meaning of the return value.

[leviw_travelin_and_unemployed, 2014-07-02 18:07:15]

LGTM with some nits.

I hope we get to a place where we actually stop modifying the render tree during
layout so this code could be further cleaned up.

https://codereview.chromium.org/368733002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderListItem.cpp (left):

https://codereview.chromium.org/368733002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderListItem.cpp:298:
markerParent->dirtyLinesFromChangedChild(m_marker);
I'm suspicious of this line being removed entirely. Perhaps do this in a
different change?

https://codereview.chromium.org/368733002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderListItem.h (right):

https://codereview.chromium.org/368733002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderListItem.h:72: bool updateMarker();
I may call this updateMarkerLocation still, since it primarily moves the marker
around.

[pdr., 2014-07-02 18:39:30]

Thanks for the reviews!

Unfortunately I don't think we can really test the change from crbug.com/390569
because the test will still fail due to crbug.com/389648. The fuzzers have
inputs aplenty in this area so I'm not worried about that. How do you feel with
landing this as a different fix for crbug.com/301196, relying on that test.

https://codereview.chromium.org/368733002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderListItem.cpp (left):

https://codereview.chromium.org/368733002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderListItem.cpp:298:
markerParent->dirtyLinesFromChangedChild(m_marker);
On 2014/07/02 18:07:14, leviw wrote:
> I'm suspicious of this line being removed entirely. Perhaps do this in a
> different change?

I agree, lets do this as a followup. Added a fixme and filed
http://crbug.com/391009

https://codereview.chromium.org/368733002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderListItem.h (right):

https://codereview.chromium.org/368733002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderListItem.h:72: bool updateMarker();
On 2014/07/02 05:59:42, skobes wrote:
> Suggest a comment explaining the meaning of the return value.

Done.

https://codereview.chromium.org/368733002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderListItem.h:72: bool updateMarker();
On 2014/07/02 18:07:14, leviw wrote:
> I may call this updateMarkerLocation still, since it primarily moves the
marker
> around.

Done.

[leviw_travelin_and_unemployed, 2014-07-02 21:18:07]

On 2014/07/02 at 18:39:30, pdr wrote:
> Thanks for the reviews!
> 
> Unfortunately I don't think we can really test the change from
crbug.com/390569 because the test will still fail due to crbug.com/389648. The
fuzzers have inputs aplenty in this area so I'm not worried about that. How do
you feel with landing this as a different fix for crbug.com/301196, relying on
that test.

I think that's fine. Can you cc me on crbug.com/390569?

> 
>
https://codereview.chromium.org/368733002/diff/60001/Source/core/rendering/Re...
> File Source/core/rendering/RenderListItem.cpp (left):
> 
>
https://codereview.chromium.org/368733002/diff/60001/Source/core/rendering/Re...
> Source/core/rendering/RenderListItem.cpp:298:
markerParent->dirtyLinesFromChangedChild(m_marker);
> On 2014/07/02 18:07:14, leviw wrote:
> > I'm suspicious of this line being removed entirely. Perhaps do this in a
> > different change?
> 
> I agree, lets do this as a followup. Added a fixme and filed
http://crbug.com/391009
> 
>
https://codereview.chromium.org/368733002/diff/60001/Source/core/rendering/Re...
> File Source/core/rendering/RenderListItem.h (right):
> 
>
https://codereview.chromium.org/368733002/diff/60001/Source/core/rendering/Re...
> Source/core/rendering/RenderListItem.h:72: bool updateMarker();
> On 2014/07/02 05:59:42, skobes wrote:
> > Suggest a comment explaining the meaning of the return value.
> 
> Done.
> 
>
https://codereview.chromium.org/368733002/diff/60001/Source/core/rendering/Re...
> Source/core/rendering/RenderListItem.h:72: bool updateMarker();
> On 2014/07/02 18:07:14, leviw wrote:
> > I may call this updateMarkerLocation still, since it primarily moves the
marker
> > around.
> 
> Done.

[pdr., 2014-07-02 21:20:02]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-07-02 21:21:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pdr@chromium.org/368733002/80001

[commit-bot: I haz the power, 2014-07-02 22:23:50]

Change committed as 177428