List marker pseudo elements.

List marker pseudo elements.

This CL converts list markers to use pseudo elements which are attached into
the DOM tree as needed instead of the ad-hoc method of updateListMarkers
used previously.

The code is currently behind a RuntimeEnabledFeature so we can land this without
effecting the current behaviour and then turn on when ready.

BUG=370461

[Julien - ping for review, 2014-12-05 19:44:02]

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

[Julien - ping for review, 2014-12-05 19:44:04]

Drive-by quick review. The direction seems OK.

https://codereview.chromium.org/778003003/diff/1/Source/core/css/RuleSetTest.cpp
File Source/core/css/RuleSetTest.cpp (right):

https://codereview.chromium.org/778003003/diff/1/Source/core/css/RuleSetTest....
Source/core/css/RuleSetTest.cpp:59: }
Only one unit test?

https://codereview.chromium.org/778003003/diff/1/Source/core/css/parser/Bison...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/778003003/diff/1/Source/core/css/parser/Bison...
Source/core/css/parser/BisonCSSParser-in.cpp:315: }
return type != CSSSelection::PseudoMarker?

https://codereview.chromium.org/778003003/diff/1/Source/core/css/resolver/Sty...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/778003003/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleResolver.cpp:736: if (pseudoId <
FIRST_INTERNAL_PSEUDOID && pseudoId != FIRST_LETTER &&
(RuntimeEnabledFeatures::listMarkerPseudoElementEnabled() && pseudoId != MARKER)
&& !parentRenderer->style()->hasPseudoStyle(pseudoId))
Maybe we should start about thinking of doing something better about this
condition as it gets very hairy.

https://codereview.chromium.org/778003003/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleResolver.cpp:823: }
This code is just super weirdly structured.

https://codereview.chromium.org/778003003/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/778003003/diff/1/Source/core/dom/Element.cpp#...
Source/core/dom/Element.cpp:1360: // were attached as they may need to move the
list marker.
Move the list maker? Usually moving renderers is a bad idea as it may leave
pointers around (less and less the case but we have had a lot of issue with that
in the past)

https://codereview.chromium.org/778003003/diff/1/Source/core/dom/MarkerPseudo...
File Source/core/dom/MarkerPseudoElement.cpp (right):

https://codereview.chromium.org/778003003/diff/1/Source/core/dom/MarkerPseudo...
Source/core/dom/MarkerPseudoElement.cpp:52: }
Does that code really still apply with a pseudo?

https://codereview.chromium.org/778003003/diff/1/Source/core/dom/MarkerPseudo...
Source/core/dom/MarkerPseudoElement.cpp:74:
ASSERT(parentRenderer->isListItem());
*cough, cough* toRenderListItem(parentRenderer) *cough, cough*

https://codereview.chromium.org/778003003/diff/1/Source/core/dom/MarkerPseudo...
Source/core/dom/MarkerPseudoElement.cpp:76: ASSERT(renderer());
Probably bad assumption no? How about changing display to none above the
pseudo-element?

https://codereview.chromium.org/778003003/diff/1/Source/core/dom/MarkerPseudo...
Source/core/dom/MarkerPseudoElement.cpp:109: ASSERT(renderer());
Same comment, is this assumption correct?

https://codereview.chromium.org/778003003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderListItem.cpp (right):

https://codereview.chromium.org/778003003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderListItem.cpp:61: if
(!RuntimeEnabledFeatures::listMarkerPseudoElementEnabled()) {
We prefer early return.

https://codereview.chromium.org/778003003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderListItem.cpp:227: return isEmpty();
Really nice infinite loop?

[dsinclair, 2015-01-23 20:46:36]

https://codereview.chromium.org/778003003/diff/1/Source/core/css/parser/Bison...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/778003003/diff/1/Source/core/css/parser/Bison...
Source/core/css/parser/BisonCSSParser-in.cpp:315: }
On 2014/12/05 19:44:03, Julien Chaffraix - PST wrote:
> return type != CSSSelection::PseudoMarker?

Done. I'd originally written this thinking we'd have more of these, but this
works for now and we can switch to something else later if needed.

https://codereview.chromium.org/778003003/diff/1/Source/core/css/resolver/Sty...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/778003003/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleResolver.cpp:736: if (pseudoId <
FIRST_INTERNAL_PSEUDOID && pseudoId != FIRST_LETTER &&
(RuntimeEnabledFeatures::listMarkerPseudoElementEnabled() && pseudoId != MARKER)
&& !parentRenderer->style()->hasPseudoStyle(pseudoId))
On 2014/12/05 19:44:03, Julien Chaffraix - PST wrote:
> Maybe we should start about thinking of doing something better about this
> condition as it gets very hairy.

Done.

https://codereview.chromium.org/778003003/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleResolver.cpp:823: }
On 2014/12/05 19:44:03, Julien Chaffraix - PST wrote:
> This code is just super weirdly structured.

Code is a work in progress, I though I needed to do some magic here to set the
style, but turns out I don't think I need it anymore. Re-structuring with that
in mind.

https://codereview.chromium.org/778003003/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/778003003/diff/1/Source/core/dom/Element.cpp#...
Source/core/dom/Element.cpp:1360: // were attached as they may need to move the
list marker.
On 2014/12/05 19:44:03, Julien Chaffraix - PST wrote:
> Move the list maker? Usually moving renderers is a bad idea as it may leave
> pointers around (less and less the case but we have had a lot of issue with
that
> in the past)

Yea, it sucks but we still have to do it. It's possible we've inserted something
before the list marker, or removed something that effects the list marker. We
have to get the parent to look at the tree and see if everything is correct.

This worked before because we'd do it in layout. It was the same concept, in
that it would look at who the line boxes were and decide where the list marker
goes.

https://codereview.chromium.org/778003003/diff/1/Source/core/dom/MarkerPseudo...
File Source/core/dom/MarkerPseudoElement.cpp (right):

https://codereview.chromium.org/778003003/diff/1/Source/core/dom/MarkerPseudo...
Source/core/dom/MarkerPseudoElement.cpp:52: }
On 2014/12/05 19:44:03, Julien Chaffraix - PST wrote:
> Does that code really still apply with a pseudo?

Yes, we still have to put the renderer somewhere. This code figures out where in
the render tree the RenderListItem will get inserted.

https://codereview.chromium.org/778003003/diff/1/Source/core/dom/MarkerPseudo...
Source/core/dom/MarkerPseudoElement.cpp:74:
ASSERT(parentRenderer->isListItem());
On 2014/12/05 19:44:03, Julien Chaffraix - PST wrote:
> *cough, cough* toRenderListItem(parentRenderer) *cough, cough*

Done.

https://codereview.chromium.org/778003003/diff/1/Source/core/dom/MarkerPseudo...
Source/core/dom/MarkerPseudoElement.cpp:76: ASSERT(renderer());
On 2014/12/05 19:44:04, Julien Chaffraix - PST wrote:
> Probably bad assumption no? How about changing display to none above the
> pseudo-element?

I believe this is safe, if we have a pseudo element, the we'll have a renderer.
If there is no renderer, we shouldn't have the pseudo element.

https://codereview.chromium.org/778003003/diff/1/Source/core/dom/MarkerPseudo...
Source/core/dom/MarkerPseudoElement.cpp:80: ASSERT(markerParent);
This one, on the other hand, is a bad assumption. If we aren't in the tree yet
we won't have a parent.

https://codereview.chromium.org/778003003/diff/1/Source/core/dom/MarkerPseudo...
Source/core/dom/MarkerPseudoElement.cpp:109: ASSERT(renderer());
On 2014/12/05 19:44:03, Julien Chaffraix - PST wrote:
> Same comment, is this assumption correct?

As above.

https://codereview.chromium.org/778003003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderListItem.cpp (right):

https://codereview.chromium.org/778003003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderListItem.cpp:61: if
(!RuntimeEnabledFeatures::listMarkerPseudoElementEnabled()) {
On 2014/12/05 19:44:04, Julien Chaffraix - PST wrote:
> We prefer early return.

Done.

https://codereview.chromium.org/778003003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderListItem.cpp:227: return isEmpty();
On 2014/12/05 at 19:44:04, Julien Chaffraix - CET wrote:
> Really nice infinite loop?

Done.

[Julien - ping for review, 2015-02-04 01:56:34]

Another drive by review. I would need to sit down and burn through the logic of
the patch to give more insightful comments but it will be difficult this week.

https://codereview.chromium.org/778003003/diff/100001/Source/core/css/RuleSet...
File Source/core/css/RuleSetTest.cpp (right):

https://codereview.chromium.org/778003003/diff/100001/Source/core/css/RuleSet...
Source/core/css/RuleSetTest.cpp:59: }
We should split this fix / test outside of this change that is already big.

https://codereview.chromium.org/778003003/diff/100001/Source/core/css/parser/...
File Source/core/css/parser/CSSGrammar.y (right):

https://codereview.chromium.org/778003003/diff/100001/Source/core/css/parser/...
Source/core/css/parser/CSSGrammar.y:1362:
!parser->isSelectorAllowedInParsingMode(type)) {
Doh'!?

https://codereview.chromium.org/778003003/diff/100001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/778003003/diff/100001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:756: if
(RuntimeEnabledFeatures::listMarkerPseudoElementEnabled() && pseudoId == MARKER
&& parentRenderer->style()->display() != LIST_ITEM)
pseudoId == MARKER && parentRenderer->style()->display() != LIST_ITEM is
repeated 3 times, we need a function to share the code.

https://codereview.chromium.org/778003003/diff/100001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:830: else if
(pseudoStyleRequest.pseudoId != MARKER || parentStyle->display() != LIST_ITEM)
This code is pretty confusing now IMHO. Keeping the old logic (with an
_unconditional_ applyMatchedProperties called).

https://codereview.chromium.org/778003003/diff/100001/Source/core/dom/MarkerP...
File Source/core/dom/MarkerPseudoElement.cpp (right):

https://codereview.chromium.org/778003003/diff/100001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:54: PassRefPtr<RenderStyle>
MarkerPseudoElement::customStyleForRenderer()
Is this function ever called?

https://codereview.chromium.org/778003003/diff/100001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:78: }
Couldn't we just have a CSS rule to handle that?

https://codereview.chromium.org/778003003/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderListItem.cpp (right):

https://codereview.chromium.org/778003003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderListItem.cpp:500:
toRenderListMarker(element->renderer())->updateMarginsAndContent();
This will need some explanations.

https://codereview.chromium.org/778003003/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderListMarker.cpp (right):

https://codereview.chromium.org/778003003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderListMarker.cpp:1651: RefPtr<RenderStyle> newStyle =
MarkerPseudoElement::styleForListMarkerRenderer(*m_listItem, style());
styleRef() (and more generally let's use const RenderStyle&)

https://codereview.chromium.org/778003003/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderObjectChildList.cpp (right):

https://codereview.chromium.org/778003003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderObjectChildList.cpp:51: if
(RuntimeEnabledFeatures::listMarkerPseudoElementEnabled()) {
Adding a branch here is not a great idea. It would be better if
MarkerPseudoElement handled it instead of in this general function.

[dsinclair, 2015-02-04 04:41:33]

https://codereview.chromium.org/778003003/diff/100001/Source/core/css/RuleSet...
File Source/core/css/RuleSetTest.cpp (right):

https://codereview.chromium.org/778003003/diff/100001/Source/core/css/RuleSet...
Source/core/css/RuleSetTest.cpp:59: }
On 2015/02/04 at 01:56:33, Julien Chaffraix - AEST wrote:
> We should split this fix / test outside of this change that is already big.


This is the test for UA only pseudo elements. That part of the patch can be
split out, I'm just trying to get everything to work correctly first, heh.

https://codereview.chromium.org/778003003/diff/100001/Source/core/css/parser/...
File Source/core/css/parser/CSSGrammar.y (right):

https://codereview.chromium.org/778003003/diff/100001/Source/core/css/parser/...
Source/core/css/parser/CSSGrammar.y:1362:
!parser->isSelectorAllowedInParsingMode(type)) {
On 2015/02/04 at 01:56:33, Julien Chaffraix - AEST wrote:
> Doh'!?

? Are you saying this is incorrect, or you don't like the way it's written with
a !?

https://codereview.chromium.org/778003003/diff/100001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/778003003/diff/100001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:830: else if
(pseudoStyleRequest.pseudoId != MARKER || parentStyle->display() != LIST_ITEM)
On 2015/02/04 at 01:56:33, Julien Chaffraix - AEST wrote:
> This code is pretty confusing now IMHO. Keeping the old logic (with an
_unconditional_ applyMatchedProperties called).

Not sure what you mean, do you mean keeping the old logic with the unconditional
applyCallbackSelectors?

The applyCallbackSelectors will only be called in the MARKER or LIST_ITEM case
at the moment. If it isn't a marker then we will return as we did previously.

https://codereview.chromium.org/778003003/diff/100001/Source/core/dom/MarkerP...
File Source/core/dom/MarkerPseudoElement.cpp (right):

https://codereview.chromium.org/778003003/diff/100001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:54: PassRefPtr<RenderStyle>
MarkerPseudoElement::customStyleForRenderer()
On 2015/02/04 at 01:56:33, Julien Chaffraix - AEST wrote:
> Is this function ever called?

Yes. This is an override of a method on Element. It's called from
Element::styleForRenderer.

https://codereview.chromium.org/778003003/diff/100001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:78: }
On 2015/02/04 at 01:56:34, Julien Chaffraix - AEST wrote:
> Couldn't we just have a CSS rule to handle that?

A CSS rule to handle what, the resetting of the margins?

https://codereview.chromium.org/778003003/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderListItem.cpp (right):

https://codereview.chromium.org/778003003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderListItem.cpp:500:
toRenderListMarker(element->renderer())->updateMarginsAndContent();
On 2015/02/04 at 01:56:34, Julien Chaffraix - AEST wrote:
> This will need some explanations.


This is the one bit that was needed from the old updateMarkerLocation() code.
The renderer() for the MARKER element is the RenderListMarker, so when the list
item changes we update the marker.

https://codereview.chromium.org/778003003/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderObjectChildList.cpp (right):

https://codereview.chromium.org/778003003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderObjectChildList.cpp:51: if
(RuntimeEnabledFeatures::listMarkerPseudoElementEnabled()) {
On 2015/02/04 at 01:56:34, Julien Chaffraix - AEST wrote:
> Adding a branch here is not a great idea. It would be better if
MarkerPseudoElement handled it instead of in this general function.

The problem is, MarkerPseudoElement isn't invoked. The Marker can be inserted
into child renderers down the tree. That child renderer gets removed from the
tree. When we cleanup the RenderListMarker (which is the
MarkerPseudoElement->renderer()) we have to tell the MarkerPseudoElement that
we're going away. Otherwise, we end up destroying the RenderListMarker but
MarkerPseudoElement points at a renderer without a parent() which causes crashes
later.

Suggestions on a better way to handle this?

[dsinclair, 2015-02-04 15:30:59]

https://codereview.chromium.org/778003003/diff/100001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/778003003/diff/100001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:756: if
(RuntimeEnabledFeatures::listMarkerPseudoElementEnabled() && pseudoId == MARKER
&& parentRenderer->style()->display() != LIST_ITEM)
On 2015/02/04 at 01:56:33, Julien Chaffraix - AEST wrote:
> pseudoId == MARKER && parentRenderer->style()->display() != LIST_ITEM is
repeated 3 times, we need a function to share the code.

They're slightly different. This one is == && != and the other one on is != ||
!=

https://codereview.chromium.org/778003003/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderListMarker.cpp (right):

https://codereview.chromium.org/778003003/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderListMarker.cpp:1651: RefPtr<RenderStyle> newStyle =
MarkerPseudoElement::styleForListMarkerRenderer(*m_listItem, style());
On 2015/02/04 at 01:56:34, Julien Chaffraix - AEST wrote:
> styleRef() (and more generally let's use const RenderStyle&)


Making this a const RenderStyle& is a much larger change as we have to make
anything that calls Element::styleForRenderer take a const RenderStyle&. I think
this should stay until we convert the rest of the code over to const
RenderStyle&.

[esprehn, 2015-04-20 16:04:06]

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

[esprehn, 2015-04-20 16:04:08]

You need to do this a different way, you're rebuilding inherit style changes and
other things.

https://codereview.chromium.org/778003003/diff/180001/Source/core/css/parser/...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/778003003/diff/180001/Source/core/css/parser/...
Source/core/css/parser/BisonCSSParser-in.cpp:245: bool
BisonCSSParser::isSelectorAllowedInParsingMode(CSSSelector::PseudoType type)
Let's not add any new features to the old parser.

https://codereview.chromium.org/778003003/diff/180001/Source/core/dom/MarkerP...
File Source/core/dom/MarkerPseudoElement.cpp (right):

https://codereview.chromium.org/778003003/diff/180001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:23: LayoutObject*
MarkerPseudoElement::getParentOfFirstLineBox(LayoutBlockFlow* curr, const
LayoutObject* marker)
Remove get prefix.

https://codereview.chromium.org/778003003/diff/180001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:63: return
styleForListMarkerLayoutObject(*parent, layoutObject()->style());
This isn't right,  you're rebuilding what is Inheritance style changes.  This is
also unstable, when you first attach this returns null and then later starts
returning a value.

https://codereview.chromium.org/778003003/diff/180001/Source/core/layout/Layo...
File Source/core/layout/LayoutListItem.cpp (right):

https://codereview.chromium.org/778003003/diff/180001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:101: 
This is an example why you can't do this, it's very easy to mess it up.

https://codereview.chromium.org/778003003/diff/180001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:102: if (PseudoElement* element =
toElement(node())->pseudoElement(MARKER))
You can't do synchronous subtree notifications like this.

https://codereview.chromium.org/778003003/diff/180001/Source/core/layout/Layo...
File Source/core/layout/LayoutListMarker.cpp (right):

https://codereview.chromium.org/778003003/diff/180001/Source/core/layout/Layo...
Source/core/layout/LayoutListMarker.cpp:1058: setReplaced(true); // pretend to
be replaced
Link to the spec?

https://codereview.chromium.org/778003003/diff/180001/Source/core/layout/Layo...
Source/core/layout/LayoutListMarker.cpp:1105:
node()->parentElement()->layoutObject()->containingBlock()->updateLogicalWidth();
You can't update logical widths from inside a style change (especially not on
your containingBlock), more things might change during style recalc or in its
descendants still. All you can do is mark yourself dirty.

[dsinclair, 2015-04-21 01:14:14]

On 2015/04/20 at 16:04:08, esprehn wrote:
>
https://codereview.chromium.org/778003003/diff/180001/Source/core/layout/Layo...
> Source/core/layout/LayoutListMarker.cpp:1105:
node()->parentElement()->layoutObject()->containingBlock()->updateLogicalWidth();
> You can't update logical widths from inside a style change (especially not on
your containingBlock), more things might change during style recalc or in its
descendants still. All you can do is mark yourself dirty.


I'll give it a try to mark the containing block as dirty and see if that fixes
the issue. This is the bit that had to be done for the inside list markers
otherwise the containing block ends up being too small and we warp text lines by
accident.

[dsinclair, 2015-04-21 20:23:09]

dsinclair@chromium.org changed reviewers:
+ leviw@chromium.org
- jchaffraix@chromium.org

[dsinclair, 2015-04-21 20:23:10]

PTAL.

https://codereview.chromium.org/778003003/diff/180001/Source/core/css/parser/...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/778003003/diff/180001/Source/core/css/parser/...
Source/core/css/parser/BisonCSSParser-in.cpp:245: bool
BisonCSSParser::isSelectorAllowedInParsingMode(CSSSelector::PseudoType type)
On 2015/04/20 at 16:04:08, esprehn wrote:
> Let's not add any new features to the old parser.

Done.

https://codereview.chromium.org/778003003/diff/180001/Source/core/dom/MarkerP...
File Source/core/dom/MarkerPseudoElement.cpp (right):

https://codereview.chromium.org/778003003/diff/180001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:23: LayoutObject*
MarkerPseudoElement::getParentOfFirstLineBox(LayoutBlockFlow* curr, const
LayoutObject* marker)
On 2015/04/20 at 16:04:08, esprehn wrote:
> Remove get prefix.

Done.

https://codereview.chromium.org/778003003/diff/180001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:63: return
styleForListMarkerLayoutObject(*parent, layoutObject()->style());
On 2015/04/20 at 16:04:08, esprehn wrote:
> This isn't right,  you're rebuilding what is Inheritance style changes.  This
is also unstable, when you first attach this returns null and then later starts
returning a value.


I'm not sure I understand, what is the correct way to do this?

The code in styleForListMarkerLayoutObject is moved from
LayoutListMarker::listItemStyleDidChange() and does the same as it did
previously. This method is just calling down to that in order to retrieve the
style.

https://codereview.chromium.org/778003003/diff/180001/Source/core/layout/Layo...
File Source/core/layout/LayoutListItem.cpp (right):

https://codereview.chromium.org/778003003/diff/180001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:101: 
On 2015/04/20 at 16:04:08, esprehn wrote:
> This is an example why you can't do this, it's very easy to mess it up.

Done.

https://codereview.chromium.org/778003003/diff/180001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:102: if (PseudoElement* element =
toElement(node())->pseudoElement(MARKER))
On 2015/04/20 at 16:04:08, esprehn wrote:
> You can't do synchronous subtree notifications like this.

Done.

https://codereview.chromium.org/778003003/diff/180001/Source/core/layout/Layo...
File Source/core/layout/LayoutListMarker.cpp (right):

https://codereview.chromium.org/778003003/diff/180001/Source/core/layout/Layo...
Source/core/layout/LayoutListMarker.cpp:1058: setReplaced(true); // pretend to
be replaced
On 2015/04/20 at 16:04:08, esprehn wrote:
> Link to the spec?

This is from the constructor below. I needed to add a second constructor in
order to provide the node pointer for the LayoutBox call. I'm not sure why these
are set on the ListMarker.

https://codereview.chromium.org/778003003/diff/180001/Source/core/layout/Layo...
Source/core/layout/LayoutListMarker.cpp:1105:
node()->parentElement()->layoutObject()->containingBlock()->updateLogicalWidth();
On 2015/04/20 at 16:04:08, esprehn wrote:
> You can't update logical widths from inside a style change (especially not on
your containingBlock), more things might change during style recalc or in its
descendants still. All you can do is mark yourself dirty.


Done. If I remove the updateMarginsAndContent() things no longer work, is that
one OK to do inside style did change?

[esprehn, 2015-04-22 07:45:42]

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

[esprehn, 2015-04-22 07:45:46]

How do the default list markers work? ex.

<ol>
  <li>
</ol>

If I never specify ::marker in my CSS, what creates the marker for that <li>?

https://codereview.chromium.org/778003003/diff/180001/Source/core/layout/Layo...
File Source/core/layout/LayoutListMarker.cpp (right):

https://codereview.chromium.org/778003003/diff/180001/Source/core/layout/Layo...
Source/core/layout/LayoutListMarker.cpp:1105:
node()->parentElement()->layoutObject()->containingBlock()->updateLogicalWidth();
On 2015/04/21 20:23:10, dsinclair wrote:
> On 2015/04/20 at 16:04:08, esprehn wrote:
> > You can't update logical widths from inside a style change (especially not
on
> your containingBlock), more things might change during style recalc or in its
> descendants still. All you can do is mark yourself dirty.
> 
> 
> Done. If I remove the updateMarginsAndContent() things no longer work, is that
> one OK to do inside style did change?

You should do this in layout, we already do for the isImage() case, just move
that out of the if statement.

https://codereview.chromium.org/778003003/diff/260001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:718: return
MarkerPseudoElement::create(parent);
if (pseudoId == MARKER) {
  ASSERT(RuntimeEnabledFeatures::listMarkerPseudoElementEnabled());
  return ...;
}

we should never even get here if it's disabled. Otherwise you'll return a
regular PseudoElement which is the wrong kind and that's bad.

https://codereview.chromium.org/778003003/diff/260001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:724: bool
StyleResolver::pseudoElementRequired(PseudoId pseudoId, LayoutObject&
parentRenderer)
This function doesn't use any state on |this|, it should be only in the .cpp
file and static.

https://codereview.chromium.org/778003003/diff/260001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:728: if
(RuntimeEnabledFeatures::listMarkerPseudoElementEnabled() && pseudoId == MARKER)
I don't think you need the RuntimeEnabledFeatures flag here, we should just
never even get pseudoId == MARKER out of the system.

https://codereview.chromium.org/778003003/diff/260001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:749: if
(RuntimeEnabledFeatures::listMarkerPseudoElementEnabled() && pseudoId == MARKER
&& parentRenderer->style()->display() != LIST_ITEM)
ditto, no runtime check.

https://codereview.chromium.org/778003003/diff/260001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:823: else if
(pseudoStyleRequest.pseudoId != MARKER || parentStyle->display() != LIST_ITEM)
Can you explain this? Why is MARKER and LIST_ITEM special? You're changing the
behavior of all list items which doesn't seem right.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/Element...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2770: && (pseudoId == MARKER &&
layoutObject()->style()->display() != LIST_ITEM))) {
What's up with this check for != LIST_ITEM all over?

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2806: if (isPseudoElement() && (pseudoId != MARKER
|| isMarkerPseudoElement()))
Marker shouldn't need a special case here. PseudoElements can never have nested
pseudo elements. This branch doesn't need to change.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/Element...
File Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/Element...
Source/core/dom/ElementRareData.h:163: RefPtrWillBeMember<PseudoElement>
m_marker;
We really probably need to introduce a PseudoElementData object that holds all
these ptrs, we're making ElementRareData really big for something that's pretty
rare now.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/LayoutT...
File Source/core/dom/LayoutTreeBuilder.cpp (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/LayoutT...
Source/core/dom/LayoutTreeBuilder.cpp:85: if (LayoutObject* markerLayoutObject =
MarkerPseudoElement::parentOfFirstLineBox(toLayoutBlockFlow(m_layoutObjectParent),
nullptr))
This needs to be encapsulated, this code shouldn't need to know about
LayoutBlockFlow.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/LayoutT...
Source/core/dom/LayoutTreeBuilder.cpp:120: if (m_node->isMarkerPseudoElement()
&& m_layoutObjectParent->style()->display() == LIST_ITEM)
We should never even get here if the parent is the wrong display type.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/LayoutT...
Source/core/dom/LayoutTreeBuilder.cpp:152: LayoutObject* nextLayoutObject =
this->nextLayoutObject(parentLayoutObject);
remove the argument, just call parentLayoutObject again for markers, they're
rare.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
File Source/core/dom/MarkerPseudoElement.cpp (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:56: if (!layoutObject())
This isn't right, you should never look at the layout tree from inside
customStyleForLayoutObject.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:59: LayoutObject* parent =
layoutObject()->parent();
You need to call LayoutTreeBuilderTraversal::parent here and return the right
value always.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:66: PassRefPtr<ComputedStyle>
MarkerPseudoElement::styleForListMarkerLayoutObject(LayoutObject& parent, const
ComputedStyle* style)
styleForListMarker(...) no need for LayoutObject in the name.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:77:
newStyle->setMarginEnd(style->marginRight());
This code should just be inlined in the above method.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:94:
marker->setStyle(ComputedStyle::clone(style));
Don't clone the style, you never need to do that in createLayoutObject.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:121: marker->updateMarginsAndContent();
Layout calls this for isImage(), make it call it every time and remove this.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:128:
parentLayoutObject->containingBlock()->updateLogicalWidth();
You can't update logical widths inside style recalc. You can only mark dirty
bits.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:139:
marker->setStyle(newStyle.release());
::attach went through the customStyleForLayoutObject code, that thing needs to
always return the right style. You shouldn't set the style sometimes there when
updating and sometimes here when attaching.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
File Source/core/dom/MarkerPseudoElement.h (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.h:35: void attachListMarkerLayoutObject();
attachListMarker(), no need for LayoutObject in the name.

https://codereview.chromium.org/778003003/diff/260001/Source/core/html/HTMLOL...
File Source/core/html/HTMLOListElement.cpp (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/html/HTMLOL...
Source/core/html/HTMLOListElement.cpp:125: updateItemValues();
Why is this needed? All updateItemValues does is:
updateItemValuesForOrderedList

which goes into each list item doing:

    if (!m_hasExplicitValue) {
        m_isValueUpToDate = false;
        if (m_marker)
           
m_marker->setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation(LayoutInvalidationReason::ListValueChange);
    }

but every list item is brand new, we just attached them, that means they all
need layout already, all need paint invalidation, and the m_isValueUpToDate
field starts as false. So this shouldn't do anything.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
File Source/core/layout/LayoutListItem.cpp (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:98: if (!notifyAncestorsOfSubtreeChange())
The tree walk should handle this for you, not the impls.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:101: if (PseudoElement* element =
toElement(node())->pseudoElement(MARKER))
Having the layout tree reach back into the DOM like this seems unfortunate.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:104:
LayoutBlockFlow::handleSubtreeModifications();
tree walk should be external to the notification callbacks.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:175: Node* otherList =
enclosingList(toLayoutListItem(layoutObject));
I'm really not sure all this is a code improvement, it just makes it all more
verbose. Maybe layouter? or just object?

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:240: return !(element && lastChild() !=
element->layoutObject());
Run demorgans.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:326: PseudoElement* element =
toElement(node())->pseudoElement(MARKER);
I think you just want to keep m_marker, going out through the DOM to get back to
one of your descendants is kind of silly.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:327: if (element &&
element->layoutObject()) {
How does the element exist but not have a layoutObject? Earlier in updateValue()
you don't null check layoutObject.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:352: MarkerPseudoElement* markerElement =
toMarkerPseudoElement(toElement(node())->pseudoElement(MARKER));
I really think you want to keep m_marker and not do this. Going out through the
DOM all the time in layout is bad. We want to go the other direction with the
layout code and make it talk the DOM less (especially if we ever want this
system to be thread safe).

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:474: return
toLayoutListMarker(element->layoutObject())->text();
ditto.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:487:
element->layoutObject()->setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation(LayoutInvalidationReason::ListValueChange);
same.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:527:
toLayoutListMarker(element->layoutObject())->updateMarginsAndContent();
ditto

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.cpp (left):

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.cpp:3064: ASSERT(!node());
Removing this means you caused a bug.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.cpp:2753: if (pseudo < FIRST_INTERNAL_PSEUDOID
&& !style()->hasPseudoStyle(pseudo) && style()->display() != LIST_ITEM)
remove list item check here. List item shouldn't be special, this isn't right.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
File Source/core/layout/LayoutObjectChildList.cpp (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutObjectChildList.cpp:43: //
RuntimeEnabledFeatures::listMarkerPseudoElementEnabled
??

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutObjectChildList.cpp:44: // List markers are owned by
the list marker pseudo element adn so don't get destroyed by this container.
typo.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutObjectChildList.cpp:45: // else
huh?

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Text...
File Source/core/layout/TextAutosizer.cpp (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Text...
Source/core/layout/TextAutosizer.cpp:484: if (PseudoElement* element =
toElement(parent->node())->pseudoElement(MARKER)) {
remove, leave m_marker instead. The attachListMarker method on the PseudoElement
object should call LayoutListItem::setMarker.

[dsinclair, 2015-04-22 20:00:39]

The default markers were suppose to be done in UA style but I seem to have
dropped it and put in some hacky code in response. I've re-added the UA style
and dropped the code instead.

https://codereview.chromium.org/778003003/diff/260001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:718: return
MarkerPseudoElement::create(parent);
On 2015/04/22 at 07:45:45, esprehn wrote:
> if (pseudoId == MARKER) {
>   ASSERT(RuntimeEnabledFeatures::listMarkerPseudoElementEnabled());
>   return ...;
> }
> 
> we should never even get here if it's disabled. Otherwise you'll return a
regular PseudoElement which is the wrong kind and that's bad.


Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:724: bool
StyleResolver::pseudoElementRequired(PseudoId pseudoId, LayoutObject&
parentRenderer)
On 2015/04/22 at 07:45:45, esprehn wrote:
> This function doesn't use any state on |this|, it should be only in the .cpp
file and static.

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:728: if
(RuntimeEnabledFeatures::listMarkerPseudoElementEnabled() && pseudoId == MARKER)
On 2015/04/22 at 07:45:44, esprehn wrote:
> I don't think you need the RuntimeEnabledFeatures flag here, we should just
never even get pseudoId == MARKER out of the system.

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:749: if
(RuntimeEnabledFeatures::listMarkerPseudoElementEnabled() && pseudoId == MARKER
&& parentRenderer->style()->display() != LIST_ITEM)
On 2015/04/22 at 07:45:45, esprehn wrote:
> ditto, no runtime check.

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:823: else if
(pseudoStyleRequest.pseudoId != MARKER || parentStyle->display() != LIST_ITEM)
On 2015/04/22 at 07:45:45, esprehn wrote:
> Can you explain this? Why is MARKER and LIST_ITEM special? You're changing the
behavior of all list items which doesn't seem right.


The LIST_ITEM bit was removed. The MARKER bit is required because it's possible
we're in here for a MARKER and there is no stylesheet entry. We still need to
return a style for the MARKER as it's based off the parent style.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/Element...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2770: && (pseudoId == MARKER &&
layoutObject()->style()->display() != LIST_ITEM))) {
On 2015/04/22 at 07:45:45, esprehn wrote:
> What's up with this check for != LIST_ITEM all over?


If you have something like: <div style="display: list-item"> it will have a
MARKER, but, we then change to display:block we're going to have a MARKER but no
longer be a list item. In that case, we have to destroy the MARKER as it's no
longer valid.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2806: if (isPseudoElement() && (pseudoId != MARKER
|| isMarkerPseudoElement()))
On 2015/04/22 at 07:45:45, esprehn wrote:
> Marker shouldn't need a special case here. PseudoElements can never have
nested pseudo elements. This branch doesn't need to change.


With marker they can. If we have something like:

    #a:after {
        display: list-item;
        content: "ONE";
    }

Then the ::after pseudo element will have a ::marker pseudo element under it.
Added a comment.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/Element...
File Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/Element...
Source/core/dom/ElementRareData.h:163: RefPtrWillBeMember<PseudoElement>
m_marker;
On 2015/04/22 at 07:45:45, esprehn wrote:
> We really probably need to introduce a PseudoElementData object that holds all
these ptrs, we're making ElementRareData really big for something that's pretty
rare now.


File crbug.com/479745 to move this to a PseudoElementData object.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/LayoutT...
File Source/core/dom/LayoutTreeBuilder.cpp (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/LayoutT...
Source/core/dom/LayoutTreeBuilder.cpp:85: if (LayoutObject* markerLayoutObject =
MarkerPseudoElement::parentOfFirstLineBox(toLayoutBlockFlow(m_layoutObjectParent),
nullptr))
On 2015/04/22 at 07:45:45, esprehn wrote:
> This needs to be encapsulated, this code shouldn't need to know about
LayoutBlockFlow.


Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/LayoutT...
Source/core/dom/LayoutTreeBuilder.cpp:120: if (m_node->isMarkerPseudoElement()
&& m_layoutObjectParent->style()->display() == LIST_ITEM)
On 2015/04/22 at 07:45:45, esprehn wrote:
> We should never even get here if the parent is the wrong display type.

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/LayoutT...
Source/core/dom/LayoutTreeBuilder.cpp:152: LayoutObject* nextLayoutObject =
this->nextLayoutObject(parentLayoutObject);
On 2015/04/22 at 07:45:45, esprehn wrote:
> remove the argument, just call parentLayoutObject again for markers, they're
rare.

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
File Source/core/dom/MarkerPseudoElement.cpp (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:56: if (!layoutObject())
On 2015/04/22 at 07:45:45, esprehn wrote:
> This isn't right, you should never look at the layout tree from inside
customStyleForLayoutObject.

Removed customStyleForLayoutObjects and did calls from ::attach as the marker
style needs me to know the list item styling.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:59: LayoutObject* parent =
layoutObject()->parent();
On 2015/04/22 at 07:45:45, esprehn wrote:
> You need to call LayoutTreeBuilderTraversal::parent here and return the right
value always.

Removed.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:66: PassRefPtr<ComputedStyle>
MarkerPseudoElement::styleForListMarkerLayoutObject(LayoutObject& parent, const
ComputedStyle* style)
On 2015/04/22 at 07:45:45, esprehn wrote:
> styleForListMarker(...) no need for LayoutObject in the name.

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:77:
newStyle->setMarginEnd(style->marginRight());
On 2015/04/22 at 07:45:45, esprehn wrote:
> This code should just be inlined in the above method.


It can be in the future when we remove the old code paths, but at the moment
this is called through them and needs to be separate. Added a FIXME.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:94:
marker->setStyle(ComputedStyle::clone(style));
On 2015/04/22 at 07:45:45, esprehn wrote:
> Don't clone the style, you never need to do that in createLayoutObject.

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:121: marker->updateMarginsAndContent();
On 2015/04/22 at 07:45:45, esprehn wrote:
> Layout calls this for isImage(), make it call it every time and remove this.

That will mean we'll do it on every layout, where this will just do it when the
marker is attached.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:128:
parentLayoutObject->containingBlock()->updateLogicalWidth();
On 2015/04/22 at 07:45:45, esprehn wrote:
> You can't update logical widths inside style recalc. You can only mark dirty
bits.

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.cpp:139:
marker->setStyle(newStyle.release());
On 2015/04/22 at 07:45:45, esprehn wrote:
> ::attach went through the customStyleForLayoutObject code, that thing needs to
always return the right style. You shouldn't set the style sometimes there when
updating and sometimes here when attaching.

Style is set here only now.

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
File Source/core/dom/MarkerPseudoElement.h (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/dom/MarkerP...
Source/core/dom/MarkerPseudoElement.h:35: void attachListMarkerLayoutObject();
On 2015/04/22 at 07:45:45, esprehn wrote:
> attachListMarker(), no need for LayoutObject in the name.

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/html/HTMLOL...
File Source/core/html/HTMLOListElement.cpp (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/html/HTMLOL...
Source/core/html/HTMLOListElement.cpp:125: updateItemValues();
On 2015/04/22 at 07:45:45, esprehn wrote:
> Why is this needed? All updateItemValues does is:
> updateItemValuesForOrderedList
> 
> which goes into each list item doing:
> 
>     if (!m_hasExplicitValue) {
>         m_isValueUpToDate = false;
>         if (m_marker)
>            
m_marker->setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation(LayoutInvalidationReason::ListValueChange);
>     }
> 
> but every list item is brand new, we just attached them, that means they all
need layout already, all need paint invalidation, and the m_isValueUpToDate
field starts as false. So this shouldn't do anything.


This works fine for lists that aren't reversed. The problem is, with a reversed
list we do something along the lines of:

- add first item, see only one item, set value to 1
- add second item, see previous was 1, set value to 0
- ...

We quickly end up with negative list values. There is logic in
LayoutListItem::updateListMarkerNumbers which tells it to not call updateValue()
if listNode->needsAttach(). Adding this call here means we can do one walk to
update all of the values for reverse lists at the same time and should save the
O(n^2) walk to do it as we attach.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
File Source/core/layout/LayoutListItem.cpp (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:98: if (!notifyAncestorsOfSubtreeChange())
On 2015/04/22 at 07:45:46, esprehn wrote:
> The tree walk should handle this for you, not the impls.

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:101: if (PseudoElement* element =
toElement(node())->pseudoElement(MARKER))
On 2015/04/22 at 07:45:46, esprehn wrote:
> Having the layout tree reach back into the DOM like this seems unfortunate.

I'm not sure how to fix this one? The PseudoElement knows where to insert the
layoutObject and we know that descendants have changed, so we have to ask it to
do the work.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:104:
LayoutBlockFlow::handleSubtreeModifications();
On 2015/04/22 at 07:45:46, esprehn wrote:
> tree walk should be external to the notification callbacks.

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:175: Node* otherList =
enclosingList(toLayoutListItem(layoutObject));
On 2015/04/22 at 07:45:46, esprehn wrote:
> I'm really not sure all this is a code improvement, it just makes it all more
verbose. Maybe layouter? or just object?

Ack.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:240: return !(element && lastChild() !=
element->layoutObject());
On 2015/04/22 at 07:45:46, esprehn wrote:
> Run demorgans.

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:326: PseudoElement* element =
toElement(node())->pseudoElement(MARKER);
On 2015/04/22 at 07:45:46, esprehn wrote:
> I think you just want to keep m_marker, going out through the DOM to get back
to one of your descendants is kind of silly.

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:327: if (element &&
element->layoutObject()) {
On 2015/04/22 at 07:45:46, esprehn wrote:
> How does the element exist but not have a layoutObject? Earlier in
updateValue() you don't null check layoutObject.

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:352: MarkerPseudoElement* markerElement =
toMarkerPseudoElement(toElement(node())->pseudoElement(MARKER));
On 2015/04/22 at 07:45:46, esprehn wrote:
> I really think you want to keep m_marker and not do this. Going out through
the DOM all the time in layout is bad. We want to go the other direction with
the layout code and make it talk the DOM less (especially if we ever want this
system to be thread safe).

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:474: return
toLayoutListMarker(element->layoutObject())->text();
On 2015/04/22 at 07:45:45, esprehn wrote:
> ditto.

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:487:
element->layoutObject()->setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation(LayoutInvalidationReason::ListValueChange);
On 2015/04/22 at 07:45:46, esprehn wrote:
> same.

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:527:
toLayoutListMarker(element->layoutObject())->updateMarginsAndContent();
On 2015/04/22 at 07:45:46, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.cpp (left):

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.cpp:3064: ASSERT(!node());
On 2015/04/22 at 07:45:46, esprehn wrote:
> Removing this means you caused a bug.

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.cpp:2753: if (pseudo < FIRST_INTERNAL_PSEUDOID
&& !style()->hasPseudoStyle(pseudo) && style()->display() != LIST_ITEM)
On 2015/04/22 at 07:45:46, esprehn wrote:
> remove list item check here. List item shouldn't be special, this isn't right.

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
File Source/core/layout/LayoutObjectChildList.cpp (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutObjectChildList.cpp:43: //
RuntimeEnabledFeatures::listMarkerPseudoElementEnabled
On 2015/04/22 at 07:45:46, esprehn wrote:
> ??

I need to update this comment when we remove the old code. This way I can find
it. Seemed like the simplest way to mark since the ownership changes.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutObjectChildList.cpp:44: // List markers are owned by
the list marker pseudo element adn so don't get destroyed by this container.
On 2015/04/22 at 07:45:46, esprehn wrote:
> typo.

Done.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Layo...
Source/core/layout/LayoutObjectChildList.cpp:45: // else
On 2015/04/22 at 07:45:46, esprehn wrote:
> huh?

Ditto.

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Text...
File Source/core/layout/TextAutosizer.cpp (right):

https://codereview.chromium.org/778003003/diff/260001/Source/core/layout/Text...
Source/core/layout/TextAutosizer.cpp:484: if (PseudoElement* element =
toElement(parent->node())->pseudoElement(MARKER)) {
On 2015/04/22 at 07:45:46, esprehn wrote:
> remove, leave m_marker instead. The attachListMarker method on the
PseudoElement object should call LayoutListItem::setMarker.

Done.

[esprehn, 2015-04-22 21:40:09]

Let's talk about this next week,  I'm concerned you're using this system
that was designed for things that are rare for lists which are definitely
not rare. What  other browsers support ::marker?  Why do we think this is
important?

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dsinclair, 2015-04-22 23:22:13]

On 2015/04/22 at 21:40:09, esprehn wrote:
> Let's talk about this next week,  I'm concerned you're using this system
> that was designed for things that are rare for lists which are definitely
> not rare. What  other browsers support ::marker?  Why do we think this is
> important?
> 

No browsers support ::marker (and even with this change we won't support
::marker as I've setup the CSS to make it UA style only). But, it puts us in a
good position to make the style visible and match the spec. The spec itself
(http://www.w3.org/TR/css3-lists/#markers) is still in Working Draft state.

I'm trying to fix the modifying layout tree during layout issue, ::marker is
just a useful side effect from the implementation.

Adding Tab if he has any comments on why ::marker is important.

[dsinclair, 2015-04-22 23:22:38]

+tabatkins

[esprehn, 2015-04-23 06:12:42]

We should discuss this next week. As currently implemented this is going to
make every layout slower with a whole tree walk and also make all lists on
the web with markers (default or otherwise)  much slower.

I don't really think fixing the layout tree modification is worth making
all layouts slower.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[esprehn, 2015-04-23 07:16:28]

Sorry that's every style recalc that modifies the tree, which is initial
page load.  Can you try running the html5-full-render benchmark?

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.