Summary: | [CSS Regions] Fix WHITESPACE issues in the CSS grammar. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mihai Maerean <mmaerean> | ||||||||||||
Component: | WebCore Misc. | Assignee: | Mihai Maerean <mmaerean> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | commit-queue, darin, esprehn+autocc, glenn, gyuyoung.kim, kling, macpherson, menard, mibalan, mihnea, rune, WebkitBugTracker | ||||||||||||
Priority: | P2 | Keywords: | AdobeTracked | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
URL: | https://codereview.chromium.org/25607005 | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 57312 | ||||||||||||||
Attachments: |
|
Description
Mihai Maerean
2013-10-20 03:46:06 PDT
Created attachment 214713 [details]
patch
Comment on attachment 214713 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=214713&action=review > Source/WebCore/css/CSSGrammar.y.in:1589 > + space '+' space { Why are '+' and '-' treated differently than '*' and '/' below? Do you need to add space: definition above? Comment on attachment 214713 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=214713&action=review >> Source/WebCore/css/CSSGrammar.y.in:1589 >> + space '+' space { > > Why are '+' and '-' treated differently than '*' and '/' below? Do you need to add space: definition above? The grammar requires spaces around binary ‘+’ and ‘-’ operators. The ‘*’ and ‘/’ operators do not require spaces. http://www.w3.org/TR/css3-values/#calc-syntax An expression like 70px+40px should return an error. Using the same form as for * and / leads to the calc expressions without the whitespaces to succeed instead of returning an error, which breaks the css3/calc/calc-errors.html test. I will upload a new patch that includes these comments to why explain the changes are needed. Created attachment 214733 [details]
patch. incorporates the feedback.
Created attachment 214739 [details]
patch. incorporates the feedback: add test for the '-' (minus) operator.
Created attachment 214741 [details]
patch
replace ’ (â) character from the html page with the ' character.
Comment on attachment 214741 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=214741&action=review > Source/WebCore/css/CSSGrammar.y.in:373 > +space: WHITESPACE| space WHITESPACE ; There should be a space between WHITESPACE and | Created attachment 214743 [details]
patch. incorporates the feedback: There should be a space between WHITESPACE and |
Comment on attachment 214743 [details] patch. incorporates the feedback: There should be a space between WHITESPACE and | Rejecting attachment 214743 [details] from commit-queue. mmaerean@adobe.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights. Comment on attachment 214743 [details] patch. incorporates the feedback: There should be a space between WHITESPACE and | View in context: https://bugs.webkit.org/attachment.cgi?id=214743&action=review > Source/WebCore/css/CSSGrammar.y.in:373 > +/* for expressions that require at least one whitespace to be present, like the + and - operators in calc expressions */ > +space: WHITESPACE | space WHITESPACE ; I don’t understand why this is needed. WHITESPACE should match an arbitrarily long run of whitespace, so this “space” should be no different than just WHITESPACE. Can you construct a test that fails if we use WHITESPACE instead of space in calc_func_operator? If not, please don’t add this production. Comment on attachment 214743 [details] patch. incorporates the feedback: There should be a space between WHITESPACE and | View in context: https://bugs.webkit.org/attachment.cgi?id=214743&action=review > Source/WebCore/css/CSSGrammar.y.in:1592 > + * The grammar requires spaces around binary â+â and â-â operators. > + * The '*' and '/' operators do not require spaces. > + * http://www.w3.org/TR/css3-values/#calc-syntax Where’s the test coverage for this? I’d rather omit this comment but include thorough test coverage demonstrating parse failure when the + and - don’t have spaces on either side. Comment on attachment 214743 [details] patch. incorporates the feedback: There should be a space between WHITESPACE and | Clearing flags on attachment: 214743 Committed r157720: <http://trac.webkit.org/changeset/157720> (In reply to comment #0) > A single WHITESPACE token consumes consecutive spaces, but does not consume > spaces separated by comments. So this secret is the reason we can never use WHITESPACE. But this is only in the bug report, not in the comments. That’s unnecessarily mysterious. I think the patch is fine to land as is, but I do think it’s got some unclear parts. Comment on attachment 214743 [details] patch. incorporates the feedback: There should be a space between WHITESPACE and | View in context: https://bugs.webkit.org/attachment.cgi?id=214743&action=review >> Source/WebCore/css/CSSGrammar.y.in:1592 >> + * http://www.w3.org/TR/css3-values/#calc-syntax > > Where’s the test coverage for this? I’d rather omit this comment but include thorough test coverage demonstrating parse failure when the + and - don’t have spaces on either side. The test you mention already exists: http://trac.webkit.org/browser/trunk/LayoutTests/css3/calc/calc-errors.html |