Bug 159294

Created attachment 282429 [details] Patch

Comment on attachment 282429 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=282429&action=review > Source/WebCore/rendering/RenderGrid.cpp:245 > + void nextState() A function like this that creates side effects and has no return value should normally have a verb phrase as its name, not a noun phrase. So instead of "next state" it would be "advance to next state", or anything else that is a verb, not a noun. > Source/WebCore/rendering/RenderGrid.cpp:264 > + bool isValidTransitionForDirection(GridTrackSizingDirection direction) Not sure this needs "for direction" in its name, although I suppose it’s OK. > Source/WebCore/rendering/RenderGrid.cpp:268 > + return direction == ForColumns ? true : false; No need for the no-op "? true : false". > Source/WebCore/rendering/RenderGrid.cpp:270 > + return direction == ForRows ? true : false; Ditto. > Source/WebCore/rendering/RenderGrid.cpp:276 > + return direction == ForRows ? true : false; Ditto. > Source/WebCore/rendering/RenderGrid.h:208 > + bool m_hasAnyOrthogonalChild; I’m a little concerned that the lifetime of this data member is tricky. It’s valid only after placeItemsOnGrid is called. Is there an obvious guarantee that repeatTracksSizingIfNeeded is only called after placeItemsOnGrid?

Comment on attachment 282429 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=282429&action=review >> Source/WebCore/rendering/RenderGrid.h:208 >> + bool m_hasAnyOrthogonalChild; > > I’m a little concerned that the lifetime of this data member is tricky. It’s valid only after placeItemsOnGrid is called. Is there an obvious guarantee that repeatTracksSizingIfNeeded is only called after placeItemsOnGrid? A quick idea would be to use an ASSERT(!m_gridIsDirty); in RenderGrid::repeatTracksSizingIfNeeded(). Also maybe it's worth to reset m_hasAnyOrthogonalChild in RenderGrid::clearGrid().

(In reply to comment #3) > Comment on attachment 282429 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=282429&action=review > > >> Source/WebCore/rendering/RenderGrid.h:208 > >> + bool m_hasAnyOrthogonalChild; > > > > I’m a little concerned that the lifetime of this data member is tricky. It’s valid only after placeItemsOnGrid is called. Is there an obvious guarantee that repeatTracksSizingIfNeeded is only called after placeItemsOnGrid? > > A quick idea would be to use an ASSERT(!m_gridIsDirty); in > RenderGrid::repeatTracksSizingIfNeeded(). I like this idea. > > Also maybe it's worth to reset m_hasAnyOrthogonalChild in > RenderGrid::clearGrid(). I think there is no need to clear the value, as it's initialized every time the placeItemsOnGrid function is called.

(In reply to comment #2) > Comment on attachment 282429 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=282429&action=review > > > Source/WebCore/rendering/RenderGrid.cpp:245 > > + void nextState() > > A function like this that creates side effects and has no return value > should normally have a verb phrase as its name, not a noun phrase. So > instead of "next state" it would be "advance to next state", or anything > else that is a verb, not a noun. > > > Source/WebCore/rendering/RenderGrid.cpp:264 > > + bool isValidTransitionForDirection(GridTrackSizingDirection direction) > > Not sure this needs "for direction" in its name, although I suppose it’s OK. > > > Source/WebCore/rendering/RenderGrid.cpp:268 > > + return direction == ForColumns ? true : false; > > No need for the no-op "? true : false". > > > Source/WebCore/rendering/RenderGrid.cpp:270 > > + return direction == ForRows ? true : false; > > Ditto. > > > Source/WebCore/rendering/RenderGrid.cpp:276 > > + return direction == ForRows ? true : false; > > Ditto. > > > Source/WebCore/rendering/RenderGrid.h:208 > > + bool m_hasAnyOrthogonalChild; > > I’m a little concerned that the lifetime of this data member is tricky. It’s > valid only after placeItemsOnGrid is called. Is there an obvious guarantee > that repeatTracksSizingIfNeeded is only called after placeItemsOnGrid? As rego suggested, I'll add an ASSERT to be sure the grid is not dirty, hence the placeItemsOnGrid completed its execution.

Created attachment 283659 [details] Patch

Created attachment 283692 [details] Patch

Comment on attachment 283692 [details] Patch Clearing flags on attachment: 283692 Committed r203252: <http://trac.webkit.org/changeset/203252>

All reviewed patches have been landed. Closing bug.

The last patch I've submitted for landing didn't have the layout tests, which were already reviewed in the first patch. I'll reopen this bug to land the tests.

Created attachment 284200 [details] Missing layout tests This patch complements the previous one, which somehow didn't contain the proper layout tests.

Created attachment 284201 [details] Missing Layout tests

Comment on attachment 284201 [details] Missing Layout tests Rejecting attachment 284201 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 284201, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog is not at the top of the file. Full output: http://webkit-queues.webkit.org/results/1723440

Created attachment 284204 [details] Missing Layout tests

Comment on attachment 284204 [details] Missing Layout tests Clearing flags on attachment: 284204 Committed r203501: <http://trac.webkit.org/changeset/203501>

All reviewed patches have been landed. Closing bug.