[Webkit-unassigned] [Bug 82579] [EFL] Global history delegate support for EFL port

[bugzilla-daemon at webkit.org]

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





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

Informal r- on my side. And, it looks this patch is a little huge.

> LayoutTests/ChangeLog:5
> +

Missing description for this patch.

> Source/WebKit/ChangeLog:5
> +

ditto.

> Source/WebKit/efl/ChangeLog:7
> +

Though this patch is huge, you don't add any descriptions for this patch.

> Source/WebKit/efl/ewk/ewk_history_delegate.cpp:37
> +    Ewk_History_Delegate* res = new Ewk_History_Delegate;

Do not use abbreviation in local variable.

> Source/WebKit/efl/ewk/ewk_history_delegate.cpp:48
> +void ewk_history_delegate_free(Ewk_History_Delegate* hd)

We don't use abbreviation in function implementation. hd -> historyDelegation ?

> Source/WebKit/efl/ewk/ewk_history_delegate.cpp:59
> +void ewk_history_delegate_did_navigate_with_navigation_data(Ewk_History_Delegate* hd, const Evas_Object* webView, Ewk_Navigation_Data* navigationData, const Evas_Object* webFrame)

ditto. hd -> historyDelegation.

In addition, it looks webView means view and webFrame means ewkFrame. If so, it is better to use *ewkView* and *ewkFrame* to be consistent in existing name rule.

> Source/WebKit/efl/ewk/ewk_history_delegate.h:65
> +    void (*didNavigateWithNavigationData)(Ewk_History_Delegate* hd, const Evas_Object* webView, Ewk_Navigation_Data* navigationData, const Evas_Object* webFrame);

We have to adhere efl coding style for public header. So, you should use abbreviation variable name.

Please see below link,
http://trac.webkit.org/wiki/EFLWebKitCodingStyle

> Source/WebKit/efl/ewk/ewk_history_delegate.h:81
> +

If this functions is API, you miss API description.

> Source/WebKit/efl/ewk/ewk_view.cpp:3919
> +Ewk_History_Delegate* ewk_view_history_delegate_get(const Evas_Object *ewkView)

Move '*' to data type side.

> Source/WebKit/efl/ewk/ewk_view.cpp:3926
> +void ewk_view_history_delegate_set(const Evas_Object *ewkView, Ewk_History_Delegate *delegate)

ditto.

> Tools/ChangeLog:7
> +

Missing description.

> Tools/DumpRenderTree/efl/HistoryDelegate.cpp:37
> +static void didNavigateWithNavigationData(Ewk_History_Delegate* /*hd*/, const Evas_Object* /*webView*/, Ewk_Navigation_Data* navigationData, const Evas_Object* /*webFrame*/)

It seems that /*hd*/, /*webView*/ /*webFrame*/) is unneeded.

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