Add Length::isPercent and use it in tables.

Add Length::isPercent and use it in tables

We could previously crash when a td had a calc height with a percent.

Rename Length::hasPercent -> Length::isPercentOrCalc, which affects many
files.

Our code was confused: it called Length::percent after checking
Length::hasPercent, but Length::hasPercent returns true for calc lengths
which don't have a legitimate percent. This failed a DCHECK in debug but
can cause crashes in release.

The only logic change is in LayoutTableSection.cpp.

BUG=631413
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/9258f7b0502c95dd9cf3cf4756416fc180b42278
Cr-Commit-Position: refs/heads/master@{#416715}

[dgrogan, 2016-08-26 00:27:17]

dgrogan@chromium.org changed reviewers:
+ alancutter@chromium.org

[dgrogan, 2016-08-26 00:27:18]

Hi Alan,

Warning -- naive patch ahead.

We briefly talked about tables and calc lengths at lunch at blinkon but I forgot
the substance.

I'm not looking to make tables obey calc lengths yet, I just want to fix the
crash.

https://codereview.chromium.org/2286543002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/table/integer-overflow-crash.html
(right):

https://codereview.chromium.org/2286543002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/table/integer-overflow-crash.html:5:
html,body,table,tr { height: calc(100% - 1px); }
I haven't dug in yet to understand why calc height on just the tr is
insufficient to trigger the crash.

https://codereview.chromium.org/2286543002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/2286543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:944: else if
(m_grid[r].logicalHeight.isPercent())
This is the only change tested by the layout test.

[alancutter (OOO until 2018), 2016-08-29 03:34:37]

Warning: Naive review ahead.

LGTM, not using a meaningless integer as a percentage when calc() is used has
got to be better than treating calc() like a non-percentage (whether that's
correct or not).

[dgrogan, 2016-08-31 20:07:04]

dgrogan@chromium.org changed reviewers:
+ mstensho@opera.com

[dgrogan, 2016-08-31 20:07:04]

mstensho, could you take a look?

[mstensho (USE GERRIT), 2016-08-31 20:30:33]

I don't really understand how calc() works in Length, and why it should be
treated as a percentage for most purposes (well, I do understand that calc() and
percentages have this thing in common that they are harder to resolve to pixels
than nice and friendly CSS lengths, but...). I guess all I'm saying is that I'm
not a good reviewer for this. :)

Having both isPercent() and hasPercent() seems bad, though.

https://codereview.chromium.org/2286543002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/Length.h (right):

https://codereview.chromium.org/2286543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Length.h:265: bool isPercent() const { return
type() == Percent; }
This is a great name for what it does. But what about hasPercent() now? Looks
confusing to me.

[dgrogan, 2016-08-31 22:03:42]

On 2016/08/31 20:30:33, mstensho wrote:
> I don't really understand how calc() works in Length, and why it should be
> treated as a percentage for most purposes

> (well, I do understand that calc() and
> percentages have this thing in common that they are harder to resolve to
pixels
> than nice and friendly CSS lengths, but...)

I'll let Alan chime in here if he'd like, but I suspect that difficulty is all
there is.

> Having both isPercent() and hasPercent() seems bad, though.

There are a few superficial ways around the confusion this might induce. (1)
Rename hasPercent -> isPercentOrCalc and change all the callers. (2) Don't add
isPercent and change LayoutTableSection to use length.type() == Percent.

Would either of those be satisfying?

[mstensho (USE GERRIT), 2016-09-01 05:56:06]

On 2016/08/31 22:03:42, dgrogan wrote:
> On 2016/08/31 20:30:33, mstensho wrote:
> > I don't really understand how calc() works in Length, and why it should be
> > treated as a percentage for most purposes
> 
> > (well, I do understand that calc() and
> > percentages have this thing in common that they are harder to resolve to
> pixels
> > than nice and friendly CSS lengths, but...)
> 
> I'll let Alan chime in here if he'd like, but I suspect that difficulty is all
> there is.
> 
> > Having both isPercent() and hasPercent() seems bad, though.
> 
> There are a few superficial ways around the confusion this might induce. (1)
> Rename hasPercent -> isPercentOrCalc and change all the callers. (2) Don't add
> isPercent and change LayoutTableSection to use length.type() == Percent.
> 
> Would either of those be satisfying?

Both WFM. #1 sounds nicer, but would obviously require a large patch.

[alancutter (OOO until 2018), 2016-09-02 04:18:11]

Renaming hasPercent() to be more descriptive and distinct from isPercent() SGTM.

[dgrogan, 2016-09-06 17:26:27]

Description was changed from

==========
Add Length::isPercent and use it in tables.

We could previously crash when a td had calc height with a percent.

BUG=631413
==========

to

==========
Add Length::isPercent and use it in tables.

We could previously crash when a td had calc height with a percent.

BUG=631413
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[dgrogan, 2016-09-06 17:41:49]

Description was changed from

==========
Add Length::isPercent and use it in tables.

We could previously crash when a td had calc height with a percent.

BUG=631413
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Add Length::isPercent and use it in tables

We could previously crash when a td had a calc height with a percent.

Rename Length::hasPercent -> Length::isPercentOrCalc, which affects many
files.

Our code was confused: it called Length::percent after checking
Length::hasPercent, but Length::hasPercent returns true for calc lengths
which don't have a legitimate percent. This failed a DCHECK in debug but
can cause crashes in release.

The only logic change is in LayoutTableSection.cpp.

BUG=631413
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[dgrogan, 2016-09-06 17:43:55]

dgrogan@chromium.org changed reviewers:
+ eae@chromium.org

[dgrogan, 2016-09-06 17:43:56]

eae, could you take advantage of your extensive OWNER reach and review this?
It's actually trivial but the rename touches a lot of files.

[eae, 2016-09-06 17:49:14]

LGTM

[dgrogan, 2016-09-06 17:50:54]

The CQ bit was checked by dgrogan@chromium.org

[dgrogan, 2016-09-06 17:50:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2286543002/#ps40001
(title: "rename hasPercent -> isPercentOrCalc")

[eae, 2016-09-06 17:51:11]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2016-09-06 17:51:44]

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-09-06 17:55:18]

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

[commit-bot: I haz the power, 2016-09-06 17:55:19]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dgrogan, 2016-09-06 18:11:34]

The CQ bit was checked by dgrogan@chromium.org

[dgrogan, 2016-09-06 18:11:35]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org,
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2286543002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-06 18:12:07]

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-09-06 20:43:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-06 20:46:32]

Description was changed from

==========
Add Length::isPercent and use it in tables

We could previously crash when a td had a calc height with a percent.

Rename Length::hasPercent -> Length::isPercentOrCalc, which affects many
files.

Our code was confused: it called Length::percent after checking
Length::hasPercent, but Length::hasPercent returns true for calc lengths
which don't have a legitimate percent. This failed a DCHECK in debug but
can cause crashes in release.

The only logic change is in LayoutTableSection.cpp.

BUG=631413
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Add Length::isPercent and use it in tables

We could previously crash when a td had a calc height with a percent.

Rename Length::hasPercent -> Length::isPercentOrCalc, which affects many
files.

Our code was confused: it called Length::percent after checking
Length::hasPercent, but Length::hasPercent returns true for calc lengths
which don't have a legitimate percent. This failed a DCHECK in debug but
can cause crashes in release.

The only logic change is in LayoutTableSection.cpp.

BUG=631413
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/9258f7b0502c95dd9cf3cf4756416fc180b42278
Cr-Commit-Position: refs/heads/master@{#416715}
==========

[commit-bot: I haz the power, 2016-09-06 20:46:33]

Patchset 4 (id:??) landed as
https://crrev.com/9258f7b0502c95dd9cf3cf4756416fc180b42278
Cr-Commit-Position: refs/heads/master@{#416715}