| Summary: | [EFL][DRT] http/tests/globalhistory testcases do not pass | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Mikhail Pozdnyakov <mikhail.pozdnyakov> |
| Component: | WebKit EFL | Assignee: | 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
Mikhail Pozdnyakov
2012-03-29 00:53:17 PDT
Created attachment 134737 [details]
Global history delegate support for EFL port
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. Created attachment 134779 [details]
global history delegate v2
Created attachment 134784 [details]
global history delegate v2
Created attachment 134795 [details]
global history delegate v2 rebased
(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. Created attachment 135285 [details]
global history delegate v2
fixed a mistake in FrameLoaderClientEfl::updateGlobalHistory function implementation.
Splitted. Now this bug is responsible for DRT part only. Created attachment 141685 [details] DRT patch Depends on patch from bug86343. Comment on attachment 141685 [details] DRT patch Attachment 141685 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12673999 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 ? (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 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. Created attachment 145524 [details]
DRT patch v2 (rebased + review comments from Chris)
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 "||" Created attachment 145527 [details]
DRT patch v3 (more review comments from Chris)
Comment on attachment 145527 [details]
DRT patch v3 (more review comments from Chris)
LGTM.
Comment on attachment 145527 [details]
DRT patch v3 (more review comments from Chris)
rs=me based on informal reviews
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> All reviewed patches have been landed. Closing bug. |