[webkit-reviews] review denied: [Bug 95178] Support CSS3 Keyboard control : [Attachment 199152] add change log and license

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Sangho Kim <thomas.kim at lge.com>'s
request for review:
Bug 95178: Support CSS3 Keyboard control
https://bugs.webkit.org/show_bug.cgi?id=95178

Attachment 199152: add change log and license
https://bugs.webkit.org/attachment.cgi?id=199152&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=199152&action=review


Thank you for the patch but this patch needs tests. Every new feature or bug
fix needs to come up with a layout test whenever possible.
See http://www.webkit.org/coding/contributing.html

There is a lot of stylistic and naming issues as I've pointed out below.

> ChangeLog:6
> +	   Reviewed by .

Please don't remove NOBODY (OOPS!). (You used webkit-patch upload or
prepare-ChangeLog in Tools/Scripts, right?)

>> ChangeLog:12
>> +	    * Source/WebCore/Target.pri
> 
> Need whitespace between colon and description 
[changelog/filechangedescriptionwhitespace] [5]

You're missing a colon after the file name.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2109
> +	       const StyleNavIndex* nav = style->getNavIndex();

Please don't use abbreviations like nav. Spell out navigation.

> Source/WebCore/css/CSSParser.cpp:2953
> +	   if ((num > 2) || (value->unit !=
CSSPrimitiveValue::CSS_PARSER_HEXCOLOR && value->unit !=
CSSPrimitiveValue::CSS_ELEMENT_ID))

No need to wrap inner conditions with parentheses. Why are you comparing the
unit to CSS_PARSER_HEXCOLOR and CSS_ELEMENT_ID? That doesn't make much sense.

> Source/WebCore/rendering/style/RenderStyle.cpp:1703
> +const StyleNavData* RenderStyle::getNav(int propId)

this name is too terse. Please give more descriptive name. Also, we don't
prefix a function with "get" unless it has an out argument.

> Source/WebCore/rendering/style/StyleNavData.cpp:27
> +StyleNavData::StyleNavData()

Please don't use abbreviations like Nav.
Also, StyleNavData is a very vague name. Please consider giving it a more
descriptive name.

> Source/WebCore/rendering/style/StyleNavData.cpp:29
> +    : m_id("")
> +    , m_target("")

Please use emptyString instead or let the default constructor run.