Summary: | [EFL] LayoutTestController does not implement callShouldCloseOnWebView | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||
Component: | WebKit EFL | Assignee: | Chris Dumez <cdumez> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | g.czajkowski, gyuyoung.kim, lucas.de.marchi, rakuco, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Chris Dumez
2012-03-27 00:09:00 PDT
Created attachment 133991 [details]
Proposed patch
Comment on attachment 133991 [details]
Proposed patch
LGTM.
Comment on attachment 133991 [details]
Proposed patch
Looks fine, thanks.
Generally is OK. One thing can be improved.
> Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.h:89
> + static bool callShouldCloseOnWebView(Evas_Object* ewkFrame);
Could you move this declaration to a section where other declarations with ewkFrame are declared instead of adding it to the end?
Created attachment 134232 [details]
Improved patch based on Grzegorz Czajkowski's comment.
Thanks for your changes. View in context: https://bugs.webkit.org/attachment.cgi?id=134232&action=review > Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.h:67 > + static bool callShouldCloseOnWebView(Evas_Object* ewkFrame); In my opinion it should be added as second DumpRencerTreeSupportEfl's method. DumpRencerTreeSupportEfl class keeps alphabetical order for methods which takes ewkFrame object. Created attachment 134249 [details]
Improved patch based on Grzegorz Czajkowski's comment.
My bad, I failed to notice the alphabetical ordering. I fixed that issue, thanks.
LGTM. Comment on attachment 134249 [details] Improved patch based on Grzegorz Czajkowski's comment. View in context: https://bugs.webkit.org/attachment.cgi?id=134249&action=review Looks good, just giving an r- because of the funky changelogs. > LayoutTests/ChangeLog:529 > +2012-03-27 Christophe Dumez <christophe.dumez@intel.com> > + > + EFL's LayoutTestController does not implement callShouldCloseOnWebView > + https://bugs.webkit.org/show_bug.cgi?id=82301 > + > + Reviewed by NOBODY (OOPS!). > + > + Implement callShouldCloseOnWebView in EFL's LayoutTestController by The changelog is in the wrong place here. This will prevent the patch from landing properly with the cq, I think. Created attachment 137557 [details]
Proposed patch
Rebase on master and fix the changelogs.
Comment on attachment 137557 [details] Proposed patch Clearing flags on attachment: 137557 Committed r114422: <http://trac.webkit.org/changeset/114422> All reviewed patches have been landed. Closing bug. |