Bug 132380 - [CSS Grid Layout] Guard RenderObject::isRenderGrid() method
Summary: [CSS Grid Layout] Guard RenderObject::isRenderGrid() method
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords:
Depends on:
Blocks: 60731
  Show dependency treegraph
 
Reported: 2014-04-30 05:02 PDT by Manuel Rego Casasnovas
Modified: 2014-05-22 04:54 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.75 KB, patch)
2014-04-30 05:03 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2014-04-30 05:02:22 PDT
[CSS Grid Layout] Guard RenderObject::isRenderGrid() method
Comment 1 Manuel Rego Casasnovas 2014-04-30 05:03:13 PDT
Created attachment 230474 [details]
Patch
Comment 2 Darin Adler 2014-04-30 09:15:41 PDT
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.
Comment 3 Manuel Rego Casasnovas 2014-05-05 04:40:56 PDT
(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 4 Benjamin Poulain 2014-05-05 15:42:31 PDT
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?
Comment 5 Manuel Rego Casasnovas 2014-05-22 04:23:11 PDT
(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 6 WebKit Commit Bot 2014-05-22 04:54:54 PDT
Comment on attachment 230474 [details]
Patch

Clearing flags on attachment: 230474

Committed r169195: <http://trac.webkit.org/changeset/169195>
Comment 7 WebKit Commit Bot 2014-05-22 04:54:57 PDT
All reviewed patches have been landed.  Closing bug.