Adding or changing any of box-shadow, outline, or border-image-outset does not need a layout..

. so just require a paint invalidation and ensure visual overflow gets recomputed.

Another go at https://codereview.chromium.org/561303002 which was reverted by
https://codereview.chromium.org/747493002.

This includes wangxianzhu's fix for tables in https://codereview.chromium.org/744493002.

The intent of this patch is to avoid layout for all types of LayoutBlock when
the outline, border-image-outset or box-shadow are updated.

BUG=396825, 434700, 437012
Committed: https://crrev.com/de7ae34361ba945b6d1ec7b3c679fecf5d94d35b
Cr-Commit-Position: refs/heads/master@{#387862}

[rhogan, 2016-03-17 12:26:50]

Description was changed from

==========
bug 396825

BUG=396825
==========

to

==========
. so just require a paint invalidation and ensure visual overflow gets
recomputed.

Another go at https://codereview.chromium.org/561303002 which was reverted by
https://codereview.chromium.org/747493002. 

This includes wangxianzhu's fix for tables in
https://codereview.chromium.org/744493002.


BUG=396825,434700,437012
==========

[rhogan, 2016-03-21 19:30:46]

Description was changed from

==========
. so just require a paint invalidation and ensure visual overflow gets
recomputed.

Another go at https://codereview.chromium.org/561303002 which was reverted by
https://codereview.chromium.org/747493002. 

This includes wangxianzhu's fix for tables in
https://codereview.chromium.org/744493002.


BUG=396825,434700,437012
==========

to

==========
. so just require a paint invalidation and ensure visual overflow gets
recomputed.

Another go at https://codereview.chromium.org/561303002 which was reverted by
https://codereview.chromium.org/747493002. 

This includes wangxianzhu's fix for tables in
https://codereview.chromium.org/744493002.

The intent of this patch is to avoid layout for all types of LayoutBlock when
the outline, border-image-outset or box-shadow are updated.

BUG=396825,434700,437012
==========

[rhogan, 2016-03-22 13:51:21]

robhogan@gmail.com changed reviewers:
+ levi.gross@axialmarket.com, wangxianzhu@chromium.org

[Xianzhu, 2016-03-22 17:02:05]

Is it possible to split this CL into the following two CLs?

1. Recompute overflow when box-shadow, outline or border-image-outset changes
(for LayoutBlock);

2. Make recomputeOverflow work for table cells and rows.

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

https://codereview.chromium.org/1809643008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2017:
setNeedsLayoutAndPrefWidthsRecalc(LayoutInvalidationReason::StyleChange);
Nit: please add a TODO here and reference bug 437012.

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

https://codereview.chromium.org/1809643008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:251: // FIXME: This
seems incorrect because cell may have visual effect overflow that should be
included in this row.
Nit: FIXME style has been deprecated by TODO.

https://www.chromium.org/blink/coding-style#TOC-Comments

https://codereview.chromium.org/1809643008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/BorderData.h (right):

https://codereview.chromium.org/1809643008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/BorderData.h:122: 
Nit: Extra blank line.

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

https://codereview.chromium.org/1809643008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:728: return true;
We can avoid this check by adjusting visualOverflowChanged to
needsPaintInvalidationObject in adjustStyleDifference in LayoutObject.cpp.

https://codereview.chromium.org/1809643008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:747: ||
!rareNonInheritedData->shadowDataEquivalent(*other.rareNonInheritedData.get())
Ditto.

[leviw_travelin_and_unemployed, 2016-03-22 18:57:45]

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

[leviw_travelin_and_unemployed, 2016-03-22 18:57:46]

I think you meant me in the reviewers list.

[leviw_travelin_and_unemployed, 2016-03-22 18:58:33]

You want to copy the subject line into the first line of the description and
keep it relatively short to make git happy.

[Xianzhu, 2016-03-23 20:28:26]

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

https://codereview.chromium.org/1809643008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:728: return true;
On 2016/03/22 17:02:05, Xianzhu wrote:
> We can avoid this check by adjusting visualOverflowChanged to
> needsPaintInvalidationObject in adjustStyleDifference in LayoutObject.cpp.

Please ignore this comment if you take the
StyleDifference::needsRecomputeOverflow() way.

https://codereview.chromium.org/1809643008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:782:
diff.setVisualOverflowChanged();
Move this to diffNeedsRecomputeOverflow() if you take the
StyleDifference::needsRecomputeOverflow() way.

https://codereview.chromium.org/1809643008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:789:
diff.setVisualOverflowChanged();
Ditto.

https://codereview.chromium.org/1809643008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/StyleDifference.h (right):

https://codereview.chromium.org/1809643008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleDifference.h:89: void
setVisualOverflowChanged() { m_propertySpecificDifferences |=
VisualOverflowChanged; }
I suggest to move this flag out of PropertyDifference and make it a normal
needsXXX flag (e.g. needsRecomputeOverflow).

The PropertyDifference bits are for cases that the required action is not clear
before knowing more contexts of the object.

[rhogan, 2016-04-12 19:29:00]

On 2016/03/22 at 17:02:05, wangxianzhu wrote:
> Is it possible to split this CL into the following two CLs?
> 
> 1. Recompute overflow when box-shadow, outline or border-image-outset changes
(for LayoutBlock);
> 
> 2. Make recomputeOverflow work for table cells and rows.
> 

I think that would create regressions in (1) that would only be solved by (2). I
expect regressions even with both elements landed so would like to land this as
a monolithic CL and deal with the regressions then; or revert, regroup and
reland if they're severe enough.

[Xianzhu, 2016-04-12 19:39:39]

On 2016/04/12 19:29:00, rhogan wrote:
> On 2016/03/22 at 17:02:05, wangxianzhu wrote:
> > Is it possible to split this CL into the following two CLs?
> > 
> > 1. Recompute overflow when box-shadow, outline or border-image-outset
changes
> (for LayoutBlock);
> > 
> > 2. Make recomputeOverflow work for table cells and rows.
> > 
> 
> I think that would create regressions in (1) that would only be solved by (2).
I
> expect regressions even with both elements landed so would like to land this
as
> a monolithic CL and deal with the regressions then; or revert, regroup and
> reland if they're severe enough.

OK.

[rhogan, 2016-04-14 20:01:56]

On 2016/04/12 at 19:39:39, wangxianzhu wrote:
> On 2016/04/12 19:29:00, rhogan wrote:
> > On 2016/03/22 at 17:02:05, wangxianzhu wrote:
> > > Is it possible to split this CL into the following two CLs?
> > > 
> > > 1. Recompute overflow when box-shadow, outline or border-image-outset
changes
> > (for LayoutBlock);
> > > 
> > > 2. Make recomputeOverflow work for table cells and rows.
> > > 
> > 
> > I think that would create regressions in (1) that would only be solved by
(2). I
> > expect regressions even with both elements landed so would like to land this
as
> > a monolithic CL and deal with the regressions then; or revert, regroup and
> > reland if they're severe enough.
> 
> OK.

OK to take another look?

[Xianzhu, 2016-04-15 04:59:16]

A suggestion: you'd better reply to each review comment in the code, and publish
them after you upload a new patch. When I see them I know you have a new patch
ready for review, and your action/opinion upon each review comment.

https://codereview.chromium.org/1809643008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableRow.cpp (right):

https://codereview.chromium.org/1809643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:251: // TODO(rhogan,
wangxianzhu): This seems incorrect because cell may have visual effect overflow
that should be included in this row.
Can you file a bug for this? Then you can use TODO(crbug.com/<bug>) here.

https://codereview.chromium.org/1809643008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableRow.h (right):

https://codereview.chromium.org/1809643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableRow.h:125: void
recomputeOverflow();
What's the relationship between 'recomputeOverflow' and
'recalcOverflowAfterStyleChange'?

https://codereview.chromium.org/1809643008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleDifference.h (right):

https://codereview.chromium.org/1809643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleDifference.h:106: enum
RecomputeOverflowType {
We don't need to define a private enum to replace a bool like this. Using bool
in the getter and setter is already clear enough.

(We do prefer enum to bool for function parameters where the meaning of
true/false is not clear at call sites.)

[rhogan, 2016-04-15 19:26:09]

On 2016/04/15 at 04:59:16, wangxianzhu wrote:
> A suggestion: you'd better reply to each review comment in the code, and
publish them after you upload a new patch. When I see them I know you have a new
patch ready for review, and your action/opinion upon each review comment.

Will do. Updated per your comments.

[rhogan, 2016-04-15 19:34:32]

https://codereview.chromium.org/1809643008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableRow.cpp (right):

https://codereview.chromium.org/1809643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:251: // TODO(rhogan,
wangxianzhu): This seems incorrect because cell may have visual effect overflow
that should be included in this row.
On 2016/04/15 at 04:59:16, Xianzhu wrote:
> Can you file a bug for this? Then you can use TODO(crbug.com/<bug>) here.

Done.

https://codereview.chromium.org/1809643008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableRow.h (right):

https://codereview.chromium.org/1809643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableRow.h:125: void
recomputeOverflow();
On 2016/04/15 at 04:59:16, Xianzhu wrote:
> What's the relationship between 'recomputeOverflow' and
'recalcOverflowAfterStyleChange'?

Nothing really - it's just a refactoring in your table-related part of this
patch. I've renamed it anyway.

https://codereview.chromium.org/1809643008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleDifference.h (right):

https://codereview.chromium.org/1809643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleDifference.h:106: enum
RecomputeOverflowType {
On 2016/04/15 at 04:59:16, Xianzhu wrote:
> We don't need to define a private enum to replace a bool like this. Using bool
in the getter and setter is already clear enough.
> 
> (We do prefer enum to bool for function parameters where the meaning of
true/false is not clear at call sites.)

Sounds good.

[Xianzhu, 2016-04-15 20:48:51]

https://codereview.chromium.org/1809643008/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableRow.h (right):

https://codereview.chromium.org/1809643008/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableRow.h:125: void
recomputeOverflow();
On 2016/04/15 19:34:32, rhogan wrote:
> On 2016/04/15 at 04:59:16, Xianzhu wrote:
> > What's the relationship between 'recomputeOverflow' and
> 'recalcOverflowAfterStyleChange'?
> 
> Nothing really - it's just a refactoring in your table-related part of this
> patch. I've renamed it anyway.

Now I see why this is named recomputeOverflow/recalculateOverflow instead of
recalcOverflowAfterStyleChange(): this function is called from both
RecalcOverflowAfterStyleChange phase and Layout phase.

If I understood correct, to keep consistency with LayoutBlock, it would be
better to name LayoutTableRow::recomputeOverflow() as
LayoutTableRow::computeOverflow().

https://codereview.chromium.org/1809643008/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/1809643008/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1099: void
LayoutTableSection::computeOverflowFromCells(unsigned totalRows, unsigned
nEffCols)
Can row overflow contributing section overflow, for example, when some row has
outline?

[rhogan, 2016-04-16 14:31:15]

On 2016/04/15 at 20:48:51, wangxianzhu wrote:
>
https://codereview.chromium.org/1809643008/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1099: void
LayoutTableSection::computeOverflowFromCells(unsigned totalRows, unsigned
nEffCols)
> Can row overflow contributing section overflow, for example, when some row has
outline?

I don't see why not - added a TODO.

[rhogan, 2016-04-16 14:31:37]

On 2016/04/15 at 20:48:51, wangxianzhu wrote:
> Now I see why this is named recomputeOverflow/recalculateOverflow instead of
recalcOverflowAfterStyleChange(): this function is called from both
RecalcOverflowAfterStyleChange phase and Layout phase.
> 
> If I understood correct, to keep consistency with LayoutBlock, it would be
better to name LayoutTableRow::recomputeOverflow() as
LayoutTableRow::computeOverflow().
> 
Updated it.

[Xianzhu, 2016-04-16 21:57:24]

lgtm

[rhogan, 2016-04-17 19:00:35]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-17 19:00:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1809643008/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1809643008/180001

[rhogan, 2016-04-17 20:05:55]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-17 20:05:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1809643008/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1809643008/200001

[commit-bot: I haz the power, 2016-04-17 21:06:32]

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

[commit-bot: I haz the power, 2016-04-17 21:06:33]

Dry run: This issue passed the CQ dry run.

[rhogan, 2016-04-18 05:59:55]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2016-04-18 05:59:56]

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

[commit-bot: I haz the power, 2016-04-18 06:00:08]

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

[commit-bot: I haz the power, 2016-04-18 06:04:53]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-04-18 06:06:10]

Description was changed from

==========
. so just require a paint invalidation and ensure visual overflow gets
recomputed.

Another go at https://codereview.chromium.org/561303002 which was reverted by
https://codereview.chromium.org/747493002. 

This includes wangxianzhu's fix for tables in
https://codereview.chromium.org/744493002.

The intent of this patch is to avoid layout for all types of LayoutBlock when
the outline, border-image-outset or box-shadow are updated.

BUG=396825,434700,437012
==========

to

==========
. so just require a paint invalidation and ensure visual overflow gets
recomputed.

Another go at https://codereview.chromium.org/561303002 which was reverted by
https://codereview.chromium.org/747493002.

This includes wangxianzhu's fix for tables in
https://codereview.chromium.org/744493002.

The intent of this patch is to avoid layout for all types of LayoutBlock when
the outline, border-image-outset or box-shadow are updated.

BUG=396825,434700,437012

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

[commit-bot: I haz the power, 2016-04-18 06:06:11]

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

[mstensho (USE GERRIT), 2016-04-18 08:19:18]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2016-04-18 08:19:19]

That commit message didn't end up pretty. The first line is:

". so just require a paint invalidation and ensure visual overflow gets
recomputed."