Bug 82301 – [EFL] LayoutTestController does not implement callShouldCloseOnWebView

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Chris Dumez]

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

[Chris Dumez]

Created attachment 133991 [details]
Proposed patch

[Gyuyoung Kim]

Comment on attachment 133991 [details]
Proposed patch

LGTM.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 133991 [details]
Proposed patch

Looks fine, thanks.

[Grzegorz Czajkowski]

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?

[Chris Dumez]

Created attachment 134232 [details]
Improved patch based on Grzegorz Czajkowski's comment.

[Grzegorz Czajkowski]

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.

[Chris Dumez]

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.

[Grzegorz Czajkowski]

LGTM.

[Martin Robinson]

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
> [phone]  Christophe Dumez  <[e-mail]>
> +
> +        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.

[Chris Dumez]

Created attachment 137557 [details]
Proposed patch

Rebase on master and fix the changelogs.

[WebKit Review Bot]

Comment on attachment 137557 [details]
Proposed patch

Clearing flags on attachment: 137557

Committed r114422: <http://trac.webkit.org/changeset/114422>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.