Bug 82579

Summary: [EFL][DRT] http/tests/globalhistory testcases do not pass
Product: WebKit Reporter: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Component: WebKit EFLAssignee: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, gyuyoung.kim, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 86343, 86370    
Bug Blocks: 82724    
Attachments:
Description Flags
Global history delegate support for EFL port
none
global history delegate v2
none
global history delegate v2
none
global history delegate v2 rebased
none
global history delegate v2
none
DRT patch
none
DRT patch v2 (rebased + review comments from Chris)
none
DRT patch v3 (more review comments from Chris) none

Description Mikhail Pozdnyakov 2012-03-29 00:53:17 PDT
EFL port needs history delegate support in order to unskip global history tests and provide better global history management for the clients.
Comment 1 Mikhail Pozdnyakov 2012-03-30 00:24:58 PDT
Created attachment 134737 [details]
Global history delegate support for EFL port
Comment 2 Gyuyoung Kim 2012-03-30 01:12:08 PDT
Comment on attachment 134737 [details]
Global history delegate support for EFL port

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.
Comment 3 Mikhail Pozdnyakov 2012-03-30 04:51:37 PDT
Created attachment 134779 [details]
global history delegate v2
Comment 4 Mikhail Pozdnyakov 2012-03-30 05:09:32 PDT
Created attachment 134784 [details]
global history delegate v2
Comment 5 Mikhail Pozdnyakov 2012-03-30 06:04:18 PDT
Created attachment 134795 [details]
global history delegate v2 rebased
Comment 6 Mikhail Pozdnyakov 2012-03-30 07:01:53 PDT
(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.
Comment 7 Mikhail Pozdnyakov 2012-04-03 00:43:22 PDT
Created attachment 135285 [details]
global history delegate v2 

fixed a mistake in FrameLoaderClientEfl::updateGlobalHistory function implementation.
Comment 8 Mikhail Pozdnyakov 2012-05-14 03:34:12 PDT
Splitted. Now this bug is responsible for DRT part only.
Comment 9 Mikhail Pozdnyakov 2012-05-14 04:01:38 PDT
Created attachment 141685 [details]
DRT patch

Depends on patch from bug86343.
Comment 10 Gyuyoung Kim 2012-05-14 04:11:18 PDT
Comment on attachment 141685 [details]
DRT patch

Attachment 141685 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12673999
Comment 11 Gyuyoung Kim 2012-05-22 18:27:28 PDT
It looks this path can pass ews after landing depending bugs(Bug 86343, Bug 86370). I think it is good to request r? after landing them. Could you clear flag until then ?
Comment 12 Mikhail Pozdnyakov 2012-05-23 04:59:11 PDT
(In reply to comment #11)
> It looks this path can pass ews after landing depending bugs(Bug 86343, Bug 86370). I think it is good to request r? after landing them. Could you clear flag until then ?

done.
Comment 13 Chris Dumez 2012-05-31 06:41:44 PDT
Comment on attachment 141685 [details]
DRT patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141685&action=review

> Tools/DumpRenderTree/efl/DumpRenderTree.cpp:201
> +    return cTestPathOrURL.contains("globalhistory/");

The Windows port checks for "/globalhistory/" instead of "globalhistory/": I think it would be safer to do like them.

> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:544
> +    Ewk_View_Navigation_Data* navigationData = static_cast<Ewk_View_Navigation_Data*>(eventInfo);

Should probably be const.
Comment 14 Mikhail Pozdnyakov 2012-06-04 00:10:36 PDT
Created attachment 145524 [details]
DRT patch v2 (rebased + review comments from Chris)
Comment 15 Chris Dumez 2012-06-04 00:19:53 PDT
Comment on attachment 145524 [details]
DRT patch v2 (rebased + review comments from Chris)

View in context: https://bugs.webkit.org/attachment.cgi?id=145524&action=review

> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:572
> +    const bool wasFailure = navigationData->has_substitute_data ||  navigationData->response->status_code >= 400;

Extra space after "||"
Comment 16 Mikhail Pozdnyakov 2012-06-04 00:37:41 PDT
Created attachment 145527 [details]
DRT patch v3 (more review comments from Chris)
Comment 17 Chris Dumez 2012-06-04 00:41:55 PDT
Comment on attachment 145527 [details]
DRT patch v3 (more review comments from Chris)

LGTM.
Comment 18 Csaba Osztrogon√°c 2012-06-06 07:43:55 PDT
Comment on attachment 145527 [details]
DRT patch v3 (more review comments from Chris)

rs=me based on informal reviews
Comment 19 WebKit Review Bot 2012-06-06 07:57:58 PDT
Comment on attachment 145527 [details]
DRT patch v3 (more review comments from Chris)

Clearing flags on attachment: 145527

Committed r119590: <http://trac.webkit.org/changeset/119590>
Comment 20 WebKit Review Bot 2012-06-06 07:58:07 PDT
All reviewed patches have been landed.  Closing bug.