[css-grid] Only force full invalidation when grid item data change

[css-grid] Only force full invalidation when grid item data change

We're currently comparing the rareData's m_gridItem pointer to decide
whether a style change should force a full paint invalidation or not.
This is causing that changes in CSS Alignment properties on a grid item
are identified as needing a full paint invalidation, since the mentioned
pointer has been changed, indeed. However, that change is not caused by
the style change, but due to reapplying the same CSS rules.

Even more, the tests ensuring the are no invalidations when there is
not a visual geometry change are failing because of that.

This change modifies the condition to inspect the grid item's style
data, in addition to the pointer itself, so we only trigger the full
invalidation if there is an actual change in the data.

BUG=620235
Committed: https://crrev.com/be05bcdf2b257500dc1b24209583b323cc2b7461
Cr-Commit-Position: refs/heads/master@{#413700}

[jfernandez, 2016-08-19 18:23:49]

Description was changed from

==========
[css-grid] force full invalidation only when grid item data change

We're currently comparing the rareData's m_gridItem pointer to decide
whether a style change should force a full paint invalidation or not.
This is causing that changes in CSS Alignment properties on a grid item
are identified as needing a full paint invalidation, since the mentioned
pointer has been changed, indeed. However, that change is not caused by
the style change, but due to reapplying the same CSS rules.

Even more, the tests ensuring the are no invalidations when there is
not a visual geometry change are failing because of that.

This change modifies the condition to inspect the grid item's style
data, in addition to the pointer itself, so we only trigger the full
invalidation if there is an actual change in the data.

BUG=620235
==========

to

==========
[css-grid] Only force full invalidation when grid item data change

We're currently comparing the rareData's m_gridItem pointer to decide
whether a style change should force a full paint invalidation or not.
This is causing that changes in CSS Alignment properties on a grid item
are identified as needing a full paint invalidation, since the mentioned
pointer has been changed, indeed. However, that change is not caused by
the style change, but due to reapplying the same CSS rules.

Even more, the tests ensuring the are no invalidations when there is
not a visual geometry change are failing because of that.

This change modifies the condition to inspect the grid item's style
data, in addition to the pointer itself, so we only trigger the full
invalidation if there is an actual change in the data.

BUG=620235
==========

[jfernandez, 2016-08-19 18:23:50]

jfernandez@igalia.com changed reviewers:
+ cbiesinger@chromium.org, rego@igalia.com, schenney@chromium.org,
svillar@igalia.com

[cbiesinger, 2016-08-19 18:55:14]

https://codereview.chromium.org/2261733002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2261733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:569: const
StyleGridItemData* gridItemDataB = m_rareNonInheritedData->m_gridItem.get();
This needs an "other"...

https://codereview.chromium.org/2261733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:570: if (!(gridItemDataA
== gridItemDataB) || (gridItemDataA && gridItemDataA && *gridItemDataA ==
*gridItemDataB))
gridItemDataA && gridItemDataA <-- needs to be B

Also, I'm confused by this entire condition. Is it even possible for one of
these to be null? And if so, you should return true if one is null but not the
other, right? I also think you misplaced the first closing parenthesis.

[jfernandez, 2016-08-19 20:41:01]

Patchset #3 (id:40001) has been deleted

[jfernandez, 2016-08-19 20:41:22]

https://codereview.chromium.org/2261733002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/2261733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:569: const
StyleGridItemData* gridItemDataB = m_rareNonInheritedData->m_gridItem.get();
On 2016/08/19 18:55:13, cbiesinger wrote:
> This needs an "other"...

Done.

https://codereview.chromium.org/2261733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:570: if (!(gridItemDataA
== gridItemDataB) || (gridItemDataA && gridItemDataA && *gridItemDataA ==
*gridItemDataB))
On 2016/08/19 18:55:13, cbiesinger wrote:
> gridItemDataA && gridItemDataA <-- needs to be B
> 

Right.

> Also, I'm confused by this entire condition. Is it even possible for one of
> these to be null? 

I think it's kind of defensive logic. Do you think we should remove it ? I'm
following the same pattern than m_counterDirectives some lines below
. 
> And if so, you should return true if one is null but not the
> other, right? I also think you misplaced the first closing parenthesis.

You right, I made a mistake with the parenthesis.

[cbiesinger, 2016-08-19 20:43:30]

Most of the code doesn't nullcheck -- timloh/bugsnash, can you advise whether
this is needed? (see previous comments in this codereview)

[jfernandez, 2016-08-19 20:53:04]

On 2016/08/19 20:43:30, cbiesinger wrote:
> Most of the code doesn't nullcheck -- timloh/bugsnash, can you advise whether
> this is needed? (see previous comments in this codereview)

The new code is dereferencing the pointer we get from the m_gridItem RefPtr. The
idea is to compare the data, not only the pointers like we were doing before. In

most cases the RareData fields are just data structures, or in the case of
RefPtr
we are just comparing the pointers. 

Knowing how the m_gridItem RareNonInherited's attribute is created, I
think it's not possible that it may be null. However, it may be non-obvious when
reading the ComputeStyle logic, so I decided to apply a defensive approach, like
in the case of m_counterDirectives.

Anyway, additional feedback is really in the matter welcome, so thanks for
point the issue out.

[cbiesinger, 2016-08-19 20:56:30]

On 2016/08/19 20:53:04, jfernandez wrote:
> On 2016/08/19 20:43:30, cbiesinger wrote:
> > Most of the code doesn't nullcheck -- timloh/bugsnash, can you advise
whether
> > this is needed? (see previous comments in this codereview)
> 
> The new code is dereferencing the pointer we get from the m_gridItem RefPtr.
The
> idea is to compare the data, not only the pointers like we were doing before.
In
> 
> most cases the RareData fields are just data structures, or in the case of
> RefPtr
> we are just comparing the pointers. 
> 
> Knowing how the m_gridItem RareNonInherited's attribute is created, I
> think it's not possible that it may be null. However, it may be non-obvious
when
> reading the ComputeStyle logic, so I decided to apply a defensive approach,
like
> in the case of m_counterDirectives.
> 
> Anyway, additional feedback is really in the matter welcome, so thanks for
> point the issue out.

Well below your code there's these:
            && *m_rareNonInheritedData->m_flexibleBox.get() != *other.m_rareNonI
      nheritedData->m_flexibleBox.get())

            && *m_rareNonInheritedData->m_multiCol.get() != *other.m_rareNonInhe
      ritedData->m_multiCol.get())

They both deref without nullchecking. I don't think it makes sense to nullcheck
something that can't be null -- that just makes it unclear what the actual
invariants of the code are.

[jfernandez, 2016-08-19 20:59:05]

On 2016/08/19 20:56:30, cbiesinger wrote:
> On 2016/08/19 20:53:04, jfernandez wrote:
> > On 2016/08/19 20:43:30, cbiesinger wrote:
> > > Most of the code doesn't nullcheck -- timloh/bugsnash, can you advise
> whether
> > > this is needed? (see previous comments in this codereview)
> > 
> > The new code is dereferencing the pointer we get from the m_gridItem RefPtr.
> The
> > idea is to compare the data, not only the pointers like we were doing
before.
> In
> > 
> > most cases the RareData fields are just data structures, or in the case of
> > RefPtr
> > we are just comparing the pointers. 
> > 
> > Knowing how the m_gridItem RareNonInherited's attribute is created, I
> > think it's not possible that it may be null. However, it may be non-obvious
> when
> > reading the ComputeStyle logic, so I decided to apply a defensive approach,
> like
> > in the case of m_counterDirectives.
> > 
> > Anyway, additional feedback is really in the matter welcome, so thanks for
> > point the issue out.
> 
> Well below your code there's these:
>             && *m_rareNonInheritedData->m_flexibleBox.get() !=
*other.m_rareNonI
>       nheritedData->m_flexibleBox.get())
> 
>             && *m_rareNonInheritedData->m_multiCol.get() !=
*other.m_rareNonInhe
>       ritedData->m_multiCol.get())
> 
> They both deref without nullchecking. I don't think it makes sense to
nullcheck
> something that can't be null -- that just makes it unclear what the actual
> invariants of the code are.

Umm, you're right, indeed. I think I'll send a new patch without the nullcheck
bits.

[cbiesinger, 2016-08-19 21:33:02]

lgtm

[jfernandez, 2016-08-20 01:14:09]

@cbiesigner, could you help me to figure out what's going on with the tests
failing on the win2 bot ? They were marked as flaky already, but I thought that
after fixing the actual bug they will pass. 

The error, however, looks like more a problem with the repaint testing functions
than the test itself. It seems that it got confused by the new approach of using
both, text based repainting and pixel test.

[Bugs Nash, 2016-08-22 04:16:59]

On 2016/08/19 at 20:43:30, cbiesinger wrote:
> Most of the code doesn't nullcheck -- timloh/bugsnash, can you advise whether
this is needed? (see previous comments in this codereview)

Seems this has been resolved, but I think you're right, it doesn't seem possible
for ComputedStyle's m_grid or m_gridItem to be null. They are only ever set in
constructors and never set to null.

[cbiesinger, 2016-08-22 15:09:15]

On 2016/08/20 01:14:09, jfernandez wrote:
> @cbiesigner, could you help me to figure out what's going on with the tests
> failing on the win2 bot ? They were marked as flaky already, but I thought
that
> after fixing the actual bug they will pass. 
> 
> The error, however, looks like more a problem with the repaint testing
functions
> than the test itself. It seems that it got confused by the new approach of
using
> both, text based repainting and pixel test.

Hm, the expected result seems wrong -- the expectation has a layout tree dump,
not the repaint rect output. Not sure why that happened. But the actual output
also doesn't look quite right -- maybe I'm not sure how to read this output but
it looks like the entire document is getting repainted?!

I also noticed that your repaint test has a HTML file for the expected output,
which also seems weird; I am not sure why you are not using only the text dump.

[jfernandez, 2016-08-22 15:23:04]

On 2016/08/22 15:09:15, cbiesinger wrote:
> On 2016/08/20 01:14:09, jfernandez wrote:
> > @cbiesigner, could you help me to figure out what's going on with the tests
> > failing on the win2 bot ? They were marked as flaky already, but I thought
> that
> > after fixing the actual bug they will pass. 
> > 
> > The error, however, looks like more a problem with the repaint testing
> functions
> > than the test itself. It seems that it got confused by the new approach of
> using
> > both, text based repainting and pixel test.
> 
> Hm, the expected result seems wrong -- the expectation has a layout tree dump,
> not the repaint rect output. Not sure why that happened. But the actual output
> also doesn't look quite right -- maybe I'm not sure how to read this output
but
> it looks like the entire document is getting repainted?!
> 

This issue is very related to what you pointed out bellow.

> I also noticed that your repaint test has a HTML file for the expected output,
> which also seems weird; I am not sure why you are not using only the text
dump.

It seems there was a change in how the repainting testing infrastructure works
now,
so it checks both the repainting area and the html references after executing
the script
that changed he CSS properties (aka.  runRepaintAndPixelTest)

Perhaps this testing scripts don't work as expected on win7 bots.

[cbiesinger, 2016-08-22 15:28:02]

But you could use runRepaintTest instead, then you don't need a reference. In
general, the repaint pixel tests would have an image as reference file, as far
as I understand it.

cc'ing Xianzhu for any further input -- please also see previous comments.

[Xianzhu, 2016-08-22 16:22:04]

On 2016/08/22 15:28:02, cbiesinger wrote:
> But you could use runRepaintTest instead, then you don't need a reference. In
> general, the repaint pixel tests would have an image as reference file, as far
> as I understand it.
> 
> cc'ing Xianzhu for any further input -- please also see previous comments.

About both text expectation and reference/pixel:
When we noticed that the text-only output of repaint test is not enough to
discover all bugs (especially painting bugs), we decided to change repaint tests
to generate both text and pixel results using runRepaintAndPixelTest(). For the
pixel result, we can use either -expected.png or -expected.html (preferred) as
the expected output.

[jfernandez, 2016-08-22 16:23:55]

On 2016/08/22 16:22:04, Xianzhu wrote:
> On 2016/08/22 15:28:02, cbiesinger wrote:
> > But you could use runRepaintTest instead, then you don't need a reference.
In
> > general, the repaint pixel tests would have an image as reference file, as
far
> > as I understand it.
> > 
> > cc'ing Xianzhu for any further input -- please also see previous comments.
> 
> About both text expectation and reference/pixel:
> When we noticed that the text-only output of repaint test is not enough to
> discover all bugs (especially painting bugs), we decided to change repaint
tests
> to generate both text and pixel results using runRepaintAndPixelTest(). For
the
> pixel result, we can use either -expected.png or -expected.html (preferred) as
> the expected output.

Any idea why this is failing only in the win7 bots ?

[cbiesinger, 2016-08-22 16:27:38]

On 2016/08/22 16:23:55, jfernandez wrote:
> Any idea why this is failing only in the win7 bots ?

Oh I figured it out.

https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/platform/...

^ Win7 has a platform-specific test expectation file. Maybe that should be
deleted :)

[jfernandez, 2016-08-22 20:44:31]

On 2016/08/22 16:27:38, cbiesinger wrote:
> On 2016/08/22 16:23:55, jfernandez wrote:
> > Any idea why this is failing only in the win7 bots ?
> 
> Oh I figured it out.
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/platform/...
> 
> ^ Win7 has a platform-specific test expectation file. Maybe that should be
> deleted :)

oh !!! They were probably generated because those tests were requested to
be rebaselined for win7.

[jfernandez, 2016-08-22 22:29:02]

The CQ bit was checked by jfernandez@igalia.com

[jfernandez, 2016-08-22 22:29:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbiesinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2261733002/#ps160001
(title: "Patch rebased.")

[commit-bot: I haz the power, 2016-08-22 22:29:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 02:05:50]

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

[commit-bot: I haz the power, 2016-08-23 02:05:52]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jfernandez, 2016-08-23 07:44:25]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2016-08-23 07:44:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 09:24:22]

Description was changed from

==========
[css-grid] Only force full invalidation when grid item data change

We're currently comparing the rareData's m_gridItem pointer to decide
whether a style change should force a full paint invalidation or not.
This is causing that changes in CSS Alignment properties on a grid item
are identified as needing a full paint invalidation, since the mentioned
pointer has been changed, indeed. However, that change is not caused by
the style change, but due to reapplying the same CSS rules.

Even more, the tests ensuring the are no invalidations when there is
not a visual geometry change are failing because of that.

This change modifies the condition to inspect the grid item's style
data, in addition to the pointer itself, so we only trigger the full
invalidation if there is an actual change in the data.

BUG=620235
==========

to

==========
[css-grid] Only force full invalidation when grid item data change

We're currently comparing the rareData's m_gridItem pointer to decide
whether a style change should force a full paint invalidation or not.
This is causing that changes in CSS Alignment properties on a grid item
are identified as needing a full paint invalidation, since the mentioned
pointer has been changed, indeed. However, that change is not caused by
the style change, but due to reapplying the same CSS rules.

Even more, the tests ensuring the are no invalidations when there is
not a visual geometry change are failing because of that.

This change modifies the condition to inspect the grid item's style
data, in addition to the pointer itself, so we only trigger the full
invalidation if there is an actual change in the data.

BUG=620235
==========

[commit-bot: I haz the power, 2016-08-23 09:24:23]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-08-23 09:26:17]

Description was changed from

==========
[css-grid] Only force full invalidation when grid item data change

We're currently comparing the rareData's m_gridItem pointer to decide
whether a style change should force a full paint invalidation or not.
This is causing that changes in CSS Alignment properties on a grid item
are identified as needing a full paint invalidation, since the mentioned
pointer has been changed, indeed. However, that change is not caused by
the style change, but due to reapplying the same CSS rules.

Even more, the tests ensuring the are no invalidations when there is
not a visual geometry change are failing because of that.

This change modifies the condition to inspect the grid item's style
data, in addition to the pointer itself, so we only trigger the full
invalidation if there is an actual change in the data.

BUG=620235
==========

to

==========
[css-grid] Only force full invalidation when grid item data change

We're currently comparing the rareData's m_gridItem pointer to decide
whether a style change should force a full paint invalidation or not.
This is causing that changes in CSS Alignment properties on a grid item
are identified as needing a full paint invalidation, since the mentioned
pointer has been changed, indeed. However, that change is not caused by
the style change, but due to reapplying the same CSS rules.

Even more, the tests ensuring the are no invalidations when there is
not a visual geometry change are failing because of that.

This change modifies the condition to inspect the grid item's style
data, in addition to the pointer itself, so we only trigger the full
invalidation if there is an actual change in the data.

BUG=620235

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

[commit-bot: I haz the power, 2016-08-23 09:26:19]

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