Bug 141748

Created attachment 246807 [details] Patch

Comment on attachment 246807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246807&action=review The patch looks good. I'm giving r- just because I think the implementation using a hash (see comments inline) deserves a try. > Source/WebCore/rendering/RenderGrid.cpp:882 > + Vector<size_t> minorAxisCursors; It should be <unsigned> > Source/WebCore/rendering/RenderGrid.cpp:886 > + } It seems to me that having a vector here is perhaps not very optimal memory-wise because I have the impression that most of them would be actually empty (0 in this case). From that POV a hash would be perhaps better, it won't require the initialization phase either although searches would be slower. Could you check the performance of that? > Source/WebCore/rendering/RenderGrid.cpp:900 > + minorAxisCursors[majorAxisPositions->resolvedInitialPosition.toInt()] = (direction == ForColumns) ? emptyGridArea->rows.resolvedInitialPosition.toInt() : emptyGridArea->columns.resolvedInitialPosition.toInt(); As direction is not going to change, perhaps we better store (direction == ForColumns) in a variable at the beginning of the function and use it here.

Created attachment 247144 [details] Patch

Thanks for the review. (In reply to comment #2) > > Source/WebCore/rendering/RenderGrid.cpp:882 > > + Vector<size_t> minorAxisCursors; > > It should be <unsigned> Right, I've changed it. > > Source/WebCore/rendering/RenderGrid.cpp:886 > > + } > > It seems to me that having a vector here is perhaps not very optimal > memory-wise because I have the impression that most of them would be > actually empty (0 in this case). From that POV a hash would be perhaps > better, it won't require the initialization phase either although searches > would be slower. Could you check the performance of that? I agree that this is not going to be the default case, actually most of the times it'll be an exception. So, for big grid avoiding the Vector might be a good idea. I've been testing with HashMap vs Vector in a 200x200 grid, and the performance results show that the HashMap is slightly faster. On top of that, IMHO the code is clearer with the map, so I'm uploading a new patch using a map instead of a vector. > > Source/WebCore/rendering/RenderGrid.cpp:900 > > + minorAxisCursors[majorAxisPositions->resolvedInitialPosition.toInt()] = (direction == ForColumns) ? emptyGridArea->rows.resolvedInitialPosition.toInt() : emptyGridArea->columns.resolvedInitialPosition.toInt(); > > As direction is not going to change, perhaps we better store (direction == > ForColumns) in a variable at the beginning of the function and use it here. Done.

Comment on attachment 247144 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247144&action=review > Source/WebCore/rendering/RenderGrid.cpp:879 > + bool isForColumns = (autoPlacementMajorAxisDirection() == ForColumns); No need for parentheses. > Source/WebCore/rendering/RenderGrid.cpp:891 > + unsigned minorAxisCursorPosition = !isGridAutoFlowDense && minorAxisCursors.contains(majorAxisInitialPosition) ? minorAxisCursors.get(majorAxisInitialPosition) : 0; This doesn't look optimal, you're querying the hashmap twice.

Created attachment 247214 [details] Patch

(In reply to comment #5) > Comment on attachment 247144 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=247144&action=review > > > Source/WebCore/rendering/RenderGrid.cpp:879 > > + bool isForColumns = (autoPlacementMajorAxisDirection() == ForColumns); > > No need for parentheses. Sure. > > Source/WebCore/rendering/RenderGrid.cpp:891 > > + unsigned minorAxisCursorPosition = !isGridAutoFlowDense && minorAxisCursors.contains(majorAxisInitialPosition) ? minorAxisCursors.get(majorAxisInitialPosition) : 0; > > This doesn't look optimal, you're querying the hashmap twice. Yeah, you're right, actually it's enough with using .get() as it'll return 0 for the case of unsiged if it's not in the map yet.

Comment on attachment 247214 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247214&action=review > Source/WebCore/rendering/RenderGrid.cpp:888 > + unsigned majorAxisInitialPosition = majorAxisPositions->resolvedInitialPosition.toInt() + 1; Instead of this better use UnsignedWithZeroKeyHashTraits in the definition of the map and... > Source/WebCore/rendering/RenderGrid.cpp:890 > + GridIterator iterator(m_grid, autoPlacementMajorAxisDirection(), majorAxisPositions->resolvedInitialPosition.toInt(), isGridAutoFlowDense ? 0 : minorAxisCursors.get(majorAxisInitialPosition)); ... you'd have to use find() here instead of get to check whether or not the cursor exists.

Created attachment 247216 [details] Patch

Thanks for the fast reviews! (In reply to comment #8) > > Source/WebCore/rendering/RenderGrid.cpp:888 > > + unsigned majorAxisInitialPosition = majorAxisPositions->resolvedInitialPosition.toInt() + 1; > > Instead of this better use UnsignedWithZeroKeyHashTraits in the definition > of the map and... True. Thanks for the pointer. > > Source/WebCore/rendering/RenderGrid.cpp:890 > > + GridIterator iterator(m_grid, autoPlacementMajorAxisDirection(), majorAxisPositions->resolvedInitialPosition.toInt(), isGridAutoFlowDense ? 0 : minorAxisCursors.get(majorAxisInitialPosition)); > > ... you'd have to use find() here instead of get to check whether or not the > cursor exists. Actually as I'm just using UnsignedWithZeroKeyHashTraits for they key, I don't need to use find() and can keep get() which makes the code simpler. New version with the suggested changes uploaded.

Comment on attachment 247216 [details] Patch Yay! much better. Ideally we would have some description before the test cases in order to know what are we really testing, because it's a bit difficult to know just taking a look at the HTML.

(In reply to comment #11) > Comment on attachment 247216 [details] > Patch > > Yay! much better. Ideally we would have some description before the test > cases in order to know what are we really testing, because it's a bit > difficult to know just taking a look at the HTML. I don't see an easy way, without entering in lots of details in the comments, which will make them useless and it'll be simpler to take a look to the HTML. In addition, we've several tests with this very same format. So, I'm landing this as it's and we could add the comments in a follow-up patch if we find a good way.

Comment on attachment 247216 [details] Patch Clearing flags on attachment: 247216 Committed r180567: <http://trac.webkit.org/changeset/180567>

All reviewed patches have been landed. Closing bug.