[webkit-reviews] review denied: [Bug 38249] Vertically shrinking flexbox has wrong height when padding is specified : [Attachment 54538] Proposed Patch

[bugzilla-daemon at webkit.org]

Shinichiro Hamaji <hamaji at chromium.org> has denied Yoshiki Hayashi
<yhayashi at google.com>'s request for review:
Bug 38249: Vertically shrinking flexbox has wrong height when padding is
specified
https://bugs.webkit.org/show_bug.cgi?id=38249

Attachment 54538: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=54538&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
Generally, this looks good. Putting r- for some style nitpicks.

LayoutTests/fast/flexbox/child-flexing.html:37
 +    overflow-y: auto;
Some properties are duplicated with verticalOuter. I'd write

div.outer {
  width: 100px;
  height: 100px;
  ...
}

Also, I'm not sure we are setting overflow-y. I think overflow: auto would be
fine.


LayoutTests/fast/flexbox/child-flexing.html:35
 +    box-align: top;
I don't think box-align is necessary to check this behavior?

LayoutTests/fast/flexbox/child-flexing.html:43
 +    background-color: green;
We usually use greens to mean "the test passes". I think it's better to choose
a different color.

LayoutTests/fast/flexbox/child-flexing.html:53
 +    border-left: 90px solid olive;
As the bug summary contains the word "padding", I think it's better to check
paddings as well.

LayoutTests/fast/flexbox/child-flexing.html:124
 +  shouldBe("element.scrollHeight", "100");
I'd add debug("vertically expanding") above this line. In this way we can see
which test is failing easily. Or, we can just change the shouldBe lines like

shouldBe("document.getElementById('expandVertical')", "100");



LayoutTests/fast/flexbox/child-flexing-expected.txt:9
 +  horizontally shrinking
These messages are unnecessary. It may be better to put all HTMLs above
id="console" into a single <div> and remove the div if
window.layoutTestController exists.

Please check printing/script-tests/pageNumerForElementById.js as an example.