Handling of percent less than 1 while extra height distribution in spanning rows.

Handling of percent less than 1 while extra height distribution in spanning rows.

In a table, only one row is present with rowspan value 2 and row height is less
than 1 percent.
Percent is handled as integer so it becomes 0 and extra height is not distributed
in any of the row because whole extra height should go to only percent rows.

Now, Converted the required percent unit to LayoutUnit in place of int.

R=jchaffraix@chromium.org

BUG=408066
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193449

[suchit.agrawal, 2014-12-23 13:05:30]

suchit.agrawal@gmail.com changed reviewers:
+ suchit.agrawal@gmail.com

[suchit.agrawal, 2014-12-23 13:05:30]

Please review it. Thanks

[Julien - ping for review, 2015-01-02 10:13:37]

https://codereview.chromium.org/821203003/diff/1/LayoutTests/fast/table/table...
File LayoutTests/fast/table/table-rowspan-height-less-than-one-percent.html
(right):

https://codereview.chromium.org/821203003/diff/1/LayoutTests/fast/table/table...
LayoutTests/fast/table/table-rowspan-height-less-than-one-percent.html:11:
<p>For this test to PASS, it should not crash.</p>
While it's OK to do a crash check, it's not really what you want.

The ASSERT is a *correctness* issue and if it ASSERTs, it means that we don't
compute the size correctly. That means it would be more interesting to do a
check-layout.js test.

https://codereview.chromium.org/821203003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/821203003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderTableSection.cpp:335: int toAdd = (tableHeight *
std::ceil(m_grid[row].logicalHeight.percent()) / 100) - rowsHeight[row -
rowIndex];
It's not a good change. 0.5% is not 1% but after this change, we will treat them
exactly the same :(

We should do a LayoutUnit computation that we snap down to pixels. Ideally we
should do a full LayoutUnit computation and round at paint time but that's a
broader change.

[a.suchit, 2015-01-07 11:17:09]

Sorry for delay ...

https://codereview.chromium.org/821203003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/821203003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderTableSection.cpp:335: int toAdd = (tableHeight *
std::ceil(m_grid[row].logicalHeight.percent()) / 100) - rowsHeight[row -
rowIndex];
On 2015/01/02 10:13:37, Julien Chaffraix - CET wrote:
> It's not a good change. 0.5% is not 1% but after this change, we will treat
them
> exactly the same :(
> 
> We should do a LayoutUnit computation that we snap down to pixels. Ideally we
> should do a full LayoutUnit computation and round at paint time but that's a
> broader change.

Yes you are right but should we start this change ?
OR
I can try change the percent variable and operations in float.

[Julien - ping for review, 2015-01-07 13:30:10]

https://codereview.chromium.org/821203003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/821203003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderTableSection.cpp:335: int toAdd = (tableHeight *
std::ceil(m_grid[row].logicalHeight.percent()) / 100) - rowsHeight[row -
rowIndex];
On 2015/01/07 11:17:09, a.suchit wrote:
> On 2015/01/02 10:13:37, Julien Chaffraix - CET wrote:
> > It's not a good change. 0.5% is not 1% but after this change, we will treat
> them
> > exactly the same :(
> > 
> > We should do a LayoutUnit computation that we snap down to pixels. Ideally
we
> > should do a full LayoutUnit computation and round at paint time but that's a
> > broader change.
> 
> Yes you are right but should we start this change ?
> OR
> I can try change the percent variable and operations in float.

As a rule floats should be used for local computation to prevent bad rounding
(like this bug).  We have some places (line box computations) doing computations
over floats but they are being changed to use LayoutUnits.

If we can use some LayoutUnit and fix the bug, it's the best move. I am not
asking for the broader change, just the minimal amount of LayoutUnit to fix the
logic. If that doesn't work, then the minimal amount of floats would be OK (but
I would like to know why it didn't work as it's important to know what blocking
switching to LayoutUnit).

[a.suchit2, 2015-03-16 13:43:46]

a.suchit@chromium.org changed reviewers:
+ a.suchit@chromium.org

[a.suchit2, 2015-03-16 13:43:46]

Hi Julien, Right now I did not use the Layout unit for fixing this issue. I just
fixed by using float.

LayoutUnit is a big change and fix will take time.

[a.suchit, 2015-03-17 04:55:55]

On 2015/03/16 13:43:46, a.suchit2 wrote:
> Hi Julien, Right now I did not use the Layout unit for fixing this issue. I
just
> fixed by using float.
> 
> LayoutUnit is a big change and fix will take time.

Please review it. Thanks

[Julien - ping for review, 2015-03-17 15:24:54]

On 2015/03/16 at 13:43:46, a.suchit wrote:
> Hi Julien, Right now I did not use the Layout unit for fixing this issue. I
just fixed by using float.
> 
> LayoutUnit is a big change and fix will take time.

Sorry, I should have been clearer in my comment. Line boxes is moving away from
floats and it's a pain to convert it to LayoutUnit so we should *not* introduce
floats in the table code. I am not asking to roll LayoutUnit everywhere but just
replace the use of float with LayoutUnit where you need it in this patch.

[suchit.agrawal, 2015-03-17 16:08:07]

On 2015/03/17 15:24:54, Julien Chaffraix - PST wrote:
> On 2015/03/16 at 13:43:46, a.suchit wrote:
> > Hi Julien, Right now I did not use the Layout unit for fixing this issue. I
> just fixed by using float.
> > 
> > LayoutUnit is a big change and fix will take time.
> 
> Sorry, I should have been clearer in my comment. Line boxes is moving away
from
> floats and it's a pain to convert it to LayoutUnit so we should *not*
introduce
> floats in the table code. I am not asking to roll LayoutUnit everywhere but
just
> replace the use of float with LayoutUnit where you need it in this patch.

Sorry, may be I missed your point.
I will update it tomorrow.

[a.suchit2, 2015-03-20 11:45:07]

On 2015/03/17 16:08:07, suchit.agrawal wrote:
> On 2015/03/17 15:24:54, Julien Chaffraix - PST wrote:
> > On 2015/03/16 at 13:43:46, a.suchit wrote:
> > > Hi Julien, Right now I did not use the Layout unit for fixing this issue.
I
> > just fixed by using float.
> > > 
> > > LayoutUnit is a big change and fix will take time.
> > 
> > Sorry, I should have been clearer in my comment. Line boxes is moving away
> from
> > floats and it's a pain to convert it to LayoutUnit so we should *not*
> introduce
> > floats in the table code. I am not asking to roll LayoutUnit everywhere but
> just
> > replace the use of float with LayoutUnit where you need it in this patch.
> 
> Sorry, may be I missed your point.
> I will update it tomorrow.

Hi Julien, I updated the patch. Please review it. Thanks

[a.suchit, 2015-03-24 04:54:27]

a.suchit@samsung.com changed reviewers:
+ dsinclair@chromium.org, eseidel@chromium.org, esprehn@chromium.org

[Julien - ping for review, 2015-03-26 17:05:15]

LayoutUnit makes no sense and I am sorry for sending you in the wrong direction
(feel free to push back when patches make no sense).

lgtm for patchset 5 (the one with float) that made a lot more sense.

https://codereview.chromium.org/821203003/diff/100001/Source/core/layout/Layo...
File Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/821203003/diff/100001/Source/core/layout/Layo...
Source/core/layout/LayoutTableSection.cpp:333: // Upto 2 digits of rational part
is considered for calculation.
That's a lot of computation, most of which makes little sense.

The comment makes no sense: rowHeight is a LayoutUnit so you're doing a
LayoutUnit multiplication that you cast to int :(

[a.suchit, 2015-04-09 11:47:05]

The CQ bit was checked by a.suchit@samsung.com

[a.suchit, 2015-04-09 11:47:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
jchaffraix@chromium.org
Link to the patchset: https://codereview.chromium.org/821203003/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-09 11:51:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/821203003/140001

[commit-bot: I haz the power, 2015-04-09 15:09:18]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193449

[pdr., 2015-04-10 05:08:08]

On 2015/04/09 at 15:09:18, commit-bot wrote:
> Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193449

The expected test results uploaded in this patch do not match the test results
generated by the rebaseline bot.