Bug 109880

Created attachment 188463 [details] Patch

Comment on attachment 188463 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188463&action=review As a whole, I don't think this is an improvement (though I do understand the goal). If we want to cache the grid representation between layouts, we will have to revert this change. Also computing the grid falls into logical const-ness IMO. Enforcing logical constness would only require to add a mutable statement to m_grid or (probably better) a const_cast to call placeItemsOnGrid vs patching a lot of RenderGrid's methods to enforce physical const-ness. Because of the above, I would rather see us the former instead of the latter (unless I missed something). > Source/WebCore/rendering/RenderGrid.cpp:471 > + ASSERT(grid.isEmpty()); This ASSERT becomes pretty useless. It was to catch people not calling clear() on the grid at the end of layout / computePreferredLogicalWidths. > Source/WebCore/rendering/RenderGrid.h:53 > + typedef Vector<Vector<Vector<RenderBox*, 1> > > GridData; I am not super fan of the naming. GridData is a pretty generic and already overloaded term (e.g StyleGridData). I would just stick with Grid as this is really what it is. If you want something less generic, GridRepresentation could work (btw, this is the typedef you said was not super useful, guess it was :))

(In reply to comment #2) > (From update of attachment 188463 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=188463&action=review > > As a whole, I don't think this is an improvement (though I do understand the goal). > > If we want to cache the grid representation between layouts, we will have to revert this change. Also computing the grid falls into logical const-ness IMO. Enforcing logical constness would only require to add a mutable statement to m_grid or (probably better) a const_cast to call placeItemsOnGrid vs patching a lot of RenderGrid's methods to enforce physical const-ness. Because of the above, I would rather see us the former instead of the latter (unless I missed something). Assuming a world where we know that we never want to cache the grid representation, do you still think this is a step in the wrong direction? I don't think we should write the code around a performance optimization that may or may not apply in the future. Also, undoing this change is a tiny amount of work. The reason I like stack allocating it is because for someone new to the code it's much easier to trace what's going on. Also, as it is, in the current the member variable is just a way to avoid passing an argument around, which is a bad reason to make something a member IMO. Also, it means we need to be careful to always call m_grid.clear(). What if we add an early return in the middle of layoutGridItems? > > Source/WebCore/rendering/RenderGrid.cpp:471 > > + ASSERT(grid.isEmpty()); > > This ASSERT becomes pretty useless. It was to catch people not calling clear() on the grid at the end of layout / computePreferredLogicalWidths. Yeah. I thought about it and decided it could be valuable to assert that someone isn't calling placeItemsOnGrid multiple times on the same grid. That's obviously not gonna happen in the current code, but I could imagine it in the future. > > Source/WebCore/rendering/RenderGrid.h:53 > > + typedef Vector<Vector<Vector<RenderBox*, 1> > > GridData; > > I am not super fan of the naming. GridData is a pretty generic and already overloaded term (e.g StyleGridData). > > I would just stick with Grid as this is really what it is. If you want something less generic, GridRepresentation could work (btw, this is the typedef you said was not super useful, guess it was :)) I can change it to Grid. Now that we're passing it around everywhere, it's clearly more readable to have a typedef IMO.

Comment on attachment 188463 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188463&action=review > Assuming a world where we know that we never want to cache the grid representation, do you still think this is a step in the wrong direction? I don't think we should write the code around a performance optimization that may or may not apply in the future. Also, undoing this change is a tiny amount of work. That's assuming my objection was only based on performance: adding an extra argument to 5 functions is a cognitive load and I have been complaining about having lots of arguments already. Also if you wanted it on the stack, you should have requested it when reviewing the patch for the representation yesterday. It feels weird to undo that the next day. So yes, even if you take performance refactorings out of the equation, I still think it's not the right direction. But we can agree to disagree on this. > The reason I like stack allocating it is because for someone new to the code it's much easier to trace what's going on. Also, as it is, in the current the member variable is just a way to avoid passing an argument around, which is a bad reason to make something a member IMO. Also, it means we need to be careful to always call m_grid.clear(). What if we add an early return in the middle of layoutGridItems? That's a far-fetched argument. We rarely early return in layout and I don't see why this would happen in grid. >>> Source/WebCore/rendering/RenderGrid.cpp:471 >>> + ASSERT(grid.isEmpty()); >> >> This ASSERT becomes pretty useless. It was to catch people not calling clear() on the grid at the end of layout / computePreferredLogicalWidths. > > Yeah. I thought about it and decided it could be valuable to assert that someone isn't calling placeItemsOnGrid multiple times on the same grid. That's obviously not gonna happen in the current code, but I could imagine it in the future. As I tell people, I can't read your mind, neither your ChangeLog nor a comment explains why this was kept so how am I to know? This ASSERT would need a comment as you are changing the meaning of it: it used to be a functional ASSERT (if you haven't cleared the grid, there are grid items left in it so we will be wrong), now it's more a performance one (you would end up with duplicates in the grid but that shouldn't impact our layout algorithm).

(In reply to comment #4) > (From update of attachment 188463 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=188463&action=review > > > Assuming a world where we know that we never want to cache the grid representation, do you still think this is a step in the wrong direction? I don't think we should write the code around a performance optimization that may or may not apply in the future. Also, undoing this change is a tiny amount of work. > > That's assuming my objection was only based on performance: adding an extra argument to 5 functions is a cognitive load and I have been complaining about having lots of arguments already. By this logic we could use a static instead of a member variable, no? > Also if you wanted it on the stack, you should have requested it when reviewing the patch for the representation yesterday. It feels weird to undo that the next day. This problem only occurred to me when I went to implement computeIntrinsicLogicalWidths and couldn't because of the const issue. > >>> Source/WebCore/rendering/RenderGrid.cpp:471 > >>> + ASSERT(grid.isEmpty()); > >> > >> This ASSERT becomes pretty useless. It was to catch people not calling clear() on the grid at the end of layout / computePreferredLogicalWidths. > > > > Yeah. I thought about it and decided it could be valuable to assert that someone isn't calling placeItemsOnGrid multiple times on the same grid. That's obviously not gonna happen in the current code, but I could imagine it in the future. > > As I tell people, I can't read your mind, neither your ChangeLog nor a comment explains why this was kept so how am I to know? > > This ASSERT would need a comment as you are changing the meaning of it: it used to be a functional ASSERT (if you haven't cleared the grid, there are grid items left in it so we will be wrong), now it's more a performance one (you would end up with duplicates in the grid but that shouldn't impact our layout algorithm). I'm fine removing the assert.

Hmm...a static doesn't work if you have a nested grid. I'll just make it mutable.