Update list markers in notify change.

Update list markers in notify change.

This CL updates the list item code to place the LayoutListMaker into
the tree during the NotifyChange phase. This moves the layout tree
modification outside of the actual layout walk.

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

[dsinclair, 2015-04-29 18:58:22]

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

[dsinclair, 2015-04-29 18:58:23]

PTAL.

This is the implementation we discussed in the meeting where we move
updateListMarkers to happen during notifyChange. This CL depends on the notify
change CL. All layout tests pass locally.

[ojan, 2015-04-30 02:56:45]

This mostly looks good.

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

https://codereview.chromium.org/1110233003/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:102:
containingBlock()->setPreferredLogicalWidthsDirty();
I'm not 100% sure, but I think you want to setPreferredLogicalWidthsDirty() on
the LayoutListItem itself since it's the preferred width of the list item itself
that changes. That will mark up the tree appropriately.

https://codereview.chromium.org/1110233003/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:464: setNeedsSubtreeChangeNotification();
Since we set this in the constructor, do we need to call this here? I only see
one caller of this function and that's inside attach, where were should have
just created the LayoutListItem.

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

https://codereview.chromium.org/1110233003/diff/80001/Source/core/layout/Text...
Source/core/layout/TextAutosizer.cpp:479: if (parent->isListItem()) {
I think it'd be better to keep this code in LayoutListItem::layout instead of in
the generic TextAutosizer code. It avoids a needless branch for non list-items
that way.

pdr, mind taking a look over this since you understand the auto-sizing code? I
believe the code inside this if-statement can just be wholesale moved back into
LayoutListItem::layout.

[ojan, 2015-04-30 03:31:04]

https://codereview.chromium.org/1110233003/diff/80001/Source/core/layout/Layo...
File Source/core/layout/LayoutListItem.h (right):

https://codereview.chromium.org/1110233003/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.h:77: virtual void subtreeDidChange()
override;
s/override/final

[dsinclair, 2015-04-30 17:52:30]

PTAL.

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

https://codereview.chromium.org/1110233003/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:102:
containingBlock()->setPreferredLogicalWidthsDirty();
On 2015/04/30 at 02:56:45, ojan wrote:
> I'm not 100% sure, but I think you want to setPreferredLogicalWidthsDirty() on
the LayoutListItem itself since it's the preferred width of the list item itself
that changes. That will mark up the tree appropriately.

Done. This was copied from the original code which used containingBlock instead
of the ListItem.

https://codereview.chromium.org/1110233003/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.cpp:464: setNeedsSubtreeChangeNotification();
On 2015/04/30 at 02:56:45, ojan wrote:
> Since we set this in the constructor, do we need to call this here? I only see
one caller of this function and that's inside attach, where were should have
just created the LayoutListItem.

Good catch. Done.

https://codereview.chromium.org/1110233003/diff/80001/Source/core/layout/Layo...
File Source/core/layout/LayoutListItem.h (right):

https://codereview.chromium.org/1110233003/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutListItem.h:77: virtual void subtreeDidChange()
override;
On 2015/04/30 at 03:31:04, ojan wrote:
> s/override/final

Done.

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

https://codereview.chromium.org/1110233003/diff/80001/Source/core/layout/Text...
Source/core/layout/TextAutosizer.cpp:479: if (parent->isListItem()) {
On 2015/04/30 at 02:56:45, ojan wrote:
> I think it'd be better to keep this code in LayoutListItem::layout instead of
in the generic TextAutosizer code. It avoids a needless branch for non
list-items that way.
> 
> pdr, mind taking a look over this since you understand the auto-sizing code? I
believe the code inside this if-statement can just be wholesale moved back into
LayoutListItem::layout.


From reading the comment in the old LayoutListItem::layout it sounds like that
was also the wrong place for it:

        // The marker must be autosized before calling
        // updateMarkerLocationAndInvalidateWidth. It cannot be done in the
        // parent's beginLayout because it is not yet in the render tree.

We should now have the marker in the tree before ::layout is kicked off. It
looks like this is called through the beginLayout part of the autosizer?

[ojan, 2015-05-01 01:09:54]

ojan@chromium.org changed reviewers:
+ pdr@chromium.org, skobes@chromium.org

[ojan, 2015-05-01 01:09:55]

Other than the text autosizing thing, this looks great.

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

https://codereview.chromium.org/1110233003/diff/80001/Source/core/layout/Text...
Source/core/layout/TextAutosizer.cpp:479: if (parent->isListItem()) {
On 2015/04/30 17:52:30, dsinclair wrote:
> On 2015/04/30 at 02:56:45, ojan wrote:
> > I think it'd be better to keep this code in LayoutListItem::layout instead
of
> in the generic TextAutosizer code. It avoids a needless branch for non
> list-items that way.
> > 
> > pdr, mind taking a look over this since you understand the auto-sizing code?
I
> believe the code inside this if-statement can just be wholesale moved back
into
> LayoutListItem::layout.
> 
> 
> From reading the comment in the old LayoutListItem::layout it sounds like that
> was also the wrong place for it:
> 
>         // The marker must be autosized before calling
>         // updateMarkerLocationAndInvalidateWidth. It cannot be done in the
>         // parent's beginLayout because it is not yet in the render tree.
> 
> We should now have the marker in the tree before ::layout is kicked off. It
> looks like this is called through the beginLayout part of the autosizer?

Yeah. I don't really understand this comment. pdr or skobes should take a look
to see what the right thing to do here is.

[skobes, 2015-05-01 17:58:16]

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

https://codereview.chromium.org/1110233003/diff/80001/Source/core/layout/Text...
Source/core/layout/TextAutosizer.cpp:479: if (parent->isListItem()) {
On 2015/05/01 01:09:55, ojan wrote:
> On 2015/04/30 17:52:30, dsinclair wrote:
> > From reading the comment in the old LayoutListItem::layout it sounds like
that
> > was also the wrong place for it:
> > 
> >         // The marker must be autosized before calling
> >         // updateMarkerLocationAndInvalidateWidth. It cannot be done in the
> >         // parent's beginLayout because it is not yet in the render tree.
> > 
> > We should now have the marker in the tree before ::layout is kicked off. It
> > looks like this is called through the beginLayout part of the autosizer?
> 
> Yeah. I don't really understand this comment. pdr or skobes should take a look
> to see what the right thing to do here is.

TextAutosizer::inflateListItem was introduced because of the strange way the
list item marker was added to the tree during layout.  We had to inflate the
marker before the call to LayoutListItem::updateMarkerLocation, so we added a
new hook, since by the time we call inflate() on the list item it is too late to
autosize.

The change that added it is https://codereview.chromium.org/145003009/.

If we no longer add the marker during layout, perhaps inflateListItem can go
away.  In any case it doesn't make much sense to call inflateListItem from
wihtin inflate.

You can run the tests in fast/text-autosizing to verify that you are not
regressing the behavior.

[dsinclair, 2015-05-01 21:22:09]

On 2015/05/01 at 17:58:16, skobes wrote:
>
https://codereview.chromium.org/1110233003/diff/80001/Source/core/layout/Text...
> File Source/core/layout/TextAutosizer.cpp (right):
> 
>
https://codereview.chromium.org/1110233003/diff/80001/Source/core/layout/Text...
> Source/core/layout/TextAutosizer.cpp:479: if (parent->isListItem()) {
> On 2015/05/01 01:09:55, ojan wrote:
> > On 2015/04/30 17:52:30, dsinclair wrote:
> > > From reading the comment in the old LayoutListItem::layout it sounds like
that
> > > was also the wrong place for it:
> > > 
> > >         // The marker must be autosized before calling
> > >         // updateMarkerLocationAndInvalidateWidth. It cannot be done in
the
> > >         // parent's beginLayout because it is not yet in the render tree.
> > > 
> > > We should now have the marker in the tree before ::layout is kicked off.
It
> > > looks like this is called through the beginLayout part of the autosizer?
> > 
> > Yeah. I don't really understand this comment. pdr or skobes should take a
look
> > to see what the right thing to do here is.
> 
> TextAutosizer::inflateListItem was introduced because of the strange way the
list item marker was added to the tree during layout.  We had to inflate the
marker before the call to LayoutListItem::updateMarkerLocation, so we added a
new hook, since by the time we call inflate() on the list item it is too late to
autosize.
> 
> The change that added it is https://codereview.chromium.org/145003009/.
> 
> If we no longer add the marker during layout, perhaps inflateListItem can go
away.  In any case it doesn't make much sense to call inflateListItem from
wihtin inflate.
> 
> You can run the tests in fast/text-autosizing to verify that you are not
regressing the behavior.


I played with this a bit this afternoon. It seems like I need the special case.
In the common case, we could patch it into the while loop and treat
listItemMarker the same as text.

The problem comes when the listItemMarker is inside something else. e.g. the
marker ends up inside a form inside the list item (the last test of
fast/text-autosizing/list-marker-with-images-and-forms-autosizing.html). Then,
as far as I can tell, the form becomes the cluster root and we size the list
marker incorrectly. We need the list item as the cluster root in order to get
the right multiplier.

Does that make sense?

[skobes, 2015-05-01 23:07:17]

On 2015/05/01 21:22:09, dsinclair wrote:
> The problem comes when the listItemMarker is inside something else. e.g. the
> marker ends up inside a form inside the list item

That's really strange... shouldn't the marker always be a child of the list
item?

If there is a cluster root inside a list item I wouldn't expect the marker's
multiplier to match the contents of the cluster.

[dsinclair, 2015-05-02 00:18:33]

On 2015/05/01 at 23:07:17, skobes wrote:
> On 2015/05/01 21:22:09, dsinclair wrote:
> > The problem comes when the listItemMarker is inside something else. e.g. the
> > marker ends up inside a form inside the list item
> 
> That's really strange... shouldn't the marker always be a child of the list
item?
> 
> If there is a cluster root inside a list item I wouldn't expect the marker's
multiplier to match the contents of the cluster.


The marker is inserted with the first line box under the list item. So, we have
a tree like:

<li>
  <form>
    <input>
  </form>
</li>

The first line box is around the <input> so the marker ends up as a child of the
<form> and we get a layout tree like (note, not real layout names, heh).

LayoutListItem
  LayoutForm
    LayoutListMarker
    LayoutInput

From briefly looking at the cluster code, it looked like the <form> is a cluster
root so when we get to the ListMarker we get the multiplier from the form, and
not the correct multiplier from the ListItem.

[skobes, 2015-05-02 00:27:35]

On 2015/05/02 00:18:33, dsinclair wrote:
> The marker is inserted with the first line box under the list item. So, we
have
> a tree like:
> 
> <li>
>   <form>
>     <input>
>   </form>
> </li>
> 
> The first line box is around the <input> so the marker ends up as a child of
the
> <form> and we get a layout tree like (note, not real layout names, heh).
> 
> LayoutListItem
>   LayoutForm
>     LayoutListMarker
>     LayoutInput

I see, thanks for the explanation.

> From briefly looking at the cluster code, it looked like the <form> is a
cluster
> root so when we get to the ListMarker we get the multiplier from the form, and
> not the correct multiplier from the ListItem.

Yes we should try to ensure that all the markers in the same list are the same
size.  Can you add a comment to this effect above the call to inflateListItem?

[dsinclair, 2015-05-02 00:50:04]

On 2015/05/02 at 00:27:35, skobes wrote:
> Yes we should try to ensure that all the markers in the same list are the same
size.  Can you add a comment to this effect above the call to inflateListItem?


Done.  (Don't you love that feeling when you upload your changes to the wrong CL
and then leave for the day?)

[skobes, 2015-05-02 00:59:59]

lgtm

[esprehn, 2015-05-06 05:17:53]

https://codereview.chromium.org/1110233003/diff/160001/Source/core/layout/Lay...
File Source/core/layout/LayoutListItem.cpp (left):

https://codereview.chromium.org/1110233003/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutListItem.cpp:285: layoutState->setFlowThread(nullptr);
can we get rid of the setter now?

https://codereview.chromium.org/1110233003/diff/160001/Source/core/layout/Lay...
File Source/core/layout/LayoutListItem.cpp (right):

https://codereview.chromium.org/1110233003/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutListItem.cpp:48:
setConsumesSubtreeChangeNotification();
setConsumesSubtreeChangeNotification should maybe call
registerSubtreeChangeListenerOnDescendants(true) ?

https://codereview.chromium.org/1110233003/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutListItem.cpp:50: notifyOfSubtreeChange();
You can't have parents here, this doesn't make sense.

https://codereview.chromium.org/1110233003/diff/160001/Source/core/layout/Tex...
File Source/core/layout/TextAutosizer.cpp (right):

https://codereview.chromium.org/1110233003/diff/160001/Source/core/layout/Tex...
Source/core/layout/TextAutosizer.cpp:468: //  we try to get the
clusterMultiplier we have the wrong cluster root to work from and get
We don't usually indent lines of comments like this.

https://codereview.chromium.org/1110233003/diff/160001/Source/core/layout/Tex...
Source/core/layout/TextAutosizer.cpp:471: if (item->marker()) {
if (LayoutListMarker* marker = item->marker()) {
  ...
}

[dsinclair, 2015-05-06 13:25:06]

https://codereview.chromium.org/1110233003/diff/160001/Source/core/layout/Lay...
File Source/core/layout/LayoutListItem.cpp (left):

https://codereview.chromium.org/1110233003/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutListItem.cpp:285: layoutState->setFlowThread(nullptr);
On 2015/05/06 at 05:17:52, esprehn wrote:
> can we get rid of the setter now?

Done.

https://codereview.chromium.org/1110233003/diff/160001/Source/core/layout/Lay...
File Source/core/layout/LayoutListItem.cpp (right):

https://codereview.chromium.org/1110233003/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutListItem.cpp:48:
setConsumesSubtreeChangeNotification();
On 2015/05/06 at 05:17:52, esprehn wrote:
> setConsumesSubtreeChangeNotification should maybe call
registerSubtreeChangeListenerOnDescendants(true) ?

I'd prefer to keep them separate to keep it clearer what's happening.

https://codereview.chromium.org/1110233003/diff/160001/Source/core/layout/Lay...
Source/core/layout/LayoutListItem.cpp:50: notifyOfSubtreeChange();
On 2015/05/06 at 05:17:52, esprehn wrote:
> You can't have parents here, this doesn't make sense.

In the case of list item we have to force the first notification in order to
place the marker the first time. In the LayoutObjectChildList code we check if
the child was notified and mark up the ancestor chain.

https://codereview.chromium.org/1110233003/diff/160001/Source/core/layout/Tex...
File Source/core/layout/TextAutosizer.cpp (right):

https://codereview.chromium.org/1110233003/diff/160001/Source/core/layout/Tex...
Source/core/layout/TextAutosizer.cpp:468: //  we try to get the
clusterMultiplier we have the wrong cluster root to work from and get
On 2015/05/06 at 05:17:52, esprehn wrote:
> We don't usually indent lines of comments like this.

Weird, I don't usually indent comments like that either.

https://codereview.chromium.org/1110233003/diff/160001/Source/core/layout/Tex...
Source/core/layout/TextAutosizer.cpp:471: if (item->marker()) {
On 2015/05/06 at 05:17:52, esprehn wrote:
> if (LayoutListMarker* marker = item->marker()) {
>   ...
> }

Done.

[esprehn, 2015-05-06 16:10:28]

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

https://codereview.chromium.org/1110233003/diff/180001/Source/core/layout/Lay...
Source/core/layout/LayoutListItem.cpp:50: notifyOfSubtreeChange();
This isn't right though, it's like calling setNeedsStyleRecalc if you're not
even in the tree. Abs calling this schedules a frame. Creating a layout object
shouldn't have side effects.

[dsinclair, 2015-05-06 17:10:15]

On 2015/05/06 at 16:10:28, esprehn wrote:
>
https://codereview.chromium.org/1110233003/diff/180001/Source/core/layout/Lay...
> File Source/core/layout/LayoutListItem.cpp (right):
> 
>
https://codereview.chromium.org/1110233003/diff/180001/Source/core/layout/Lay...
> Source/core/layout/LayoutListItem.cpp:50: notifyOfSubtreeChange();
> This isn't right though, it's like calling setNeedsStyleRecalc if you're not
even in the tree. Abs calling this schedules a frame. Creating a layout object
shouldn't have side effects.

Removed, this will be handled when things are added to the child lists.

[esprehn, 2015-05-06 21:15:13]

lgtm

[dsinclair, 2015-05-07 00:09:43]

dsinclair@chromium.org changed reviewers:
+ chrishtr@chromium.org, wangxianzhu@chromium.org

[dsinclair, 2015-05-07 00:09:44]

chrishtr, pdr or wangxainzhu the below test is failing on the bots with this
patch due to what looks a missing layer. Running it locally it looks like it
works, do you know if this just a rebaseline or is something wrong?

compositing/contents-opaque/visibility-hidden.html

[Xianzhu, 2015-05-07 01:10:39]

wangxianzhu@chromium.org changed reviewers:
+ alokp@chromium.org

[Xianzhu, 2015-05-07 01:11:10]

On 2015/05/07 00:09:44, dsinclair wrote:
> chrishtr, pdr or wangxainzhu the below test is failing on the bots with this
> patch due to what looks a missing layer. Running it locally it looks like it
> works, do you know if this just a rebaseline or is something wrong?
> 
> compositing/contents-opaque/visibility-hidden.html

The actual result seems correct because it doesn't contain the layer whose main
object has been set visiblity:hidden.

+alokp@ who is the original author of the test.

[dsinclair, 2015-05-07 15:17:52]

On 2015/05/07 at 01:11:10, wangxianzhu wrote:
> On 2015/05/07 00:09:44, dsinclair wrote:
> > chrishtr, pdr or wangxainzhu the below test is failing on the bots with this
> > patch due to what looks a missing layer. Running it locally it looks like it
> > works, do you know if this just a rebaseline or is something wrong?
> > 
> > compositing/contents-opaque/visibility-hidden.html
> 
> The actual result seems correct because it doesn't contain the layer whose
main object has been set visiblity:hidden.
> 
> +alokp@ who is the original author of the test.

That's what it looks like to me as well. Adding in a TestExpectation entry to
rebaseline.

[dsinclair, 2015-05-07 15:17:59]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2015-05-07 15:17:59]

The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org,
esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1110233003/#ps230001
(title: "Add Rebaseline")

[commit-bot: I haz the power, 2015-05-07 15:18:14]

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

[commit-bot: I haz the power, 2015-05-07 16:51:56]

Committed patchset #13 (id:230001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195060

[brucedawson, 2015-05-11 18:02:27]

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

[brucedawson, 2015-05-11 18:02:29]

https://codereview.chromium.org/1110233003/diff/230001/Source/core/layout/Tex...
File Source/core/layout/TextAutosizer.cpp (right):

https://codereview.chromium.org/1110233003/diff/230001/Source/core/layout/Tex...
Source/core/layout/TextAutosizer.cpp:463: float multiplier =
clusterMultiplier(cluster);
This variable shadows the same named parameter to this function. This creates
some reader ambiguity in the code because it is not clear whether multiplier on
line 472 is supposed to refer to this multiplier or the function parameter.

Similarly, it is not clear to a reader whether this line is supposed to affect
the return value of the function.

In other words, the code is not necessarily buggy, but it is also not clearly
correct. Probably worth renaming or fixing to avoid possible confusion.