Bug 166883

Created attachment 298456 [details] Patch

Comment on attachment 298456 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298456&action=review > Source/WebCore/rendering/RenderBox.cpp:60 > +#if ENABLE(CSS_GRID_LAYOUT) > +#include "RenderGrid.h" > +#endif Please put conditional includes into a separate paragraph, rather than trying to sort them in with the unconditional includes. I believe the WebKit coding guidelines call for this explicitly. > Source/WebCore/rendering/RenderBox.cpp:471 > +#if ENABLE(CSS_GRID_LAYOUT) Tiny coding style thought: When there’s a long function or multiple functions surrounded by #if, or really any whole function as opposed to some code inside a function, I prefer to leave blank lines before and after the function inside the #if/#endif, otherwise the #if seems like part of the function rather than a guard for the conditional code. > Source/WebCore/rendering/RenderBox.cpp:472 > +void RenderBox::updateGridPositionAfterStyleChange(const RenderStyle& style, const RenderStyle* oldStyle) Extra space here after the comma. > Source/WebCore/rendering/RenderBox.cpp:474 > + if (!oldStyle || !parent() || !parent()->isRenderGrid()) Typically we like to write tests like this one using the is<> function, which can handle null checks for you too. if (!oldStyle || !is<RenderGrid>(parent())) > Source/WebCore/rendering/RenderBox.h:657 > + void updateGridPositionAfterStyleChange(const RenderStyle&, const RenderStyle* oldStyle); Extra space here after comma. > Source/WebCore/rendering/RenderGrid.cpp:-1956 > -void RenderGrid::clearGrid() Patch removes this function, but does not remove the declaration of it from the class definition! > Source/WebCore/rendering/RenderGrid.h:34 > +#include "RenderObject.h" This include is not needed. > Source/WebCore/rendering/RenderGrid.h:64 > + void addChild(RenderObject* newChild, RenderObject* beforeChild = 0) final; > + void removeChild(RenderObject&) final; Can these be private instead of public? When we override functions we never need them to be public unless we actually want to call them on a RenderGrid pointer rather than calling them polymorphically. Also, please use nullptr instead of 0 for the null pointer. Also, we may not need the default argument for the beforeChild argument if this function is only called polymorphically. There is no need to repeat the default argument simply to override the function. > Source/WebCore/rendering/RenderGrid.h:68 > + bool explicitGridDidResize(const RenderStyle&) const; > + bool namedGridLinesDefinitionDidChange(const RenderStyle&) const; Please make these private.

Comment on attachment 298456 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298456&action=review The patch LGTM to me too, just a few comments inline. Also one question, have you run the perftests? I guess this patch should show improvements there. > Source/WebCore/ChangeLog:30 > + (WebCore::RenderGrid::addChild): Make the grid data as dirty. Did you mean "Mark" instead of "Make"? The same happens in the next lines. BTW do we have tests for these add/remove operations? > Source/WebCore/rendering/RenderBox.cpp:482 > + if (oldStyle->gridItemColumnStart() == style.gridItemColumnStart() > + && oldStyle->gridItemColumnEnd() == style.gridItemColumnEnd() > + && oldStyle->gridItemRowStart() == style.gridItemRowStart() > + && oldStyle->gridItemRowEnd() == style.gridItemRowEnd() > + && oldStyle->order() == style.order() > + && oldStyle->hasOutOfFlowPosition() == style.hasOutOfFlowPosition()) I see you added a test for "order", but what happens with the other things. Do we have any test checking that you can dynamically change "grid-column-start" and the rest?

Comment on attachment 298456 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298456&action=review >> Source/WebCore/ChangeLog:30 >> + (WebCore::RenderGrid::addChild): Make the grid data as dirty. > > Did you mean "Mark" instead of "Make"? > The same happens in the next lines. > > BTW do we have tests for these add/remove operations? I'll rephrase the ChangeLog comments. I don't think we have tests for add/remove, so I'll add them. Thanks for pointing it out. >> Source/WebCore/rendering/RenderBox.cpp:482 >> + && oldStyle->hasOutOfFlowPosition() == style.hasOutOfFlowPosition()) > > I see you added a test for "order", but what happens with the other things. > Do we have any test checking that you can dynamically change "grid-column-start" and the rest? Yes, we do have tests to handle style changes on those properties.

Created attachment 298693 [details] Patch

Comment on attachment 298693 [details] Patch Clearing flags on attachment: 298693 Committed r210669: <http://trac.webkit.org/changeset/210669>

All reviewed patches have been landed. Closing bug.