Bug 82579 – [EFL][DRT] http/tests/globalhistory testcases do not pass

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

EFL port needs history delegate support in order to unskip global history tests and provide better global history management for the clients.

[Mikhail Pozdnyakov]

Created attachment 134737 [details]
Global history delegate support for EFL port

[Gyuyoung Kim]

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.

[Mikhail Pozdnyakov]

Created attachment 134779 [details]
global history delegate v2

[Mikhail Pozdnyakov]

Created attachment 134784 [details]
global history delegate v2

[Mikhail Pozdnyakov]

Created attachment 134795 [details]
global history delegate v2 rebased

[Mikhail Pozdnyakov]

(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.

[Mikhail Pozdnyakov]

Created attachment 135285 [details]
global history delegate v2 

fixed a mistake in FrameLoaderClientEfl::updateGlobalHistory function implementation.

[Mikhail Pozdnyakov]

Splitted. Now this bug is responsible for DRT part only.

[Mikhail Pozdnyakov]

Created attachment 141685 [details]
DRT patch

Depends on patch from bug86343.

[Gyuyoung Kim]

Comment on attachment 141685 [details]
DRT patch

Attachment 141685 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12673999

[Gyuyoung Kim]

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 ?

[Mikhail Pozdnyakov]

(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.

[Chris Dumez]

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.

[Mikhail Pozdnyakov]

Created attachment 145524 [details]
DRT patch v2 (rebased + review comments from Chris)

[Chris Dumez]

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 "||"

[Mikhail Pozdnyakov]

Created attachment 145527 [details]
DRT patch v3 (more review comments from Chris)

[Chris Dumez]

Comment on attachment 145527 [details]
DRT patch v3 (more review comments from Chris)

LGTM.

[Csaba Osztrogonác]

Comment on attachment 145527 [details]
DRT patch v3 (more review comments from Chris)

rs=me based on informal reviews

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.