[Bug 34067] CSS rule containing a string unclosed at the end of stylesheet is ignored.
bugzill...@webkit.org
WebKit Review Bot <webkit.r...@gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |webkit-bot-watchers@googleg
| |roups.com,
| |webkit.r...@gmail.com
--- Comment #19 from WebKit Review Bot <webkit.r...@gmail.com> 2010-01-28 03:54:14 PST ---
Attachment 47602 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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
bugzill...@webkit.org
--- Comment #20 from Yuzo Fujishima <yu...@google.com> 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
bugzill...@webkit.org
--- Comment #21 from WebKit Review Bot <webkit.r...@gmail.com> 2010-01-28 05:00:21 PST ---
Attachment 47602 did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/216671
bugzill...@webkit.org
WebKit Review Bot <webkit.r...@gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |g...@gnome.org,
| |xan....@gmail.com
--- Comment #22 from WebKit Review Bot <webkit.r...@gmail.com> 2010-01-28 05:00:34 PST ---
Attachment 47602 did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/216673
bugzill...@webkit.org
Yuzo Fujishima <yu...@google.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #47602|0 |1
is obsolete| |
Attachment #47602|review?, commit-queue? |
Flag| |
--- Comment #23 from Yuzo Fujishima <yu...@google.com> 2010-01-28 14:08:16 PST ---
(From update of attachment 47602)
Sorry, this patch contains a few issues. Make it obsolete.
bugzill...@webkit.org
Yuzo Fujishima <yu...@google.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #47684| |review?, commit-queue?
Flag| |
--- Comment #24 from Yuzo Fujishima <yu...@google.com> 2010-01-28 23:58:34 PST ---
Created an attachment (id=47684)
--> (https://bugs.webkit.org/attachment.cgi?id=47684)
Change flex rule such that unclosed string/url is properly handled.
bugzill...@webkit.org
--- Comment #25 from Yuzo Fujishima <yu...@google.com> 2010-01-29 00:02:16 PST ---
Hi, reviewers,
Can you review the latest patch?
Yuzo
bugzill...@webkit.org
--- Comment #26 from WebKit Review Bot <webkit.r...@gmail.com> 2010-01-29 00:03:11 PST ---
Attachment 47684 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.
--
bugzill...@webkit.org
--- Comment #27 from Eric Seidel <er...@webkit.org> 2010-02-01 15:35:37 PST ---
(From update of attachment 47684)
Please explain the changes to fast/css/font_property_normal.html in the
ChangeLog.
bugzill...@webkit.org
--- Comment #28 from Yuzo Fujishima <yu...@google.com> 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
bugzill...@webkit.org
--- Comment #29 from Robert Blaut <web...@blaut.biz> 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.
bugzill...@webkit.org
Yuzo Fujishima <yu...@google.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #47684|0 |1
is obsolete| |
Attachment #47684|review?, commit-queue? |
Flag| |
bugzill...@webkit.org
Yuzo Fujishima <yu...@google.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #48317| |review?
Flag| |
--- Comment #30 from Yuzo Fujishima <yu...@google.com> 2010-02-07 22:35:33 PST ---
Created an attachment (id=48317)
--> (https://bugs.webkit.org/attachment.cgi?id=48317)
Change flex rule such that unclosed string/url is properly handled.
--
bugzill...@webkit.org
--- Comment #31 from Yuzo Fujishima <yu...@google.com> 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
bugzill...@webkit.org
--- Comment #32 from WebKit Review Bot <webkit.r...@gmail.com> 2010-02-07 22:41:14 PST ---
Attachment 48317 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.
--
bugzill...@webkit.org
--- Comment #33 from Yuzo Fujishima <yu...@google.com> 2010-02-10 01:37:04 PST ---
As to the style check failure,
please see https://bugs.webkit.org/show_bug.cgi?id=34787
bugzill...@webkit.org
Yuzo Fujishima <yu...@google.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #48317|0 |1
is obsolete| |
Attachment #48317|review? |
Flag| |
bugzill...@webkit.org
Yuzo Fujishima <yu...@google.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #48621| |review?
Flag| |
--- Comment #34 from Yuzo Fujishima <yu...@google.com> 2010-02-11 21:39:26 PST ---
Created an attachment (id=48621)
--> (https://bugs.webkit.org/attachment.cgi?id=48621)
Change flex rule such that unclosed string/url is properly handled.
--
bugzill...@webkit.org
--- Comment #35 from Yuzo Fujishima <yu...@google.com> 2010-02-11 21:40:26 PST ---
Added NOLINT for yy_* identifiers.
bugzill...@webkit.org
Darin Adler <da...@apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #48621|review? |review-
Flag| |
--- Comment #36 from Darin Adler <da...@apple.com> 2010-02-12 11:17:23 PST ---
(From update of attachment 48621)
> -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.
bugzill...@webkit.org
Yuzo Fujishima <yu...@google.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #48792| |review?
Flag| |
--- Comment #37 from Yuzo Fujishima <yu...@google.com> 2010-02-15 23:45:15 PST ---
Created an attachment (id=48792)
--> (https://bugs.webkit.org/attachment.cgi?id=48792)
Change flex rule such that unclosed string/url is properly handled.
--
bugzill...@webkit.org
Yuzo Fujishima <yu...@google.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #48621|0 |1
is obsolete| |
Attachment #48621|review- |
Flag| |
bugzill...@webkit.org
--- Comment #38 from Yuzo Fujishima <yu...@google.com> 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.
--
bugzill...@webkit.org
Yuzo Fujishima <yu...@google.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #48792|0 |1
is obsolete| |
Attachment #48792|review? |
Flag| |
bugzill...@webkit.org
Yuzo Fujishima <yu...@google.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #48868| |review?
Flag| |
--- Comment #39 from Yuzo Fujishima <yu...@google.com> 2010-02-17 00:49:59 PST ---
Created an attachment (id=48868)
--> (https://bugs.webkit.org/attachment.cgi?id=48868)
Change flex rule such that unclosed string/url is properly handled.
--
bugzill...@webkit.org
--- Comment #40 from Yuzo Fujishima <yu...@google.com> 2010-02-17 00:50:55 PST ---
Added explanation to WebCore/ChangeLog.
bugzill...@webkit.org
--- Comment #41 from Yuzo Fujishima <yu...@google.com> 2010-03-08 00:29:31 PST ---
Ping? This is blocking 6891.
bugzill...@webkit.org
Yuzo Fujishima <yu...@google.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
AssignedTo|webkit-u...@lists.web |yu...@google.com
|kit.org |
bugzill...@webkit.org
--- Comment #42 from Yuzo Fujishima <yu...@google.com> 2010-03-14 23:43:02 PST ---
Ping again.
bugzill...@webkit.org
Eric Seidel <er...@webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |er...@webkit.org
--- Comment #43 from Eric Seidel <er...@webkit.org> 2010-03-15 13:16:54 PST ---
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.
bugzill...@webkit.org
--- Comment #44 from Eric Seidel <er...@webkit.org> 2010-03-15 13:18:21 PST ---
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. :)
bugzill...@webkit.org
--- Comment #45 from Eric Seidel <er...@webkit.org> 2010-03-15 13:21:22 PST ---
(From update of attachment 48868)
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.
bugzill...@webkit.org
Adam Barth <aba...@webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |aba...@webkit.org
--- Comment #46 from Adam Barth <aba...@webkit.org> 2010-03-22 09:08:03 PST ---
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?
bugzill...@webkit.org
--- Comment #47 from Yuzo Fujishima <yu...@google.com> 2010-03-31 23:39:38 PST ---
Hi, Darin,
Would you mind taking another look?
Yuzo
bugzill...@webkit.org
--- Comment #56 from Eric Seidel <er...@webkit.org> 2010-05-17 01:48:28 PST ---
This is a very scary change to review. :(