Content of cell should avoid repeating headers when it straddles multiple pages

Content of cell should avoid repeating headers when it straddles multiple pages

The right thing to do here is not covered by the spec, so for now just fix
our rendering bug by doing what firefox does.

Opened a discussion at https://lists.w3.org/Archives/Public/www-style/2016Dec/0070.html.

BUG=675453
Committed: https://crrev.com/0ab5bebc6d97953cf7ee417a80cf453a5145dd8e
Cr-Commit-Position: refs/heads/master@{#439906}

[rhogan, 2016-12-18 15:43:56]

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

[commit-bot: I haz the power, 2016-12-18 15:44:03]

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, 2016-12-18 17:34:04]

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

[commit-bot: I haz the power, 2016-12-18 17:34:04]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rhogan, 2016-12-19 19:36:22]

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

[commit-bot: I haz the power, 2016-12-19 19:37:07]

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, 2016-12-19 21:56:48]

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

[commit-bot: I haz the power, 2016-12-19 21:56:48]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rhogan, 2016-12-20 10:32:43]

Description was changed from

==========
Content of cell should avoid repeating headers when it straddles multiple pages

BUG=675453
==========

to

==========
Content of cell should avoid repeating headers when it straddles multiple pages

The right thing to do here is not covered by the spec, so for now just fix
our rendering bug by doing what firefox does.

Opened a discussion at
https://lists.w3.org/Archives/Public/www-style/2016Dec/0070.html.

BUG=675453
==========

[rhogan, 2016-12-20 10:32:43]

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

[mstensho (USE GERRIT), 2016-12-20 11:47:01]

https://codereview.chromium.org/2587673003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-cell-straddles-page-expected.txt
(right):

https://codereview.chromium.org/2587673003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fragmentation/single-line-cells-repeating-thead-cell-straddles-page-expected.txt:3:
layer at (0,0) size 800x160
Please make a reftest instead.

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

https://codereview.chromium.org/2587673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:5593: LayoutTableCell* cell
= toLayoutTableCell(const_cast<LayoutBox*>(this));
Why not keep the constness?

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

https://codereview.chromium.org/2587673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:322:
LayoutTableSection* header = table()->header();
Might want to do an early return for rowIndex()

if (rowIndex())
  return false;

[rhogan, 2016-12-20 16:06:38]

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

https://codereview.chromium.org/2587673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:322:
LayoutTableSection* header = table()->header();
On 2016/12/20 at 11:47:00, mstensho wrote:
> Might want to do an early return for rowIndex()
> 
> if (rowIndex())
>   return false;

return !rowIndex() && ..; returns just as quickly no? Or is it just about
readability?

[rhogan, 2016-12-20 16:08:10]

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

[commit-bot: I haz the power, 2016-12-20 16:08:36]

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

[mstensho (USE GERRIT), 2016-12-20 16:50:36]

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

https://codereview.chromium.org/2587673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:322:
LayoutTableSection* header = table()->header();
On 2016/12/20 16:06:37, rhogan wrote:
> On 2016/12/20 at 11:47:00, mstensho wrote:
> > Might want to do an early return for rowIndex()
> > 
> > if (rowIndex())
> >   return false;
> 
> return !rowIndex() && ..; returns just as quickly no? Or is it just about
> readability?

I was thinking that you could put it in front of the declaration of 'header'.
And THEN it would be WAAAAAAAAY faster for all rows but the first. :)

Also, slightly more readable, IMHO.

[rhogan, 2016-12-20 16:55:13]

On 2016/12/20 at 16:50:36, mstensho wrote:
>
https://codereview.chromium.org/2587673003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutTableRow.cpp (right):
> 
>
https://codereview.chromium.org/2587673003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:322:
LayoutTableSection* header = table()->header();
> On 2016/12/20 16:06:37, rhogan wrote:
> > On 2016/12/20 at 11:47:00, mstensho wrote:
> > > Might want to do an early return for rowIndex()
> > > 
> > > if (rowIndex())
> > >   return false;
> > 
> > return !rowIndex() && ..; returns just as quickly no? Or is it just about
> > readability?
> 
> I was thinking that you could put it in front of the declaration of 'header'.
And THEN it would be WAAAAAAAAY faster for all rows but the first. :)
> 
> Also, slightly more readable, IMHO.

Oh right, sure!

[commit-bot: I haz the power, 2016-12-20 17:55:31]

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

[commit-bot: I haz the power, 2016-12-20 17:55:32]

Dry run: This issue passed the CQ dry run.

[rhogan, 2016-12-20 18:01:24]

On 2016/12/20 at 16:55:13, rhogan wrote:
> On 2016/12/20 at 16:50:36, mstensho wrote:
> >
https://codereview.chromium.org/2587673003/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/LayoutTableRow.cpp (right):
> > 
> >
https://codereview.chromium.org/2587673003/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:322:
LayoutTableSection* header = table()->header();
> > On 2016/12/20 16:06:37, rhogan wrote:
> > > On 2016/12/20 at 11:47:00, mstensho wrote:
> > > > Might want to do an early return for rowIndex()
> > > > 
> > > > if (rowIndex())
> > > >   return false;
> > > 
> > > return !rowIndex() && ..; returns just as quickly no? Or is it just about
> > > readability?
> > 
> > I was thinking that you could put it in front of the declaration of
'header'. And THEN it would be WAAAAAAAAY faster for all rows but the first. :)
> > 
> > Also, slightly more readable, IMHO.
> 
> Oh right, sure!

Updated there for you now.

[mstensho (USE GERRIT), 2016-12-20 18:20:55]

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

https://codereview.chromium.org/2587673003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:322:
LayoutTableSection* header = table()->header();
On 2016/12/20 16:50:36, mstensho wrote:
> On 2016/12/20 16:06:37, rhogan wrote:
> > On 2016/12/20 at 11:47:00, mstensho wrote:
> > > Might want to do an early return for rowIndex()
> > > 
> > > if (rowIndex())
> > >   return false;
> > 
> > return !rowIndex() && ..; returns just as quickly no? Or is it just about
> > readability?
> 
> I was thinking that you could put it in front of the declaration of 'header'.
> And THEN it would be WAAAAAAAAY faster for all rows but the first. :)
> 
> Also, slightly more readable, IMHO.

Did you mean to address this too?

[rhogan, 2016-12-20 18:30:27]

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

[commit-bot: I haz the power, 2016-12-20 18:30:51]

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

[rhogan, 2016-12-20 18:30:56]

On 2016/12/20 at 18:20:55, mstensho wrote:
>
https://codereview.chromium.org/2587673003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutTableRow.cpp (right):
> 
>
https://codereview.chromium.org/2587673003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:322:
LayoutTableSection* header = table()->header();
> On 2016/12/20 16:50:36, mstensho wrote:
> > On 2016/12/20 16:06:37, rhogan wrote:
> > > On 2016/12/20 at 11:47:00, mstensho wrote:
> > > > Might want to do an early return for rowIndex()
> > > > 
> > > > if (rowIndex())
> > > >   return false;
> > > 
> > > return !rowIndex() && ..; returns just as quickly no? Or is it just about
> > > readability?
> > 
> > I was thinking that you could put it in front of the declaration of
'header'.
> > And THEN it would be WAAAAAAAAY faster for all rows but the first. :)
> > 
> > Also, slightly more readable, IMHO.
> 
> Did you mean to address this too?

Whoops forgot to press enter! There now.

[mstensho (USE GERRIT), 2016-12-20 18:38:21]

lgtm

[commit-bot: I haz the power, 2016-12-20 21:35:03]

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

[commit-bot: I haz the power, 2016-12-20 21:35:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rhogan, 2016-12-20 21:38:17]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2016-12-20 21:39:06]

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-12-20 23:00:52]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482269897123850,
"parent_rev": "3c14898503983f7c30a54e7093e2b8d073cb3af0", "commit_rev":
"ebd360462e6cfe17a50e713e91947431d0e57e39"}

[commit-bot: I haz the power, 2016-12-20 23:01:20]

Description was changed from

==========
Content of cell should avoid repeating headers when it straddles multiple pages

The right thing to do here is not covered by the spec, so for now just fix
our rendering bug by doing what firefox does.

Opened a discussion at
https://lists.w3.org/Archives/Public/www-style/2016Dec/0070.html.

BUG=675453
==========

to

==========
Content of cell should avoid repeating headers when it straddles multiple pages

The right thing to do here is not covered by the spec, so for now just fix
our rendering bug by doing what firefox does.

Opened a discussion at
https://lists.w3.org/Archives/Public/www-style/2016Dec/0070.html.

BUG=675453

Review-Url: https://codereview.chromium.org/2587673003
==========

[commit-bot: I haz the power, 2016-12-20 23:01:21]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-20 23:04:35]

Description was changed from

==========
Content of cell should avoid repeating headers when it straddles multiple pages

The right thing to do here is not covered by the spec, so for now just fix
our rendering bug by doing what firefox does.

Opened a discussion at
https://lists.w3.org/Archives/Public/www-style/2016Dec/0070.html.

BUG=675453

Review-Url: https://codereview.chromium.org/2587673003
==========

to

==========
Content of cell should avoid repeating headers when it straddles multiple pages

The right thing to do here is not covered by the spec, so for now just fix
our rendering bug by doing what firefox does.

Opened a discussion at
https://lists.w3.org/Archives/Public/www-style/2016Dec/0070.html.

BUG=675453

Committed: https://crrev.com/0ab5bebc6d97953cf7ee417a80cf453a5145dd8e
Cr-Commit-Position: refs/heads/master@{#439906}
==========

[commit-bot: I haz the power, 2016-12-20 23:04:37]

Patchset 5 (id:??) landed as
https://crrev.com/0ab5bebc6d97953cf7ee417a80cf453a5145dd8e
Cr-Commit-Position: refs/heads/master@{#439906}