Bug 72331

Created attachment 115066 [details] Proposed plumbing 1.

Attachment 115066 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Last 3072 characters of output: pectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3853: Path does not exist. fast/js/regexp-caching.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3854: Path does not exist. fast/js/toString-overrides.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3855: Path does not exist. fast/js/toString-recursion.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3856: Path does not exist. http/tests/security/xss-eval.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3858: Path does not exist. fast/dom/javascript-url-exception-isolation.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3859: Path does not exist. fast/borders/inline-mask-overlay-image-outset-vertical-rl.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3861: Path does not exist. fast/dom/rtl-scroll-to-leftmost-and-resize.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3863: Path does not exist. accessibility/adjacent-continuations-cause-assertion-failure.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3865: Path does not exist. inspector/debugger/script-formatter.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3868: Path does not exist. fast/js/mozilla/strict/15.4.5.1.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3870: Path does not exist. fast/canvas/canvas-transforms-fillRect-shadow.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3872: Path does not exist. media/track/tracklist-is-reachable.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3873: Path does not exist. media/track/track-cues-cuechange.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3875: Path does not exist. http/tests/inspector-enabled/dedicated-workers-list.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3876: Path does not exist. http/tests/inspector-enabled/dedicated-workers-list.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3878: Path does not exist. http/tests/security/cross-frame-access-custom.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3880: Path does not exist. fast/loader/javascript-url-in-embed.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3882: Path does not exist. security/crypto-random-values-types.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3884: Path does not exist. media/track/track-webvtt-tc004-magic-header.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3886: Path does not exist. http/tests/inspector/resource-tree/resource-tree-frame-add.html [test/expectations] [2] Total errors found: 2130 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 115066 [details] Proposed plumbing 1. View in context: https://bugs.webkit.org/attachment.cgi?id=115066&action=review I think the style bot error is not related to your change. r- for missing Skipped file entries. > Source/WebCore/rendering/style/RenderStyle.h:868 > + void setDisplay(EDisplay v) { ASSERT(v >= INLINE && v <= NONE); noninherited_flags._effectiveDisplay = v; } > + void setOriginalDisplay(EDisplay v) { ASSERT(v >= INLINE && v <= NONE); noninherited_flags._originalDisplay = v; } Isn't the assert redundant with the type checking done by the compiler? As in, if it's a valid number for EDisplay, it's between INLINE and NONE. > LayoutTests/ChangeLog:15 > + * platform/chromium/test_expectations.txt: > + SKIP the css-grid-layout tests for now. Do we need to add fast/css-grid-layout to all the main (win/mac/gtk/qt) Skipped files as well?

Comment on attachment 115066 [details] Proposed plumbing 1. View in context: https://bugs.webkit.org/attachment.cgi?id=115066&action=review >> Source/WebCore/rendering/style/RenderStyle.h:868 >> + void setOriginalDisplay(EDisplay v) { ASSERT(v >= INLINE && v <= NONE); noninherited_flags._originalDisplay = v; } > > Isn't the assert redundant with the type checking done by the compiler? As in, if it's a valid number for EDisplay, it's between INLINE and NONE. No. CSSPrimitiveValueMappings.h is doing a cast from unsigned to EDisplay in operator EDisplay() which defeats any type checking done by the compiler :( This check could be moved to CSSPrimitiveValueMappings.h if you prefer but my feeling is that it is safer here. >> LayoutTests/ChangeLog:15 >> + SKIP the css-grid-layout tests for now. > > Do we need to add fast/css-grid-layout to all the main (win/mac/gtk/qt) Skipped files as well? Good catch!

Created attachment 115224 [details] Proposed fix 2: Add all platforms to Skipped list. Moved the ASSERT as it made more sense after thinking about it.

Comment on attachment 115224 [details] Proposed fix 2: Add all platforms to Skipped list. Moved the ASSERT as it made more sense after thinking about it. View in context: https://bugs.webkit.org/attachment.cgi?id=115224&action=review > Source/WebCore/ChangeLog:23 > + * css/CSSPrimitiveValueMappings.h: > + (WebCore::CSSPrimitiveValue::operator EDisplay): Nit: Looks like we could merge this with the css/CSSPrimitiveValueMappings.h above. > Source/WebCore/ChangeLog:28 > + * css/CSSValueKeywords.in: Nit: This looks like a duplicate entry.

Comment on attachment 115224 [details] Proposed fix 2: Add all platforms to Skipped list. Moved the ASSERT as it made more sense after thinking about it. View in context: https://bugs.webkit.org/attachment.cgi?id=115224&action=review >> Source/WebCore/ChangeLog:23 >> + (WebCore::CSSPrimitiveValue::operator EDisplay): > > Nit: Looks like we could merge this with the css/CSSPrimitiveValueMappings.h above. We could, though I don't think it would be more readable. I really want to underline this related-but-not-strictly-plumbing change. >> Source/WebCore/ChangeLog:28 >> + * css/CSSValueKeywords.in: > > Nit: This looks like a duplicate entry. Good catch will be changed.

Created attachment 115237 [details] Patch for landing

Comment on attachment 115237 [details] Patch for landing Rejecting attachment 115237 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: at 3268. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej patching file LayoutTests/platform/efl/Skipped patching file LayoutTests/platform/gtk/Skipped patching file LayoutTests/platform/mac/Skipped patching file LayoutTests/platform/qt/Skipped patching file LayoutTests/platform/win/Skipped patching file LayoutTests/platform/wincairo/Skipped Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/10310710

Committed r100338: <http://trac.webkit.org/changeset/100338>