This is an intermediate step towards the full implementation of the CSS Box Alignment Module Level 3. The justify-self is the first property to be implemented, so after that we can adapt and share code between the other properties of the specs (align-self and align-items). After having a share source code for all of them, the justify-items property should be implemented to complete the specification.
Created attachment 234031 [details] Patch
Comment on attachment 234031 [details] Patch Attachment 234031 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6108163447193600 New failing tests: fast/canvas/canvas-color-serialization.html
Created attachment 234037 [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 234031 [details] Patch Attachment 234031 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5934996246233088 New failing tests: fast/canvas/canvas-color-serialization.html
Created attachment 234039 [details] Archive of layout-test-results from webkit-ews-06 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 234031 [details] Patch Attachment 234031 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5167190873473024 New failing tests: fast/canvas/canvas-color-serialization.html
Created attachment 234043 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 234031 [details] Patch Attachment 234031 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4729116557312000 New failing tests: media/W3C/video/networkState/networkState_during_loadstart.html fast/canvas/canvas-color-serialization.html
Created attachment 234047 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 234070 [details] patch rebased
Comment on attachment 234070 [details] patch rebased Attachment 234070 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5198563973791744 New failing tests: fast/canvas/canvas-color-serialization.html
Created attachment 234071 [details] Archive of layout-test-results from webkit-ews-06 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 234070 [details] patch rebased Attachment 234070 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5093312872579072 New failing tests: fast/canvas/canvas-color-serialization.html media/W3C/audio/canPlayType/canPlayType_application_octet_stream_with_codecs_1.html
Created attachment 234073 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 234070 [details] patch rebased Attachment 234070 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5849289636970496 New failing tests: fast/canvas/canvas-color-serialization.html
Created attachment 234075 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 234070 [details] patch rebased Attachment 234070 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4784338025578496 New failing tests: fast/canvas/canvas-color-serialization.html
Created attachment 234077 [details] Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 234103 [details] Fixed root cause of the canvas-color test failure.
Comment on attachment 234103 [details] Fixed root cause of the canvas-color test failure. View in context: https://bugs.webkit.org/attachment.cgi?id=234103&action=review > Source/WebCore/css/CSSParser.cpp:1371 > - color = document->page()->theme().systemColor(id).rgb(); > - return true; > + Color parsedColor = document->page()->theme().systemColor(id); > + color = parsedColor.rgb(); > + return parsedColor.isValid(); This seems fine, but I'm quite concerned that there was a bug that only manifests with this patch applied and that we can't reproduce normally. > Source/WebCore/css/CSSParser.cpp:3123 > + if (value->id == CSSValueTrue || value->id == CSSValueSafe) > + overflowAlignmentKeyword = cssValuePool().createIdentifierValue(value->id); > + else > + return false; Please write this the other way around, as an early return. if (id != True && id != Safe) return false; overflowAlignmentKeyword = ... > Source/WebCore/css/CSSParser.cpp:3132 > + if (isItemPositionKeyword(value->id)) > + position = cssValuePool().createIdentifierValue(value->id); > + else > + return false; Same here. > Source/WebCore/css/CSSParser.cpp:3144 > + addProperty(propId, createPrimitiveValuePair(position, overflowAlignmentKeyword), important); > + else > + addProperty(propId, position.release(), important); I assume createPrimitiveValuePair releases position? Do you even need to do this? > Source/WebCore/css/CSSValueKeywords.in:563 > + > + Nit: double space
(In reply to comment #20) > (From update of attachment 234103 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=234103&action=review > > > Source/WebCore/css/CSSParser.cpp:1371 > > - color = document->page()->theme().systemColor(id).rgb(); > > - return true; > > + Color parsedColor = document->page()->theme().systemColor(id); > > + color = parsedColor.rgb(); > > + return parsedColor.isValid(); > > This seems fine, but I'm quite concerned that there was a bug that only manifests with this patch applied and that we can't reproduce normally. > I've been investigating and finally got the answer to this weird issue. First of all, I think the portion patch specific to the parseSystemColor is clearly necessary, since the function should return whether the color value, passed as string, is a valid parsed color or not. Current implementation returns false only if the string is not resolved to a valid CSSValueKeywordID. The canvas-color-serialization.html test fails when trying invalid color values, assuming they are not valid keywords, when passing "true" as color. The reason why this patch, apparently unrelated to the color parsing logic, makes the test to fail is that the CSSValueTrue keyword is defined for the overflow-alignment "true". Hence, "true" is now a valid CSSValueKeywordID and current implementation does not detect, because of insufficient/wrong checks, that is not a valid color. So, the parseSystemColor should return parsedColor.isValid(); instead. I filed a separated bug (#f134661) or this issue, so I could remove that portion of the reviewed patch and land a cleaner one, focusing only in the justify-self parsing logic.
Created attachment 234664 [details] Patch Parch rebased. Also, removed the canvas color fix because it was already fixed in r170933.
Created attachment 234754 [details] Patch Parch rebase and ready for landing.
Comment on attachment 234754 [details] Patch Attachment 234754 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5090585635454976 New failing tests: media/media-fragments/TC0001.html
Created attachment 234758 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 234774 [details] Patch Retrying the same patch.
Comment on attachment 234774 [details] Patch Clearing flags on attachment: 234774 Committed r171010: <http://trac.webkit.org/changeset/171010>
All reviewed patches have been landed. Closing bug.