Bug 82579 - [EFL][DRT] http/tests/globalhistory testcases do not pass
Summary: [EFL][DRT] http/tests/globalhistory testcases do not pass
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL:
Keywords:
Depends on: 86343 86370
Blocks: 82724
  Show dependency treegraph
 
Reported: 2012-03-29 00:53 PDT by Mikhail Pozdnyakov
Modified: 2012-06-06 07:58 PDT (History)
5 users (show)

See Also:


Attachments
Global history delegate support for EFL port (37.94 KB, patch)
2012-03-30 00:24 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
global history delegate v2 (20.60 KB, patch)
2012-03-30 04:51 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
global history delegate v2 (36.40 KB, patch)
2012-03-30 05:09 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
global history delegate v2 rebased (36.38 KB, patch)
2012-03-30 06:04 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
global history delegate v2 (36.07 KB, patch)
2012-04-03 00:43 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
DRT patch (8.58 KB, patch)
2012-05-14 04:01 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
DRT patch v2 (rebased + review comments from Chris) (8.50 KB, patch)
2012-06-04 00:10 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
DRT patch v3 (more review comments from Chris) (8.50 KB, patch)
2012-06-04 00:37 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.