Bug 72331 - Add the needed plumbing to parse display: -webkit-grid
Summary: Add the needed plumbing to parse display: -webkit-grid
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks: 60731
  Show dependency treegraph
 
Reported: 2011-11-14 16:55 PST by Julien Chaffraix
Modified: 2011-11-15 15:05 PST (History)
4 users (show)

See Also:


Attachments
Proposed plumbing 1. (12.47 KB, patch)
2011-11-14 17:08 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Proposed fix 2: Add all platforms to Skipped list. Moved the ASSERT as it made more sense after thinking about it. (15.91 KB, patch)
2011-11-15 13:07 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (16.02 KB, patch)
2011-11-15 13:53 PST, Julien Chaffraix
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2011-11-14 16:55:50 PST
Let's add the needed CSS parsing of the new display value including some tests.

Simple patch forthcoming.
Comment 1 Julien Chaffraix 2011-11-14 17:08:53 PST
Created attachment 115066 [details]
Proposed plumbing 1.
Comment 2 WebKit Review Bot 2011-11-14 17:12:14 PST
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 3 Tony Chang 2011-11-15 09:30:12 PST
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 4 Julien Chaffraix 2011-11-15 09:41:30 PST
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!
Comment 5 Julien Chaffraix 2011-11-15 13:07:40 PST
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 6 Tony Chang 2011-11-15 13:37:39 PST
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 7 Julien Chaffraix 2011-11-15 13:49:56 PST
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.
Comment 8 Julien Chaffraix 2011-11-15 13:53:51 PST
Created attachment 115237 [details]
Patch for landing
Comment 9 WebKit Review Bot 2011-11-15 14:40:34 PST
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
Comment 10 Julien Chaffraix 2011-11-15 15:05:46 PST
Committed r100338: <http://trac.webkit.org/changeset/100338>