[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 #6 from Mikhail Pozdnyakov <mikhail.pozdnyakov at intel.com>  2012-03-30 07:01:53 PST ---
(In reply to comment #2)
> (From update of attachment 134737 [details])
> 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.
Thank you for review.
Comments are met in "global history delegate v2 rebased" patch.

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