132380 2014-04-30 05:02:22 -0700 [CSS Grid Layout] Guard RenderObject::isRenderGrid() method 2014-05-22 04:54:57 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 60731 1 rego rego benjamin commit-queue esprehn+autocc glenn kondapallykalyan oldest_to_newest 1005354 0 rego 2014-04-30 05:02:22 -0700 [CSS Grid Layout] Guard RenderObject::isRenderGrid() method 1005355 1 230474 rego 2014-04-30 05:03:13 -0700 Created attachment 230474 Patch 1005385 2 230474 darin 2014-04-30 09:15:41 -0700 Comment on attachment 230474 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. 1006597 3 rego 2014-05-05 04:40:56 -0700 (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. 1006760 4 230474 benjamin 2014-05-05 15:42:31 -0700 Comment on attachment 230474 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? 1010942 5 rego 2014-05-22 04:23:11 -0700 (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. 1010944 6 230474 commit-queue 2014-05-22 04:54:54 -0700 Comment on attachment 230474 Patch Clearing flags on attachment: 230474 Committed r169195: <http://trac.webkit.org/changeset/169195> 1010945 7 commit-queue 2014-05-22 04:54:57 -0700 All reviewed patches have been landed. Closing bug. 230474 2014-04-30 05:03:13 -0700 2014-05-22 04:54:54 -0700 Patch bug-132380-20140430140250.patch text/plain 2812 rego U3VidmVyc2lvbiBSZXZpc2lvbjogMTY3OTkzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMzJiZDA1OWFlYTZlNDRi NTc4OGUzMjFmNGJiOTQwZTY2ODNjZjdjZC4uYWRiMGNhMTYwZDg1NjgyM2IwZGU2MWQ4ZjJkYTk1 NGMzOTk5NTAwNyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE4IEBACiAyMDE0LTA0LTMwICBNYW51 ZWwgUmVnbyBDYXNhc25vdmFzICA8cmVnb0BpZ2FsaWEuY29tPgogCisgICAgICAgIFtDU1MgR3Jp ZCBMYXlvdXRdIEd1YXJkIFJlbmRlck9iamVjdDo6aXNSZW5kZXJHcmlkKCkgbWV0aG9kCisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMzIzODAKKworICAg ICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBHdWFyZCBSZW5kZXJP YmplY3Q6OmlzUmVuZGVyR3JpZCgpIG1ldGhvZCB1bmRlciBFTkFCTEVfQ1NTX0dSSURfTEFZT1VU IGNvbXBpbGF0aW9uIGZsYWcuCisKKyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVyQm94LmNwcDoK KyAgICAgICAgKFdlYkNvcmU6OlJlbmRlckJveDo6Y29tcHV0ZUxvZ2ljYWxXaWR0aEluUmVnaW9u KToKKyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVyT2JqZWN0Lmg6CisKKzIwMTQtMDQtMzAgIE1h bnVlbCBSZWdvIENhc2Fzbm92YXMgIDxyZWdvQGlnYWxpYS5jb20+CisKICAgICAgICAgW0NTUyBH cmlkIExheW91dF0gRW5hYmxlIHJ1bnRpbWUgZmVhdHVyZSBieSBkZWZhdWx0CiAgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMzIxODkKIApkaWZmIC0tZ2l0 IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckJveC5jcHAgYi9Tb3VyY2UvV2ViQ29y ZS9yZW5kZXJpbmcvUmVuZGVyQm94LmNwcAppbmRleCA1ZGVhMTA2ODZhMDIxOGNiNDIzMzEwMTY0 MTAzMjczNWVhNTZmZjY2Li44OWNkYzdlZmJlMzU3ZWEzMWUzZWFjNWNmMTFhNDU3ODIyMmQ1YmMy IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyQm94LmNwcAorKysg Yi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyQm94LmNwcApAQCAtMjMwMSw3ICsyMzAx LDExIEBAIHZvaWQgUmVuZGVyQm94Ojpjb21wdXRlTG9naWNhbFdpZHRoSW5SZWdpb24oTG9naWNh bEV4dGVudENvbXB1dGVkVmFsdWVzJiBjb21wdXRlCiAgICAgfQogICAgIAogICAgIGlmICghaGFz UGVycGVuZGljdWxhckNvbnRhaW5pbmdCbG9jayAmJiBjb250YWluZXJMb2dpY2FsV2lkdGggJiYg Y29udGFpbmVyTG9naWNhbFdpZHRoICE9IChjb21wdXRlZFZhbHVlcy5tX2V4dGVudCArIGNvbXB1 dGVkVmFsdWVzLm1fbWFyZ2lucy5tX3N0YXJ0ICsgY29tcHV0ZWRWYWx1ZXMubV9tYXJnaW5zLm1f ZW5kKQotICAgICAgICAmJiAhaXNGbG9hdGluZygpICYmICFpc0lubGluZSgpICYmICFjYi0+aXNG bGV4aWJsZUJveEluY2x1ZGluZ0RlcHJlY2F0ZWQoKSAmJiAhY2ItPmlzUmVuZGVyR3JpZCgpKSB7 CisgICAgICAgICYmICFpc0Zsb2F0aW5nKCkgJiYgIWlzSW5saW5lKCkgJiYgIWNiLT5pc0ZsZXhp YmxlQm94SW5jbHVkaW5nRGVwcmVjYXRlZCgpCisjaWYgRU5BQkxFKENTU19HUklEX0xBWU9VVCkK KyAgICAgICAgJiYgIWNiLT5pc1JlbmRlckdyaWQoKQorI2VuZGlmCisgICAgICAgICkgewogICAg ICAgICBMYXlvdXRVbml0IG5ld01hcmdpbiA9IGNvbnRhaW5lckxvZ2ljYWxXaWR0aCAtIGNvbXB1 dGVkVmFsdWVzLm1fZXh0ZW50IC0gY2ItPm1hcmdpblN0YXJ0Rm9yQ2hpbGQoKnRoaXMpOwogICAg ICAgICBib29sIGhhc0ludmVydGVkRGlyZWN0aW9uID0gY2ItPnN0eWxlKCkuaXNMZWZ0VG9SaWdo dERpcmVjdGlvbigpICE9IHN0eWxlKCkuaXNMZWZ0VG9SaWdodERpcmVjdGlvbigpOwogICAgICAg ICBpZiAoaGFzSW52ZXJ0ZWREaXJlY3Rpb24pCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9y ZW5kZXJpbmcvUmVuZGVyT2JqZWN0LmggYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVy T2JqZWN0LmgKaW5kZXggODkwOTRkYmViZDQxMjY5NjQ5Yjc4YjdjYzQ5MmM0YzRkMWU3NTIwMC4u M2I4ZDM0OGJkN2YyYWM4MDJkMTI5MTk4ZGJjN2M2YzkxNWJkZGI0OCAxMDA2NDQKLS0tIGEvU291 cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlck9iamVjdC5oCisrKyBiL1NvdXJjZS9XZWJDb3Jl L3JlbmRlcmluZy9SZW5kZXJPYmplY3QuaApAQCAtMzUyLDkgKzM1Miw5IEBAIHB1YmxpYzoKICAg ICB2aXJ0dWFsIGJvb2wgaXNSZW5kZXJGdWxsU2NyZWVuKCkgY29uc3QgeyByZXR1cm4gZmFsc2U7 IH0KICAgICB2aXJ0dWFsIGJvb2wgaXNSZW5kZXJGdWxsU2NyZWVuUGxhY2Vob2xkZXIoKSBjb25z dCB7IHJldHVybiBmYWxzZTsgfQogI2VuZGlmCi0KKyNpZiBFTkFCTEUoQ1NTX0dSSURfTEFZT1VU KQogICAgIHZpcnR1YWwgYm9vbCBpc1JlbmRlckdyaWQoKSBjb25zdCB7IHJldHVybiBmYWxzZTsg fQotCisjZW5kaWYKICAgICB2aXJ0dWFsIGJvb2wgaXNSZW5kZXJGbG93VGhyZWFkKCkgY29uc3Qg eyByZXR1cm4gZmFsc2U7IH0KICAgICB2aXJ0dWFsIGJvb2wgaXNSZW5kZXJOYW1lZEZsb3dUaHJl YWQoKSBjb25zdCB7IHJldHVybiBmYWxzZTsgfQogICAgIGJvb2wgaXNJbkZsb3dSZW5kZXJGbG93 VGhyZWFkKCkgY29uc3QgeyByZXR1cm4gaXNSZW5kZXJGbG93VGhyZWFkKCkgJiYgIWlzT3V0T2ZG bG93UG9zaXRpb25lZCgpOyB9Cg==