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

Issue 1405393002: Crashes when percent calculation of row's height goes very high. (Closed)

Created:
5 years, 2 months ago by a.suchit
Modified:
4 years, 11 months ago
CC:
blink-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Crashes when percent calculation of row's height goes very high. When calculating the percent value of the row becomes negative because percent calculation goes beyond integer limit So height of the row does not increase due to negative value considered as 0. In this case extra rowspanning height remains same and after that no other row present to consume remaining extra row spanning height. So assertion fails. Now, While calculating the percent height of the row, percent would be considered max 100. R=jchaffraix@chromium.org BUG=408066 Committed: https://crrev.com/4a095d9e3d1e01175ef2c757d686a350bf3d88fc Cr-Commit-Position: refs/heads/master@{#370924}

Patch Set 1 #

Total comments: 13

Patch Set 2 : Review comments addressed #

Total comments: 20

Patch Set 3 : Review comments addressed #

Total comments: 6

Patch Set 4 : Rebase expected file #

Patch Set 5 : Review comments addressed #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+33 lines, -6 lines) Patch
A third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html View 1 2 3 4 1 chunk +22 lines, -0 lines 3 comments Download
A third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large-expected.txt View 1 2 3 4 1 chunk +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutTableSection.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutTableSection.cpp View 1 2 3 4 2 chunks +3 lines, -5 lines 2 comments Download

Messages

Total messages: 37 (12 generated)
a.suchit2
Layout test remain to add. I will add on Monday.
5 years, 2 months ago (2015-10-16 14:55:57 UTC) #2
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1405393002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1405393002/1
5 years, 2 months ago (2015-10-16 14:56:41 UTC) #4
Julien - ping for review
Trimming the reviewer list (some are not working on Blink anymore). Please only include only ...
5 years, 2 months ago (2015-10-16 15:39:20 UTC) #7
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 2 months ago (2015-10-16 16:42:02 UTC) #9
mstensho (USE GERRIT)
Can you please proofread and improve the description? The first line ("RowSpan cell's percent height ...
5 years, 2 months ago (2015-10-16 18:19:13 UTC) #10
a.suchit2
Thanks for the review. Please check my comments. https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp#newcode317 third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:317: // ...
5 years, 2 months ago (2015-10-19 10:01:04 UTC) #12
mstensho (USE GERRIT)
https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp#newcode317 third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:317: // those total percent is 100. Other percent rows ...
5 years, 2 months ago (2015-10-19 18:17:47 UTC) #13
a.suchit2
https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp#newcode317 third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:317: // those total percent is 100. Other percent rows ...
5 years, 2 months ago (2015-10-20 05:15:08 UTC) #14
mstensho (USE GERRIT)
https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1405393002/diff/1/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp#newcode317 third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:317: // those total percent is 100. Other percent rows ...
5 years, 2 months ago (2015-10-20 08:12:37 UTC) #15
a.suchit2
https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp#newcode660 third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:660: if (totalPercent < 100 && !totalAutoRowsHeight && !totalRemainingRowsHeight) { ...
5 years, 1 month ago (2015-10-30 01:43:50 UTC) #18
a.suchit2
On 2015/10/30 01:43:50, a.suchit2 wrote: > https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp > File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): > > https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp#newcode660 > ...
5 years, 1 month ago (2015-10-31 11:54:59 UTC) #19
mstensho (USE GERRIT)
https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html File third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html (right): https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html#newcode1 third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:1: <!DOCTYPE html> Expectation file missing. Right now this looks ...
5 years, 1 month ago (2015-11-02 10:46:42 UTC) #20
mstensho (USE GERRIT)
ping
5 years ago (2015-12-04 14:10:59 UTC) #21
a.suchit2
Sorry for too much delay. https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp#newcode307 third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:307: LayoutTableCell* cell, float totalPercent, ...
4 years, 11 months ago (2016-01-12 06:46:35 UTC) #22
a.suchit2
https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html File third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html (right): https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html#newcode1 third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:1: <!DOCTYPE html> On 2015/11/02 10:46:42, mstensho wrote: > Expectation ...
4 years, 11 months ago (2016-01-13 13:37:59 UTC) #23
a.suchit2
On 2016/01/13 13:37:59, a.suchit2 wrote: > https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html > File > third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html > (right): > > ...
4 years, 11 months ago (2016-01-18 07:08:38 UTC) #24
mstensho (USE GERRIT)
https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html File third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html (right): https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html#newcode11 third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:11: <td height="876543210" rowspan="2">This row span cell height is too ...
4 years, 11 months ago (2016-01-18 14:37:03 UTC) #25
a.suchit2
https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html File third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html (right): https://codereview.chromium.org/1405393002/diff/20001/third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html#newcode11 third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-high.html:11: <td height="876543210" rowspan="2">This row span cell height is too ...
4 years, 11 months ago (2016-01-19 13:02:17 UTC) #26
mstensho (USE GERRIT)
lgtm https://codereview.chromium.org/1405393002/diff/80001/third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html File third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html (right): https://codereview.chromium.org/1405393002/diff/80001/third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html#newcode12 third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html:12: <br/>Page is crashing on assression when table row ...
4 years, 11 months ago (2016-01-20 13:00:59 UTC) #27
dgrogan
https://codereview.chromium.org/1405393002/diff/80001/third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html File third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html (right): https://codereview.chromium.org/1405393002/diff/80001/third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html#newcode12 third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html:12: <br/>Page is crashing on assression when table row span ...
4 years, 11 months ago (2016-01-21 01:38:13 UTC) #29
a.suchit2
https://codereview.chromium.org/1405393002/diff/80001/third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html File third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html (right): https://codereview.chromium.org/1405393002/diff/80001/third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html#newcode12 third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html:12: <br/>Page is crashing on assression when table row span ...
4 years, 11 months ago (2016-01-21 07:08:40 UTC) #30
dgrogan
On 2016/01/21 07:08:40, a.suchit2 wrote: > https://codereview.chromium.org/1405393002/diff/80001/third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html > File > third_party/WebKit/LayoutTests/fast/table/table-rowspan-table-height-and-row-precent-height-too-large.html > (right): > > ...
4 years, 11 months ago (2016-01-21 18:18:51 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1405393002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1405393002/80001
4 years, 11 months ago (2016-01-22 06:51:49 UTC) #33
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years, 11 months ago (2016-01-22 07:57:55 UTC) #35
commit-bot: I haz the power
4 years, 11 months ago (2016-01-22 07:59:25 UTC) #37
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/4a095d9e3d1e01175ef2c757d686a350bf3d88fc
Cr-Commit-Position: refs/heads/master@{#370924}

Powered by Google App Engine
This is Rietveld 408576698