Separate repaint and layout requirements of StyleDifference (Step 1)

Separate repaint and layout requirements of StyleDifference (Step 1)

Previously StyleDifference was an enum that proximately bigger values
imply smaller values (e.g. StyleDifferenceLayout implies
StyleDifferenceRepaint). This causes unnecessary repaints in some cases
on layout change.

Convert StyleDifference to a structure containing relatively independent
flags.

This change doesn't directly improve the result, but can make further
repaint optimizations possible.

Step 1 doesn't change any functionality. RenderStyle still generate the
legacy StyleDifference enum when comparing styles and convert the result
to the new StyleDifference. Implicit requirements are not handled during
the conversion.

Converted call sites to use the new StyleDifference according to the
following conversion rules:
- diff == StyleDifferenceEqual (&& !context) => diff.hasNoChange()
- diff == StyleDifferenceRepaint => diff.needsRepaintObjectOnly()
- diff == StyleDifferenceRepaintLayer => diff.needsRepaintLayer()
- diff == StyleDifferenceRepaint || diff == StyleDifferenceRepaintLayer => diff.needsRepaintLayer()
- diff >= StyleDifferenceRepaint => diff.needsRepaint() || diff.needsLayout()
- diff >= StyleDifferenceRepaintLayer => diff.needsRepaintLayer() || diff.needsLayout()
- diff > StyleDifferenceRepaintLayer => diff.needsLayout()
- diff == StyleDifferencePositionedMovementLayoutOnly => diff.needsPositionedMovementLayoutOnly()
- diff == StyleDifferenceLayout => diff.needsFullLayout()

BUG=358460
TEST=All existing layout tests.
R=eseidel@chromium.org, esprehn@chromium.org, jchaffraix@chromium.org

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

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

[Xianzhu, 2014-04-16 01:32:36]

PTAL.

Still checking layout test results. There are no over-repaints or
under-repaints. Some repaint test need to be rebaselined because of new
duplicated repaint rects or  original duplicated repaint rects combined. Will
list the reason of the changes when finish checking them.

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

Updated layout tests. Added explanations of layout test changes in CL
description.

[eseidel, 2014-04-17 19:21:51]

If I were doing this, I probably would have done it in smaller pieces.

I would have started by creating a StyleDifference class (like you did) and
deploying it to all consumers (like you did) but I would have left the old enum,
and left RenderStyle::diff() alone (Returning the old enum) and given
StyleDifference a constructor which knew how to map from the old enum to the new
world.

That CL would have no behavior change.

Then in a separate CL I would have taught RenderStyle::diff about
StyleDifference (removing the old enum), still no behavior change.

Finally in a 3rd CL I would have made the behavior change you're making here.

This work is *awesome* and I'm super excited about this CL.  It's somewhat
difficult to review at this size/complexity, but I'll give it a shot. :)

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

On 2014/04/17 19:21:51, eseidel wrote:
> If I were doing this, I probably would have done it in smaller pieces.
> 
> I would have started by creating a StyleDifference class (like you did) and
> deploying it to all consumers (like you did) but I would have left the old
enum,
> and left RenderStyle::diff() alone (Returning the old enum) and given
> StyleDifference a constructor which knew how to map from the old enum to the
new
> world.
> 
> That CL would have no behavior change.
> 
> Then in a separate CL I would have taught RenderStyle::diff about
> StyleDifference (removing the old enum), still no behavior change.
> 
> Finally in a 3rd CL I would have made the behavior change you're making here.
> 
> This work is *awesome* and I'm super excited about this CL.  It's somewhat
> difficult to review at this size/complexity, but I'll give it a shot. :)

Thanks Eric for the suggestions. Will do.

[eseidel, 2014-04-17 20:03:33]

Yeah, so reviewing this again, this looks fantastic.  I think that either we can
get Elliot to review this, or if I'm going to review it I will need to do so in
smaller pieces.

Once again, thank you very much for working on this.  This is clearly a much
better design for this.

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

https://codereview.chromium.org/236203020/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerModelObject.cpp:108: if (diff ==
StyleDifferenceLayout || diff == StyleDifferenceSimplifiedLayout) {
Presumably this is the behavior change, correct?

[ojan, 2014-04-17 20:06:30]

On 2014/04/17 20:03:33, eseidel wrote:
> Yeah, so reviewing this again, this looks fantastic.  I think that either we
can
> get Elliot to review this, or if I'm going to review it I will need to do so
in
> smaller pieces.

+1 to breaking it up into smaller pieces. It doesn't just help the reviewer. It
also helps people who look at the code years from now and are trying to
understand the change.

[Xianzhu, 2014-04-17 20:18:50]

Separated change about zIndex into https://codereview.chromium.org/240253007.

Will convert this change to the first step change that eseidel suggested.

[Xianzhu, 2014-04-18 01:56:08]

PTAL. The latest Patch Set keeps functionality unchanged.

[esprehn, 2014-04-18 03:31:21]

Looking this over...

[esprehn, 2014-04-18 05:38:02]

lgtm, this seems reasonable. Hopefully we can get rid of the Legacy enum
swiftly.

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

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-04-18 16:14:02]

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

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

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

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

Failed to apply patch for Source/core/rendering/RenderLayerModelObject.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/RenderLayerModelObject.cpp
  Hunk #1 FAILED at 98.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/rendering/RenderLayerModelObject.cpp.rej

Patch:       Source/core/rendering/RenderLayerModelObject.cpp
Index: Source/core/rendering/RenderLayerModelObject.cpp
diff --git a/Source/core/rendering/RenderLayerModelObject.cpp
b/Source/core/rendering/RenderLayerModelObject.cpp
index
c413fd2466fd060d5c557839a9a922859af09942..6d7d3e040979c7e5b480c2fabdf9c081a1ed4f0d
100644
--- a/Source/core/rendering/RenderLayerModelObject.cpp
+++ b/Source/core/rendering/RenderLayerModelObject.cpp
@@ -98,14 +98,14 @@ void RenderLayerModelObject::styleWillChange(StyleDifference
diff, const RenderS
     if (oldStyle) {
         // Do a repaint with the old style first through RenderLayerRepainter.
         // RenderObject::styleWillChange takes care of repainting objects
without RenderLayers.
-        if (parent() && diff == StyleDifferenceRepaintLayer) {
+        if (parent() && diff.needsRepaintLayer()) {
            
layer()->repainter().repaintIncludingNonCompositingDescendants(containerForRepaint());
             if (oldStyle->hasClip() != newStyle.hasClip()
                 || oldStyle->clip() != newStyle.clip())
                 layer()->clipper().clearClipRectsIncludingDescendants();
         }
 
-        if (diff == StyleDifferenceLayout || diff ==
StyleDifferenceSimplifiedLayout) {
+        if (diff.needsFullLayout() || diff.needsSimplifiedLayout()) {
             // When a layout hint happens, we go ahead and do a repaint of the
layer, since the layer could
             // end up being destroyed.
             if (hasLayer()) {

[Julien - ping for review, 2014-04-18 16:37:00]

It really looks like we are over-encapsulating on this one: we are bundling
several mostly independent variables into a single class which makes for super
awkward code. This is all the more awkward as we don't move the setting logic to
the new class.

I would rather see a staged migration of each fields (StyleDifference,
contextSensitiveProperties, other booleans) instead of bundling all of them in a
single massive patch.

https://codereview.chromium.org/236203020/diff/60001/Source/core/rendering/st...
File Source/core/rendering/style/StyleDifference.h (right):

https://codereview.chromium.org/236203020/diff/60001/Source/core/rendering/st...
Source/core/rendering/style/StyleDifference.h:20: bool noChange() const
Our coding style mandates a verb in boolean getters so it could be
hasNoChange().

https://codereview.chromium.org/236203020/diff/60001/Source/core/rendering/st...
Source/core/rendering/style/StyleDifference.h:29: // no other flags are set
because it's implied by other flags.
It's weird that the getter doesn't check the other properties then.

https://codereview.chromium.org/236203020/diff/60001/Source/core/rendering/st...
Source/core/rendering/style/StyleDifference.h:103: };
We are going to change the code to not allow both PositionedMovement and
SimplifiedLayout at the same time as part of
https://codereview.chromium.org/241113004/ which should land today or tomorrow.

[Xianzhu, 2014-04-18 17:46:55]

PTAL.

On 2014/04/18 16:37:00, Julien Chaffraix - PST wrote:
> It really looks like we are over-encapsulating on this one: we are bundling
> several mostly independent variables into a single class which makes for super
> awkward code. This is all the more awkward as we don't move the setting logic
to
> the new class.
> 

My intent was to make StyleDifference more like a data structure with little or
no logic, to just bundle the return values of
RenderStyle::visualInvalidationDiff(), and make it possible to set layout and
repaint requirements independently. I think this is like other data structures
in blink such as FontDescriptions, etc.

Would you suggest to move the logic of RenderStyle::visualInvalidationDiff()
into this class? I ever thought of this, but it needs a much bigger change. If
we'd need it, it'd be Step 4 of this series of changes.

> I would rather see a staged migration of each fields (StyleDifference,
> contextSensitiveProperties, other booleans) instead of bundling all of them in
a
> single massive patch.
> 

I thought all changes to the callsites are straight forward so included them in
this non-functional-change CL.

https://codereview.chromium.org/236203020/diff/60001/Source/core/rendering/st...
File Source/core/rendering/style/StyleDifference.h (right):

https://codereview.chromium.org/236203020/diff/60001/Source/core/rendering/st...
Source/core/rendering/style/StyleDifference.h:20: bool noChange() const
On 2014/04/18 16:37:00, Julien Chaffraix - PST wrote:
> Our coding style mandates a verb in boolean getters so it could be
> hasNoChange().

Done.

https://codereview.chromium.org/236203020/diff/60001/Source/core/rendering/st...
Source/core/rendering/style/StyleDifference.h:29: // no other flags are set
because it's implied by other flags.
On 2014/04/18 16:37:00, Julien Chaffraix - PST wrote:
> It's weird that the getter doesn't check the other properties then.

Added other checks.

However for other implications (e.g. needsFullLayout implies needsRepaintSelf),
I'd not include them in Step 1. In following steps, I will remove the layout to
repaint implication.

https://codereview.chromium.org/236203020/diff/60001/Source/core/rendering/st...
Source/core/rendering/style/StyleDifference.h:103: };
On 2014/04/18 16:37:00, Julien Chaffraix - PST wrote:
> We are going to change the code to not allow both PositionedMovement and
> SimplifiedLayout at the same time as part of
> https://codereview.chromium.org/241113004/ which should land today or
tomorrow.

Glad to see the change has been landed :)
Rebasing on it.

[eseidel, 2014-04-18 18:01:12]

lgtm

This is amazing.  Thank you.

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

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-04-18 19:17:30]

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

[Julien - ping for review, 2014-04-18 19:56:59]

The CQ bit was unchecked by jchaffraix@chromium.org

[Julien - ping for review, 2014-04-18 20:35:14]

lgtm.

I won't block this change on that but consider not including the
context-sensitive flags. The flags can be moved when we need them.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
File Source/core/rendering/style/StyleDifference.h (right):

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:8: #include
"core/rendering/style/RenderStyleConstants.h"
Once you've removed the legacy type shouldn't this go away (ie do we want a
FIXME for that?)

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:12: // The difference between two
styles.
Let's have more class comments:

// This class represents the difference between two computed styles
(RenderStyle).
// The difference can be of 3 types:
// - Layout difference
// - Repaint difference
// - Recompositing difference

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:16: : m_needsRecompositeLayer(0)
Let's use false here.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:57: m_contextSensitiveFlags |=
TextOrColorChanged;
Why do we need an extra enum instead of reusing the existing one (including the
value names)?

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:66: && !isContextSensitive();
The context sensitive flags are really not differences per se so including them
looks suspicious.

They are just used to temporarily keep track of a specific change in order to
determine the correct course of action (usually to avoid having a style
difference as it's unneeded). Why do we include it here?

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:71: void
setNeedsRecompositeLayer() { m_needsRecompositeLayer = 1; }
Please use the boolean 'true' here.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:74: void resetNeedsRepaint() {
m_repaintType = NoRepaint; }
See comment below.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:89: void resetNeedsLayout() {
m_layoutType = NoLayout; }
These getters are named needsLayout / clearNeedsLayout on RenderObject so I
think we should be consistent.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:91: // The position of this
positioned object has been updated.
I would change position to "offset" as this is what we usually use for that
(position having a meaning in CSS).

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:101: // A full layout is required.
Not really a helpful comment. A "full layout" is kind of a generic name that
doesn't mean much to the reader.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:113: // what work is needed.
I really think this should be a FIXME: these flags should be internal and not
exposed. What should be important is the final answer as to what was modified,
not these intermediate details.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:120: bool opacityChanged() const {
return m_contextSensitiveFlags & OpacityChanged; }
Again those getters should have a conjugated verb: opacityDidChange() /
transformDidChange() / ...

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:124: void setFilterChanged() {
m_contextSensitiveFlags |= FilterChanged; }
The setters for these properties are unused so we shouldn't add them.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:134: NoRepaint,
We should probably make these start at 0, it would ensure that !m_repaintType
returns what you expect (do I need no repaint?)

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:141: NoLayout,
Same comment.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:154: unsigned
m_contextSensitiveFlags : 4;
I think this is my biggest gripe with the change (and the reason I mentioned
splitting it): I don't think we should add it to the class as it is written as
it's orthogonal and doesn't give us much. I could see a future where we move the
diff function here and it would make sense to move this enum too but for now, it
seems very artificial to me.

[Xianzhu, 2014-04-18 22:02:26]

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
File Source/core/rendering/style/StyleDifference.h (right):

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:8: #include
"core/rendering/style/RenderStyleConstants.h"
On 2014/04/18 20:35:15, Julien Chaffraix - PST wrote:
> Once you've removed the legacy type shouldn't this go away (ie do we want a
> FIXME for that?)

Done.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:12: // The difference between two
styles.
On 2014/04/18 20:35:15, Julien Chaffraix - PST wrote:
> Let's have more class comments:
> 
> // This class represents the difference between two computed styles
> (RenderStyle).
> // The difference can be of 3 types:
> // - Layout difference
> // - Repaint difference
> // - Recompositing difference

Done.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:16: : m_needsRecompositeLayer(0)
On 2014/04/18 20:35:15, Julien Chaffraix - PST wrote:
> Let's use false here.

Done.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:57: m_contextSensitiveFlags |=
TextOrColorChanged;
On 2014/04/18 20:35:15, Julien Chaffraix - PST wrote:
> Why do we need an extra enum instead of reusing the existing one (including
the
> value names)?

This change was reduced from my original whole big change (Patch Set 4) which
also removes ContextSensitiveStyleDifferenceProperty from
StyleDifferenceConstants.h.

Reverted changes about context sensitive properties.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:66: && !isContextSensitive();
On 2014/04/18 20:35:15, Julien Chaffraix - PST wrote:
> The context sensitive flags are really not differences per se so including
them
> looks suspicious.
> 
> They are just used to temporarily keep track of a specific change in order to
> determine the correct course of action (usually to avoid having a style
> difference as it's unneeded). Why do we include it here?

Actually directly treating StyleDifferenceEqual as no-change while there is
context sensitive flag set caused a bug in SimplifyMarkupCommand. Some changed
properties (such as color and text) were ignored. The resolution was to also
check !context. Now still use the original method.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:71: void
setNeedsRecompositeLayer() { m_needsRecompositeLayer = 1; }
On 2014/04/18 20:35:15, Julien Chaffraix - PST wrote:
> Please use the boolean 'true' here.

Done.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:74: void resetNeedsRepaint() {
m_repaintType = NoRepaint; }
On 2014/04/18 20:35:15, Julien Chaffraix - PST wrote:
> See comment below.

Done.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:89: void resetNeedsLayout() {
m_layoutType = NoLayout; }
On 2014/04/18 20:35:15, Julien Chaffraix - PST wrote:
> These getters are named needsLayout / clearNeedsLayout on RenderObject so I
> think we should be consistent.

Done.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:91: // The position of this
positioned object has been updated.
On 2014/04/18 20:35:15, Julien Chaffraix - PST wrote:
> I would change position to "offset" as this is what we usually use for that
> (position having a meaning in CSS).

Done.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:101: // A full layout is required.
On 2014/04/18 20:35:15, Julien Chaffraix - PST wrote:
> Not really a helpful comment. A "full layout" is kind of a generic name that
> doesn't mean much to the reader.

Removed. The function name seems descriptive enough.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:113: // what work is needed.
On 2014/04/18 20:35:15, Julien Chaffraix - PST wrote:
> I really think this should be a FIXME: these flags should be internal and not
> exposed. What should be important is the final answer as to what was modified,
> not these intermediate details.

Reverted changes about context sensitive properties.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:134: NoRepaint,
On 2014/04/18 20:35:15, Julien Chaffraix - PST wrote:
> We should probably make these start at 0, it would ensure that !m_repaintType
> returns what you expect (do I need no repaint?)

Done.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:141: NoLayout,
On 2014/04/18 20:35:15, Julien Chaffraix - PST wrote:
> Same comment.

Done.

https://codereview.chromium.org/236203020/diff/190001/Source/core/rendering/s...
Source/core/rendering/style/StyleDifference.h:154: unsigned
m_contextSensitiveFlags : 4;
On 2014/04/18 20:35:15, Julien Chaffraix - PST wrote:
> I think this is my biggest gripe with the change (and the reason I mentioned
> splitting it): I don't think we should add it to the class as it is written as
> it's orthogonal and doesn't give us much. I could see a future where we move
the
> diff function here and it would make sense to move this enum too but for now,
it
> seems very artificial to me.

Agreed. Reverted the changes about context sensitive properties.

[Xianzhu, 2014-04-18 22:06:22]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-04-18 22:06:37]

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

[Xianzhu, 2014-04-18 23:16:58]

The CQ bit was unchecked by wangxianzhu@chromium.org

[Xianzhu, 2014-04-18 23:17:08]

The CQ bit was checked by wangxianzhu@chromium.org

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

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

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

Committed patchset #7 manually as r171983 (presubmit successful).

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

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

The reason for reverting is: Reason for revert:
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-22 22:44:06]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-04-22 22:44:32]

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

[commit-bot: I haz the power, 2014-04-22 23:19:15]

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

[commit-bot: I haz the power, 2014-04-22 23:19:16]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[Xianzhu, 2014-04-22 23:24:39]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-04-22 23:26:47]

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

[commit-bot: I haz the power, 2014-04-23 01:09:31]

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

[commit-bot: I haz the power, 2014-04-23 01:09:32]

Failed to apply patch for Source/core/rendering/RenderBox.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/RenderBox.cpp
  Hunk #1 FAILED at 143.
  Hunk #2 succeeded at 153 (offset -2 lines).
  Hunk #3 succeeded at 209 (offset -2 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
Source/core/rendering/RenderBox.cpp.rej

Patch:       Source/core/rendering/RenderBox.cpp
Index: Source/core/rendering/RenderBox.cpp
diff --git a/Source/core/rendering/RenderBox.cpp
b/Source/core/rendering/RenderBox.cpp
index
ccc166953e4b7c57a76e787f19dbe9e08f48b8b7..3a046961f55facc5489986e3fd4a5097b47daf9c
100644
--- a/Source/core/rendering/RenderBox.cpp
+++ b/Source/core/rendering/RenderBox.cpp
@@ -143,10 +143,10 @@ void RenderBox::styleWillChange(StyleDifference diff,
const RenderStyle& newStyl
     if (oldStyle) {
         // The background of the root element or the body element could
propagate up to
         // the canvas.  Just dirty the entire canvas when our style changes
substantially.
-        if (diff >= StyleDifferenceRepaint && node() &&
-            (isHTMLHtmlElement(*node()) || isHTMLBodyElement(*node()))) {
+        if ((diff.needsRepaint() || diff.needsLayout()) && node()
+            && (isHTMLHtmlElement(*node()) || isHTMLBodyElement(*node()))) {
 
-            if (!RuntimeEnabledFeatures::repaintAfterLayoutEnabled() || diff !=
StyleDifferenceLayout)
+            if (!RuntimeEnabledFeatures::repaintAfterLayoutEnabled() ||
!diff.needsFullLayout())
                 view()->repaint();
 
             if (oldStyle->hasEntirelyFixedBackground() !=
newStyle.hasEntirelyFixedBackground())
@@ -155,7 +155,7 @@ void RenderBox::styleWillChange(StyleDifference diff, const
RenderStyle& newStyl
 
         // When a layout hint happens and an object's position style changes,
we have to do a layout
         // to dirty the render tree using the old position value now.
-        if (diff == StyleDifferenceLayout && parent() && oldStyle->position()
!= newStyle.position()) {
+        if (diff.needsFullLayout() && parent() && oldStyle->position() !=
newStyle.position()) {
             markContainingBlocksForLayout();
             if (oldStyle->position() == StaticPosition)
                 repaint();
@@ -211,7 +211,7 @@ void RenderBox::styleDidChange(StyleDifference diff, const
RenderStyle* oldStyle
     }
 
     // Our opaqueness might have changed without triggering layout.
-    if (diff == StyleDifferenceRepaint || diff == StyleDifferenceRepaintLayer)
{
+    if (diff.needsRepaint()) {
         RenderObject* parentToInvalidate = parent();
         for (unsigned i = 0; i < backgroundObscurationTestMaxDepth &&
parentToInvalidate; ++i) {
             parentToInvalidate->invalidateBackgroundObscurationStatus();

[Xianzhu, 2014-04-23 01:56:49]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-04-23 01:57:12]

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

[commit-bot: I haz the power, 2014-04-23 02:31:03]

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

[commit-bot: I haz the power, 2014-04-23 02:31:04]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_compile_dbg

[Xianzhu, 2014-04-23 04:25:56]

The CQ bit was checked by wangxianzhu@chromium.org

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

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

[commit-bot: I haz the power, 2014-04-23 07:16:02]

Change committed as 172331