Bug 82301 - [EFL] LayoutTestController does not implement callShouldCloseOnWebView
Summary: [EFL] LayoutTestController does not implement callShouldCloseOnWebView
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: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-27 00:09 PDT by Chris Dumez
Modified: 2012-05-06 22:49 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch (5.03 KB, patch)
2012-03-27 00:12 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Improved patch based on Grzegorz Czajkowski's comment. (5.71 KB, patch)
2012-03-28 01:10 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Improved patch based on Grzegorz Czajkowski's comment. (5.60 KB, patch)
2012-03-28 03:02 PDT, Chris Dumez
mrobinson: review-
Details | Formatted Diff | Diff
Proposed patch (5.36 KB, patch)
2012-04-17 10:29 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-03-27 00:09:00 PDT
EFL's LayoutTestController does not implement callShouldCloseOnWebView, preventing the following test case to be removed from the skip list:
 * fast/events/onbeforeunload-focused-iframe.html
Comment 1 Chris Dumez 2012-03-27 00:12:42 PDT
Created attachment 133991 [details]
Proposed patch
Comment 2 Gyuyoung Kim 2012-03-27 00:43:55 PDT
Comment on attachment 133991 [details]
Proposed patch

LGTM.
Comment 3 Raphael Kubo da Costa (:rakuco) 2012-03-27 07:41:25 PDT
Comment on attachment 133991 [details]
Proposed patch

Looks fine, thanks.
Comment 4 Grzegorz Czajkowski 2012-03-27 23:17:15 PDT
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?
Comment 5 Chris Dumez 2012-03-28 01:10:48 PDT
Created attachment 134232 [details]
Improved patch based on Grzegorz Czajkowski's comment.
Comment 6 Grzegorz Czajkowski 2012-03-28 01:34:35 PDT
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.
Comment 7 Chris Dumez 2012-03-28 03:02:54 PDT
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.
Comment 8 Grzegorz Czajkowski 2012-03-28 04:15:59 PDT
LGTM.
Comment 9 Martin Robinson 2012-04-17 10:18:20 PDT
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.
Comment 10 Chris Dumez 2012-04-17 10:29:11 PDT
Created attachment 137557 [details]
Proposed patch

Rebase on master and fix the changelogs.
Comment 11 WebKit Review Bot 2012-04-17 13:06:16 PDT
Comment on attachment 137557 [details]
Proposed patch

Clearing flags on attachment: 137557

Committed r114422: <http://trac.webkit.org/changeset/114422>
Comment 12 WebKit Review Bot 2012-04-17 13:06:22 PDT
All reviewed patches have been landed.  Closing bug.