Summary: | [CSS Grid Layout] Resolve grid-{end|after} integer against the end|after edge | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> | ||||||
Component: | Layout and Rendering | Assignee: | Julien Chaffraix <jchaffraix> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | allan.jensen, eric, esprehn+autocc, macpherson, menard, ojan.autocc, ojan, tony, webkit.review.bot, xan.lopez | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 103314 | ||||||||
Attachments: |
|
Description
Julien Chaffraix
2013-03-08 13:12:43 PST
Created attachment 192282 [details]
Proposed change 1: Propagate PositionSide information from CSS to do the right resolution in RenderGrid.
Comment on attachment 192282 [details] Proposed change 1: Propagate PositionSide information from CSS to do the right resolution in RenderGrid. View in context: https://bugs.webkit.org/attachment.cgi?id=192282&action=review > Source/WebCore/rendering/RenderGrid.cpp:341 > + const GridPosition& initialPosition = (direction == ForColumns) ? child->style()->gridItemStart() : child->style()->gridItemBefore(); > + const GridPosition& finalPosition = (direction == ForColumns) ? child->style()->gridItemEnd() : child->style()->gridItemAfter(); I think it's a bit weird that we add the side information to GridPosition because when we read the values here, it's clear what side we're talking about. Another way to do this is to make another class (e.g., GridPositionWithSide) and construct it here to pass around. This would avoid using the extra memory in RenderStyle and having to construct the grid position with side information. Anyway, the current code is OK, but it seems redundant to store the direction information in 2 different ways and trying to make sure it stays in sync. > Source/WebCore/rendering/style/GridPosition.h:51 > + GridPosition(GridPositionSide side) explicit Comment on attachment 192282 [details] Proposed change 1: Propagate PositionSide information from CSS to do the right resolution in RenderGrid. View in context: https://bugs.webkit.org/attachment.cgi?id=192282&action=review >> Source/WebCore/rendering/RenderGrid.cpp:341 >> + const GridPosition& finalPosition = (direction == ForColumns) ? child->style()->gridItemEnd() : child->style()->gridItemAfter(); > > I think it's a bit weird that we add the side information to GridPosition because when we read the values here, it's clear what side we're talking about. > > Another way to do this is to make another class (e.g., GridPositionWithSide) and construct it here to pass around. This would avoid using the extra memory in RenderStyle and having to construct the grid position with side information. > > Anyway, the current code is OK, but it seems redundant to store the direction information in 2 different ways and trying to make sure it stays in sync. You have a point, I thought that the side would be required elsewhere but only resolveGridPositionsFromStyle requires it. We are probably better off just passing it to the function explicitly for now. I can always revive something along the lines you said or this patch if the need shows up. Created attachment 192315 [details]
Patch for landing
Comment on attachment 192315 [details] Patch for landing Clearing flags on attachment: 192315 Committed r145297: <http://trac.webkit.org/changeset/145297> All reviewed patches have been landed. Closing bug. |