Bug 168771

Created attachment 302504 [details] Patch

Created attachment 302506 [details] Patch

Comment on attachment 302506 [details] Patch Attachment 302506 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3178618 New failing tests: imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-a-bitrate.html

Created attachment 302508 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 302506 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302506&action=review Nice work on this, the patch LGTM but I've some comments on the test. > Source/WebCore/ChangeLog:15 > + Test: css3/parse-place-content.html Nit: Bad indentation on this line. > Source/WebCore/css/parser/CSSPropertyParser.cpp:5265 > + if (identMatches<CSSValueStart, CSSValueEnd, CSSValueCenter, CSSValueFlexStart, CSSValueFlexEnd, CSSValueLeft, CSSValueRight>(id)) > + return CSSContentDistributionValue::create(CSSValueInvalid, range.consumeIncludingWhitespace().id(), CSSValueInvalid); Couldn't this be merged with the first "if"? > LayoutTests/css3/parse-place-content-expected.txt:21 > +FAIL Test setting values through JS. assert_equals: placeContent specified value is not what it should.. expected "center" but got "center center" Do we want a FAIL here? > LayoutTests/css3/parse-place-content.html:61 > + <p>Test to verify that the new alignment values are parsed as invalid if Grid Layout is disabled and in any case they do not cause a crash because assertions in flexbox layout code.</p> Is this accurate? Where are you disabling Grid Layout? > LayoutTests/css3/resources/alignment-parsing-utils-th.js:5 > + assert_equals(eval('element.style.' + property), value, property + ' specified value is not what it should..'); Nit: Extra dot at the end. Why do you need the eval() call? > LayoutTests/css3/resources/alignment-parsing-utils-th.js:6 > + assert_equals(eval("window.getComputedStyle(" + elementID + ", '').getPropertyValue('" + propertyID + "')"), computedValue, property + " is not what is should."); Nit: Typo "is". > LayoutTests/css3/resources/alignment-parsing-utils-th.js:9 > +function checkBadValues(element, property, propertyID, value) You're not calling this function. > LayoutTests/css3/resources/alignment-parsing-utils-th.js:17 > +function checkInitialValues(element, property, propertyID, value, initial) Ditto. > LayoutTests/css3/resources/alignment-parsing-utils-th.js:25 > +function checkInheritValues(property, propertyID, value) Ditto. > LayoutTests/css3/resources/alignment-parsing-utils-th.js:38 > +function checkLegacyValues(property, propertyID, value) Ditto. > LayoutTests/css3/resources/alignment-parsing-utils-th.js:50 > +function checkSupportedValues(elementID, property) Ditto.

Comment on attachment 302506 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302506&action=review >> Source/WebCore/css/parser/CSSPropertyParser.cpp:5265 >> + return CSSContentDistributionValue::create(CSSValueInvalid, range.consumeIncludingWhitespace().id(), CSSValueInvalid); > > Couldn't this be merged with the first "if"? The first one checks: normal | <baseline-position> The second one checks: <content-position> The third one checks: <item-position> I'd rather keep these 3 groups separated, even if some of them imply the same return value. I think this structure of the code matches better the spec syntax. >> LayoutTests/css3/parse-place-content-expected.txt:21 >> +FAIL Test setting values through JS. assert_equals: placeContent specified value is not what it should.. expected "center" but got "center center" > > Do we want a FAIL here? Yeah, this is because we still don't know how to serialize the shorthand value and WK seems to do different than Blink. I'll add a comment to explain why we deliberately fail, but I'd prefer to keep the test as it is, for now. >> LayoutTests/css3/resources/alignment-parsing-utils-th.js:9 >> +function checkBadValues(element, property, propertyID, value) > > You're not calling this function. This script file is just an adaptation of the one we already have for the alignment related tests, but in this case, using the testharness.th functions. Even if some of these functions are not used by my test, I think it's useful to have the whole file already adapted for future tests or for adapting the ones we already have.

Created attachment 302514 [details] Patch Applied suggested changes.

Comment on attachment 302514 [details] Patch Attachment 302514 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3179232 New failing tests: editing/spelling/spellcheck-async.html

Created attachment 302521 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 302514 [details] Patch Attachment 302514 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3179459 New failing tests: media/modern-media-controls/volume-down-support/volume-down-support.html fast/dom/timer-throttling-hidden-page-non-nested.html

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

Comment on attachment 302514 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302514&action=review > LayoutTests/css3/parse-place-content.html:217 > + // We will get FAIL for placeContent specified values tests because we still don't know how to serialize this shortand. Nit: Please add "FIXME" here. > LayoutTests/css3/resources/alignment-parsing-utils-th.js:9 > +function checkBadValues(element, property, propertyID, value) I'm still not convinced to add so many methods that we don't use. Are you sure we're going to use them in the future?

Created attachment 303059 [details] Patch

Comment on attachment 303059 [details] Patch Clearing flags on attachment: 303059 Committed r213230: <http://trac.webkit.org/changeset/213230>

All reviewed patches have been landed. Closing bug.

Comment on attachment 303059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303059&action=review > Source/WebCore/css/CSSProperties.json:3094 > + "place-content": { > + "codegen-properties": { > + "longhands": [ > + "align-content", > + "justify-content" > + ] > + } > + }, You should have added a "specification" section here.

Comment on attachment 303059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303059&action=review >> Source/WebCore/css/CSSProperties.json:3094 >> + }, > > You should have added a "specification" section here. I'm not sure what that "specification" section would be; is there any property in the CSSProperties.json file I can use as example ?

(In reply to comment #17) > Comment on attachment 303059 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=303059&action=review > > >> Source/WebCore/css/CSSProperties.json:3094 > >> + }, > > > > You should have added a "specification" section here. > > I'm not sure what that "specification" section would be; is there any > property in the CSSProperties.json file I can use as example ? Update to TOT. You'll see lots of them.