Check if length isSpecified before accessing.

Check if length isSpecified before accessing.

When calculating the minimumValueForLength we have to make sure
that the provided value is specified before using.

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

[dsinclair, 2015-05-26 15:17:55]

dsinclair@chromium.org changed reviewers:
+ jchaffraix@chromium.org, mstensho@opera.com, robhogan@chromium.org

[dsinclair, 2015-05-26 15:17:55]

PTAL.

[Julien - ping for review, 2015-05-26 16:06:37]

lgtm

https://codereview.chromium.org/1160463004/diff/20001/LayoutTests/fast/table/...
File LayoutTests/fast/table/tr-min-content-crash-expected.txt (right):

https://codereview.chromium.org/1160463004/diff/20001/LayoutTests/fast/table/...
LayoutTests/fast/table/tr-min-content-crash-expected.txt:1: Test passes if it
does not CRASH in debug.
The output should include what are you testing. I would advise to put also a
link to the bug.

https://codereview.chromium.org/1160463004/diff/20001/LayoutTests/fast/table/...
File LayoutTests/fast/table/tr-min-content-crash.html (right):

https://codereview.chromium.org/1160463004/diff/20001/LayoutTests/fast/table/...
LayoutTests/fast/table/tr-min-content-crash.html:3: .min { height:
-webkit-min-content; }
min-content would also work (and it's not prefixed).

We should add tests for max-content and -webkit-fit-content too.

https://codereview.chromium.org/1160463004/diff/20001/LayoutTests/fast/table/...
LayoutTests/fast/table/tr-min-content-crash.html:12: tr =
document.createElementNS("http://www.w3.org/1999/xhtml", "tr");
No need for NameSpace here, createElement should work.

https://codereview.chromium.org/1160463004/diff/20001/LayoutTests/fast/table/...
LayoutTests/fast/table/tr-min-content-crash.html:13: tr.setAttribute("class",
"min");
tr.style.height = "-webkit-min-content"; (eliminates the style sheet which is
just noise)

https://codereview.chromium.org/1160463004/diff/20001/LayoutTests/fast/table/...
LayoutTests/fast/table/tr-min-content-crash.html:15:
document.documentElement.appendChild(tr);
I really think we should just have the element already in the tree (we can keep
the <style> element in this case) instead of doing an artificial appendChild.

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

https://codereview.chromium.org/1160463004/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutTableSection.cpp:730: m_rowPos[r + 1] =
std::max(m_rowPos[r], 0);
We should probably put a comment about why it's OK to ignore the other values.
The reason is that the row already accounts for the cell's intrinsic logical
height (the other values should never show up here).

[dsinclair, 2015-05-26 17:23:46]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2015-05-26 17:23:46]

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/1160463004/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-05-26 17:24:09]

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

[commit-bot: I haz the power, 2015-05-26 18:36:17]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195919

[dsinclair, 2015-05-27 15:20:07]

https://codereview.chromium.org/1160463004/diff/20001/LayoutTests/fast/table/...
File LayoutTests/fast/table/tr-min-content-crash-expected.txt (right):

https://codereview.chromium.org/1160463004/diff/20001/LayoutTests/fast/table/...
LayoutTests/fast/table/tr-min-content-crash-expected.txt:1: Test passes if it
does not CRASH in debug.
On 2015/05/26 at 16:06:37, Julien Chaffraix - PST wrote:
> The output should include what are you testing. I would advise to put also a
link to the bug.

Done.

https://codereview.chromium.org/1160463004/diff/20001/LayoutTests/fast/table/...
File LayoutTests/fast/table/tr-min-content-crash.html (right):

https://codereview.chromium.org/1160463004/diff/20001/LayoutTests/fast/table/...
LayoutTests/fast/table/tr-min-content-crash.html:3: .min { height:
-webkit-min-content; }
On 2015/05/26 at 16:06:37, Julien Chaffraix - PST wrote:
> min-content would also work (and it's not prefixed).
> 
> We should add tests for max-content and -webkit-fit-content too.


The test does not crash if I use min-content.

https://codereview.chromium.org/1160463004/diff/20001/LayoutTests/fast/table/...
LayoutTests/fast/table/tr-min-content-crash.html:12: tr =
document.createElementNS("http://www.w3.org/1999/xhtml", "tr");
On 2015/05/26 at 16:06:37, Julien Chaffraix - PST wrote:
> No need for NameSpace here, createElement should work.

Done.

https://codereview.chromium.org/1160463004/diff/20001/LayoutTests/fast/table/...
LayoutTests/fast/table/tr-min-content-crash.html:13: tr.setAttribute("class",
"min");
On 2015/05/26 at 16:06:37, Julien Chaffraix - PST wrote:
> tr.style.height = "-webkit-min-content"; (eliminates the style sheet which is
just noise)

Done.

https://codereview.chromium.org/1160463004/diff/20001/LayoutTests/fast/table/...
LayoutTests/fast/table/tr-min-content-crash.html:15:
document.documentElement.appendChild(tr);
On 2015/05/26 at 16:06:37, Julien Chaffraix - PST wrote:
> I really think we should just have the element already in the tree (we can
keep the <style> element in this case) instead of doing an artificial
appendChild.

Done.

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

https://codereview.chromium.org/1160463004/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutTableSection.cpp:730: m_rowPos[r + 1] =
std::max(m_rowPos[r], 0);
On 2015/05/26 at 16:06:37, Julien Chaffraix - PST wrote:
> We should probably put a comment about why it's OK to ignore the other values.
The reason is that the row already accounts for the cell's intrinsic logical
height (the other values should never show up here).

Done.