[Webkit-unassigned] [Bug 128422] Web Inspector: update check-webkit-style to flag single quotes in WebInspectorUI projects

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from Diego Pino <dpino at igalia.com>  2014-02-18 01:22:11 PST ---
(From update of attachment 224055)
View in context: https://bugs.webkit.org/attachment.cgi?id=224055&action=review

>> Tools/Scripts/webkitpy/style/checkers/js.py:64
>> +                continue
> 
> Oh, what comments are these? The first part of the regex very clearly handles '//.*?$' so its surprising to me that some might have been missed.

Yes, I don't know because it seems to be working OK for more comments like this even if this very same file. If I remove that checking I got:

CodeMirrorAdditions.js(44):         // If the stream isn't at the end of line then we found the end quote.
CodeMirrorAdditions.js(111):             // We don't expect other tokens between attribute and string since

Actually, it's not happening for just these two lines (there are two because they contain a single-quote), it's happening for more lines in CodeMirrorAdditions.js. I isolated the function in its own file, run it and I got:

        // If the stream isn't at the end of line then we found the end quote.
        // In the case, change _linkTokenize to parse the end of the link next.
        // Otherwise _linkTokenize will stay as-is to parse more of the link.
        // Eat the quote character to style it with the base style.
        // Clean up the state.
            // Call the link tokenizer instead.
        // Remember the start position so we can rewind if needed.
            // Look for "href" or "src" attributes. If found then we should
            // expect a string later that should get the "link" style instead.
            // This is a link, so setup the state to process it next.
            // The attribute may or may not be quoted.
            // Rewind the steam to the start of this token.
            // Eat the open quote of the string so the string style
            // will be used for the quote character.
            // We don't expect other tokens between attribute and string since

>> Tools/Scripts/webkitpy/style/checkers/js.py:71
>> +                self._handle_style_error(line_number, "js/syntax", 5, "Line contains single-quote character.")
> 
> This is pretty basic, which may be okay. It checks for the first single quote and verifies that it is not within the first double quoted string on the list. Likewise for regex. I don't think it can give false positives, but issues that I think would go undetected include:
> 
>     foo("a'b", 'c');
>     foo("a/'b", '/c');
>     foo(/'/, 'c');
> 
> You might be able to detect these by (1) checking all the single quotes on a line and (2) if you do a find and rfind around the position of the character itself:
> http://docs.python.org/2/library/string.html#string.find
> 
> Alternatively, now that comments are removed you could just search for single quoted strings. The regular expression in _remove_comments looks like it would already be sufficient if you ignore double quoted strings.
> 
> However, that being said our code tends to be rather straightforward, so this looks good to me.

OK, I think I will leave like this then. Thanks for pointing those cases.

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