[Webkit-unassigned] [Bug 95178] Support CSS3 Keyboard control

[bugzilla-daemon at webkit.org]

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


Ryosuke Niwa <rniwa at webkit.org> changed:

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




--- Comment #12 from Ryosuke Niwa <rniwa at webkit.org>  2013-04-22 23:39:58 PST ---
(From update of attachment 199152)
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.

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