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

Issue 208263013: Col width is not honored when dynamically updated and it would not make table narrower (Closed)

Created:
6 years, 9 months ago by Gurpreet
Modified:
5 years, 11 months ago
CC:
blink-reviews, mstensho+blink_opera.com, bemjb+rendering_chromium.org, dsinclair, zoltan1, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, pdr., lgombos, gyuyoung-inactive, rwlbuis, inferno
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Col width is not honored when dynamically updated and it would not make table narrower Increasing the table width by increasing the colgroup width was working but decreasing the table width by decreasing the colgroup width is not working. When colgroup width is defined table cell should adjust according to that. On decreasing colgroup width the cells maxPreferredLogicalWidth was still set to the earlier value. Setting the setPreferredLogicalWidthsDirty to true so that cells pref width is calculated again. R=jchaffraix@chromium.org,mstensho@opera.com BUG=340252, 104316 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188289

Patch Set 1 #

Patch Set 2 : Merged https://codereview.chromium.org/208263012/ to this patch. #

Total comments: 1

Patch Set 3 : Reverting to Patch Set 1 #

Total comments: 8

Patch Set 4 : Modified the test cases #

Total comments: 1

Patch Set 5 : Modified test case and the table traversal. #

Total comments: 4

Patch Set 6 : Added condition for table caption. #

Total comments: 1

Patch Set 7 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+258 lines, -1 line) Patch
A LayoutTests/fast/table/resize-table-using-col-height-vertical-writing-mode.html View 1 2 3 4 1 chunk +26 lines, -0 lines 0 comments Download
A LayoutTests/fast/table/resize-table-using-col-height-vertical-writing-mode-expected.txt View 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/fast/table/resize-table-width-using-col-colgroup-span.html View 1 2 3 4 1 chunk +50 lines, -0 lines 0 comments Download
A LayoutTests/fast/table/resize-table-width-using-col-colgroup-span-expected.txt View 1 chunk +10 lines, -0 lines 0 comments Download
A LayoutTests/fast/table/resize-table-width-using-col-span.html View 1 2 3 4 1 chunk +51 lines, -0 lines 0 comments Download
A LayoutTests/fast/table/resize-table-width-using-col-span-expected.txt View 1 chunk +10 lines, -0 lines 0 comments Download
A LayoutTests/fast/table/resize-table-width-using-col-width.html View 1 2 3 4 1 chunk +26 lines, -0 lines 0 comments Download
A LayoutTests/fast/table/resize-table-width-using-col-width-expected.txt View 1 chunk +2 lines, -0 lines 0 comments Download
A LayoutTests/fast/table/resize-table-width-using-multiple-col-span.html View 1 2 3 4 1 chunk +55 lines, -0 lines 0 comments Download
A LayoutTests/fast/table/resize-table-width-using-multiple-col-span-expected.txt View 1 chunk +10 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderTableCol.cpp View 1 2 3 4 5 6 1 chunk +15 lines, -1 line 0 comments Download

Messages

Total messages: 39 (3 generated)
Gurpreet
Hi Julien. Can you please review? Thanks.
6 years, 9 months ago (2014-03-24 08:35:43 UTC) #1
mstensho (USE GERRIT)
Does this fix https://code.google.com/p/chromium/issues/detail?id=104316 ?
6 years, 9 months ago (2014-03-24 08:44:31 UTC) #2
Gurpreet
On 2014/03/24 08:44:31, Morten Stenshorne wrote: > Does this fix https://code.google.com/p/chromium/issues/detail?id=104316 ? Yes it does.
6 years, 9 months ago (2014-03-24 10:21:56 UTC) #3
Julien - ping for review
On 2014/03/24 10:21:56, Gurpreet wrote: > On 2014/03/24 08:44:31, Morten Stenshorne wrote: > > Does ...
6 years, 9 months ago (2014-03-25 00:04:02 UTC) #4
Gurpreet
Hi Julien done with the changes as you suggested. Please have a look. Thanks.
6 years, 9 months ago (2014-03-26 13:30:19 UTC) #5
Julien - ping for review
https://codereview.chromium.org/208263013/diff/20001/Source/core/rendering/RenderTableSection.cpp File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/208263013/diff/20001/Source/core/rendering/RenderTableSection.cpp#newcode1546 Source/core/rendering/RenderTableSection.cpp:1546: m_grid.clear(); As stated in codereview.chromium.org/208263012 (based on the comments ...
6 years, 8 months ago (2014-03-29 01:04:21 UTC) #6
Gurpreet
On 2014/03/29 01:04:21, Julien Chaffraix - PST wrote: > https://codereview.chromium.org/208263013/diff/20001/Source/core/rendering/RenderTableSection.cpp > File Source/core/rendering/RenderTableSection.cpp (right): > ...
6 years, 8 months ago (2014-04-01 14:39:39 UTC) #7
Julien - ping for review
On 2014/04/01 14:39:39, Gurpreet wrote: > On 2014/03/29 01:04:21, Julien Chaffraix - PST wrote: > ...
6 years, 8 months ago (2014-04-03 16:54:49 UTC) #8
Gurpreet
Hi Julien. In RenderTableCol::styleDidChange before setting the preferred logical widths dirty table->recalcSectionsIfNeeded(); is being called ...
6 years, 8 months ago (2014-04-07 11:08:07 UTC) #9
Gurpreet
Hi Julien. In RenderTableCol::styleDidChange before setting the preferred logical widths dirty table->recalcSectionsIfNeeded(); is being called ...
6 years, 7 months ago (2014-04-30 07:58:32 UTC) #10
mstensho (USE GERRIT)
I'm not sure I understand how the crash relates to everything else, because I'm not ...
6 years, 3 months ago (2014-08-27 11:40:01 UTC) #11
Gurpreet
Hi sorry about the late reply. You mean considering patch 1 there is no crash ...
6 years, 2 months ago (2014-10-09 12:18:14 UTC) #12
mstensho (USE GERRIT)
On 2014/10/09 12:18:14, Gurpreet wrote: > Hi sorry about the late reply. You mean considering ...
6 years, 2 months ago (2014-10-10 11:31:14 UTC) #13
Gurpreet
Oh thats good. Hi Julien. What do you suggest then?
6 years, 2 months ago (2014-10-10 12:01:57 UTC) #14
mstensho (USE GERRIT)
Maybe we could finish this? I think the crash fixes should be removed from this ...
5 years, 11 months ago (2015-01-07 10:06:11 UTC) #15
Gurpreet
Will submit new patch within a day or two i.e what was submitted in Patch ...
5 years, 11 months ago (2015-01-07 10:09:56 UTC) #16
mstensho (USE GERRIT)
On 2015/01/07 10:09:56, Gurpreet wrote: > Will submit new patch within a day or two ...
5 years, 11 months ago (2015-01-07 10:19:01 UTC) #17
Gurpreet
Done. Please review.
5 years, 11 months ago (2015-01-09 12:38:03 UTC) #18
mstensho (USE GERRIT)
https://codereview.chromium.org/208263013/diff/80001/LayoutTests/fast/table/resize-table-using-col-height-vertical-writing-mode.html File LayoutTests/fast/table/resize-table-using-col-height-vertical-writing-mode.html (right): https://codereview.chromium.org/208263013/diff/80001/LayoutTests/fast/table/resize-table-using-col-height-vertical-writing-mode.html#newcode2 LayoutTests/fast/table/resize-table-using-col-height-vertical-writing-mode.html:2: <html> Please omit unnecessary HTML, HEAD and BODY tags ...
5 years, 11 months ago (2015-01-09 21:21:36 UTC) #19
Gurpreet
Thanks a lot for the review. I have modified the test cases as per your ...
5 years, 11 months ago (2015-01-12 08:45:48 UTC) #20
mstensho (USE GERRIT)
> RenderTableCol calling recalcSectionsIfNeeded() is actually not required. The > original patch also doesnot have ...
5 years, 11 months ago (2015-01-12 09:05:50 UTC) #21
Gurpreet
Sorry for the delay. Done. Please review.
5 years, 11 months ago (2015-01-12 13:01:04 UTC) #22
Gurpreet
Also tested my earlier patch and recent patch. It was asserting with the test code ...
5 years, 11 months ago (2015-01-12 13:02:29 UTC) #23
mstensho (USE GERRIT)
https://codereview.chromium.org/208263013/diff/120001/Source/core/rendering/RenderTableCol.cpp File Source/core/rendering/RenderTableCol.cpp (right): https://codereview.chromium.org/208263013/diff/120001/Source/core/rendering/RenderTableCol.cpp#newcode67 Source/core/rendering/RenderTableCol.cpp:67: if (child->isRenderTableCol()) Please change to "if (!child->isRenderTableSection())\n continue;"). Need ...
5 years, 11 months ago (2015-01-12 14:00:09 UTC) #24
Gurpreet
https://codereview.chromium.org/208263013/diff/120001/Source/core/rendering/RenderTableCol.cpp File Source/core/rendering/RenderTableCol.cpp (right): https://codereview.chromium.org/208263013/diff/120001/Source/core/rendering/RenderTableCol.cpp#newcode67 Source/core/rendering/RenderTableCol.cpp:67: if (child->isRenderTableCol()) On 2015/01/12 14:00:09, mstensho wrote: > Please ...
5 years, 11 months ago (2015-01-12 14:14:14 UTC) #25
mstensho (USE GERRIT)
https://codereview.chromium.org/208263013/diff/120001/Source/core/rendering/RenderTableCol.cpp File Source/core/rendering/RenderTableCol.cpp (right): https://codereview.chromium.org/208263013/diff/120001/Source/core/rendering/RenderTableCol.cpp#newcode67 Source/core/rendering/RenderTableCol.cpp:67: if (child->isRenderTableCol()) On 2015/01/12 14:14:14, Gurpreet wrote: > On ...
5 years, 11 months ago (2015-01-12 16:07:52 UTC) #26
Gurpreet
Done. Please review. Thanks.
5 years, 11 months ago (2015-01-13 05:09:56 UTC) #27
mstensho (USE GERRIT)
https://codereview.chromium.org/208263013/diff/140001/Source/core/rendering/RenderTableCol.cpp File Source/core/rendering/RenderTableCol.cpp (right): https://codereview.chromium.org/208263013/diff/140001/Source/core/rendering/RenderTableCol.cpp#newcode67 Source/core/rendering/RenderTableCol.cpp:67: if (child->isTableCaption() || !child->isTableSection()) A caption isn't a section, ...
5 years, 11 months ago (2015-01-13 08:34:48 UTC) #28
Gurpreet
Some confusion maybe from my side :( Earlier in review comment https://codereview.chromium.org/208263013/#msg24 you had mentioned ...
5 years, 11 months ago (2015-01-13 08:38:34 UTC) #29
mstensho (USE GERRIT)
On 2015/01/13 08:38:34, Gurpreet wrote: > Some confusion maybe from my side :( > Earlier ...
5 years, 11 months ago (2015-01-13 09:02:30 UTC) #30
Gurpreet
Submitted new patch. You are right. This check "if (!child->isRenderTableSection())" is enough :) and will ...
5 years, 11 months ago (2015-01-13 09:26:58 UTC) #31
mstensho (USE GERRIT)
lgtm
5 years, 11 months ago (2015-01-13 09:38:38 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/208263013/160001
5 years, 11 months ago (2015-01-13 09:43:15 UTC) #34
commit-bot: I haz the power
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/45891)
5 years, 11 months ago (2015-01-13 11:54:04 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/208263013/160001
5 years, 11 months ago (2015-01-13 11:56:35 UTC) #38
commit-bot: I haz the power
5 years, 11 months ago (2015-01-13 12:55:50 UTC) #39
Message was sent while issue was closed.
Committed patchset #7 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188289

Powered by Google App Engine
This is Rietveld 408576698