Summary: | CSS Grid Layout: Add support for parsing multiple grid-columns or grid-rows | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> | ||||||||
Component: | CSS | Assignee: | Julien Chaffraix <jchaffraix> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | macpherson, ojan, tony, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 60731 | ||||||||||
Attachments: |
|
Description
Julien Chaffraix
2011-11-28 17:37:11 PST
Created attachment 116867 [details]
Proposed change 1: implement the new syntax.
Attachment 116867 [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
Source/WebCore/css/CSSStyleSelector.cpp:2522: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 10 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 116867 [details] Proposed change 1: implement the new syntax. View in context: https://bugs.webkit.org/attachment.cgi?id=116867&action=review > Source/WebCore/css/CSSStyleSelector.cpp:2520 > + if (type == CSSPrimitiveValue::CSS_PERCENTAGE) > + length = Length(primitiveValue->getDoubleValue(), Percent); > + return true; > + } Missing { here. I think that confused the style checker. > Source/WebCore/css/CSSStyleSelector.cpp:2544 > + // Handle the special property 'none'. Nit: You might want the comment to say how you 'none' is different. Comment on attachment 116867 [details] Proposed change 1: implement the new syntax. View in context: https://bugs.webkit.org/attachment.cgi?id=116867&action=review >> Source/WebCore/css/CSSStyleSelector.cpp:2520 >> + } > > Missing { here. I think that confused the style checker. Good catch, I missed it and thought style checker was confused. >> Source/WebCore/css/CSSStyleSelector.cpp:2544 >> + // Handle the special property 'none'. > > Nit: You might want the comment to say how you 'none' is different. I am not sure I follow your nit here. Do you mind clarifying? Comment on attachment 116867 [details] Proposed change 1: implement the new syntax. View in context: https://bugs.webkit.org/attachment.cgi?id=116867&action=review >>> Source/WebCore/css/CSSStyleSelector.cpp:2544 >>> + // Handle the special property 'none'. >> >> Nit: You might want the comment to say how you 'none' is different. > > I am not sure I follow your nit here. Do you mind clarifying? Something like, "We use a single undefined length to specify a track-list of 'none'." Or maybe just remove the comment. Comment on attachment 116867 [details] Proposed change 1: implement the new syntax. View in context: https://bugs.webkit.org/attachment.cgi?id=116867&action=review >>>> Source/WebCore/css/CSSStyleSelector.cpp:2544 >>>> + // Handle the special property 'none'. >>> >>> Nit: You might want the comment to say how you 'none' is different. >> >> I am not sure I follow your nit here. Do you mind clarifying? > > Something like, "We use a single undefined length to specify a track-list of 'none'." Or maybe just remove the comment. Fine enough, I will remove it in the patch to land. I have seen a couple of missing of return in the implementation. I don't understand why it was not caught by the cases but I need to investigate before landing this patch as it may mean that our current test coverage is not good enough. > I have seen a couple of missing of return in the implementation. I don't understand why it was not caught by the cases but I need to investigate before landing this patch as it may mean that our current test coverage is not good enough.
I found out the issue: some code was unneeded (basically I was trying to handle several values on a CSSPrimitiveValue which was never reached). I have extended the testing a bit and removed this dead code in the to-be-landed-soon patch.
Created attachment 117982 [details]
Patch for landing
Comment on attachment 117982 [details]
Patch for landing
Something is fishy on this patch.
(In reply to comment #9) > (From update of attachment 117982 [details]) > Something is fishy on this patch. Actually it was fine, I just needed to double-check the logic. My comment #7 was half true: some code was unneeded and it is not because it would try to parse several values for a CSSPrimitiveValue but because the parser consider most of our input as a CSSValueList (even for 1 item). Created attachment 117987 [details]
Patch for landing for tomorrow.
Comment on attachment 117987 [details] Patch for landing for tomorrow. Rejecting attachment 117987 [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: 112974 --non-interactive --force --accept theirs-conflict --ignore-externals returned non-zero exit status 1 in /mnt/git/webkit-commit-queue/Source/WebKit/chromium Error: 'depot_tools/gclient sync --force --reset --delete_unversioned_trees' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 109. Re-trying 'depot_tools/gclient sync --force --reset --delete_unversioned_trees' No such file or directory at /mnt/git/webkit-commit-queue/Tools/Scripts/webkitdirs.pm line 2078. Full output: http://queues.webkit.org/results/10736846 Committed r102183: <http://trac.webkit.org/changeset/102183> |