[webkit-reviews] review denied: [Bug 109994] Convert buttons from DeprecatedFlexBox to nondeprecated FlexibleBox : [Attachment 188685] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Christian Biesinger
<cbiesinger at chromium.org>'s request for review:
Bug 109994: Convert buttons from DeprecatedFlexBox to nondeprecated FlexibleBox
https://bugs.webkit.org/show_bug.cgi?id=109994

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=188685&action=review


Looks great overall. Just a few trivial changes to the code. The new test
baselines need a bit of explanation though (i.e. it's not obvious to my why
some of the new results are correct). The collapsed button cases are fine since
that matches Gecko, but the others are less clear to me. I can believe they're
correct, it just needs an explanation of why they're changing in the ChangeLog.


> Source/WebCore/rendering/RenderBlock.cpp:7896
>      if (display == BOX || display == INLINE_BOX) {
>	   newBox =
RenderDeprecatedFlexibleBox::createAnonymous(parent->document());
>	   newDisplay = BOX;

Can you add a FIXME to get rid of this part once we get rid of all internal
RenderDeprecatedFlexibleBox uses? There should never be a future case where we
want to create an anonymous deprecated flexbox.

> Source/WebCore/rendering/RenderButton.cpp:-81
>	   // RenderBlock::setStyle is going to apply a new style to the inner
block, which
> -	   // will have the initial box flex value, 0. The current value is 1,
because we set
> +	   // will have the initial flex value, 0. The current value is 1,
because we set
>	   // it right below. Here we change it back to 0 to avoid getting a
spurious layout hint
> -	   // because of the difference.
> -	   m_inner->style()->setBoxFlex(0);

Ugh. Not your fault, but we really should fix this. This is a gross hack. Maybe
add a FIXME while you're here?

> Source/WebCore/rendering/RenderButton.cpp:84
> +	   m_inner->style()->setFlexGrow(0);
> +	   m_inner->style()->setMinWidth(Length());
> +	   m_inner->style()->setMarginTop(Length(0, Fixed));
> +	   m_inner->style()->setMarginBottom(Length(0, Fixed));

Better to use initialValue() off of m_inner->style() or newStyle to avoid code
duplication, not that the initialValue of these could realistically change. It
almost makes it more clear what's going on.

> Source/WebCore/rendering/RenderButton.cpp:116
> +    // min-width: 0; is needed for correct shrinking
> +    innerStyle->setMinWidth(Length(0, Fixed));

Maybe add a FIXME to remove this (and the corresponding line in
styleWillChange) once we change the min-width of flex-items back to 0 since the
spec seems like it's going to be changing.

> Source/WebCore/rendering/RenderButton.cpp:118
> +    // we use margin: auto for correct centering. align-items: center
> +    // does not produce correct results when the content overflows.

Instead of "correct", which is kind of vague, can you describe the actual
behavior, e.g.,
// Use margin:auto instead of align-items:center to get safe centering, i.e.
// when the content overflows, treat it the same as align-items: flex-start.

> LayoutTests/ChangeLog:26
> +	   * fast/forms/button-generated-content-expected.txt:
> +	   *
platform/chromium-linux/css2.1/20110323/replaced-elements-001-expected.png:
> +	   *
platform/chromium-linux/css2.1/20110323/replaced-elements-001-expected.txt:
> +	   *
platform/chromium-linux/fast/forms/button-generated-content-expected.png:
> +	   *
platform/chromium-linux/fast/forms/button-generated-content-expected.txt:
> +	   * platform/chromium-linux/fast/forms/select-baseline-expected.png:
> +	   * platform/chromium-linux/fast/forms/select-baseline-expected.txt:
> +	   *
platform/chromium-linux/svg/custom/foreign-object-skew-expected.png:
> +	   *
platform/chromium-linux/svg/custom/foreign-object-skew-expected.txt:
> +	   *
platform/chromium-linux/tables/mozilla/bugs/bug92647-2-expected.png:
> +	   *
platform/chromium-linux/tables/mozilla/bugs/bug92647-2-expected.txt:
> +	   * platform/mac/css2.1/20110323/replaced-elements-001-expected.txt:
> +	   * platform/mac/fast/forms/select-baseline-expected.txt:
> +	   * platform/mac/svg/custom/foreign-object-skew-expected.txt:

Please add comments explaining why the results are different. For example, for
button-generated-content, the collapsed buttons are no longer centered, but
this matches Firefox behavior. Also, all the buttons seem to be 2px shorter. Is
that correct?

> LayoutTests/ChangeLog:29
> +	   have the height of the line. See the testcase in the bug.

"the bug" is a bit confusing. Best to just link to the bug even though it'a
already in the description title. Or, even better, just link to the test case
itself.

Also, please add that the new rendering matches Firefox.