Don't always repaint on zIndex change

Don't always repaint on zIndex change

If zIndex of a composited layer changes and there is no other conditions
triggering repaint, we should not repaint the layer.

Now treat zIndex change like opacity change about repainting.

BUG=364636, 365865
TEST=reuse existing compositing/repaint/should-not-repaint-composited-descendants.html

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

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

[Xianzhu, 2014-04-17 22:38:56]

I'm not sure if this is correct way to add new types of context sensitive css
changes.

Another idea is not to let RenderStyle::visualInvalidationDiff() generate the
context sensitive property bits, but just set a contextSensitive flag after
comparing all assured style differences.

After https://codereview.chromium.org/240253007/,
RenderStyle::visualInvailidationDiff() might treat context sensitive changes
like the following:
  StyleDifference diff;
  if (this == &other || *this == other)
     return diff;
  
  // Assured style differences
  ...

  diff.setContextSensitive();
  return diff;

Then in RenderObject:
  if (diff.isContextSensitive()) {
    if (newStyle.opacity() != oldStyle.opacity())
       ...
    if (newStyle.zIndex() != oldStyle.zIndex())
       ...
    ...
  }

wdyt?

[Xianzhu, 2014-04-17 23:27:09]

On 2014/04/17 22:38:56, Xianzhu wrote:
> I'm not sure if this is correct way to add new types of context sensitive css
> changes.
> 
> Another idea is not to let RenderStyle::visualInvalidationDiff() generate the
> context sensitive property bits, but just set a contextSensitive flag after
> comparing all assured style differences.
> 
> After https://codereview.chromium.org/240253007/,
> RenderStyle::visualInvailidationDiff() might treat context sensitive changes
> like the following:
>   StyleDifference diff;
>   if (this == &other || *this == other)
>      return diff;
>   
>   // Assured style differences
>   ...
> 
>   diff.setContextSensitive();
>   return diff;
> 
> Then in RenderObject:
>   if (diff.isContextSensitive()) {
>     if (newStyle.opacity() != oldStyle.opacity())
>        ...
>     if (newStyle.zIndex() != oldStyle.zIndex())
>        ...
>     ...
>   }
> 
> wdyt?

trchen@ suggested to still compare the context sensitive properties in
RenderStyle::visualInvalidationDiff() because the above method may cause twice
comparison of the styles.

I'd like to delay this CL after https://codereview.chromium.org/240253007/.

[ojan, 2014-04-18 01:46:22]

lgtm

https://codereview.chromium.org/240253007/diff/20001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/240253007/diff/20001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.cpp:603: if (position() !=
StaticPosition) {
This isn't the fault of your patch, but now that I look at this code, it isn't
quite right. There are other conditions where zIndex applies than just the
position. See the following code in StyleAdjuster::adjustRenderStyle:
    if (style->position() == StaticPosition &&
!parentStyleForcesZIndexToCreateStackingContext(parentStyle))
        style->setHasAutoZIndex();

I don't think fixing this is very high priority, but mind putting a FIXME in
this code while you're here?

[ojan, 2014-04-18 01:57:31]

On 2014/04/17 22:38:56, Xianzhu wrote:
> I'm not sure if this is correct way to add new types of context sensitive css
> changes.
> 
> Another idea is not to let RenderStyle::visualInvalidationDiff() generate the
> context sensitive property bits, but just set a contextSensitive flag after
> comparing all assured style differences.
> 
> After https://codereview.chromium.org/240253007/,
> RenderStyle::visualInvailidationDiff() might treat context sensitive changes
> like the following:
>   StyleDifference diff;
>   if (this == &other || *this == other)
>      return diff;
>   
>   // Assured style differences
>   ...
> 
>   diff.setContextSensitive();
>   return diff;
> 
> Then in RenderObject:
>   if (diff.isContextSensitive()) {
>     if (newStyle.opacity() != oldStyle.opacity())
>        ...
>     if (newStyle.zIndex() != oldStyle.zIndex())
>        ...
>     ...
>   }
> 
> wdyt?

Sorry, I don't really understand what the proposal is.

[Xianzhu, 2014-04-18 16:05:40]

https://codereview.chromium.org/240253007/diff/20001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/240253007/diff/20001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.cpp:603: if (position() !=
StaticPosition) {
On 2014/04/18 01:46:23, ojan wrote:
> This isn't the fault of your patch, but now that I look at this code, it isn't
> quite right. There are other conditions where zIndex applies than just the
> position. See the following code in StyleAdjuster::adjustRenderStyle:
>     if (style->position() == StaticPosition &&
> !parentStyleForcesZIndexToCreateStackingContext(parentStyle))
>         style->setHasAutoZIndex();
> 
> I don't think fixing this is very high priority, but mind putting a FIXME in
> this code while you're here?

My understanding about the code is that it forces autoZIndex when non-auto
zIndex is not applicable. Auto noticed that in other cases zIndex will be forced
to 0 in StyleAdjuster.cpp. So StyleAdjuster seems to already handle the
applicable conditions of ZIndex. I'm wondering if it'd be correct to just remove
the condition at line 603 for zIndex.

[Xianzhu, 2014-04-18 16:13:25]

On 2014/04/18 01:57:31, ojan wrote:
> On 2014/04/17 22:38:56, Xianzhu wrote:
> > I'm not sure if this is correct way to add new types of context sensitive
css
> > changes.
> > 
> > Another idea is not to let RenderStyle::visualInvalidationDiff() generate
the
> > context sensitive property bits, but just set a contextSensitive flag after
> > comparing all assured style differences.
> > 
> > After https://codereview.chromium.org/240253007/,
> > RenderStyle::visualInvailidationDiff() might treat context sensitive changes
> > like the following:
> >   StyleDifference diff;
> >   if (this == &other || *this == other)
> >      return diff;
> >   
> >   // Assured style differences
> >   ...
> > 
> >   diff.setContextSensitive();
> >   return diff;
> > 
> > Then in RenderObject:
> >   if (diff.isContextSensitive()) {
> >     if (newStyle.opacity() != oldStyle.opacity())
> >        ...
> >     if (newStyle.zIndex() != oldStyle.zIndex())
> >        ...
> >     ...
> >   }
> > 
> > wdyt?
> 
> Sorry, I don't really understand what the proposal is.

The proposal is based on my concern (not sure) about potential too many context
sensitive properties in the future when we discover more css changes that
require different actions based on the context. The proposal intents to move
comparison of context sensitive properties from
RenderStyle::visualInvalidationDiff() to RenderObject::adjustStyleDifference(),
so that for future context sensitive properties we don't have to add new
ContextSensitiveStyleDifferenceProperty bits.

However, now I think this would not be very beneficial and not worth doing it
for now.

[ojan, 2014-04-18 17:20:03]

lgtm

https://codereview.chromium.org/240253007/diff/20001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/240253007/diff/20001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.cpp:603: if (position() !=
StaticPosition) {
On 2014/04/18 16:05:41, Xianzhu wrote:
> On 2014/04/18 01:46:23, ojan wrote:
> > This isn't the fault of your patch, but now that I look at this code, it
isn't
> > quite right. There are other conditions where zIndex applies than just the
> > position. See the following code in StyleAdjuster::adjustRenderStyle:
> >     if (style->position() == StaticPosition &&
> > !parentStyleForcesZIndexToCreateStackingContext(parentStyle))
> >         style->setHasAutoZIndex();
> > 
> > I don't think fixing this is very high priority, but mind putting a FIXME in
> > this code while you're here?
> 
> My understanding about the code is that it forces autoZIndex when non-auto
> zIndex is not applicable. Auto noticed that in other cases zIndex will be
forced
> to 0 in StyleAdjuster.cpp. So StyleAdjuster seems to already handle the
> applicable conditions of ZIndex. I'm wondering if it'd be correct to just
remove
> the condition at line 603 for zIndex.

I think you're right. In either case, that should be a separate patch with a
test-case to go with it. So, a FIXME for now?

[Xianzhu, 2014-04-18 17:28:19]

On 2014/04/18 17:20:03, ojan wrote:
> lgtm
> 
>
https://codereview.chromium.org/240253007/diff/20001/Source/core/rendering/st...
> File Source/core/rendering/style/RenderStyle.cpp (right):
> 
>
https://codereview.chromium.org/240253007/diff/20001/Source/core/rendering/st...
> Source/core/rendering/style/RenderStyle.cpp:603: if (position() !=
> StaticPosition) {
> On 2014/04/18 16:05:41, Xianzhu wrote:
> > On 2014/04/18 01:46:23, ojan wrote:
> > > This isn't the fault of your patch, but now that I look at this code, it
> isn't
> > > quite right. There are other conditions where zIndex applies than just the
> > > position. See the following code in StyleAdjuster::adjustRenderStyle:
> > >     if (style->position() == StaticPosition &&
> > > !parentStyleForcesZIndexToCreateStackingContext(parentStyle))
> > >         style->setHasAutoZIndex();
> > > 
> > > I don't think fixing this is very high priority, but mind putting a FIXME
in
> > > this code while you're here?
> > 
> > My understanding about the code is that it forces autoZIndex when non-auto
> > zIndex is not applicable. Auto noticed that in other cases zIndex will be
> forced
> > to 0 in StyleAdjuster.cpp. So StyleAdjuster seems to already handle the
> > applicable conditions of ZIndex. I'm wondering if it'd be correct to just
> remove
> > the condition at line 603 for zIndex.
> 
> I think you're right. In either case, that should be a separate patch with a
> test-case to go with it. So, a FIXME for now?

Sure will do. As this change will conflict with
https://codereview.chromium.org/236203020/, I will rebase this CL on that CL
after that CL is landed.

[Xianzhu, 2014-04-18 23:18:37]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-04-18 23:18:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/240253007/60001

[commit-bot: I haz the power, 2014-04-19 00:27:34]

Change committed as 171989

[aboxhall, 2014-04-22 20:14:08]

A revert of this CL has been created in
https://codereview.chromium.org/247293006/ by aboxhall@chromium.org.

The reason for reverting is: Depends on
https://codereview.chromium.org/203463007/ which broke ChromeOS login window:
https://code.google.com/p/chromium/issues/detail?id=365507.

[Xianzhu, 2014-04-23 15:54:29]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-04-23 15:54:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/240253007/100001

[commit-bot: I haz the power, 2014-04-23 15:54:59]

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

[commit-bot: I haz the power, 2014-04-23 15:54:59]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 1114.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
f08385e6afa9d53acc93b219c34ac4e2bb4ec55b..3a496e3ae49f96808fd049d080435498aaed5834
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1114,6 +1114,3 @@ crbug.com/308992
svg/as-object/sizing/svg-in-object-placeholder-height-percentag
 
 # Expectations changes due to switiching to In-renderer Shared Worker.
 crbug.com/327256 fast/workers/shared-worker-lifecycle.html [ Failure Pass ]
-
-crbug.com/365865
virtual/softwarecompositing/repaint/should-not-repaint-composited-descendants.html
[ Failure ]
-crbug.com/365865
compositing/repaint/should-not-repaint-composited-descendants.html [ Failure ]

[Xianzhu, 2014-04-23 16:01:35]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-04-23 16:01:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/240253007/100002

[commit-bot: I haz the power, 2014-04-23 17:14:47]

Change committed as 172393