[webkit-reviews] review granted: [Bug 130124] [CSS Grid layout] Initial position in span not correctly computed sometimes : [Attachment 226497] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 130124: [CSS Grid layout] Initial position in span not correctly computed
sometimes
https://bugs.webkit.org/show_bug.cgi?id=130124

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=226497&action=review


> Source/WebCore/rendering/RenderGrid.cpp:1035
>      const size_t* firstLineBeforeOppositePosition =
std::lower_bound(gridLines.begin(), gridLines.end(), resolvedOppositePosition);

> -    if (firstLineBeforeOppositePosition != gridLines.end())
> +    if (firstLineBeforeOppositePosition != gridLines.end()) {
> +	   if (*firstLineBeforeOppositePosition > resolvedOppositePosition &&
firstLineBeforeOppositePosition != gridLines.begin())
> +	       --firstLineBeforeOppositePosition;
> +
>	   firstLineBeforeOppositePositionIndex =
firstLineBeforeOppositePosition - gridLines.begin();
> +    }

Awkward. Makes me think we should put this whole thing into a helper function
rather than invoking lower_bound directly in-line.

>
LayoutTests/fast/css-grid-layout/grid-item-named-grid-line-resolution-expected.
txt:26
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS

Not great test output here. Worth thinking in the future about things we can
include in tests to make it clear what we are testing rather than just a lot of
PASS/FAIL. Helpful in many cases, for example, when some tests are failing it
helps you see which ones they are.