Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(84)

Issue 2534413004: Made varied number of cells in each row based on row's requirement. (Closed)

Created:
4 years ago by a.suchit
Modified:
3 years, 3 months ago
CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Made varied number of cells in each row based on row's requirement. Rowspan-only-row(s) was not getting height when there is at least one empty cell present. Number of the cells are created in the row based on the required column in that row. So Empty cells would not be present at the end of the row. R=jchaffraix@chromium.org, mstensho@opera.com BUG=396653 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/1060f916d9f7026fa22dd3bb6b563294ec7f8224 Cr-Commit-Position: refs/heads/master@{#441620}

Patch Set 1 #

Patch Set 2 : Added Layout test #

Patch Set 3 : Updated layout test file #

Patch Set 4 : Removed png test #

Patch Set 5 : Corrected layout test #

Patch Set 6 : Reduced layout test specific to fix and described. #

Patch Set 7 : Layout test rebaseline #

Patch Set 8 : Rebase #

Patch Set 9 : new patch (Removed unwanted cells in rows.) #

Patch Set 10 : Rebase #

Patch Set 11 : Fixed Layout test crash. #

Total comments: 13

Patch Set 12 : Review comments addressed #

Total comments: 7

Patch Set 13 : Rebaseline layout tests #

Patch Set 14 : Reset TestExpectations file #

Unified diffs Side-by-side diffs Delta from patch set Stats (+325 lines, -210 lines) Patch
A third_party/WebKit/LayoutTests/fast/table/rowspan-only-rows-height-distribution.html View 1 2 3 4 5 6 7 8 1 chunk +51 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/table/rowspan-only-rows-height-distribution-expected.txt View 1 2 3 4 5 1 chunk +7 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/table/table-all-rowspans-height-distribution-in-rows.html View 1 2 3 4 5 6 7 8 9 6 chunks +30 lines, -30 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/table/table-all-rowspans-height-distribution-in-rows-except-overlapped.html View 1 chunk +5 lines, -5 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-1.html View 1 chunk +5 lines, -5 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-2.html View 1 chunk +5 lines, -5 lines 0 comments Download
M third_party/WebKit/LayoutTests/platform/linux/tables/mozilla/bugs/bug220536-expected.png View 1 2 3 4 5 6 7 8 Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/linux/tables/mozilla/bugs/bug220536-expected.txt View 1 2 3 4 5 6 7 8 1 chunk +8 lines, -8 lines 0 comments Download
M third_party/WebKit/LayoutTests/platform/mac/fast/table/table-all-rowspans-height-distribution-in-rows-except-overlapped-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +30 lines, -10 lines 0 comments Download
M third_party/WebKit/LayoutTests/platform/mac/fast/table/table-all-rowspans-height-distribution-in-rows-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 7 chunks +64 lines, -70 lines 0 comments Download
M third_party/WebKit/LayoutTests/platform/mac/fast/table/table-rowspan-height-distribution-in-rows-1-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +30 lines, -10 lines 0 comments Download
M third_party/WebKit/LayoutTests/platform/mac/fast/table/table-rowspan-height-distribution-in-rows-2-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +30 lines, -10 lines 0 comments Download
M third_party/WebKit/LayoutTests/platform/mac/tables/mozilla/bugs/bug220536-expected.png View 1 2 3 4 5 6 7 8 9 10 11 12 Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/mac/tables/mozilla/bugs/bug220536-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +8 lines, -8 lines 0 comments Download
M third_party/WebKit/LayoutTests/platform/win/tables/mozilla/bugs/bug220536-expected.png View 1 2 3 4 5 6 7 8 9 10 11 12 Binary file 0 comments Download
M third_party/WebKit/LayoutTests/platform/win/tables/mozilla/bugs/bug220536-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +8 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutTable.cpp View 1 2 3 4 5 6 7 8 9 3 chunks +2 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutTableSection.h View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +13 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutTableSection.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 19 chunks +23 lines, -31 lines 0 comments Download
M third_party/WebKit/Source/core/layout/TableLayoutAlgorithmAuto.cpp View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/paint/TableSectionPainter.cpp View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +4 lines, -3 lines 0 comments Download

Messages

Total messages: 81 (45 generated)
a.suchit2
Please review.
4 years ago (2016-12-01 06:31:00 UTC) #2
suchit.agrawal
Some behavior of the table (in rowspan case) is changed but changed behavior is matching ...
4 years ago (2016-12-01 06:38:25 UTC) #4
a.suchit
4 years ago (2016-12-01 06:55:06 UTC) #6
mstensho (USE GERRIT)
Please add a minimal test for this.
4 years ago (2016-12-01 08:37:38 UTC) #7
a.suchit
On 2016/12/01 08:37:38, mstensho wrote: > Please add a minimal test for this. Ohh sorry. ...
4 years ago (2016-12-01 09:35:09 UTC) #8
suchit.agrawal
Layout test added please review. Thanks
4 years ago (2016-12-02 00:51:25 UTC) #10
mstensho (USE GERRIT)
On 2016/12/02 00:51:25, suchit.agrawal wrote: > Layout test added please review. Thanks Please write a ...
4 years ago (2016-12-02 09:01:02 UTC) #11
a.suchit
On 2016/12/02 09:01:02, mstensho wrote: > On 2016/12/02 00:51:25, suchit.agrawal wrote: > > Layout test ...
4 years ago (2016-12-02 11:24:00 UTC) #12
mstensho (USE GERRIT)
On 2016/12/02 11:24:00, a.suchit wrote: > On 2016/12/02 09:01:02, mstensho wrote: > > On 2016/12/02 ...
4 years ago (2016-12-02 11:47:34 UTC) #13
a.suchit
On 2016/12/02 11:47:34, mstensho wrote: > On 2016/12/02 11:24:00, a.suchit wrote: > > On 2016/12/02 ...
4 years ago (2016-12-02 12:29:44 UTC) #14
a.suchit
Please review.
4 years ago (2016-12-06 12:35:44 UTC) #15
mstensho (USE GERRIT)
On 2016/12/06 12:35:44, a.suchit wrote: > Please review. Sorry about the delay. This patch is ...
4 years ago (2016-12-06 12:41:06 UTC) #16
mstensho (USE GERRIT)
Seems to me that the root cause of the problem is that LayoutTable::numEffectiveColumns() stays at ...
4 years ago (2016-12-06 23:39:22 UTC) #21
a.suchit
On 2016/12/06 23:39:22, mstensho wrote: > Seems to me that the root cause of the ...
4 years ago (2016-12-08 05:45:24 UTC) #22
mstensho (USE GERRIT)
On 2016/12/08 05:45:24, a.suchit wrote: > On 2016/12/06 23:39:22, mstensho wrote: > > Seems to ...
4 years ago (2016-12-08 07:42:46 UTC) #29
a.suchit
On 2016/12/08 07:42:46, mstensho wrote: > On 2016/12/08 05:45:24, a.suchit wrote: > > On 2016/12/06 ...
4 years ago (2016-12-08 10:34:52 UTC) #32
mstensho (USE GERRIT)
On 2016/12/08 10:34:52, a.suchit wrote: > On 2016/12/08 07:42:46, mstensho wrote: > > On 2016/12/08 ...
4 years ago (2016-12-08 10:39:48 UTC) #33
a.suchit
On 2016/12/08 10:39:48, mstensho wrote: > On 2016/12/08 10:34:52, a.suchit wrote: > > On 2016/12/08 ...
4 years ago (2016-12-08 10:53:43 UTC) #34
a.suchit
On 2016/12/08 10:53:43, a.suchit wrote: > On 2016/12/08 10:39:48, mstensho wrote: > > On 2016/12/08 ...
4 years ago (2016-12-08 11:08:34 UTC) #35
mstensho (USE GERRIT)
On 2016/12/08 11:08:34, a.suchit wrote: > On 2016/12/08 10:53:43, a.suchit wrote: > > On 2016/12/08 ...
4 years ago (2016-12-08 11:36:46 UTC) #36
a.suchit
On 2016/12/08 11:36:46, mstensho wrote: > On 2016/12/08 11:08:34, a.suchit wrote: > > On 2016/12/08 ...
4 years ago (2016-12-19 07:15:30 UTC) #37
mstensho (USE GERRIT)
On 2016/12/19 07:15:30, a.suchit wrote: > I will take some time to updates about our ...
4 years ago (2016-12-20 08:57:50 UTC) #38
a.suchit
On 2016/12/20 08:57:50, mstensho wrote: > On 2016/12/19 07:15:30, a.suchit wrote: > > I will ...
3 years, 11 months ago (2016-12-29 05:30:45 UTC) #53
mstensho (USE GERRIT)
Typo in description: "Emtpy" https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/LayoutTests/TestExpectations File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/LayoutTests/TestExpectations#newcode2414 third_party/WebKit/LayoutTests/TestExpectations:2414: # Affter Landing patch https://codereview.chromium.org/2534413004/ ...
3 years, 11 months ago (2017-01-03 15:18:41 UTC) #54
a.suchit
https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/LayoutTests/TestExpectations File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/LayoutTests/TestExpectations#newcode2414 third_party/WebKit/LayoutTests/TestExpectations:2414: # Affter Landing patch https://codereview.chromium.org/2534413004/ below tests should rebaseline ...
3 years, 11 months ago (2017-01-04 10:35:19 UTC) #56
mstensho (USE GERRIT)
https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp#newcode1742 third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1742: ensureCols(row, pos + 1 + 1); pos + 2, ...
3 years, 11 months ago (2017-01-04 10:56:06 UTC) #57
a.suchit
https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp#newcode1742 third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1742: ensureCols(row, pos + 1 + 1); On 2017/01/04 10:56:06, ...
3 years, 11 months ago (2017-01-05 03:34:31 UTC) #64
a.suchit
https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/LayoutTests/TestExpectations File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/LayoutTests/TestExpectations#newcode2414 third_party/WebKit/LayoutTests/TestExpectations:2414: # Affter Landing patch https://codereview.chromium.org/2534413004/ below tests should rebaseline ...
3 years, 11 months ago (2017-01-05 03:35:47 UTC) #65
a.suchit
third_party/WebKit/LayoutTests/fast/table/table-all-rowspans-height-distribution-in-rows.html third_party/WebKit/LayoutTests/fast/table/table-all-rowspans-height-distribution-in-rows-except-overlapped.html third_party/WebKit/LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-1.html third_party/WebKit/LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-2.html These test files are set based on the linux platform and ...
3 years, 11 months ago (2017-01-05 03:47:07 UTC) #66
mstensho (USE GERRIT)
lgtm! On 2017/01/05 03:47:07, a.suchit wrote: > third_party/WebKit/LayoutTests/fast/table/table-all-rowspans-height-distribution-in-rows.html > third_party/WebKit/LayoutTests/fast/table/table-all-rowspans-height-distribution-in-rows-except-overlapped.html > third_party/WebKit/LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-1.html > third_party/WebKit/LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-2.html > ...
3 years, 11 months ago (2017-01-05 10:41:18 UTC) #69
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2534413004/260001
3 years, 11 months ago (2017-01-05 10:43:47 UTC) #71
commit-bot: I haz the power
Committed patchset #14 (id:260001)
3 years, 11 months ago (2017-01-05 10:48:29 UTC) #74
commit-bot: I haz the power
Patchset 14 (id:??) landed as https://crrev.com/1060f916d9f7026fa22dd3bb6b563294ec7f8224 Cr-Commit-Position: refs/heads/master@{#441620}
3 years, 11 months ago (2017-01-05 10:50:49 UTC) #76
dgrogan
https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp#newcode1742 third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1742: ensureCols(row, pos + 1 + 1); On 2017/01/04 10:56:06, ...
3 years, 3 months ago (2017-09-19 00:47:43 UTC) #78
mstensho (USE GERRIT)
https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp#newcode1742 third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1742: ensureCols(row, pos + 1 + 1); On 2017/09/19 00:47:43, ...
3 years, 3 months ago (2017-09-19 19:10:27 UTC) #79
a.suchit2
3 years, 3 months ago (2017-09-20 04:52:09 UTC) #81
Message was sent while issue was closed.
https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1742:
ensureCols(row, pos + 1 + 1);
On 2017/09/19 00:47:43, dgrogan wrote:
> On 2017/01/04 10:56:06, mstensho (USE GERRIT) wrote:
> > pos + 2, perhaps? :)
> 
> Why is this extra +1 needed? Or, actually, why the ensureCols call at all? The
> .insert call on the following line increases the vector length by 1, which
> should be sufficient?
> 
> I ask because I'm looking at https://crbug.com/761192 , which deals with this
> case:
> 
> <table>
>   <tr>
>     <td colspan=5></td>
>   </tr>
>   <tr>
>     <td></td>
>   </tr>
> </table>
> 
> EnsureCols increases the width of the grid cells vector to 2 then the insert
> increases it to 3, but there should only be 2 effective columns here.
> 
> Morten, how well do you remember this? Suchit is probably going to take a
while
> to see this email :)

EnsureCols is used to ensure that insert would be successful for position (pos +
1).

'pos' represents to position(index).
we are trying to insert new column at (current position + 1) so (pos + 1).
As EnsureCols accept the number of columns so number of columns at (pos + 1)
position would be (pos + 1 + 1).

Note : Before this patch EnsureCols was EnsureCol and EnsureCol was accepting
position but EnsureCols accepts number of Columns.

Powered by Google App Engine
This is Rietveld 408576698