Bug 145545 – [EFL] Implement load_started callback in EwkPageClient

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Hyungwook Lee]

Implement load_started callback in EwkPageClient.

We provide load_finished callback now.
struct EwkPageClient {
    int version;
    void *data;

    /**
     * Callbacks to report load finished.
     *
     * @param page page to be finished
     * @param data data of a page client
     */
    void (*load_finished)(Ewk_Page *page, void *data);
};

We need to provide load_started callback to web extension module.

[Hyungwook Lee]

Created attachment 254046 [details]
patch

[Gyuyoung Kim]

Comment on attachment 254046 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254046&action=review

> Source/WebKit2/UIProcess/API/efl/tests/extensions/extension_sample.cpp:46
> +{

I think this patch needs to test for loadStarted() callback, isn't it ?

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_page.cpp:100
> +void EwkPage::didStartProvisionalLoadForFrame(WKBundlePageRef, WKBundleFrameRef frame, WKTypeRef*, const void * clientInfo)

s/void * clientInfo/void* clientInfo/g

[Ryuan Choi]

Comment on attachment 254046 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254046&action=review

>> Source/WebKit2/UIProcess/API/efl/tests/extensions/extension_sample.cpp:46
>> +{
> 
> I think this patch needs to test for loadStarted() callback, isn't it ?

+1

[Hyungwook Lee]

Created attachment 254063 [details]
patch

[Gyuyoung Kim]

Comment on attachment 254063 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254063&action=review

> Source/WebKit2/UIProcess/API/efl/tests/extensions/extension_sample.cpp:33
> +static Eina_Bool load_started = false;

WebKit EFL decided to follow WebKit style except for public API. So please do not use _ in variable.

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_page.cpp:100
> +void EwkPage::didStartProvisionalLoadForFrame(WKBundlePageRef, WKBundleFrameRef frame, WKTypeRef*, const void * clientInfo)

There is still a space between "void * clientInfo"

> Source/WebKit2/WebProcess/InjectedBundle/API/efl/ewk_page.h:55
> +     * @param data data of a page clinet

typo: clinet -> client.

[Hyungwook Lee]

Created attachment 254064 [details]
Applying reviewer's feedback.

[Hyungwook Lee]

Created attachment 254065 [details]
patch

[Gyuyoung Kim]

Comment on attachment 254065 [details]
patch

LGTM

[Build Bot]

Comment on attachment 254065 [details]
patch

Attachment 254065 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5162679798857728

New failing tests:
platform/mac-wk2/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-slow-vertical.html

[Build Bot]

Created attachment 254068 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Ryuan Choi]

Comment on attachment 254065 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254065&action=review

> Source/WebKit2/UIProcess/API/efl/tests/extensions/extension_sample.cpp:56
> +    if (!pageLoadStarted)
> +        return;
> +    pageLoadStarted = false;

Hmm, I think that this is correct idea to test this.

How about making test_ewk2_extension.cpp to test these APIs instead of using javascript binding test.

[Gyuyoung Kim]

(In reply to comment #10)
> Created attachment 254068 [details]
> Archive of layout-test-results from ews104 for mac-mavericks-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-wk2-ews.
> Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

Unrelated error report because this patch is only for EFL port.

[Gyuyoung Kim]

(In reply to comment #11)
> Comment on attachment 254065 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254065&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/tests/extensions/extension_sample.cpp:56
> > +    if (!pageLoadStarted)
> > +        return;
> > +    pageLoadStarted = false;
> 
> Hmm, I think that this is correct idea to test this.
> 
> How about making test_ewk2_extension.cpp to test these APIs instead of using
> javascript binding test.

+1. It would be good to file a new bug to add test_ewk2_extension.cpp. Hyungwook, could you do it ?

[WebKit Commit Bot]

Comment on attachment 254065 [details]
patch

Clearing flags on attachment: 254065

Committed r185138: <http://trac.webkit.org/changeset/185138>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Hyungwook Lee]

Comment on attachment 254065 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254065&action=review

>>> Source/WebKit2/UIProcess/API/efl/tests/extensions/extension_sample.cpp:56
>>> +    pageLoadStarted = false;
>> 
>> Hmm, I think that this is correct idea to test this.
>> 
>> How about making test_ewk2_extension.cpp to test these APIs instead of using javascript binding test.
> 
> +1. It would be good to file a new bug to add test_ewk2_extension.cpp. Hyungwook, could you do it ?

Yes, I will consider to implement test_ewk2_extension.cpp