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

[bugzilla-daemon at webkit.org]

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





--- Comment #38 from Yuzo Fujishima <yuzo at 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.

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