Bug 153206

Actually this issue is affecting any Replaced Box being defined as a grid item.

Created attachment 269219 [details] Test case to reproduce the issue

Created attachment 269222 [details] Patch

Comment on attachment 269222 [details] Patch Attachment 269222 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/708288 New failing tests: imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection.html

Created attachment 269224 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 269226 [details] Patch

Comment on attachment 269226 [details] Patch Attachment 269226 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/708592 New failing tests: imported/w3c/web-platform-tests/streams-api/readable-streams/count-queuing-strategy-integration.html

Created attachment 269228 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 269226 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269226&action=review > Source/WebCore/rendering/RenderBox.cpp:2525 > +// FIXME can/should move this inside specific layout classes (flex. grid) ? Can we reactoring columnFlexItemHasStretchAlignment logic ? Formatting errors: No spaces before "?", missing ":" after FIXME, missing capital of "can", instead of "Can we reactoring" it should be "Can we refactor". > Source/WebCore/rendering/RenderBox.cpp:2528 > + RenderBlock* cb = containingBlock(); What guarantees containingBlock can't be null? If we have a guarantee that it can't be null, why not have the function return a reference instead of a pointer? Better to call this containingBlock rather than cb. Better to use a reference rather than a pointer since we don't handle null here. ASSERT(containingBlock()); // <comment explaining why it can't be null> auto& containingBlock = *this->containingBlock(); > Source/WebCore/rendering/RenderBox.cpp:2529 > + const RenderStyle& styleToUse = style(); I think this local variable can also be named style, not sure what "to use" adds and since this is the same as the "style" function seems fine to use the name. auto& style = this->style(); > Source/WebCore/rendering/RenderBox.cpp:2531 > + bool allowedToStretch = styleToUse.logicalWidth().isAuto() && !styleToUse.marginStart().isAuto() && !styleToUse.marginEnd().isAuto(); Could early exit based on this before computing containingBlock or hasPerpendicularContainingBlock. if (!style.logicalWidth().isAuto() || style.marginStart().isAuto() || style.marginEnd().isAuto()) return false; Or if you prefer the named boolean: if (!allowedToStretch) return false;

Created attachment 269238 [details] Patch

Comment on attachment 269238 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269238&action=review > Source/WebCore/rendering/RenderBox.cpp:2525 > +// FIXME: Can/Should we move this inside specific layout classes (flex. grid)? Can we reactor columnFlexItemHasStretchAlignment logic? Typo: "reactor" here instead of "refactor". > Source/WebCore/rendering/RenderBox.cpp:2531 > + RenderBlock* cb = containingBlock(); As I mentioned before, in WebKit coding style the local variable should be named containingBlock, not cb.

Created attachment 269260 [details] Patch

Comment on attachment 269260 [details] Patch Clearing flags on attachment: 269260 Committed r195284: <http://trac.webkit.org/changeset/195284>

All reviewed patches have been landed. Closing bug.