[Webkit-unassigned] [Bug 66027] Implementation for "nav-up" "nav-down" "nav-left" "nav-right" properties in CSS3-UI module

[bugzilla-daemon at webkit.org]

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





--- Comment #12 from Gyuyoung Kim <gyuyoung.kim at samsung.com>  2012-07-17 04:03:54 PST ---
(From update of attachment 123446)
View in context: https://bugs.webkit.org/attachment.cgi?id=123446&action=review

In my opinion, this patch should to have test cases as well. Then, I think reviewers can start to review this patch. By the way, I wonder if you notify this new feature to webkit-dev. If you didn't it, you have to do it first.

>> Source/WebCore/ChangeLog:3
>> +        Need a short description and bug URL (OOPS!)
> 
> It looks you need to study how to write changelog first before submitting a patch. Please read webkit contribution guideline first.

WebKit contribution guidance.
 - http://www.webkit.org/coding/contributing.html

> Source/WebCore/css/CSSStyleApplyProperty.cpp:1625
> +                || primitiveValue->primitiveType() == CSSPrimitiveValue::CSS_PARSER_HEXCOLOR)

Wrong indentation for *||* operator.

> Source/WebCore/css/CSSStyleSelector.cpp:4465
> +        for (size_t i = 0; i < 2; i++) {

WebKit prefers to use ++i instead of i++.

> Source/WebCore/css/CSSStyleSelector.h:355
> +    StyleNavigationValue getStyleNavigationValue(CSSValue*);

Nit : Add an empty line.

> Source/WebCore/page/FocusController.cpp:840
> +    // TODO:// which one is right?

WebKit prefer to use FIXME: instead of TODO:

> Source/WebCore/rendering/style/StyleNavigationData.h:2
> + * Copyright (C) 2011 Kyounga Ra (kyounga.ra at gmail.com)

s/2011/2012/g

> Source/WebCore/rendering/style/StyleNavigationData.h:33
> +    StyleNavigationData(const StyleNavigationData& o)

Use *explicit* keyword.

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