Reference table row via the cell for row background display item client.

Reference table row via the cell for row background display item client.

BUG=666800
Committed: https://crrev.com/91b1dd79c0916b9c329058622919f190958ec626
Cr-Commit-Position: refs/heads/master@{#434110}

[wkorman, 2016-11-23 00:29:18]

wkorman@chromium.org changed reviewers:
+ wangxianzhu@chromium.org

[wkorman, 2016-11-23 00:29:19]

There's an open question (I just haven't traced further) re: why we see the
ASIDE table cell create a RowBackgroundDisplayItemClient for itself with the row
being the same LayoutTableRow instance that the LABEL uses, when the layout tree
shows they each have their own (see tree below).

I determined this was what appeared to be happening by logging in the
RowBackgroundDisplayItemClient ctor, see log at http://crbug.com/666800#c10

But it seems reasonable to reference the row via the cell in any case. I
confirmed the test fails without patch and passes with.

BEFORE RE-STYLING

LayoutView 0x59c6f004010               	#document
  LayoutBlockFlow 0x59c6f014010        	HTML
    LayoutTable (anonymous) 0x59c6f030010
      LayoutTableSection (anonymous) 0x59c6f040010
        LayoutTableRow (anonymous) 0x59c6f04c010
          LayoutTableCell 0x59c6f020010	LABEL class="c13"
      LayoutTableCol 0x59c6f04c118     	COLGROUP class="c11"
      LayoutTableSection (anonymous) 0x59c6f040170
        LayoutTableRow (anonymous) 0x59c6f04c220
          LayoutTableCell 0x59c6f0202a0	ASIDE class="c13"
    LayoutBlockFlow 0x59c6f014138      	BODY

AFTER RE-STYLING

LayoutView 0x59c6f004010              	#document
  LayoutBlockFlow 0x59c6f014010        	HTML
    LayoutTable (anonymous) 0x59c6f030010
      LayoutTableCol 0x59c6f04c010     	LABEL class="c7"
      LayoutTableCol 0x59c6f04c118     	COLGROUP class="c11"
      LayoutTableSection (anonymous) 0x59c6f040170
        LayoutTableRow (anonymous) 0x59c6f04c220
          LayoutTableCell 0x59c6f0202a0	ASIDE class="c13"
    LayoutBlockFlow 0x59c6f014138      	BODY

[Xianzhu, 2016-11-23 00:56:25]

https://codereview.chromium.org/2522923003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/paint/tables/table-cell-changes-to-colgroup.html
(right):

https://codereview.chromium.org/2522923003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/paint/tables/table-cell-changes-to-colgroup.html:7:
.c13 { display: table-cell; border-style: ridge; will-change: transform; }
Are all of the CSS properties necessary for reproducing the bug?

https://codereview.chromium.org/2522923003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/paint/tables/table-cell-changes-to-colgroup.html:19:
document.documentElement.appendChild(nodes[3]);
Can the script be replaced with direct HTML?

https://codereview.chromium.org/2522923003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/paint/tables/table-cell-changes-to-colgroup.html:24:
nodes[1].setAttribute('class', 'c7');
Please replace nodes[<n>] and c<n> with meaningful names.

[Xianzhu, 2016-11-23 00:57:22]

On 2016/11/23 00:29:19, wkorman wrote:
> There's an open question (I just haven't traced further) re: why we see the
> ASIDE table cell create a RowBackgroundDisplayItemClient for itself with the
row
> being the same LayoutTableRow instance that the LABEL uses, when the layout
tree
> shows they each have their own (see tree below).

Will reply in the bug.

[wkorman, 2016-11-23 01:02:49]

On 2016/11/23 00:29:19, wkorman wrote:
> There's an open question (I just haven't traced further) re: why we see the
> ASIDE table cell create a RowBackgroundDisplayItemClient for itself with the
row
> being the same LayoutTableRow instance that the LABEL uses, when the layout
tree
> shows they each have their own (see tree below).

The answer to this question is that, when the document first goes through layout
and paint, they do share the same row. After script-driven restyling it ends up
looking like the above. Referencing the row indirectly via cell is IMO more
elegant than adding logic wherever we might need to, on DOM mutation, to
reinstantiate the RBDIC if the row instance for a cell changes.

When we create the RBDIC for the ASIDE cell:

LayoutView 0x36eed4c04010              	#document
  LayoutBlockFlow 0x36eed4c14010       	HTML
    LayoutTable (anonymous) 0x36eed4c30010
      LayoutTableSection (anonymous) 0x36eed4c40010
        LayoutTableRow (anonymous) 0x36eed4c4c010
          LayoutTableCell 0x36eed4c20010	LABEL class="c13"
          LayoutTableCell 0x36eed4c20158	COLGROUP class="c13"
*         LayoutTableCell 0x36eed4c202a0	ASIDE class="c13"
    LayoutBlockFlow 0x36eed4c14138     	BODY

And when we go to paint its section after the restyling:

LayoutView 0x36eed4c04010              	#document
  LayoutBlockFlow 0x36eed4c14010       	HTML
    LayoutTable (anonymous) 0x36eed4c30010
      LayoutTableCol 0x36eed4c4c010    	LABEL class="c7"
      LayoutTableCol 0x36eed4c4c118    	COLGROUP class="c11"
*     LayoutTableSection (anonymous) 0x36eed4c40170
        LayoutTableRow (anonymous) 0x36eed4c4c220
          LayoutTableCell 0x36eed4c202a0	ASIDE class="c13"
    LayoutBlockFlow 0x36eed4c14138     	BODY

[wkorman, 2016-11-23 02:05:22]

https://codereview.chromium.org/2522923003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/paint/tables/table-cell-changes-to-colgroup.html
(right):

https://codereview.chromium.org/2522923003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/paint/tables/table-cell-changes-to-colgroup.html:7:
.c13 { display: table-cell; border-style: ridge; will-change: transform; }
On 2016/11/23 00:56:25, Xianzhu wrote:
> Are all of the CSS properties necessary for reproducing the bug?

I spent time taking some out, I was able to pull a few more on re-review and
fixed formatting too.

https://codereview.chromium.org/2522923003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/paint/tables/table-cell-changes-to-colgroup.html:19:
document.documentElement.appendChild(nodes[3]);
On 2016/11/23 00:56:25, Xianzhu wrote:
> Can the script be replaced with direct HTML?

Have tried a number of ways without success. Main challenge is adding the
colgroup in a declarative manner that still allows referencing it via script to
fiddle its class. Can do it if I wrap it in an explicit table, but then issue
doesn't repro for some reason. Spent enough time to deem not worth further.
Conceptually we should be able to create a failing test case since we thoroughly
understand situation, it's just spending the time. Can pursue further if you
think warranted.

https://codereview.chromium.org/2522923003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/paint/tables/table-cell-changes-to-colgroup.html:24:
nodes[1].setAttribute('class', 'c7');
On 2016/11/23 00:56:25, Xianzhu wrote:
> Please replace nodes[<n>] and c<n> with meaningful names.

Done.

[Xianzhu, 2016-11-23 02:44:48]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-11-23 02:44:48]

lgtm

[commit-bot: I haz the power, 2016-11-23 02:45:12]

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-11-23 04:10:48]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479869088174630,
"parent_rev": "190ba0a10bb8ff264697a3d76db9991af5cd0d22", "commit_rev":
"4229f465d5fc328491229e97223fce8074271ed5"}

[commit-bot: I haz the power, 2016-11-23 04:12:19]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-23 04:16:40]

Description was changed from

==========
Reference table row via the cell for row background display item client.

BUG=666800
==========

to

==========
Reference table row via the cell for row background display item client.

BUG=666800

Committed: https://crrev.com/91b1dd79c0916b9c329058622919f190958ec626
Cr-Commit-Position: refs/heads/master@{#434110}
==========

[commit-bot: I haz the power, 2016-11-23 04:16:41]

Patchset 2 (id:??) landed as
https://crrev.com/91b1dd79c0916b9c329058622919f190958ec626
Cr-Commit-Position: refs/heads/master@{#434110}