Created attachment 230474 [details] Patch

Comment on attachment 230474 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230474&action=review > Source/WebCore/ChangeLog:8 > + Guard RenderObject::isRenderGrid() method under ENABLE_CSS_GRID_LAYOUT compilation flag. Is this the best way to do it? I could imagine that instead of turning it off entirely we could just make it be an inline that returns false so no code would be generated at the call site. Adding the #if in RenderBox.cpp seems to make the code less elegant rather than more.

(In reply to comment #2) > (From update of attachment 230474 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=230474&action=review > > > Source/WebCore/ChangeLog:8 > > + Guard RenderObject::isRenderGrid() method under ENABLE_CSS_GRID_LAYOUT compilation flag. > > Is this the best way to do it? > > I could imagine that instead of turning it off entirely we could just make it be an inline that returns false so no code would be generated at the call site. Adding the #if in RenderBox.cpp seems to make the code less elegant rather than more. I was just adding it following the advice in webkit-dev of being "very aggressive on the #ifdefs". However, I agree that it makes the code ugly so we might forget about this change. BTW, we have similar code for FULLSCREEN_API for example in http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderObject.h#L481 About making it inline, it would be a bit weird as it'll be the only isRenderXXX() that will be inline.

Comment on attachment 230474 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230474&action=review > Source/WebCore/rendering/RenderBox.cpp:2307 > + && !isFloating() && !isInline() && !cb->isFlexibleBoxIncludingDeprecated() > +#if ENABLE(CSS_GRID_LAYOUT) > + && !cb->isRenderGrid() > +#endif I am fine with this, it is supposed to be temporary. When does it make sense to layout differently for isFlexibleBoxIncludingDeprecated() and isRenderGrid()? From a quick view it seems they share many similar "stretching" properties. Wouldn't it make sense to replace isFlexibleBoxIncludingDeprecated() by something that accounts for grid layout?

(In reply to comment #4) > (From update of attachment 230474 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=230474&action=review > > > Source/WebCore/rendering/RenderBox.cpp:2307 > > + && !isFloating() && !isInline() && !cb->isFlexibleBoxIncludingDeprecated() > > +#if ENABLE(CSS_GRID_LAYOUT) > > + && !cb->isRenderGrid() > > +#endif > > I am fine with this, it is supposed to be temporary. > > When does it make sense to layout differently for isFlexibleBoxIncludingDeprecated() and isRenderGrid()? From a quick view it seems they share many similar "stretching" properties. Wouldn't it make sense to replace isFlexibleBoxIncludingDeprecated() by something that accounts for grid layout? I cannot think in a good name for a method merging isFlexibleBoxIncludingDeprecated() and isRenderGrid(). Moreover there're similar cases like isFloating() and isInline() so I guess we could land it as it's for the moment.

Comment on attachment 230474 [details] Patch Clearing flags on attachment: 230474 Committed r169195: <http://trac.webkit.org/changeset/169195>

All reviewed patches have been landed. Closing bug.