[webkit-reviews] review granted: [Bug 76197] [CSS Line Grid] Implement baseline grid alignment : [Attachment 122667] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Dave Hyatt
<hyatt at apple.com>'s request for review:
Bug 76197: [CSS Line Grid] Implement baseline grid alignment
https://bugs.webkit.org/show_bug.cgi?id=76197

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=122667&action=review


> Source/WebCore/rendering/LayoutState.cpp:234
> +	   for (LayoutState* currentState = m_next; currentState; currentState
= currentState->m_next) {
> +	       if (currentState->m_currentLineGrid == currentGrid)
> +		   continue;
> +	       currentGrid = currentState->m_currentLineGrid;
> +	       if (currentGrid->style()->lineGrid() ==
block->style()->lineGrid()) {
> +		   m_currentLineGrid = currentGrid;
> +		   m_currentLineGridOffset =
currentState->m_currentLineGridOffset;
> +		   return;

Would this make sense in its own findEnclosingLineGrid() method, for
readability?

> Source/WebCore/rendering/LayoutState.h:123
> +    // The current line grid that we're snapping to and the offset of the
start of the grid.
> +    RenderBlock* m_currentLineGrid;
> +    LayoutSize m_currentLineGridOffset;

LayoutState was originally added just as an optimization (and, I think it still
serves that purpose?). This new code makes LayoutState something that alters
how layout behaves, which I don't really like. I'd prefer that the "current
grid" stuff be tracked outside of LayoutState.

Maybe this ship already sailed with isPaginated(). What if something inside a
transform (where LayoutState is disabled) establishes a local line grid?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2785
> +    // FIXME: If any of the characteristics of the box change compared to
the old one, then we need to do a deep dirtying
> +    // (similar to what happens when the page height changes). Ideally,
though, we only do this if someone is actually snapping
> +    // to this grid.

File a bug to track this?

> LayoutTests/ChangeLog:16
> +	   * platform/mac/fast/line-grid/line-grid-inside-columns-expected.png:
Added.

This result needs to be regenerated. We have mock scrollbars on now.

> LayoutTests/fast/line-grid/line-grid-floating.html:7
> +.grid { -webkit-line-grid: simple; -webkit-line-grid-snap: baseline; 
> +	   font-size:36px; float:left;border:2px solid black;
> +	   padding:10px; margin:5px }

Can you unwrap these rules pls?