Bug 75782

Created attachment 122322 [details] Patch

Comment on attachment 122322 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122322&action=review r- because of missing test cases. > Source/WebCore/rendering/RenderBox.cpp:1803 > + EFlexAlign align = style()->flexItemAlign(); > + if (align == AlignAuto) > + align = parent()->style()->flexAlign(); > + if (align != AlignStretch) > + return true; Nit: I would probably just do "if (itemalign == stretch || (itemalign == auto && parent->itemalign == stretch))". *shrug* > Source/WebCore/rendering/RenderFlexibleBox.cpp:621 > +static EFlexAlign flexAlign(RenderBox* flexItem) Nit: Maybe name it flexAlignForChild? > LayoutTests/css3/flexbox/flex-align.html:124 > +</div> > + It would be nice to have a test with flex-align set and flex-item-align not set. > LayoutTests/fast/css/getComputedStyle/computed-style-expected.txt:147 > --webkit-flex-item-align: stretch; > +-webkit-flex-align: stretch; There are port specific results for these tests too, right?

Created attachment 122359 [details] Patch

Please make sure to update all the other platform results before landing.

Comment on attachment 122359 [details] Patch Attachment 122359 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11227415 New failing tests: svg/css/getComputedStyle-basic.xhtml css3/flexbox/flex-align.html fast/css/getComputedStyle/computed-style.html fast/css/getComputedStyle/computed-style-without-renderer.html

(In reply to comment #5) > (From update of attachment 122359 [details]) > Attachment 122359 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/11227415 > > New failing tests: > svg/css/getComputedStyle-basic.xhtml > css3/flexbox/flex-align.html > fast/css/getComputedStyle/computed-style.html > fast/css/getComputedStyle/computed-style-without-renderer.html css3/flexbox/flex-align.html failing is a bit suspicious.

Committed r104972: <http://trac.webkit.org/changeset/104972>

Reverted r104972 for reason: Broke some tests Committed r104985: <http://trac.webkit.org/changeset/104985>

Antti, Hyatt: Would one of you mind taking a quick look at the CSSComputedStyleDeclaration changes in this patch and letting me know if I'm doing something obviously wrong? It's just two lines (the CSSPropertyWebkitFlexItemAlign changes). Hmm. The following tests crashed on chromium win and linux (they're not run on chromium-mac): http/tests/inspector/extensions-ignore-cache.html inspector/styles/styles-new-API.html inspector/styles/parse-utf8-bom.html inspector/styles/media-queries.html It's strange they didn't crash on the chromium EWS. Here's the stack: WebCore::CSSComputedStyleDeclaration::getPropertyCSSValue() [0xb8e289] WebCore::CSSComputedStyleDeclaration::getPropertyCSSValue() [0xb9807e] WebCore::CSSComputedStyleDeclaration::getPropertyValue() [0xb88402] WebCore::CSSStyleDeclaration::getPropertyValue() [0xbd011f] WebCore::InspectorStyle::populateAllProperties() [0xeb01aa] WebCore::InspectorStyle::buildArrayForComputedStyle() [0xeb09a3] WebCore::InspectorCSSAgent::getComputedStyleForNode() [0xea60fa] WebCore::InspectorBackendDispatcher::CSS_getComputedStyleForNode() [0x11f6c91] WebCore::InspectorBackendDispatcher::dispatch() [0x1200e9a] WebKit::WebDevToolsAgentImpl::dispatchOnInspectorBackend() [0x4a0c8f]

Created attachment 122529 [details] Patch

Committed r105015: <http://trac.webkit.org/changeset/105015> Took out the special CSSComputedStyleDeclaration behavior since it's almost certainly what caused the crash. Will fix it in bug 76326.