Rename isTreatedAsOrStackingContext to isStacked

Rename isTreatedAsOrStackingContext to isStacked and move out of ComputedStyle

Previous isTreatedAsStackingContext is confusing because besides
positioned elements we also treats some other elements as "pseudo
stacking contexts" (see ObjectPainter::paintPseudoStackingContext()
==> renamed to ObjectPainter::paintAllPhasesAtomically()).

The actual difference between elements "isTreatedAsStackingContext" and
other pseudo stacking contexts is that the former are treated as
"z-index:0" so are stacked in the containing stacking context.

Then isTreatedAsOrStackingContext() is to check if the element should
be stacked, so rename it to isStacked().

Added a chapter in Source/core/paint/README.md to explain how we understand
and use the concepts.

Also avoid "pseudo stacking context". Rename
ObjectPainter::paintPseudoStackingContext() to
ObjectPainter::paintAllPhasesAtomically().

Committed: https://crrev.com/c717c257efe380a6498363f60a350d5835c545bb
Cr-Commit-Position: refs/heads/master@{#381657}

[Xianzhu, 2016-03-14 22:10:15]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org, pdr@chromium.org, trchen@chromium.org

[trchen, 2016-03-15 00:12:14]

I'm not sure if the new term is less confusing than what we already have...
Maybe we should discuss with people before proceeding.

To summarize, we have 3 sets of things:
A. Anything that paints position:static && z-index:auto (i.e. normal-flow)
descendants without interleaving with things outside of the subtree.
B. Things that are positioned, but with z-index:auto. 
C. Things that have non-auto z-index.

Both B and C are subset of A.
An example of A that is not B or C is inline-block.
Only C can host child stacking context.
Both B and C paints as child stacking context of nearest ancestor that is a C,
and their painting order are determined by z-index.

For A that is not B or C will be painted by
ObjectPainter::paintAsPseudoStackingContext
Both B and C will be self-painting layer and painted by PaintLayerPainter.

Everybody agrees that C should be called stacking context.
Oftentimes A is referred as pseudo-stacking context, but the term is not defined
by the spec.
B doesn't really have a name, and we called the union of B and C
"isTreatedAsOrStackingContext" and you proposed to rename it to
"isStackedContent". I'm thinking perhaps "participatesZIndexSorting" or
something alike would be clearer?

[Xianzhu, 2016-03-15 01:11:02]

On 2016/03/15 00:12:14, trchen wrote:
> I'm not sure if the new term is less confusing than what we already have...
> Maybe we should discuss with people before proceeding.
> 

Yes, definitely we need discussion and I uploaded this CL for it :)
Your following analysis is really helpful for discussion.

> To summarize, we have 3 sets of things:
> A. Anything that paints position:static && z-index:auto (i.e. normal-flow)
> descendants without interleaving with things outside of the subtree.
> B. Things that are positioned, but with z-index:auto. 
> C. Things that have non-auto z-index.
> 
> Both B and C are subset of A.
> An example of A that is not B or C is inline-block.
> Only C can host child stacking context.
> Both B and C paints as child stacking context of nearest ancestor that is a C,
> and their painting order are determined by z-index.
> 
> For A that is not B or C will be painted by
> ObjectPainter::paintAsPseudoStackingContext
> Both B and C will be self-painting layer and painted by PaintLayerPainter.
> 
> Everybody agrees that C should be called stacking context.
> Oftentimes A is referred as pseudo-stacking context, but the term is not
defined
> by the spec.

In the new comments in the code I define elements "treated as if it created a
new stacking context, but ..." as pseudo stacking context. Can we use this
definition?

> B doesn't really have a name, and we called the union of B and C
> "isTreatedAsOrStackingContext" and you proposed to rename it to
> "isStackedContent". I'm thinking perhaps "participatesZIndexSorting" or
> something alike would be clearer?

I think both isStackedContent or participatesZIndexSorting are better than
isTreatedAsOrStackingContext(). I have no preference.

Chris/pdr what are your opinions?

[Xianzhu, 2016-03-15 17:49:53]

chrishtr/pdr what are your preferences of the naming:
isTreatedAsOrStackingContext, isStackedContent or participatesZIndexOrdering?

[Xianzhu, 2016-03-15 17:53:35]

I still prefer isStackedContent because it's shorter and its meaning is clear. I
will use it in the context like paintInvalidationContainerForStackedContent,
which could be 
paintInvalidationContainerForDescendantsTreatedOrIsStackingContext or
paintInvalidationContainerForDescendantsParticipatingZIndexOrdering.

[pdr., 2016-03-15 20:12:07]

This doesn't really seem to belong on ComputedStyle since it's just a paint
concept. What if we removed the isTreatedAsOrStackingContext from ComputedStyle
and moved it to a static on ObjectPainter?

All of the proposed names sound like layout concepts. What about
paintsAsStackingContext?

[Xianzhu, 2016-03-15 20:38:48]

On 2016/03/15 20:12:07, pdr wrote:
> This doesn't really seem to belong on ComputedStyle since it's just a paint
> concept. What if we removed the isTreatedAsOrStackingContext from
ComputedStyle
> and moved it to a static on ObjectPainter?
>

Perhaps PaintLayerStackingNode is a better place for it because it affects
stacking?

> All of the proposed names sound like layout concepts. What about
> paintsAsStackingContext?

There are differences between paintsAsStackingContext and isStackedContent. The
former is A in trchen@'s document, the latter is B+C. A>B+C.
We paint B+C as SelfPaintingLayers, and paint A-(B+C) with
ObjectPainter::paintAsPseudoStackingContext().

[pdr., 2016-03-15 21:37:59]

On 2016/03/15 at 20:38:48, wangxianzhu wrote:
> On 2016/03/15 20:12:07, pdr wrote:
> > This doesn't really seem to belong on ComputedStyle since it's just a paint
> > concept. What if we removed the isTreatedAsOrStackingContext from
ComputedStyle
> > and moved it to a static on ObjectPainter?
> >
> 
> Perhaps PaintLayerStackingNode is a better place for it because it affects
stacking?

+1

> 
> > All of the proposed names sound like layout concepts. What about
> > paintsAsStackingContext?
> 
> There are differences between paintsAsStackingContext and isStackedContent.
The former is A in trchen@'s document, the latter is B+C. A>B+C.
> We paint B+C as SelfPaintingLayers, and paint A-(B+C) with
ObjectPainter::paintAsPseudoStackingContext().

Thanks for restating this. I didn't fully understand this before. I definitely
agree with you that we should rename this area so it is more understandable.

"pseudo-" means "fake" so it's a little weird for set A to be called "psuedo
stacking contexts" when it includes C which are real stacking contexts. Is it
correct that "A-(B+C)" are painted as pseudo stacking contexts while "(B+C)" are
real stacking contexts? If so, could A be "paintsAsStackingContext()", which
returns true for "paintsAsRealStackingContext() ||
paintsAsPseudoStackingContext()"?

I agree with trchen that it would be best to avoid new terms, even though I like
"isStackedContent" since it describes what is actually happening.

[chrishtr, 2016-03-15 21:46:37]

On 2016/03/15 at 21:37:59, pdr wrote:
> On 2016/03/15 at 20:38:48, wangxianzhu wrote:
> > On 2016/03/15 20:12:07, pdr wrote:
> > > This doesn't really seem to belong on ComputedStyle since it's just a
paint
> > > concept. What if we removed the isTreatedAsOrStackingContext from
ComputedStyle
> > > and moved it to a static on ObjectPainter?
> > >
> > 
> > Perhaps PaintLayerStackingNode is a better place for it because it affects
stacking?
> 
> +1
> 
> > 
> > > All of the proposed names sound like layout concepts. What about
> > > paintsAsStackingContext?
> > 
> > There are differences between paintsAsStackingContext and isStackedContent.
The former is A in trchen@'s document, the latter is B+C. A>B+C.
> > We paint B+C as SelfPaintingLayers, and paint A-(B+C) with
ObjectPainter::paintAsPseudoStackingContext().
> 
> Thanks for restating this. I didn't fully understand this before. I definitely
agree with you that we should rename this area so it is more understandable.
> 
> "pseudo-" means "fake" so it's a little weird for set A to be called "psuedo
stacking contexts" when it includes C which are real stacking contexts. Is it
correct that "A-(B+C)" are painted as pseudo stacking contexts while "(B+C)" are
real stacking contexts? If so, could A be "paintsAsStackingContext()", which
returns true for "paintsAsRealStackingContext() ||
paintsAsPseudoStackingContext()"?
> 
> I agree with trchen that it would be best to avoid new terms, even though I
like "isStackedContent" since it describes what is actually happening.

Please please please document these findings in Markdown also.

[Xianzhu, 2016-03-15 21:53:13]

On 2016/03/15 21:37:59, pdr wrote:
> On 2016/03/15 at 20:38:48, wangxianzhu wrote:
> > On 2016/03/15 20:12:07, pdr wrote:
> > > This doesn't really seem to belong on ComputedStyle since it's just a
paint
> > > concept. What if we removed the isTreatedAsOrStackingContext from
> ComputedStyle
> > > and moved it to a static on ObjectPainter?
> > >
> > 
> > Perhaps PaintLayerStackingNode is a better place for it because it affects
> stacking?
> 
> +1
> 
> > 
> > > All of the proposed names sound like layout concepts. What about
> > > paintsAsStackingContext?
> > 
> > There are differences between paintsAsStackingContext and isStackedContent.
> The former is A in trchen@'s document, the latter is B+C. A>B+C.
> > We paint B+C as SelfPaintingLayers, and paint A-(B+C) with
> ObjectPainter::paintAsPseudoStackingContext().
> 
> Thanks for restating this. I didn't fully understand this before. I definitely
> agree with you that we should rename this area so it is more understandable.
> 
> "pseudo-" means "fake" so it's a little weird for set A to be called "psuedo
> stacking contexts" when it includes C which are real stacking contexts. Is it
> correct that "A-(B+C)" are painted as pseudo stacking contexts while "(B+C)"
are
> real stacking contexts?

A are not all "pseudo stacking contexts" (if we define "pseudo stacking
contexts" as the elements "as if it created a new stacking context, but..."
described in the spec).

A-C are "pseudo stacking contexts", including B and A-(B+C) (which we could
refer as D) which are different about whether they are stacked.

Only C are real stacking contexts.

(It may look a bit weird that ObjectPainter::paintAsPseudoStackingContext()
doesn't handle B though B are also pseudo stacking contexts. However, this is
not because the function doesn't accept B. In theory, we can let
ObjectPainter::paintAsPseudoStackingContext() paint B if PaintLayerPainter
doesn't handle them.)

> If so, could A be "paintsAsStackingContext()", which
> returns true for "paintsAsRealStackingContext() ||
> paintsAsPseudoStackingContext()"?

This is true, but it seems we don't need paintsAsStackingContext(), because A
are never handled in the same way. B+C and A-(B+C) are always handled in
distinct code paths.

> I agree with trchen that it would be best to avoid new terms, even though I
like
> "isStackedContent" since it describes what is actually happening.

[pdr., 2016-03-15 22:40:54]

Ok, let me try to summarize. We have 3 classes of "stacking-context-ish"
painting situations:
B. Positioned, but with z-index:auto. CSS2.1 E.2 step 7.2.1 says "treat the
element as if it created a new stacking context". These have a paint layer.
C. Non-auto z-index. CSS2.1 E.2 step 3 and step 9 cover this. These are real
stacking contexts and create paint layers.
D. Inline-block, flexbox cells, inline-table etc. CSS2.1 E.2 step 8 says "treat
the element as if it created a new stacking context". These do not have a paint
layer and paint through paintAsPseudoStackingContext.

In reading the spec, I think D should include "Non-positioned floating
descendants" as described in CSS2.1 E.2 step 5 which also paint through
paintAsPseudoStackingContext. This is the FIXME in isTreatedAsStackingContext
and we should make sure to keep or fix that fixme.

There's also a part of CSS2.1 E.2 step 8 which says "for those with 'z-index:
0', treat the stacking context generated atomically." I think these should also
be in D?

[Xianzhu, 2016-03-15 23:36:52]

On 2016/03/15 22:40:54, pdr wrote:
> Ok, let me try to summarize. We have 3 classes of "stacking-context-ish"
> painting situations:
> B. Positioned, but with z-index:auto. CSS2.1 E.2 step 7.2.1 says "treat the
> element as if it created a new stacking context". These have a paint layer.
> C. Non-auto z-index. CSS2.1 E.2 step 3 and step 9 cover this. These are real
> stacking contexts and create paint layers.
> D. Inline-block, flexbox cells, inline-table etc. CSS2.1 E.2 step 8 says
"treat
> the element as if it created a new stacking context". These do not have a
paint
> layer and paint through paintAsPseudoStackingContext.
> 
> In reading the spec, I think D should include "Non-positioned floating
> descendants" as described in CSS2.1 E.2 step 5 which also paint through
> paintAsPseudoStackingContext. This is the FIXME in isTreatedAsStackingContext
> and we should make sure to keep or fix that fixme.

Thanks for the summary. I'm writing the markdown and will include above into it.

I removed the FIXME about floating objects because I think it's not a bug, but
just an alternative implementation which we haven't investigated its feasibility
yet.

> There's also a part of CSS2.1 E.2 step 8 which says "for those with 'z-index:
> 0', treat the stacking context generated atomically." I think these should
also
> be in D?

The statement makes positioned objects with z-index:0 no difference from actual
stacking contexts, so we can just treat them as real stacking contexts.

[Xianzhu, 2016-03-15 23:52:45]

Rethought about the difference between positioned elements with 'z-index:auto'
and other pseudo stacking contexts. Previously I thought it was about whether
they are managed by stacking contexts, but now I realized that the difference is
only because of our current implementation. The special case of
paintInvalidationContainerForStackedContents is also caused by our current
implementation.

I think alternatively, we could treat positioned elements 'z-index:auto' in the
same way as floating objects (which is on the contrary of the FIXME about
floating object suggests). We would need PaintPhaseAutoZIndexPositioned paint
phase and paint it between PaintPhaseForeground and positive z-index
descendants.

Pros:
- Simplified PaintLayerStackNode which no longer need to handle
isTreatedAsStackingContext() specially;
- Less PaintLayers;
- No need to consider stacking contexts about paintInvalidationContainer for
positioned elements with 'z-index:auto';
- Simpler concepts (no more B);
- Reduced number of PaintLayerStackingNodes (which can be created for real
stacking contexts only).

Cons:
- Another tree walk for the paint phase (but we can use empty phase
optimization)
- Additional development work for it

[trchen, 2016-03-16 00:05:09]

On 2016/03/15 23:52:45, Xianzhu wrote:
> Rethought about the difference between positioned elements with 'z-index:auto'
> and other pseudo stacking contexts. Previously I thought it was about whether
> they are managed by stacking contexts, but now I realized that the difference
is
> only because of our current implementation. The special case of
> paintInvalidationContainerForStackedContents is also caused by our current
> implementation.
> 
> I think alternatively, we could treat positioned elements 'z-index:auto' in
the
> same way as floating objects (which is on the contrary of the FIXME about
> floating object suggests). We would need PaintPhaseAutoZIndexPositioned paint
> phase and paint it between PaintPhaseForeground and positive z-index
> descendants.

z-index:auto and z-index:0 are considered equal for stacking context sorting.
(They interleave by DOM order.) You just can't paint every z-index:auto before
painting positive flow (which includes z-index:0). They can be even composited
for overlapping!

I think PaintLayerStackingNode will stay in the new world. We do want something
to cache the sorted child stacking context list instead of sorting everytime we
paint. However I agree with you that whether it is painted using
PaintLayerPainter::paint() or ObjectPainter::paintAsPseudoStackingContext()
shouldn't make a difference.

> Pros:
> - Simplified PaintLayerStackNode which no longer need to handle
> isTreatedAsStackingContext() specially;
> - Less PaintLayers;
> - No need to consider stacking contexts about paintInvalidationContainer for
> positioned elements with 'z-index:auto';
> - Simpler concepts (no more B);
> - Reduced number of PaintLayerStackingNodes (which can be created for real
> stacking contexts only).
> 
> Cons:
> - Another tree walk for the paint phase (but we can use empty phase
> optimization)
> - Additional development work for it

[Xianzhu, 2016-03-16 00:12:07]

On 2016/03/16 00:05:09, trchen wrote:
> On 2016/03/15 23:52:45, Xianzhu wrote:
> > Rethought about the difference between positioned elements with
'z-index:auto'
> > and other pseudo stacking contexts. Previously I thought it was about
whether
> > they are managed by stacking contexts, but now I realized that the
difference
> is
> > only because of our current implementation. The special case of
> > paintInvalidationContainerForStackedContents is also caused by our current
> > implementation.
> > 
> > I think alternatively, we could treat positioned elements 'z-index:auto' in
> the
> > same way as floating objects (which is on the contrary of the FIXME about
> > floating object suggests). We would need PaintPhaseAutoZIndexPositioned
paint
> > phase and paint it between PaintPhaseForeground and positive z-index
> > descendants.
> 
> z-index:auto and z-index:0 are considered equal for stacking context sorting.
> (They interleave by DOM order.) You just can't paint every z-index:auto before
> painting positive flow (which includes z-index:0). They can be even composited
> for overlapping!
> 

Thanks for explanation! Just remembered you explained this to me before but I
just forgot :(
Now I see this is the real difference between B and D. Question resolved :)

> I think PaintLayerStackingNode will stay in the new world. We do want
something
> to cache the sorted child stacking context list instead of sorting everytime
we
> paint. However I agree with you that whether it is painted using
> PaintLayerPainter::paint() or ObjectPainter::paintAsPseudoStackingContext()
> shouldn't make a difference.
> 
> > Pros:
> > - Simplified PaintLayerStackNode which no longer need to handle
> > isTreatedAsStackingContext() specially;
> > - Less PaintLayers;
> > - No need to consider stacking contexts about paintInvalidationContainer for
> > positioned elements with 'z-index:auto';
> > - Simpler concepts (no more B);
> > - Reduced number of PaintLayerStackingNodes (which can be created for real
> > stacking contexts only).
> > 
> > Cons:
> > - Another tree walk for the paint phase (but we can use empty phase
> > optimization)
> > - Additional development work for it

[pdr., 2016-03-16 00:21:16]

I learned a lot in this review too.

What are you thoughts about the direction of this patch now? Would you like to
continue with a rename under the "stackedContent" name? If so, would you keep
around the "pseudoStackingContext" term or replace it too (maybe in another
patch)?

[pdr., 2016-03-16 00:23:02]

On 2016/03/15 at 23:36:52, wangxianzhu wrote:
> I removed the FIXME about floating objects because I think it's not a bug, but
just an alternative implementation which we haven't investigated its feasibility
yet.

I think we actually do this already in BlockFlowPainter::paintFloats? Or is that
something else?

[Xianzhu, 2016-03-16 00:37:41]

On 2016/03/16 00:21:16, pdr wrote:
> I learned a lot in this review too.
> 
> What are you thoughts about the direction of this patch now? Would you like to
> continue with a rename under the "stackedContent" name? If so, would you keep
> around the "pseudoStackingContext" term or replace it too (maybe in another
> patch)?

I would like to use a even shorter name: isStacked().

How about 'ObjectPainter::paintAllPhasesAtomically()' to replace
ObjectPainter::paintAsPseudoStackingContext()?
Then we would no longer have the confusing "pseudoStackingContext" term.

On 2016/03/16 00:23:02, pdr wrote:
> On 2016/03/15 at 23:36:52, wangxianzhu wrote:
> > I removed the FIXME about floating objects because I think it's not a bug,
but
> just an alternative implementation which we haven't investigated its
feasibility
> yet.
> 
> I think we actually do this already in BlockFlowPainter::paintFloats? Or is
that
> something else?

Yes we call ObjectPainter::paintAsPseudoStackingContext(), but don't treat it as
stacking context like positioned objects. I think the FIXME no longer applies
after we make the concepts clear.

[Xianzhu, 2016-03-16 00:59:38]

Description was changed from

==========
Rename isTreatedAsOrStackingContext to isStackedContent

Previous isTreatedAsStackingContext is confusing because besides
positioned elements we also treats some other elements as "pseudo
stacking contexts" (see ObjectPainter::paintPseudoStackingContext()).

The actual difference between elements "isTreatedAsStackingContext" and
other pseudo stacking contexts is that the former are treated as
"z-index:0" so are stacked in the containing stacking context.

Then isTreatedAsOrStackingContext() is to check if the element should
be stacked, so rename it to isStackedContent().
==========

to

==========
Rename isTreatedAsOrStackingContext to isStacked and move out of ComputedStyle

Previous isTreatedAsStackingContext is confusing because besides
positioned elements we also treats some other elements as "pseudo
stacking contexts" (see ObjectPainter::paintPseudoStackingContext()
==> renamed to ObjectPainter::paintAllPhasesAtomically()).

The actual difference between elements "isTreatedAsStackingContext" and
other pseudo stacking contexts is that the former are treated as
"z-index:0" so are stacked in the containing stacking context.

Then isTreatedAsOrStackingContext() is to check if the element should
be stacked, so rename it to isStacked().

Added a chapter in Source/core/paint/README.md to explain how we understand
and use the concepts.

Also avoid "pseudo stacking context". Rename
ObjectPainter::paintPseudoStackingContext() to
ObjectPainter::paintAllPhasesAtomically().
==========

[Xianzhu, 2016-03-16 01:01:19]

Ptal:
- added a markdown chapter;
- use even shorter name: isStacked;
- avoid "pseudo stacking context" term. Renamed
ObjectPainter::paintAsPseudoStackingContext() to
ObjectPainter::paintAllPhasesAtomically().

[pdr., 2016-03-16 02:22:53]

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:299: // a stacked
content, creating this layer may cause this object and its
Nit: "... and this object is stacked content, ..."

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BlockPainter.h (right):

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BlockPainter.h:33: void
paintChildAllPhasesAtomically(const LayoutBox&, const PaintInfo&, const
LayoutPoint&);
Nit: paintAllChildPhasesAtomically sounds a little better.

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/ObjectPainter.h (right):

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ObjectPainter.h:43: // and
PaintPhaseTextClip) or nothing (other paint phases) according to
paintInfo.phase.
Optional: maybe include a link to the markdown here:
https://chromium.googlesource.com/chromium/src.git/+/master/third_party/WebKi...

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerStackingNode.h (right):

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerStackingNode.h:131: // but not
managed in stacks. See ObjectPainter::paintAllPhasesAtomically().)
Optional, may want to add a link to the markdown too:
https://chromium.googlesource.com/chromium/src.git/+/master/third_party/WebKi...

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/README.md (right):

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/README.md:37: They are painted as pseudo
stacking contexts by `ObjectPainter::paintAsPseudoStackingContext()`.
Nit: update to be paintAllPhasesAtomically

[pdr., 2016-03-16 06:40:23]

LGTM

There's a lot of good stuff in this patch: paintAllPhasesAtomically is a great
improvement, as are the updated docs and moving layer code out of ComputedStyle.

isStacked... do not like, but I cannot think of a better name that is short.
PaintLayerStackingNode itself is pretty broken (all paint layers have one!?) and
isStacked matches the class name, so "stacked" isn't as out of place as it seems
at first. Please give chrishtr a chance to review this before CQing.

[chrishtr, 2016-03-16 16:26:16]

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerStackingNode.cpp (right):

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerStackingNode.cpp:64: m_isStacked
= shouldStackObject(*layoutObject());
Why is this code caching shouldStackObject(*layoutObject())? Can't it be easily
recomputed?

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/README.md (right):

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/README.md:19: They are positioned elements
with `z-index: auto` (E.2.8 in the documentation).
Add a paragraph below that explains what "treated as stacking contexts" means.
A reasonable summary may be:

Execute all of the steps of the painting algorithm explained in
https://www.w3.org/TR/CSS21/zindex.html for this element, except ignore any
descendants which are positioned or have non-auto z-index.

[Xianzhu, 2016-03-16 18:10:24]

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:299: // a stacked
content, creating this layer may cause this object and its
On 2016/03/16 02:22:53, pdr wrote:
> Nit: "... and this object is stacked content, ..."

Done.

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BlockPainter.h (right):

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BlockPainter.h:33: void
paintChildAllPhasesAtomically(const LayoutBox&, const PaintInfo&, const
LayoutPoint&);
On 2016/03/16 02:22:53, pdr wrote:
> Nit: paintAllChildPhasesAtomically sounds a little better.

Done.

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/ObjectPainter.h (right):

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ObjectPainter.h:43: // and
PaintPhaseTextClip) or nothing (other paint phases) according to
paintInfo.phase.
On 2016/03/16 02:22:53, pdr wrote:
> Optional: maybe include a link to the markdown here:
>
https://chromium.googlesource.com/chromium/src.git/+/master/third_party/WebKi...

Done.

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerStackingNode.cpp (right):

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerStackingNode.cpp:64: m_isStacked
= shouldStackObject(*layoutObject());
On 2016/03/16 16:26:16, chrishtr wrote:
> Why is this code caching shouldStackObject(*layoutObject())? Can't it be
easily
> recomputed?

According to the comment for
PaintLayerStackingNode::isTreatedAsOrStackingContext() (now isStacked), we need
to save the current value and in future updateIsStacked() we need to do
something when it changes.

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerStackingNode.h (right):

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerStackingNode.h:131: // but not
managed in stacks. See ObjectPainter::paintAllPhasesAtomically().)
On 2016/03/16 02:22:53, pdr wrote:
> Optional, may want to add a link to the markdown too:
>
https://chromium.googlesource.com/chromium/src.git/+/master/third_party/WebKi...

Done.

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/README.md (right):

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/README.md:19: They are positioned elements
with `z-index: auto` (E.2.8 in the documentation).
On 2016/03/16 16:26:16, chrishtr wrote:
> Add a paragraph below that explains what "treated as stacking contexts" means.
> A reasonable summary may be:
> 
> Execute all of the steps of the painting algorithm explained in
> https://www.w3.org/TR/CSS21/zindex.html for this element, except ignore any
> descendants which are positioned or have non-auto z-index.

Done.

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/README.md:37: They are painted as pseudo
stacking contexts by `ObjectPainter::paintAsPseudoStackingContext()`.
On 2016/03/16 02:22:53, pdr wrote:
> Nit: update to be paintAllPhasesAtomically

Done.

[chrishtr, 2016-03-16 18:15:09]

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerStackingNode.cpp (right):

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerStackingNode.cpp:64: m_isStacked
= shouldStackObject(*layoutObject());
On 2016/03/16 at 18:10:24, Xianzhu wrote:
> On 2016/03/16 16:26:16, chrishtr wrote:
> > Why is this code caching shouldStackObject(*layoutObject())? Can't it be
easily
> > recomputed?
> 
> According to the comment for
PaintLayerStackingNode::isTreatedAsOrStackingContext() (now isStacked), we need
to save the current value and in future updateIsStacked() we need to do
something when it changes.

Ok then I suggest renaming updateIsStacked to styleChanged(), and renaming
isStacked() to shouldStackObject (or shouldStackObjectInParentStackingContext,
but that seems long). shouldStackObject should take no arguments.

[Xianzhu, 2016-03-16 21:28:41]

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerStackingNode.cpp (right):

https://codereview.chromium.org/1798263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerStackingNode.cpp:64: m_isStacked
= shouldStackObject(*layoutObject());
On 2016/03/16 18:15:09, chrishtr wrote:
> On 2016/03/16 at 18:10:24, Xianzhu wrote:
> > On 2016/03/16 16:26:16, chrishtr wrote:
> > > Why is this code caching shouldStackObject(*layoutObject())? Can't it be
> easily
> > > recomputed?
> > 
> > According to the comment for
> PaintLayerStackingNode::isTreatedAsOrStackingContext() (now isStacked), we
need
> to save the current value and in future updateIsStacked() we need to do
> something when it changes.
> 
> Ok then I suggest renaming updateIsStacked to styleChanged(), and renaming
> isStacked() to shouldStackObject (or shouldStackObjectInParentStackingContext,
> but that seems long). shouldStackObject should take no arguments.

The static shouldStackObject was also for another call from
LayoutBoxModelObject::createLayer() which previously called
ComputedStyle::isTreatedAsOrStackingContext().

The new patch set adds back ComputedStyle::isStacked() so that
LayoutBoxModelObject::createLayer() can call it. I think though isStacked() is a
painting concept, it's convenient to let ComputedStyle provide such a method.

The new patch set also combines
PaintLayerStackingNode::updateStackingNodesAfterStyleChange() and
PaintLayerStackingNode::updateIsStacked() into
PaintLayerStackingNode::styleDidChange().

Should we choose isStacked() instead of shouldStackObject()? I think the
"stacked" is clearly defined in the comments and markdown.

[chrishtr, 2016-03-16 21:40:44]

lgtm

[Xianzhu, 2016-03-16 21:55:35]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-03-16 21:55:36]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1798263002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2016-03-16 21:55:58]

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

[commit-bot: I haz the power, 2016-03-16 22:45:24]

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

[commit-bot: I haz the power, 2016-03-16 22:45:26]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Xianzhu, 2016-03-17 00:54:48]

Removing PaintLayerStackingNode::m_isStacked breaks the logic when a layer is
removed during style change. In PaintLayer::removeChild(), the current stacked
status (instead of stacked status of the new style which is causing the layer to
be removed) is checked before dirtying the parent stacking context's z-order
list. We should add the flag back.

[Xianzhu, 2016-03-17 02:27:33]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-03-17 02:27:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1798263002/#ps140001
(title: "Fix windows compile")

[commit-bot: I haz the power, 2016-03-17 02:27:47]

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

[commit-bot: I haz the power, 2016-03-17 03:48:08]

Description was changed from

==========
Rename isTreatedAsOrStackingContext to isStacked and move out of ComputedStyle

Previous isTreatedAsStackingContext is confusing because besides
positioned elements we also treats some other elements as "pseudo
stacking contexts" (see ObjectPainter::paintPseudoStackingContext()
==> renamed to ObjectPainter::paintAllPhasesAtomically()).

The actual difference between elements "isTreatedAsStackingContext" and
other pseudo stacking contexts is that the former are treated as
"z-index:0" so are stacked in the containing stacking context.

Then isTreatedAsOrStackingContext() is to check if the element should
be stacked, so rename it to isStacked().

Added a chapter in Source/core/paint/README.md to explain how we understand
and use the concepts.

Also avoid "pseudo stacking context". Rename
ObjectPainter::paintPseudoStackingContext() to
ObjectPainter::paintAllPhasesAtomically().
==========

to

==========
Rename isTreatedAsOrStackingContext to isStacked and move out of ComputedStyle

Previous isTreatedAsStackingContext is confusing because besides
positioned elements we also treats some other elements as "pseudo
stacking contexts" (see ObjectPainter::paintPseudoStackingContext()
==> renamed to ObjectPainter::paintAllPhasesAtomically()).

The actual difference between elements "isTreatedAsStackingContext" and
other pseudo stacking contexts is that the former are treated as
"z-index:0" so are stacked in the containing stacking context.

Then isTreatedAsOrStackingContext() is to check if the element should
be stacked, so rename it to isStacked().

Added a chapter in Source/core/paint/README.md to explain how we understand
and use the concepts.

Also avoid "pseudo stacking context". Rename
ObjectPainter::paintPseudoStackingContext() to
ObjectPainter::paintAllPhasesAtomically().
==========

[commit-bot: I haz the power, 2016-03-17 03:48:10]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-03-17 03:49:59]

Description was changed from

==========
Rename isTreatedAsOrStackingContext to isStacked and move out of ComputedStyle

Previous isTreatedAsStackingContext is confusing because besides
positioned elements we also treats some other elements as "pseudo
stacking contexts" (see ObjectPainter::paintPseudoStackingContext()
==> renamed to ObjectPainter::paintAllPhasesAtomically()).

The actual difference between elements "isTreatedAsStackingContext" and
other pseudo stacking contexts is that the former are treated as
"z-index:0" so are stacked in the containing stacking context.

Then isTreatedAsOrStackingContext() is to check if the element should
be stacked, so rename it to isStacked().

Added a chapter in Source/core/paint/README.md to explain how we understand
and use the concepts.

Also avoid "pseudo stacking context". Rename
ObjectPainter::paintPseudoStackingContext() to
ObjectPainter::paintAllPhasesAtomically().
==========

to

==========
Rename isTreatedAsOrStackingContext to isStacked and move out of ComputedStyle

Previous isTreatedAsStackingContext is confusing because besides
positioned elements we also treats some other elements as "pseudo
stacking contexts" (see ObjectPainter::paintPseudoStackingContext()
==> renamed to ObjectPainter::paintAllPhasesAtomically()).

The actual difference between elements "isTreatedAsStackingContext" and
other pseudo stacking contexts is that the former are treated as
"z-index:0" so are stacked in the containing stacking context.

Then isTreatedAsOrStackingContext() is to check if the element should
be stacked, so rename it to isStacked().

Added a chapter in Source/core/paint/README.md to explain how we understand
and use the concepts.

Also avoid "pseudo stacking context". Rename
ObjectPainter::paintPseudoStackingContext() to
ObjectPainter::paintAllPhasesAtomically().

Committed: https://crrev.com/c717c257efe380a6498363f60a350d5835c545bb
Cr-Commit-Position: refs/heads/master@{#381657}
==========

[commit-bot: I haz the power, 2016-03-17 03:50:04]

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