| Summary: | [EFL] Implement load_started callback in EwkPageClient | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Hyungwook Lee <hyungwook.lee> | ||||||||||||
| Component: | WebKit EFL | Assignee: | Hyungwook Lee <hyungwook.lee> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | buildbot, commit-queue, gyuyoung.kim, lucas.de.marchi, rniwa | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
Created attachment 254046 [details]
patch
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 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 Created attachment 254063 [details]
patch
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. Created attachment 254064 [details]
Applying reviewer's feedback.
Created attachment 254065 [details]
patch
Comment on attachment 254065 [details]
patch
LGTM
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 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
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. (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. (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 ? Comment on attachment 254065 [details] patch Clearing flags on attachment: 254065 Committed r185138: <http://trac.webkit.org/changeset/185138> All reviewed patches have been landed. Closing bug. 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 |
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.