RESOLVED DUPLICATE of bug 78538 34067
CSS rule containing a string unclosed at the end of stylesheet is ignored. 
https://bugs.webkit.org/show_bug.cgi?id=34067
Summary CSS rule containing a string unclosed at the end of stylesheet is ignored.
Yuzo Fujishima
Reported 2010-01-24 21:45:47 PST
http://www.w3.org/TR/CSS21/syndata.html#parsing-errors specifies how unclosed strings must be handled. For example, <style type="text/css"> p:before { content: 'Has not </style> must be handled as if it were: <style type="text/css"> p:before { content: 'Has not '; } </style>
Attachments
Testcase (247 bytes, text/html)
2010-01-24 21:47 PST, Yuzo Fujishima 		no flags
Details
Change flex rule such that unclosed string is properly closed. (10.17 KB, patch)
2010-01-24 21:57 PST, Yuzo Fujishima 		no flags
DetailsFormatted DiffDiff
Fixed the test case - in standards mode now. (216 bytes, text/html)
2010-01-26 22:49 PST, Robert Blaut 		no flags
Details
Tests equivalent to those in the patch (1.03 KB, text/html)
2010-01-27 00:39 PST, Yuzo Fujishima 		no flags
Details
Parsing unclosed strings - fixed one test case and more cases (1.62 KB, text/html)
2010-01-27 04:31 PST, Robert Blaut 		no flags
Details
Change flex rule such that unclosed string is properly closed. (12.08 KB, patch)
2010-01-28 03:46 PST, Yuzo Fujishima 		no flags
DetailsFormatted DiffDiff
New testcase (1.76 KB, text/html)
2010-01-28 03:49 PST, Yuzo Fujishima 		no flags
Details
Change flex rule such that unclosed string/url is properly handled. (19.45 KB, patch)
2010-01-28 23:58 PST, Yuzo Fujishima 		no flags
DetailsFormatted DiffDiff
Change flex rule such that unclosed string/url is properly handled. (19.64 KB, patch)
2010-02-07 22:35 PST, Yuzo Fujishima 		no flags
DetailsFormatted DiffDiff
Change flex rule such that unclosed string/url is properly handled. (20.05 KB, patch)
2010-02-11 21:39 PST, Yuzo Fujishima 		no flags
DetailsFormatted DiffDiff
Change flex rule such that unclosed string/url is properly handled. (19.28 KB, patch)
2010-02-15 23:45 PST, Yuzo Fujishima 		no flags
DetailsFormatted DiffDiff
Change flex rule such that unclosed string/url is properly handled. (19.68 KB, patch)
2010-02-17 00:49 PST, Yuzo Fujishima 		no flags
DetailsFormatted DiffDiff
Fix unclosed string/url handling. (23.04 KB, patch)
2010-04-07 17:49 PDT, Yuzo Fujishima 		no flags
DetailsFormatted DiffDiff
Fix unclosed string/url handling. (22.98 KB, patch)
2010-04-07 17:53 PDT, Yuzo Fujishima 		no flags
DetailsFormatted DiffDiff
Fix unclosed string/url handling. (24.22 KB, patch)
2010-04-28 00:52 PDT, Yuzo Fujishima 		no flags
DetailsFormatted DiffDiff
Patch (27.11 KB, patch)
2010-08-09 01:43 PDT, Yuzo Fujishima 		no flags
DetailsFormatted DiffDiff
Pre Source/WebCore move (4.55 KB, patch)
2011-01-11 00:24 PST, Yuzo Fujishima 		no flags
DetailsFormatted DiffDiff
Show Obsolete (14) View All Add attachment proposed patch, testcase, etc.
Yuzo Fujishima
Comment 1 2010-01-24 21:47:04 PST
Created attachment 47310 [details] Testcase
Yuzo Fujishima
Comment 2 2010-01-24 21:57:00 PST
Created attachment 47312 [details] Change flex rule such that unclosed string is properly closed.
Yuzo Fujishima
Comment 3 2010-01-24 22:00:09 PST
I think 6891 cannot be fixed until the unclosed string handling is fixed. Yuzo
Eric Seidel (no email)
Comment 4 2010-01-26 15:01:07 PST
Comment on attachment 47312 [details] Change flex rule such that unclosed string is properly closed. How does our current behavior compare to FF and IE?
Philippe Wittenbergh
Comment 5 2010-01-26 16:21:01 PST
I think this bug is invalid. The testcase is in quirks mode; once made into a standard mode test (with <!doctype html>), I get the following results Firefox (Gecko 1.9.0 ~ 19.3a1pre) --> only show 'failed' Opera 10.5a --> same as Gecko IE8 --> same as Gecko. testcase: http://dev.l-c-n.com/CSS2_parse/parse_content2.html (there are a few more tests where WebKit show different behaviour than Gecko & Opera in the same /CSS2_parse/ folder; I haven't fully pondered them ) See the bottom of the section, under the bullet 'Unexpected end of string' http://www.w3.org/TR/CSS21/syndata.html#parsing-errors
Robert Blaut
Comment 6 2010-01-26 22:49:30 PST
Created attachment 47498 [details] Fixed the test case - in standards mode now.
Robert Blaut
Comment 7 2010-01-26 22:56:12 PST
Yuzo, you are talking about unclosed strings but your test case shows a case with unexpected end of style sheets. Compare this clause: "Unexpected end of style sheet. User agents must close all open constructs (for example: blocks, parentheses, brackets, rules, strings, and comments) at the end of the style sheet. For example: @media screen { p:before { content: 'Hello would be treated the same as: @media screen { p:before { content: 'Hello'; } } in a conformant UA." with this: "Unexpected end of string. User agents must close strings upon reaching the end of a line, but then drop the construct (declaration or rule) in which the string was found. For example: p { color: green; font-family: 'Courier New Times color: red; color: green; } ...would be treated the same as: p { color: green; color: green; } ...because the second declaration (from 'font-family' to the semicolon after 'color: red') is invalid and is dropped." Both cases are different. So I assume the bug report title is incorrect. The test case uploaded by me is rendered correctly by Opera 10.50 alfa only. WebKit and Firefox 3.6 fails in this case.
Robert Blaut
Comment 8 2010-01-26 22:59:16 PST
(In reply to comment #5) > testcase: > http://dev.l-c-n.com/CSS2_parse/parse_content2.html Philip, your test case is invalid now. Check mismatched selector div:before. I assume you mean p:before.
Philippe Wittenbergh
Comment 9 2010-01-26 23:11:50 PST
(In reply to comment #8) > (In reply to comment #5) > > testcase: > > http://dev.l-c-n.com/CSS2_parse/parse_content2.html > > Philip, your test case is invalid now. Check mismatched selector div:before. I > assume you mean p:before. Oopsie, uploaded the wrong one. Corrected. Thanks.
Yuzo Fujishima
Comment 10 2010-01-26 23:50:30 PST
Thanks. Changed the bug summary. Is this good enough? BTW, I'd like to keep test 1 and 2 in the patch, to assure that the patch doesn't break them, although only test 3 is for "unclosed at the end of stylesheet" case.
Yuzo Fujishima
Comment 11 2010-01-26 23:53:19 PST
Eric, The following are test results. Safari 4.0 WITH this patch passes all of them. The attached Testcases, both quirks and standards modes: IE 8.0: FAIL FF 3.5: FAIL Opera 10.10: PASS Safari 4.0 WITHOUT this patch: FAIL Test 1 and 2 in the patch (as far as I can tell by watching the screen): IE 8.0: FAIL FF 3.5: PASS Opera 10.10: PASS Safari 4.0 WITHOUT this patch: PASS Test 3 (ditto): IE 8.0: FAIL FF 3.5: FAIL Opera 10.10: FAIL Safari 4.0 WITHOUT this patch: FAIL (Opera fails to handle unclosed string for font-family property but can handle it for content property. Hmm.)
Robert Blaut
Comment 12 2010-01-27 00:00:20 PST
(In reply to comment #11) > The following are test results. Yuzo, could you link to all mentioned test cases? I'd like to check them.
Yuzo Fujishima
Comment 13 2010-01-27 00:39:46 PST
Created attachment 47505 [details] Tests equivalent to those in the patch These tests should be meaningful for most browsers.
Yuzo Fujishima
Comment 14 2010-01-27 00:44:18 PST
Hi, Robert, Eric, I've attached the tests ("Tests equivalent to those in the patch") for other browsers. Correction: Test 3 passes for IE 8.0. Test 3 IE 8.0: PASS FF 3.5: FAIL Opera 10.10: FAIL Safari 4.0 WITHOUT this patch: FAIL Yuzo
Robert Blaut
Comment 15 2010-01-27 04:31:51 PST
Created attachment 47517 [details] Parsing unclosed strings - fixed one test case and more cases Yuzo, The test case 3 is incorrect and IE renders it incorrectly. Firefox and Opera are right in this case. Consider this fragment: font-family: 'Arial </style> It means that there is not only 'Arial' word but also new line in a string. So above code is equivalent to: font-family: 'Arial\'; } I've corrected your test cases and added 2 more. Only Firefox 3.6 and Opera 10.50 renders it correctly. WebKit fails in tests: 3,4,5.
Robert Blaut
Comment 16 2010-01-27 04:34:51 PST
Also notice the other bug visible in WebKit in test 3. WebKit treats: font-family: 'Arial\'; as font-family: serif; Webkit should match no font-family in this case.
Yuzo Fujishima
Comment 17 2010-01-28 03:46:50 PST
Created attachment 47602 [details] Change flex rule such that unclosed string is properly closed.
Yuzo Fujishima
Comment 18 2010-01-28 03:49:07 PST
Created attachment 47603 [details] New testcase
WebKit Review Bot
Comment 19 2010-01-28 03:54:14 PST
Attachment 47602 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/css/CSSParser.cpp:5344: yy_more_flag is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/css/CSSParser.cpp:5345: yy_more_len is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 If any of these errors are false positives, please file a bug against check-webkit-style.
Yuzo Fujishima
Comment 20 2010-01-28 03:59:49 PST
I've changed the patch such that WebKit behaves similarly to other browsers. New testcase results: (test 1, test 2, test 3, test 4) IE 8.0: (FAIL, FAIL, PASS, FAIL) FF 3.6: (PASS, PASS, PASS, PASS) Opera 10.10: (PASS, PASS, PASS, PASS) r53468: (PASS, PASS, FAIL, PASS) This patch: (PASS, PASS, PASS, PASS) Yuzo
WebKit Review Bot
Comment 21 2010-01-28 05:00:21 PST
Attachment 47602 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/216671
WebKit Review Bot
Comment 22 2010-01-28 05:00:34 PST
Attachment 47602 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/216673
Yuzo Fujishima
Comment 23 2010-01-28 14:08:16 PST
Comment on attachment 47602 [details] Change flex rule such that unclosed string is properly closed. Sorry, this patch contains a few issues. Make it obsolete.
Yuzo Fujishima
Comment 24 2010-01-28 23:58:34 PST
Created attachment 47684 [details] Change flex rule such that unclosed string/url is properly handled.
Yuzo Fujishima
Comment 25 2010-01-29 00:02:16 PST
Hi, reviewers, Can you review the latest patch? Yuzo
WebKit Review Bot
Comment 26 2010-01-29 00:03:11 PST
Attachment 47684 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/css/CSSParser.cpp:5344: yy_more_flag is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/css/CSSParser.cpp:5345: yy_more_len is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 If any of these errors are false positives, please file a bug against check-webkit-style.
Eric Seidel (no email)
Comment 27 2010-02-01 15:35:37 PST
Comment on attachment 47684 [details] Change flex rule such that unclosed string/url is properly handled. Please explain the changes to fast/css/font_property_normal.html in the ChangeLog.
Yuzo Fujishima
Comment 28 2010-02-01 17:49:10 PST
Hi, Eric, Thank you for the review. LayoutTests/Changelog contains: Note that LayoutTests/fast/css/font_property_normal.html is changed because it has contained wrong quotes. Do you need more detailed description? Yuzo
Robert Blaut
Comment 29 2010-02-01 23:22:47 PST
(In reply to comment #27) > (From update of attachment 47684 [details]) > Please explain the changes to fast/css/font_property_normal.html in the > ChangeLog. There are quotes inserted there by mistake. Probably from copy and paste of https://bugs.webkit.org/show_bug.cgi?id=5564#c1 These quotes are now incorrectly ignored by WebKit. This stylesheet: <STYLE type="text/css"> .one {font: 24pt italic;"} .two {font: 24pt italic Arial;} .three {font: 24pt italic 'Arial';} .four {font: italic 24pt;} .five {font: italic 24pt Arial;"} .six {font: italic 24pt 'Arial';} .seven {font: Arial 24pt italic;} .eight {font: 'Arial' 24pt italic;} .nine {font: Arial italic 24pt;} .ten {font: 'Arial' italic 24pt;} </STYLE> according CSS 2.1 - http://www.w3.org/TR/CSS21/syndata.html#block - "Single (') and double quotes (") must also occur in matching pairs, and characters between them are parsed as a string." is equivalent to this stylesheet: <STYLE type="text/css"> .one {font: 24pt italic;} .six {font: italic 24pt 'Arial';} .seven {font: Arial 24pt italic;} .eight {font: 'Arial' 24pt italic;} .nine {font: Arial italic 24pt;} .ten {font: 'Arial' italic 24pt;} </STYLE> After applying Yuzo patch the above mentioned test case should be fixed.
Yuzo Fujishima
Comment 30 2010-02-07 22:35:33 PST
Created attachment 48317 [details] Change flex rule such that unclosed string/url is properly handled.
Yuzo Fujishima
Comment 31 2010-02-07 22:38:22 PST
Hi, Eric, Can you take another look? As to fast/css/font_property_normal.html, I've quoted Robert's comment above. As to the style error, I believe we should ignore it because the names are defined by flex. Yuzo
WebKit Review Bot
Comment 32 2010-02-07 22:41:14 PST
Attachment 48317 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/css/CSSParser.cpp:5344: yy_more_flag is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/css/CSSParser.cpp:5345: yy_more_len is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 If any of these errors are false positives, please file a bug against check-webkit-style.
Yuzo Fujishima
Comment 33 2010-02-10 01:37:04 PST
As to the style check failure, please see https://bugs.webkit.org/show_bug.cgi?id=34787
Yuzo Fujishima
Comment 34 2010-02-11 21:39:26 PST
Created attachment 48621 [details] Change flex rule such that unclosed string/url is properly handled.
Yuzo Fujishima
Comment 35 2010-02-11 21:40:26 PST
Added NOLINT for yy_* identifiers.
Darin Adler
Comment 36 2010-02-12 11:17:23 PST
Comment on attachment 48621 [details] Change flex rule such that unclosed string/url is properly handled. > -typedef int yy_state_type; > +typedef int yy_state_type; // NOLINT What tool respects these comments? I have not seen them in WebKit before. > +#define yytext_ptr yytext Why is this added? The change log has no mention of it. Is it related to the rest of the changes somehow? > +static int yy_more_flag = 0; // NOLINT > +static int yy_more_len = 0; // NOLINT Why are we now defining these here? The change log does not explain how this change relates to what the patch is doing. > +#define yymore() ((yy_more_flag) = 1) Why is this a macro instead of a function? Why does it have a bison/flex style name? > +#define dquoted_string 3 > +#define squoted_string 4 > +#define uri 5 > +#define uri_pending 6 Why are you using underscores in these names? Can't these follow normal WebKit naming? > +#define YY_START (((yy_start) - 1) / 2) What is this for? > + int string_caller = INITIAL; > + int uri_caller = INITIAL; > + int content_length = 0; > + int content_offest = 0; Can't these follow normal WebKit naming conventions? I don't see any reason to use a different naming style here. These names seem a bit vague to me. What specific content is the "content" in "content offset"? I think we can do better naming this. I also see this code twice. Once in maketokenizer and once in the flex file. Is that correct? > + BEGIN(string_caller == uri ? uri_pending : string_caller); I'm not sure that "caller" is the clearest term we could use use here. Maybe "token" would be clearer (or perhaps "context" or "type"). I don't really understand what these represent, which I suppose is why I don't get what the names should be. > + default: > + yyterminate(); I think ASSERT_NOT_REACHED is more appropriate here than yyterminate. Unless this code can be reached. Do the new test cases cover all the code paths here? If I put an ASSERT_NOT_REACHED in each rule, would I have to remove them all to pass the test? I'm going to say review- for now because I think the new code is not well enough documented, does not follow WebKit style as much as it should, may not be well tested, and is hard for me to understand despite that fact that I am familiar with both CSS and flex. Maybe you can't solve all of those problems, but please try to solve at least some of them.
Yuzo Fujishima
Comment 37 2010-02-15 23:45:15 PST
Created attachment 48792 [details] Change flex rule such that unclosed string/url is properly handled.
Yuzo Fujishima
Comment 38 2010-02-15 23:49:10 PST
Hi, Darin, Thank you for reviewing this! (In reply to comment #36) > (From update of attachment 48621 [details]) > > -typedef int yy_state_type; > > +typedef int yy_state_type; // NOLINT > > What tool respects these comments? I have not seen them in WebKit before. WebKitTools/Scripts/check-webkit-style (more specifically, WebKitTools/Scripts/webkitpy/style/processors/cpp.py) respects them. Now that https://bugs.webkit.org/show_bug.cgi?id=34787 is closed, I removed the comments. > > > +#define yytext_ptr yytext > > Why is this added? The change log has no mention of it. Is it related to the > rest of the changes somehow? This is needed for flex to support yymore(). I've moved the definition to WebCore/css/maketokenizer. > > > +static int yy_more_flag = 0; // NOLINT > > +static int yy_more_len = 0; // NOLINT > > Why are we now defining these here? The change log does not explain how this > change relates to what the patch is doing. I've updated the Change log. Also, I've moved yy_more_{flag,len} to WebCore/css/maketokenizer. > > > +#define yymore() ((yy_more_flag) = 1) > > Why is this a macro instead of a function? Why does it have a bison/flex style > name? yymore() is a flex function (or macro): http://flex.sourceforge.net/manual/Actions.html#index-yymore_0028_0029-115 > > > +#define dquoted_string 3 > > +#define squoted_string 4 > > +#define uri 5 > > +#define uri_pending 6 > > Why are you using underscores in these names? Can't these follow normal WebKit > naming? They are flex start condition names. I've chosen to use underscores because mediaquery and forkeyword are not in CamelCase. > > > +#define YY_START (((yy_start) - 1) / 2) > > What is this for? This is now needed because I use YY_START in WebCore/css/tokenizer.flex. http://flex.sourceforge.net/manual/Start-Conditions.html#Start-Conditions I've moved it to WebCore/css/maketokenizer. > > > + int string_caller = INITIAL; > > + int uri_caller = INITIAL; > > + int content_length = 0; > > + int content_offest = 0; > > Can't these follow normal WebKit naming conventions? I don't see any reason to > use a different naming style here. They are used in tokenizer.flex where underscored names are used. I can change them to stringCaller, etc., if you prefer. > > These names seem a bit vague to me. What specific content is the "content" in > "content offset"? I think we can do better naming this. OK, renamed content to string_or_uri_content. > > I also see this code twice. Once in maketokenizer and once in the flex file. Is > that correct? I found that I can remove the ones in tokenizer.flex. Removed. > > > + BEGIN(string_caller == uri ? uri_pending : string_caller); > > I'm not sure that "caller" is the clearest term we could use use here. Maybe > "token" would be clearer (or perhaps "context" or "type"). I don't really > understand what these represent, which I suppose is why I don't get what the > names should be. http://flex.sourceforge.net/manual/Start-Conditions.html#Start-Conditions uses *_caller naming. That's why I named the variables that way. I'm open to changing the names if you prefer otherwise. > > > + default: > > + yyterminate(); > > I think ASSERT_NOT_REACHED is more appropriate here than yyterminate. Unless > this code can be reached. > > Do the new test cases cover all the code paths here? If I put an > ASSERT_NOT_REACHED in each rule, would I have to remove them all to pass the > test? The default case is reached if the start condition is either INITIAL, mediaquery, or forkeyword. > > I'm going to say review- for now because I think the new code is not well > enough documented, does not follow WebKit style as much as it should, may not > be well tested, and is hard for me to understand despite that fact that I am > familiar with both CSS and flex. Maybe you can't solve all of those problems, > but please try to solve at least some of them.
Yuzo Fujishima
Comment 39 2010-02-17 00:49:59 PST
Created attachment 48868 [details] Change flex rule such that unclosed string/url is properly handled.
Yuzo Fujishima
Comment 40 2010-02-17 00:50:55 PST
Added explanation to WebCore/ChangeLog.
Yuzo Fujishima
Comment 41 2010-03-08 00:29:31 PST
Ping? This is blocking 6891.
Yuzo Fujishima
Comment 42 2010-03-14 23:43:02 PDT
Ping again.
Eric Seidel (no email)
Comment 43 2010-03-15 13:16:54 PDT
Tip for the future: Information is always more useful in the ChangeLog than in the bug. Because ChangeLogs are what reviewers use during reviews, bugs (like this one) tend to get crowded and hard to read. In this case, I had to go digging to find if this made us match FF/IE A comment in the ChangeLog that this makes our behavior match various other browsers (listing which ones and how) would make this review easier.
Eric Seidel (no email)
Comment 44 2010-03-15 13:18:21 PDT
Basically in posting patches for review, you want to make it as easy as possible for a reviewer to say yes. Passing style, passing EWS bots, having tests, having a nice long ChangeLog, saying nice things in the ChangeLog like "fixes crash" or "matches other browsers", all make hitting that r+ button very easy. :)
Eric Seidel (no email)
Comment 45 2010-03-15 13:21:22 PDT
Comment on attachment 48868 [details] Change flex rule such that unclosed string/url is properly handled. As much as I woudl like to r+ this, I do not trust my yacc skills enough to give this a final r+. In general it looks good though.
Adam Barth
Comment 46 2010-03-22 09:08:03 PDT
I wanted to review this patch, but was unable to. It's unclear to me whether you actually address Darin's comments. You seemed to reject all of them, but I'm not sure whether he was satisfied with your responses. Who's a good person to review changes to the CSS parser?
Yuzo Fujishima
Comment 47 2010-03-31 23:39:38 PDT
Hi, Darin, Would you mind taking another look? Yuzo
Darin Adler
Comment 48 2010-04-06 09:20:12 PDT
(In reply to comment #38) > > > +#define yymore() ((yy_more_flag) = 1) > > > > Why is this a macro instead of a function? Why does it have a bison/flex style > > name? > > yymore() is a flex function (or macro): > http://flex.sourceforge.net/manual/Actions.html#index-yymore_0028_0029-115 You answered my second question, but ignored the first. Why is this a macro instead of a function? Please make it a function unless it must be a macro. > > > +#define dquoted_string 3 > > > +#define squoted_string 4 > > > +#define uri 5 > > > +#define uri_pending 6 > > > > Why are you using underscores in these names? Can't these follow normal WebKit > > naming? > > They are flex start condition names. > I've chosen to use underscores because mediaquery and forkeyword > are not in CamelCase. You should not do that. Instead please follow normal WebKit naming. You don't need to rename existing conditions, but you may if you like. > > > + int string_caller = INITIAL; > > > + int uri_caller = INITIAL; > > > + int content_length = 0; > > > + int content_offest = 0; > > > > Can't these follow normal WebKit naming conventions? I don't see any reason to > > use a different naming style here. > > They are used in tokenizer.flex where underscored names are used. > > I can change them to stringCaller, etc., if you prefer. Yes, I would like you to make that change. > > > + BEGIN(string_caller == uri ? uri_pending : string_caller); > > > > I'm not sure that "caller" is the clearest term we could use use here. Maybe > > "token" would be clearer (or perhaps "context" or "type"). I don't really > > understand what these represent, which I suppose is why I don't get what the > > names should be. > > http://flex.sourceforge.net/manual/Start-Conditions.html#Start-Conditions > uses *_caller naming. That's why I named the variables that way. > I'm open to changing the names if you prefer otherwise. Yes, I think you should use some other name. I do see that identifier named comment_caller in the example in the Flex manual, but I think it's not really clear. > > > + default: > > > + yyterminate(); > > > > I think ASSERT_NOT_REACHED is more appropriate here than yyterminate. Unless > > this code can be reached. > > > > Do the new test cases cover all the code paths here? If I put an > > ASSERT_NOT_REACHED in each rule, would I have to remove them all to pass the > > test? > > The default case is reached if the start condition is either > INITIAL, mediaquery, or forkeyword. I don't understand how this sentence answers my question. Do the test cases cover all these code paths or not?
Yuzo Fujishima
Comment 49 2010-04-07 17:49:03 PDT
Created attachment 52812 [details] Fix unclosed string/url handling.
Yuzo Fujishima
Comment 50 2010-04-07 17:53:04 PDT
Created attachment 52814 [details] Fix unclosed string/url handling.
WebKit Review Bot
Comment 51 2010-04-07 17:54:26 PDT
Attachment 52814 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/css/CSSParser.h:261: yy_more_flag is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/css/CSSParser.h:262: yy_more_len is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Yuzo Fujishima
Comment 52 2010-04-07 18:08:55 PDT
Thank you very much for the review. (In reply to comment #48) > (In reply to comment #38) > > > > +#define yymore() ((yy_more_flag) = 1) > > > > > > Why is this a macro instead of a function? Why does it have a bison/flex style > > > name? > > > > yymore() is a flex function (or macro): > > http://flex.sourceforge.net/manual/Actions.html#index-yymore_0028_0029-115 > > You answered my second question, but ignored the first. Why is this a macro > instead of a function? Please make it a function unless it must be a macro. Changed yymore to be a method of CSSParser. > > > > > +#define dquoted_string 3 > > > > +#define squoted_string 4 > > > > +#define uri 5 > > > > +#define uri_pending 6 > > > > > > Why are you using underscores in these names? Can't these follow normal WebKit > > > naming? > > > > They are flex start condition names. > > I've chosen to use underscores because mediaquery and forkeyword > > are not in CamelCase. > > You should not do that. Instead please follow normal WebKit naming. You don't > need to rename existing conditions, but you may if you like. Renamed them doubleQuotedStringState, etc. Also renamed the existing ones. I named them *State rather than *StartCondition, because State should sound more familiar to more people and, less importantly, StartCondition is too long as suffix. > > > > > + int string_caller = INITIAL; > > > > + int uri_caller = INITIAL; > > > > + int content_length = 0; > > > > + int content_offest = 0; > > > > > > Can't these follow normal WebKit naming conventions? I don't see any reason to > > > use a different naming style here. > > > > They are used in tokenizer.flex where underscored names are used. > > > > I can change them to stringCaller, etc., if you prefer. > > Yes, I would like you to make that change. > > > > > + BEGIN(string_caller == uri ? uri_pending : string_caller); > > > > > > I'm not sure that "caller" is the clearest term we could use use here. Maybe > > > "token" would be clearer (or perhaps "context" or "type"). I don't really > > > understand what these represent, which I suppose is why I don't get what the > > > names should be. > > > > http://flex.sourceforge.net/manual/Start-Conditions.html#Start-Conditions > > uses *_caller naming. That's why I named the variables that way. > > I'm open to changing the names if you prefer otherwise. > > Yes, I think you should use some other name. I do see that identifier named > comment_caller in the example in the Flex manual, but I think it's not really > clear. Renamed string_caller stringPreState, etc. I hope the meaning is clearer this way. Also, I've changed the variables to CSSParser instance variables. I believe it must not be static variable as in the previous patch. > > > > > + default: > > > > + yyterminate(); > > > > > > I think ASSERT_NOT_REACHED is more appropriate here than yyterminate. Unless > > > this code can be reached. > > > > > > Do the new test cases cover all the code paths here? If I put an > > > ASSERT_NOT_REACHED in each rule, would I have to remove them all to pass the > > > test? > > > > The default case is reached if the start condition is either > > INITIAL, mediaquery, or forkeyword. > > I don't understand how this sentence answers my question. > > Do the test cases cover all these code paths or not? Yes. yyterminate is called when the tests run.
Yuzo Fujishima
Comment 53 2010-04-28 00:52:04 PDT
Created attachment 54534 [details] Fix unclosed string/url handling.
WebKit Review Bot
Comment 54 2010-04-28 00:53:09 PDT
Attachment 54534 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/css/CSSParser.h:277: yy_more_flag is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/css/CSSParser.h:278: yy_more_len is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Yuzo Fujishima
Comment 55 2010-04-28 00:58:10 PDT
Hi, Darin, Can you take yet another look? This time I've addressed all of your comments.
Eric Seidel (no email)
Comment 56 2010-05-17 01:48:28 PDT
This is a very scary change to review. :(
Adam Barth
Comment 57 2010-06-21 16:49:44 PDT
Comment on attachment 54534 [details] Fix unclosed string/url handling. Sigh. Not sure how to make progress here. I wish the CSS spec defined a parsing algorithm in the same level of detail as the HTML spec. In any case, we need a CSS parsing expert to review this change. Drive-by: WebCore/css/CSSParser.h:266 + void yymore() {(yy_more_flag) = 1; /* Copied from flex ouput. */} Surely the parenthesis aren't needed here now that this isn't a macro.
Nikolas Zimmermann
Comment 58 2010-07-30 22:46:56 PDT
Comment on attachment 54534 [details] Fix unclosed string/url handling. LayoutTests/fast/css/parsing-unclosed-string-expected.txt:6 + Test 1(ref) Test 1(test) I don't understand these messages. You should rephrase the messages to make clear what they're supposed to explain. WebCore/css/maketokenizer:57 + #define YY_START (((yy_start) - 1) / 2) This meaning is unclear. WebCore/css/maketokenizer:58 + #define yytext_ptr yytext This seems unused. WebCore/css/tokenizer.flex:79 + yyleng--; // Remove the '\0' representing EOF. // Remove the ... --yyleng; Comment should go on the line before. WebCore/css/CSSParser.h:266 + void yymore() {(yy_more_flag) = 1; /* Copied from flex ouput. */} I don't get this concept. yymore() gets called by the lexer, but all it does is setting CSSParsers yy_more_flag member variable -- the variable seems not used at all to me. WebCore/css/CSSParser.h:278 + int yy_more_len; This is completly unused. Please clear up these issues.
Yuzo Fujishima
Comment 59 2010-08-09 01:43:49 PDT
Created attachment 63869 [details] Patch
Yuzo Fujishima
Comment 60 2010-08-09 01:44:09 PDT
Hi, thank you for the reviews. (In reply to comment #57) > (From update of attachment 54534 [details]) > Sigh. Not sure how to make progress here. I wish the CSS spec defined a parsing algorithm in the same level of detail as the HTML spec. In any case, we need a CSS parsing expert to review this change. > > Drive-by: > > WebCore/css/CSSParser.h:266 > + void yymore() {(yy_more_flag) = 1; /* Copied from flex ouput. */} > Surely the parenthesis aren't needed here now that this isn't a macro. Done. (In reply to comment #58) > (From update of attachment 54534 [details]) > LayoutTests/fast/css/parsing-unclosed-string-expected.txt:6 > + Test 1(ref) Test 1(test) > I don't understand these messages. You should rephrase the messages to make clear what they're supposed to explain. Changed such that the tests are more self-explanatory. > > WebCore/css/maketokenizer:57 > + #define YY_START (((yy_start) - 1) / 2) > This meaning is unclear. This and other seemingly unused variables/functions are actually used in piece of code generated by flex. Added comments. > > WebCore/css/maketokenizer:58 > + #define yytext_ptr yytext > This seems unused. > > > WebCore/css/tokenizer.flex:79 > + yyleng--; // Remove the '\0' representing EOF. > // Remove the ... > --yyleng; > > Comment should go on the line before. Done. > > WebCore/css/CSSParser.h:266 > + void yymore() {(yy_more_flag) = 1; /* Copied from flex ouput. */} > I don't get this concept. yymore() gets called by the lexer, but all it does is setting CSSParsers yy_more_flag member variable -- the variable seems not used at all to me. > > WebCore/css/CSSParser.h:278 > + int yy_more_len; > This is completly unused. > > Please clear up these issues.
WebKit Review Bot
Comment 61 2010-08-09 01:46:43 PDT
Attachment 63869 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/css/CSSParser.h:297: yy_more_flag is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/css/CSSParser.h:298: yy_more_len is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Yuzo Fujishima
Comment 62 2010-10-29 00:15:16 PDT
Ping? I think we should ignore style error warnings here, because the names are used by flex.
Yuzo Fujishima
Comment 63 2010-11-21 22:22:49 PST
Ping again? This is blocking 6891.
Tony Gentilcore
Comment 64 2011-01-05 11:03:24 PST
I didn't look at the patch, but just wanted to point out that Hixie has some good test cases here: http://www.hixie.ch/tests/adhoc/css/parsing/core-syntax/strings/investigation/002.html
Tony Gentilcore
Comment 65 2011-01-05 11:04:58 PST
*** Bug 18612 has been marked as a duplicate of this bug. ***
Yuzo Fujishima
Comment 66 2011-01-11 00:24:51 PST
Created attachment 78499 [details] Pre Source/WebCore move
WebKit Review Bot
Comment 67 2011-01-11 00:28:02 PST
Attachment 78499 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7337474
WebKit Review Bot
Comment 68 2011-01-11 00:31:03 PST
Attachment 78499 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7354141
Early Warning System Bot
Comment 69 2011-01-11 00:31:53 PST
Attachment 78499 [details] did not build on qt: Build output: http://queues.webkit.org/results/7438105
WebKit Review Bot
Comment 70 2011-01-11 01:14:28 PST
Attachment 78499 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7385115
WebKit Review Bot
Comment 71 2011-01-11 01:44:35 PST
Attachment 78499 [details] did not build on mac: Build output: http://queues.webkit.org/results/7413115
Yuzo Fujishima
Comment 72 2011-01-11 03:21:55 PST
Comment on attachment 78499 [details] Pre Source/WebCore move Sorry, wrong patch (webkit-patch has been confused by the change log).
Yuzo Fujishima
Comment 73 2011-01-11 03:22:56 PST
I'm releasing ownership of this bug.
Robert Hogan
Comment 74 2012-02-13 14:20:35 PST
*** This bug has been marked as a duplicate of bug 78538 ***
Note You need to log in before you can comment on or make changes to this bug.