Fix spanning cell painting background from wrong row

Fix spanning cell painting background from wrong row

Previously when we painted a table section not from the first row
and the starting row contained spanned cells, we may paint the
cell with the wrong row background.

Now expands the dirty rows/columns to the top/left to cover the grid
slots from which the spanning primary cells' originate. In this way,
we also simply painting code to just consider the primary cells
originating from the grid slots.

This is also a preparation of crbug.com/663208 to paint background of
each table section/row in one display item.

BUG=706255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2783953002
Cr-Commit-Position: refs/heads/master@{#460822}
Committed: https://chromium.googlesource.com/chromium/src/+/29bb853d07d410286f932802134ff3732777f713

[Xianzhu, 2017-03-29 17:35:51]

Description was changed from

==========
Fix spanning cell painting background from the wrong row

Previously when we painted a table section not from the first row
and the starting row contained spanned cells, we may paint the
cell with the wrong row background.

Now expands the dirty rows/columns to the top/left to cover the grid
slots from which the spanning primary cells' originate. In this way,
we also simply painting code to just consider the primary cells
originating from the grid slots.

This is also a preparation of crbug.com/663208 to paint background of
each table section/row in one display item.

BUG=706255
==========

to

==========
Fix spanning cell painting background from the wrong row

Previously when we painted a table section not from the first row
and the starting row contained spanned cells, we may paint the
cell with the wrong row background.

Now expands the dirty rows/columns to the top/left to cover the grid
slots from which the spanning primary cells' originate. In this way,
we also simply painting code to just consider the primary cells
originating from the grid slots.

This is also a preparation of crbug.com/663208 to paint background of
each table section/row in one display item.

BUG=706255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2017-03-29 17:35:52]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 17:36:31]

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

[Xianzhu, 2017-03-29 17:43:28]

wangxianzhu@chromium.org changed reviewers:
+ mstensho@opera.com, wkorman@chromium.org

[wkorman, 2017-03-29 18:12:33]

lgtm

https://codereview.chromium.org/2783953002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/2783953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:236: LayoutTableCell*
originatingCellAt(unsigned row, unsigned effectiveColumn) {
Do these methods have to be implemented in header? Prefer putting in .cpp if we
can for faster build time when modifying.

https://codereview.chromium.org/2783953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:245: return
const_cast<LayoutTableSection*>(this)->originatingCellAt(
Why do we need the const_cast and the separate method? Is primaryCellAt not
const?

https://codereview.chromium.org/2783953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:321: // a primary
cell intersecting visualRect originates.
|visualRect|

https://codereview.chromium.org/2783953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:499: // This is set
if there is any cell spanning multiple rows or cols.
Slight rephrase for clarity, something like, "Whether any cell spans multiple
rows or cols."
since one could set it to false and it would still have been "set".

[Xianzhu, 2017-03-29 18:43:50]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[Xianzhu, 2017-03-29 18:44:30]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[Xianzhu, 2017-03-29 18:44:41]

https://codereview.chromium.org/2783953002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/2783953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:236: LayoutTableCell*
originatingCellAt(unsigned row, unsigned effectiveColumn) {
On 2017/03/29 18:12:33, wkorman wrote:
> Do these methods have to be implemented in header? Prefer putting in .cpp if
we
> can for faster build time when modifying.

Done.

https://codereview.chromium.org/2783953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:245: return
const_cast<LayoutTableSection*>(this)->originatingCellAt(
On 2017/03/29 18:12:33, wkorman wrote:
> Why do we need the const_cast and the separate method? Is primaryCellAt not
> const?

This follows the strict-const style which is used in primaryCell etc., like:

  // non const version.
  Something* method() { ... }
  // const version.
  const SomeThing* method() const { return const_cast<Class*>(this)->method(); }

I usually use this style for new methods if the sibling methods are already in
this style.

Otherwise I would use the loose-const style:
  Something* method() const { ... }

https://codereview.chromium.org/2783953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:321: // a primary
cell intersecting visualRect originates.
On 2017/03/29 18:12:33, wkorman wrote:
> |visualRect|

Done.

https://codereview.chromium.org/2783953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:499: // This is set
if there is any cell spanning multiple rows or cols.
On 2017/03/29 18:12:33, wkorman wrote:
> Slight rephrase for clarity, something like, "Whether any cell spans multiple
> rows or cols."
> since one could set it to false and it would still have been "set".

Done.

[commit-bot: I haz the power, 2017-03-29 18:44:52]

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

[Xianzhu, 2017-03-29 18:49:12]

https://codereview.chromium.org/2783953002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/2783953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:245: return
const_cast<LayoutTableSection*>(this)->originatingCellAt(
On 2017/03/29 18:44:41, Xianzhu wrote:
> On 2017/03/29 18:12:33, wkorman wrote:
> > Why do we need the const_cast and the separate method? Is primaryCellAt not
> > const?
> 
> This follows the strict-const style which is used in primaryCell etc., like:
> 
>   // non const version.
>   Something* method() { ... }
>   // const version.
>   const SomeThing* method() const { return const_cast<Class*>(this)->method();
}
> 
> I usually use this style for new methods if the sibling methods are already in
> this style.
> 
> Otherwise I would use the loose-const style:
>   Something* method() const { ... }

As only the const version is used, I will remove the non-const one.

[Xianzhu, 2017-03-29 19:00:02]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 19:00:50]

Dry run: 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, 2017-03-29 21:35:07]

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

[commit-bot: I haz the power, 2017-03-29 21:35:07]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2017-03-30 18:01:33]

Description was changed from

==========
Fix spanning cell painting background from the wrong row

Previously when we painted a table section not from the first row
and the starting row contained spanned cells, we may paint the
cell with the wrong row background.

Now expands the dirty rows/columns to the top/left to cover the grid
slots from which the spanning primary cells' originate. In this way,
we also simply painting code to just consider the primary cells
originating from the grid slots.

This is also a preparation of crbug.com/663208 to paint background of
each table section/row in one display item.

BUG=706255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix spanning cell painting background from wrong row

Previously when we painted a table section not from the first row
and the starting row contained spanned cells, we may paint the
cell with the wrong row background.

Now expands the dirty rows/columns to the top/left to cover the grid
slots from which the spanning primary cells' originate. In this way,
we also simply painting code to just consider the primary cells
originating from the grid slots.

This is also a preparation of crbug.com/663208 to paint background of
each table section/row in one display item.

BUG=706255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2017-03-30 18:01:39]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-03-30 18:01:39]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2783953002/#ps60001
(title: "Remove non-const version and use faster implementation")

[commit-bot: I haz the power, 2017-03-30 18:02:27]

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, 2017-03-30 18:08:59]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490896899079570,
"parent_rev": "e5de8d551e80115b21e0e3ca7d5451d3cee6f3ba", "commit_rev":
"29bb853d07d410286f932802134ff3732777f713"}

[commit-bot: I haz the power, 2017-03-30 18:09:46]

Description was changed from

==========
Fix spanning cell painting background from wrong row

Previously when we painted a table section not from the first row
and the starting row contained spanned cells, we may paint the
cell with the wrong row background.

Now expands the dirty rows/columns to the top/left to cover the grid
slots from which the spanning primary cells' originate. In this way,
we also simply painting code to just consider the primary cells
originating from the grid slots.

This is also a preparation of crbug.com/663208 to paint background of
each table section/row in one display item.

BUG=706255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix spanning cell painting background from wrong row

Previously when we painted a table section not from the first row
and the starting row contained spanned cells, we may paint the
cell with the wrong row background.

Now expands the dirty rows/columns to the top/left to cover the grid
slots from which the spanning primary cells' originate. In this way,
we also simply painting code to just consider the primary cells
originating from the grid slots.

This is also a preparation of crbug.com/663208 to paint background of
each table section/row in one display item.

BUG=706255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2783953002
Cr-Commit-Position: refs/heads/master@{#460822}
Committed:
https://chromium.googlesource.com/chromium/src/+/29bb853d07d410286f932802134f...
==========

[commit-bot: I haz the power, 2017-03-30 18:09:47]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/29bb853d07d410286f932802134f...