Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1220043002:
[CSS Grid Layout] Removing Content Alignment logic from track sizing alg (Closed)
|
Created:
5 years, 5 months ago by jfernandez Modified:
5 years, 5 months ago CC:
blink-reviews, jfernandez, szager+layoutwatch_chromium.org, zoltan1, svillar, pdr+renderingwatchlist_chromium.org, Manuel Rego, leviw+renderwatch, blink-reviews-rendering, jchaffraix+rendering, eae+blinkwatch Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
Description[CSS Grid Layout] Removing Content Alignment logic from track sizing alg
We had implemented track stretching inside the track sizing algorithm,
which could interfere in some cases with sizing phase. Specs states that
any alignment logic must be run after the track sizing algorithm is
completed.
This patch also adds a validity indicator for the cached tracks positions,
since some content distribution values may have changed tracks sizes,
hence their positions.
BUG=249451, 376823
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198496
Patch Set 1 #
Total comments: 32
Patch Set 2 : Applied some of the suggested changes. #Patch Set 3 : Applied additional suggested changes. #
Total comments: 8
Patch Set 4 : Applied new suggested changes. #Messages
Total messages: 14 (3 generated)
jfernandez@igalia.com changed reviewers: + jchaffraix+rendering@chromium.org, svillar@igalia.com
https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... File Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:130: LayoutUnit distributionOffset; missing "m_" https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:267: rowTracksPositionsCacheIsValid = isValid; Can be a single line. Also it's called just once so I don't think we need it (I don't like the GridSizingData growing too much) https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1283: autoSizedTracksIndex.append(i); Sizing data already has the indexes of the content sized tracks. Why do you need this? Ah perhaps it's because the new value of auto, I think we can use the current vector of indexes, add here a FIXME, and leave it for a follow-up patch (adding support for auto will likely force us to modify the current vector of indexes). https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1296: // FIXME: Respecting the constraints imposed by max-height/max-width. Not sure what you mean here. Which max-height/max-width? https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1463: LayoutUnit LayoutGrid::gridAreaBreadthForChild(const LayoutBox& child, GridTrackSizingDirection direction, const Vector<GridTrack>& tracks) const Is this version still used after your patch? https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1497: m_columnPositions[numberOfColumnTracks] = m_columnPositions[numberOfColumnTracks - 1] + sizingData.columnTracks[numberOfColumnTracks - 1].baseSize(); Why this change in the loop? https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1504: m_rowPositions[numberOfRowTracks] = m_rowPositions[numberOfRowTracks - 1] + sizingData.rowTracks[numberOfRowTracks - 1].baseSize(); Why this change in the loop? https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1824: } Why are you allocating all those objects in the stack? I think I prefer you to pass a reference to a ContentAlingment object as parametter and return a boolean as you've been doing. https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1919: } Not sure that I understand this, perhaps it's because of the names rightEdgePosition -> gridRightEdgePosition leftEdgePosition -> gridLeftEdgePosition columnPosition -> childColumnPosition or something similar that could help reading the code. https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... File Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.h:39: struct ContentAlignmentData; Move this above with the rest.
Thanks for the detailed review. See my replies below. https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... File Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:130: LayoutUnit distributionOffset; On 2015/07/01 16:22:55, svillar wrote: > missing "m_" I've been told that when fields are accessible directly 'm_' prefix should be avoided, which is the case of structs. Anyway, let's add it since other strutcs of in this class already do it. https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:267: rowTracksPositionsCacheIsValid = isValid; On 2015/07/01 16:22:55, svillar wrote: > Can be a single line. Also it's called just once so I don't think we need it (I > don't like the GridSizingData growing too much) Well, we might be overprotecting our code. The reason for this is that we need to use track positions to compute the grid item's area in certain cases. For instance, whe item spans across several tracks and content alignment implies adding an offset between those tracks. We were computing grid item's area based on the base size of tracks it's laid of on, but this is not correct if there is an offset between the positions of those tracks. Hence we need to compute grid item's area based in the position of the tracks, instead of their base size. It's can be also seen as a performance improvement, since we don't have to iterate over the tracks the item is spanning across. We just need start and end tracks, and this works well too in the regular cases, no alignment and no span. The only concern, as you already mentioned, is that we might access tracks position cache before (or inside) the track sizing algorithm has completely defined base sized of all tracks, which obviously are used to determine their positions. We still need the old way of getting grid item's area precisely because of this, to be used inside the track sizing algorithm. However, as I said before, we don't use this logic that much, and it's quite simply, so perhaps we don't need such control over the tracks positions cache. https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1283: autoSizedTracksIndex.append(i); On 2015/07/01 16:22:55, svillar wrote: > Sizing data already has the indexes of the content sized tracks. Why do you need > this? > The reason is that SizingData structure uses the content-sized indexes vector as a temporary data structure to run the track sizing logic. It's reset every time its called to determine tracks sizes in a particular dimension (columns, rows). We don't store that info, neither flex-sized track indexes, for both dimensions. We can tweak the implementation calling the stretching logic right after computing tracks sizes in the same dimension; we could also integrate the content alignment logic inside the sizing algorithm, but at the end so it doesn't interfere with the sizing logic. however, I think it's much better, and spec compliant, to run any alignment code after all track sizing logic has been already executed. So, we need either to determine again the auto-sized tracks, for both dimensions, or store them inside the GridSizing structure. You commented before that you don' t like the idea of growing this data structure too much, so let's think about it. https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1296: // FIXME: Respecting the constraints imposed by max-height/max-width. On 2015/07/01 16:22:55, svillar wrote: > Not sure what you mean here. Which max-height/max-width? Well, Alignment spec states that stretching should respect "constraints imposed by max-height/max-width", which it's not so clear when we apply it to grid tracks. Somehow, we must limit the final stretching breadth to these constraints. I already asked to spec editors about this issue, so it's just a matter of wait and interpret their answer. https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1463: LayoutUnit LayoutGrid::gridAreaBreadthForChild(const LayoutBox& child, GridTrackSizingDirection direction, const Vector<GridTrack>& tracks) const On 2015/07/01 16:22:55, svillar wrote: > Is this version still used after your patch? Yes it is. As I commented before, we need something to use during the track sizing algorithm, before tracks positions are not yet determined. https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1497: m_columnPositions[numberOfColumnTracks] = m_columnPositions[numberOfColumnTracks - 1] + sizingData.columnTracks[numberOfColumnTracks - 1].baseSize(); On 2015/07/01 16:22:55, svillar wrote: > Why this change in the loop? Well, since I had to change this function because of the new ContentAligmentData structure w use now, I took the opportunity to fix a "bug", which while not causing any functional issue it makes more complicated to determine grid items position in case of RTL direction. This is also related to the change in the the findChildLogicalPosition function. The issue is that track position cache store, actually, the position of the grid lines defining those tracks. Without any distribution offset between tracks, end line of track X is the same than start line of track X+1. Once we add an offset this is not true (virtually) anymore, so I assumed what we store in tracks positions cache is the position of the start line of each track, plus the end line of the last one. It's precisely this last one line that we should not add an additional distribution offset,m since the difference between two adjacent grid lines must be just the track base size. Adding that offset does not cause any issue with grid layout because we don't use that last grid line in any computation, but when flipping positions for RTL direction. I had to do some math trick to compensate that mistake, which now isn't necessary. https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1504: m_rowPositions[numberOfRowTracks] = m_rowPositions[numberOfRowTracks - 1] + sizingData.rowTracks[numberOfRowTracks - 1].baseSize(); On 2015/07/01 16:22:55, svillar wrote: > Why this change in the loop? Same as above comment. https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1824: } On 2015/07/01 16:22:55, svillar wrote: > Why are you allocating all those objects in the stack? > > I think I prefer you to pass a reference to a ContentAlingment object as > parametter and return a boolean as you've been doing. Yes, that was my first idea. The issue is that I need to define an invalid state for the ContentAlignmentData structure. In my first implementation I defined such invalid state as -1 on both offset values. Some WebKit reviewer told me that using a magical number for defining an invalid state was not a good idea, so I thought nullptr could express that state much better. I'd be happy to get rid of the pointer allocation, but for that we should agree on a better way to define the invalid state. https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1919: } On 2015/07/01 16:22:55, svillar wrote: > Not sure that I understand this, perhaps it's because of the names > > rightEdgePosition -> gridRightEdgePosition > leftEdgePosition -> gridLeftEdgePosition > columnPosition -> childColumnPosition > > or something similar that could help reading the code. Are the names changes enough to make the code understandable ? The idea behind this logic is to flip column track positions so first column track will have the highest value, and viceversa, when grid container uses RTL direction. Using logical values, as it inline-flow rightest edge would start at 0px, makes our grid computation easier, but for perhaps, we need to adapt locations to become absolute positions, which will be then interpreted by painting logic according to flow direction. https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... File Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.h:39: struct ContentAlignmentData; On 2015/07/01 16:22:55, svillar wrote: > Move this above with the rest. Done.
https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... File Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:267: rowTracksPositionsCacheIsValid = isValid; You didn't understand me, I was not talking about the cache but the need of that method. On 2015/07/02 10:04:21, jfernandez wrote: > On 2015/07/01 16:22:55, svillar wrote: > > Can be a single line. Also it's called just once so I don't think we need it > (I > > don't like the GridSizingData growing too much) > > Well, we might be overprotecting our code. The reason for this is that we need > to use track positions to compute the grid item's area in certain cases. For > instance, whe item spans across several tracks and content alignment implies > adding an offset between those tracks. We were computing grid item's area based > on the base size of tracks it's laid of on, but this is not correct if there is > an offset between the positions of those tracks. > > Hence we need to compute grid item's area based in the position of the tracks, > instead of their base size. It's can be also seen as a performance improvement, > since we don't have to iterate over the tracks the item is spanning across. We > just need start and end tracks, and this works well too in the regular cases, no > alignment and no span. > > The only concern, as you already mentioned, is that we might access tracks > position cache before (or inside) the track sizing algorithm has completely > defined base sized of all tracks, which obviously are used to determine their > positions. We still need the old way of getting grid item's area precisely > because of this, to be used inside the track sizing algorithm. > > However, as I said before, we don't use this logic that much, and it's quite > simply, so perhaps we don't need such control over the tracks positions cache. https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1296: // FIXME: Respecting the constraints imposed by max-height/max-width. The min and max in tracks are the min and max track sizing functions. On 2015/07/02 10:04:22, jfernandez wrote: > On 2015/07/01 16:22:55, svillar wrote: > > Not sure what you mean here. Which max-height/max-width? > > Well, Alignment spec states that stretching should respect "constraints imposed > by max-height/max-width", which it's not so clear when we apply it to grid > tracks. Somehow, we must limit the final stretching breadth to these > constraints. > > I already asked to spec editors about this issue, so it's just a matter of wait > and interpret their answer. https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1497: m_columnPositions[numberOfColumnTracks] = m_columnPositions[numberOfColumnTracks - 1] + sizingData.columnTracks[numberOfColumnTracks - 1].baseSize(); I think this likely deserves a separate patch + test. Also I think we aren't doing the right thing here. So if I am not wrong both m_rowPositions and m_columnPositions store the positions of the track lines without taking into account the alignment. That's a mistake and I think we should fix that because those vectors are used to generate the computed style for example which won't have the corrent values. Or am I wrong? On 2015/07/02 10:04:22, jfernandez wrote: > On 2015/07/01 16:22:55, svillar wrote: > > Why this change in the loop? > > Well, since I had to change this function because of the new ContentAligmentData > structure w use now, I took the opportunity to fix a "bug", which while not > causing any functional issue it makes more complicated to determine grid items > position in case of RTL direction. This is also related to the change in the the > findChildLogicalPosition function. > > The issue is that track position cache store, actually, the position of the grid > lines defining those tracks. Without any distribution offset between tracks, end > line of track X is the same than start line of track X+1. Once we add an offset > this is not true (virtually) anymore, so I assumed what we store in tracks > positions cache is the position of the start line of each track, plus the end > line of the last one. It's precisely this last one line that we should not add > an additional distribution offset,m since the difference between two adjacent > grid lines must be just the track base size. > > Adding that offset does not cause any issue with grid layout because we don't > use that last grid line in any computation, but when flipping positions for RTL > direction. I had to do some math trick to compensate that mistake, which now > isn't necessary. https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1824: } On 2015/07/02 10:04:22, jfernandez wrote: > On 2015/07/01 16:22:55, svillar wrote: > > Why are you allocating all those objects in the stack? > > > > I think I prefer you to pass a reference to a ContentAlingment object as > > parametter and return a boolean as you've been doing. > > Yes, that was my first idea. The issue is that I need to define an invalid state > for the ContentAlignmentData structure. In my first implementation I defined > such invalid state as -1 on both offset values. Some WebKit reviewer told me > that using a magical number for defining an invalid state was not a good idea, > so I thought nullptr could express that state much better. I'd be happy to get > rid of the pointer allocation, but for that we should agree on a better way to > define the invalid state. You mean is not enough with returning false but you have to also store an invalid state somewhere else?
https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... File Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:267: rowTracksPositionsCacheIsValid = isValid; On 2015/07/02 11:19:28, svillar wrote: > You didn't understand me, I was not talking about the cache but the need of that > method. > > On 2015/07/02 10:04:21, jfernandez wrote: > > On 2015/07/01 16:22:55, svillar wrote: > > > Can be a single line. Also it's called just once so I don't think we need it > > (I > > > don't like the GridSizingData growing too much) > > > > Well, we might be overprotecting our code. The reason for this is that we need > > to use track positions to compute the grid item's area in certain cases. For > > instance, whe item spans across several tracks and content alignment implies > > adding an offset between those tracks. We were computing grid item's area > based > > on the base size of tracks it's laid of on, but this is not correct if there > is > > an offset between the positions of those tracks. > > > > Hence we need to compute grid item's area based in the position of the tracks, > > instead of their base size. It's can be also seen as a performance > improvement, > > since we don't have to iterate over the tracks the item is spanning across. > We > > just need start and end tracks, and this works well too in the regular cases, > no > > alignment and no span. > > > > The only concern, as you already mentioned, is that we might access tracks > > position cache before (or inside) the track sizing algorithm has completely > > defined base sized of all tracks, which obviously are used to determine their > > positions. We still need the old way of getting grid item's area precisely > > because of this, to be used inside the track sizing algorithm. > > > > However, as I said before, we don't use this logic that much, and it's quite > > simply, so perhaps we don't need such control over the tracks positions > cache. > Done. https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1296: // FIXME: Respecting the constraints imposed by max-height/max-width. On 2015/07/02 11:19:28, svillar wrote: > The min and max in tracks are the min and max track sizing functions. > > On 2015/07/02 10:04:22, jfernandez wrote: > > On 2015/07/01 16:22:55, svillar wrote: > > > Not sure what you mean here. Which max-height/max-width? > > > > Well, Alignment spec states that stretching should respect "constraints > imposed > > by max-height/max-width", which it's not so clear when we apply it to grid > > tracks. Somehow, we must limit the final stretching breadth to these > > constraints. > > > > I already asked to spec editors about this issue, so it's just a matter of > wait > > and interpret their answer. > S Stretching can be applied only for 'auto-sized" alignment subjects, in this case, grid tracks. We have assumed (to be confirmed by editors) that those auto-sized are the ones using any content-sizing function: min-content, max-content, auto, ... It's also possible to use minmax function, I guess using content-sized limits; otherwise I guess it wouldn't fit in the "auto-sized" category. However, if minmax function are used to define track sizes and at the same time we use them as max-height/max-width restrictions, we never be able to stretch tracks over its original size, as it's the purpose of this alignment value. I might be missing something, or misunderstanding, but I think this point needs further clarification. https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1497: m_columnPositions[numberOfColumnTracks] = m_columnPositions[numberOfColumnTracks - 1] + sizingData.columnTracks[numberOfColumnTracks - 1].baseSize(); On 2015/07/02 11:19:28, svillar wrote: > I think this likely deserves a separate patch + test. > I can do a new patch for this, not sure about the test because we are not changing anything in terms of functionality. It just simplifies code dealing with RTL in findChildLogicalPosition. > Also I think we aren't doing the right thing here. So if I am not wrong both > m_rowPositions and m_columnPositions store the positions of the track lines > without taking into account the alignment. No, they indeed take into account alignment, of course. That's why we are adding columnOffset/rowOffsets. That's a mistake and I think we > should fix that because those vectors are used to generate the computed style > for example which won't have the corrent values. Or am I wrong? That's not the case; we are not using those vectors to generate computed values. Computed values are just the base size of the tracks, not their positions. > > On 2015/07/02 10:04:22, jfernandez wrote: > > On 2015/07/01 16:22:55, svillar wrote: > > > Why this change in the loop? > > > > Well, since I had to change this function because of the new > ContentAligmentData > > structure w use now, I took the opportunity to fix a "bug", which while not > > causing any functional issue it makes more complicated to determine grid items > > position in case of RTL direction. This is also related to the change in the > the > > findChildLogicalPosition function. > > > > The issue is that track position cache store, actually, the position of the > grid > > lines defining those tracks. Without any distribution offset between tracks, > end > > line of track X is the same than start line of track X+1. Once we add an > offset > > this is not true (virtually) anymore, so I assumed what we store in tracks > > positions cache is the position of the start line of each track, plus the end > > line of the last one. It's precisely this last one line that we should not add > > an additional distribution offset,m since the difference between two adjacent > > grid lines must be just the track base size. > > > > Adding that offset does not cause any issue with grid layout because we don't > > use that last grid line in any computation, but when flipping positions for > RTL > > direction. I had to do some math trick to compensate that mistake, which now > > isn't necessary. > https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1824: } On 2015/07/02 11:19:28, svillar wrote: > On 2015/07/02 10:04:22, jfernandez wrote: > > On 2015/07/01 16:22:55, svillar wrote: > > > Why are you allocating all those objects in the stack? > > > > > > I think I prefer you to pass a reference to a ContentAlingment object as > > > parametter and return a boolean as you've been doing. > > > > Yes, that was my first idea. The issue is that I need to define an invalid > state > > for the ContentAlignmentData structure. In my first implementation I defined > > such invalid state as -1 on both offset values. Some WebKit reviewer told me > > that using a magical number for defining an invalid state was not a good idea, > > so I thought nullptr could express that state much better. I'd be happy to get > > rid of the pointer allocation, but for that we should agree on a better way to > > define the invalid state. > > You mean is not enough with returning false but you have to also store an > invalid state somewhere else? Well, the function returns now data structure, instead of using inout arguments. I think is clearer and more flexible to use the data structure. What I mean, if we agree don this, is that I need such data structure to be validated somehow, I guess based on the data it holds.
Applied some additional changes, as we discussed.
Forgot to submit a couple of drafts although I've already talked to jfernandez about them https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... File Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:130: LayoutUnit distributionOffset; On 2015/07/02 10:04:22, jfernandez wrote: > On 2015/07/01 16:22:55, svillar wrote: > > missing "m_" > > I've been told that when fields are accessible directly 'm_' prefix should be > avoided, which is the case of structs. Anyway, let's add it since other strutcs > of in this class already do it. The idea in general is not to allow public accessors. https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1281: ASSERT(!trackSize.maxTrackBreadth().isFlex()); I don't get this ASSERT. If the max function is flex then it should still be flex here, the functions do not change.
https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... File Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:130: LayoutUnit distributionOffset; On 2015/07/07 18:32:54, svillar wrote: > On 2015/07/02 10:04:22, jfernandez wrote: > > On 2015/07/01 16:22:55, svillar wrote: > > > missing "m_" > > > > I've been told that when fields are accessible directly 'm_' prefix should be > > avoided, which is the case of structs. Anyway, let's add it since other > strutcs > > of in this class already do it. > > The idea in general is not to allow public accessors. Yeah, but when using structs it's all assumed as public, so fields are directly accessed. GridSizingData structure follows the same approach. https://codereview.chromium.org/1220043002/diff/1/Source/core/layout/LayoutGr... Source/core/layout/LayoutGrid.cpp:1281: ASSERT(!trackSize.maxTrackBreadth().isFlex()); On 2015/07/07 18:32:54, svillar wrote: > I don't get this ASSERT. If the max function is flex then it should still be > flex here, the functions do not change. We shouldn't reach this point if there are flex tracks because they would exhaust all free available space in the container.
lgtm Please consider the changes I suggest before landing. https://codereview.chromium.org/1220043002/diff/40001/Source/core/layout/Layo... File Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1220043002/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutGrid.cpp:1435: LayoutUnit LayoutGrid::gridAreaBreadthForChildIncludingAlignmentOffsets(const LayoutBox& child, GridTrackSizingDirection direction, const GridSizingData& sizingData) const Much better name indeed https://codereview.chromium.org/1220043002/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutGrid.cpp:1878: return {0, 0}; What do you think about merging these 2 methods in a single one that receives the direction? They seem quite similar and I think we could get a clean refactoring by doing bool isForRows = direction == ForRows; ... case ContentPositionRight: return isForRows ? {0, 0} : {availableFreeSpace, 0}; etc... https://codereview.chromium.org/1220043002/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutGrid.cpp:1889: columnPosition = rightGridEdgePosition + alignmentOffset - columnPosition - child.logicalWidth(); Nit: I prefer rightGridEdgePosition - columnPosition - child.logicalWidth() to be together as this is the reversal of the column position. Actually we might even use parens to make it more explicit. Also let's change also columnPosition's name because it's a very bad one. We need to make explicit that it's the grid item offset in the row axis. I'll let you chose the name. https://codereview.chromium.org/1220043002/diff/40001/Source/core/layout/Layo... File Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/1220043002/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutGrid.h:40: Careful, extra line.
The CQ bit was checked by jfernandez@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from svillar@igalia.com Link to the patchset: https://codereview.chromium.org/1220043002/#ps60001 (title: "Applied new suggested changes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220043002/60001
Truing the CQ after applying the final suggested changes. https://codereview.chromium.org/1220043002/diff/40001/Source/core/layout/Layo... File Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/1220043002/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutGrid.cpp:1435: LayoutUnit LayoutGrid::gridAreaBreadthForChildIncludingAlignmentOffsets(const LayoutBox& child, GridTrackSizingDirection direction, const GridSizingData& sizingData) const On 2015/07/08 07:14:08, svillar wrote: > Much better name indeed Done. https://codereview.chromium.org/1220043002/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutGrid.cpp:1878: return {0, 0}; On 2015/07/08 07:14:08, svillar wrote: > What do you think about merging these 2 methods in a single one that receives > the direction? They seem quite similar and I think we could get a clean > refactoring by doing > > bool isForRows = direction == ForRows; > > ... > case ContentPositionRight: > return isForRows ? {0, 0} : {availableFreeSpace, 0}; > > etc... It´s indeed a good idea. https://codereview.chromium.org/1220043002/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutGrid.cpp:1889: columnPosition = rightGridEdgePosition + alignmentOffset - columnPosition - child.logicalWidth(); On 2015/07/08 07:14:08, svillar wrote: > Nit: I prefer rightGridEdgePosition - columnPosition - child.logicalWidth() to > be together as this is the reversal of the column position. Actually we might > even use parens to make it more explicit. > > Also let's change also columnPosition's name because it's a very bad one. We > need to make explicit that it's the grid item offset in the row axis. I'll let > you chose the name. Done. https://codereview.chromium.org/1220043002/diff/40001/Source/core/layout/Layo... File Source/core/layout/LayoutGrid.h (right): https://codereview.chromium.org/1220043002/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutGrid.h:40: On 2015/07/08 07:14:08, svillar wrote: > Careful, extra line. Done.
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=198496 |
