[CSS Grid Layout] Guard RenderObject::isRenderGrid() method

• Component: New Bugs
• Status: RESOLVED FIXED

[Manuel Rego Casasnovas]

[CSS Grid Layout] Guard RenderObject::isRenderGrid() method

[Manuel Rego Casasnovas]

Created attachment 230474 [details]
Patch

[Darin Adler]

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.

[Manuel Rego Casasnovas]

(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.

[Benjamin Poulain]

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?

[Manuel Rego Casasnovas]

(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.

[WebKit Commit Bot]

Comment on attachment 230474 [details]
Patch

Clearing flags on attachment: 230474

Committed r169195: <http://trac.webkit.org/changeset/169195>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.