[Webkit-unassigned] [Bug 34067] CSS rule containing a string unclosed at the end of stylesheet is ignored.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Feb 12 11:17:24 PST 2010


https://bugs.webkit.org/show_bug.cgi?id=34067


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #48621|review?                     |review-
               Flag|                            |




--- Comment #36 from Darin Adler <darin at 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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list