[webkit-reviews] review granted: [Bug 76197] [CSS Line Grid] Implement baseline grid alignment : [Attachment 122667] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jan 16 14:55:39 PST 2012
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?
More information about the webkit-reviews
mailing list