grid-auto-flow syntax has changed in the last versions of the spec. This is being addressed in Blink (https://codereview.chromium.org/333563003/) and should be ported to WebKit.
Created attachment 234248 [details] Patch
Comment on attachment 234248 [details] Patch Attachment 234248 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5125708200280064 New failing tests: svg/css/getComputedStyle-basic.xhtml
Created attachment 234256 [details] Archive of layout-test-results from webkit-ews-02 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 234248 [details] Patch Attachment 234248 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4803379494649856 New failing tests: svg/css/getComputedStyle-basic.xhtml
Created attachment 234257 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 234259 [details] Rebaseline for svg/css/getComputedStyle-basic.html.
Comment on attachment 234259 [details] Rebaseline for svg/css/getComputedStyle-basic.html. View in context: https://bugs.webkit.org/attachment.cgi?id=234259&action=review Promising, but I think we need some adjustments before landing. > Source/WebCore/ChangeLog:17 > + once "stack" is implemented. Looks confusing. Please reword. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2089 > + } I think we could simplify this a lot. You'll find the rationale in my last comment in StyleResolver::applyProperty() > Source/WebCore/css/CSSParser.cpp:5183 > + if (value->id != CSSValueRow && value->id != CSSValueColumn && value->id != CSSValueDense && value->id != CSSValueStack) Weird, you store value->id in firstId but you keep using value->id. > Source/WebCore/css/CSSParser.cpp:5192 > + if (value) { Let's do an early return here and avoid an indentation level. > Source/WebCore/css/CSSParser.cpp:5205 > + return parsedValues; Hmm, firstId cannot be anything else right? We should have detected it in the early return above so we should assert here. > Source/WebCore/css/CSSParser.cpp:5211 > + return parsedValues; I have a comment about how we generate the list of parsedValues. Check my last comment in StyleResolver::applyProperty() > Source/WebCore/css/StyleResolver.cpp:2805 > + I prefer something like: if (!list->length()) { state.style()->setGridAutoFlow(RenderStyle::initialGridAutoFlow()); return; } CSSPrimitiveValue* first = toCSSPrimitiveValue(list->item(0)); > Source/WebCore/css/StyleResolver.cpp:2815 > + autoFlow = AutoFlowStackRow; autoFlow = second->getValueID() == CSSValueDense ? AutoFlowRowDense : AutoFlowStackRow; > Source/WebCore/css/StyleResolver.cpp:2824 > + autoFlow = AutoFlowStackColumn; Same comment than above > Source/WebCore/css/StyleResolver.cpp:2832 > + autoFlow = AutoFlowRowDense; Again. > Source/WebCore/css/StyleResolver.cpp:2838 > + autoFlow = AutoFlowStackRow; And again. > Source/WebCore/css/StyleResolver.cpp:2843 > + } All those switch statements everywhere made me wonder. Although the css syntax is [ row | column ] && dense? | stack && [ row | column ]? we could get a more convenient representation from the parser. So after checking (in the parser) that the syntax is correct we can generate a "fixed" representation of the value list. For example, we can force that the parser always generates a list of two elements, being the first one [column|row] and the second one [dense|stack]. That will simplify a lot the code above removing unnecessary checks and avoiding the need of checks for all possible permutations. > Source/WebCore/rendering/style/RenderStyle.h:933 > + bool isGridAutoFlowAlgorithmStack() const { return (rareNonInheritedData->m_grid->m_gridAutoFlow & InternalAutoFlowAlgorithmStack) == InternalAutoFlowAlgorithmStack; } Do we really need the "== Internal..." part in all these?
Created attachment 234706 [details] Patch
(In reply to comment #7) > (From update of attachment 234259 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=234259&action=review > > Promising, but I think we need some adjustments before landing. Thanks for the detailed review, I've just uploaded a new version applying suggested changes.
Comment on attachment 234706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=234706&action=review Awesome! Thanks for the patch. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2057 > + else if (style->isGridAutoFlowDirectionColumn()) Is there any other possibility? I mean the direction is either row or column so we can use a simple "else". If we want to be ultra-sure we could always add an ASSERT(isColumn() || isRow()). > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2063 > + list->append(cssValuePool().createIdentifierValue(CSSValueStack)); Same comment here. > Source/WebCore/css/CSSParser.cpp:5191 > + // Second parameter, if any. We must improve this comment mentioning that this can be used by the grid shorthand. It isn't obvious from the code why we don't bail out in some, apparently erroneous cases.
Comment on attachment 234706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=234706&action=review Thanks for the review >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2057 >> + else if (style->isGridAutoFlowDirectionColumn()) > > Is there any other possibility? I mean the direction is either row or column so we can use a simple "else". If we want to be ultra-sure we could always add an ASSERT(isColumn() || isRow()). No, you're right the only options are row or column (I'm removing the second if and adding the ASSERT). >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2063 >> + list->append(cssValuePool().createIdentifierValue(CSSValueStack)); > > Same comment here. In this case there's one extra option isGridAutoFlowAlgorithmSparse(), so I'll keep it as it's. >> Source/WebCore/css/CSSParser.cpp:5191 >> + // Second parameter, if any. > > We must improve this comment mentioning that this can be used by the grid shorthand. It isn't obvious from the code why we don't bail out in some, apparently erroneous cases. Added explicit comment about the shorthand.
Created attachment 234753 [details] Patch for landing
The commit-queue encountered the following flaky tests while processing attachment 234753 [details]: media/video-ended-event-negative-playback.html bug 134490 The commit-queue is continuing to process your patch.
Comment on attachment 234753 [details] Patch for landing Clearing flags on attachment: 234753 Committed r170996: <http://trac.webkit.org/changeset/170996>
All reviewed patches have been landed. Closing bug.