Bug 109828

Created attachment 188346 [details] patch

Comment on attachment 188346 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=188346&action=review > Source/WebKit2/UIProcess/API/C/efl/WKView.h:40 > +typedef void (*WKViewWebProcessCrashedCallback)(WKViewRef view, WKStringRef url, const void* clientInfo); Shouldn't we use a WKURL instead of a WKString? > Source/WebKit2/UIProcess/API/C/efl/WKView.h:41 > +typedef void (*WKViewWebProcessDidRelaunchCallback)(WKViewRef view, const void* clientInfo); This one could named as a generic WKViewCallback that would be reusable. See for example WKPageCallback. > Source/WebKit2/UIProcess/API/C/efl/WKView.h:51 > + WKViewWebProcessDidRelaunchCallback webProcessDidRelaunch; Do we really need this one? > Source/WebKit2/UIProcess/efl/PageClientBase.cpp:127 > + view()->webProcessDidRelaunch(); Why don't we store the theme path in WebView instead of EwkView? > Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:69 > + WKEinaSharedString urlString(url); This should be in the if scope.

Comment on attachment 188346 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=188346&action=review > Source/WebKit2/UIProcess/efl/WebViewClient.h:46 > + void webProcessCrashed(WebView*, const WTF::String& url); WTF:: is not needed.

(In reply to comment #2) > (From update of attachment 188346 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=188346&action=review > > > Source/WebKit2/UIProcess/API/C/efl/WKView.h:40 > > +typedef void (*WKViewWebProcessCrashedCallback)(WKViewRef view, WKStringRef url, const void* clientInfo); > > Shouldn't we use a WKURL instead of a WKString? I was considering this, but I prefer WKString: 1) wep page returns string not url 2) For the most cases string enough, on the other hand it's easy to to construct WKURL on client side if needed. 3) More compact code > > > Source/WebKit2/UIProcess/API/C/efl/WKView.h:41 > > +typedef void (*WKViewWebProcessDidRelaunchCallback)(WKViewRef view, const void* clientInfo); > > This one could named as a generic WKViewCallback that would be reusable. See for example WKPageCallback. Good point, thanks > > > Source/WebKit2/UIProcess/API/C/efl/WKView.h:51 > > + WKViewWebProcessDidRelaunchCallback webProcessDidRelaunch; > > Do we really need this one? I guess so > > > Source/WebKit2/UIProcess/efl/PageClientBase.cpp:127 > > + view()->webProcessDidRelaunch(); > > Why don't we store the theme path in WebView instead of EwkView? webview is just exposing the API we miss.. IMO it should contain nether a lot of logic nor a lot of data, so would keep it as thin as possible. > > > Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:69 > > + WKEinaSharedString urlString(url); > > This should be in the if scope. ok

Comment on attachment 188346 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=188346&action=review > Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:84 > + ewkView->setThemePath(themePath); Found a problem here: actually WKViewSetThemePath should be invoked, as EwkView::setTheme checks if the given value is the same :/ Will correct it in the next version of the patch.

(In reply to comment #4) > (In reply to comment #2) > > (From update of attachment 188346 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=188346&action=review > > > > > Source/WebKit2/UIProcess/API/C/efl/WKView.h:40 > > > +typedef void (*WKViewWebProcessCrashedCallback)(WKViewRef view, WKStringRef url, const void* clientInfo); > > > > Shouldn't we use a WKURL instead of a WKString? > I was considering this, but I prefer WKString: > 1) wep page returns string not url > 2) For the most cases string enough, on the other hand it's easy to > to construct WKURL on client side if needed. > 3) More compact code It is just a matter of calling toCopiedURLAPI() instead of toCopiedAPI() so 3) is not really a good argument IMHO. On the other hand, providing a WKURL to the client provide it with more useful information (e.g. it can easily get the host). It's not a big deal, but I think it makes some sense. > > > > > Source/WebKit2/UIProcess/API/C/efl/WKView.h:41 > > > +typedef void (*WKViewWebProcessDidRelaunchCallback)(WKViewRef view, const void* clientInfo); > > > > This one could named as a generic WKViewCallback that would be reusable. See for example WKPageCallback. > Good point, thanks > > > > > Source/WebKit2/UIProcess/API/C/efl/WKView.h:51 > > > + WKViewWebProcessDidRelaunchCallback webProcessDidRelaunch; > > > > Do we really need this one? > I guess so > > > > > Source/WebKit2/UIProcess/efl/PageClientBase.cpp:127 > > > + view()->webProcessDidRelaunch(); > > > > Why don't we store the theme path in WebView instead of EwkView? > webview is just exposing the API we miss.. IMO it should contain nether > a lot of logic nor a lot of data, so would keep it as thin as possible. Well, you will need to set the theme on the view anyway with WKViewSetThemePath(). I think it is weird to store the theme path on EwkView side but then call WKViewSetThemePath() on the WebView. Kenneth, do you have an opinion on this? I think we should try to get it right as we are adding new C API that we may end up removing afterwards.

> Well, you will need to set the theme on the view anyway with WKViewSetThemePath(). I think it is weird to store the theme path on EwkView side but then call WKViewSetThemePath() on the WebView. > > Kenneth, do you have an opinion on this? I think we should try to get it right as we are adding new C API that we may end up removing afterwards. I agree with you and have already talked with misha

> Well, you will need to set the theme on the view anyway with WKViewSetThemePath(). I think it is weird to store the theme path on EwkView side but then call WKViewSetThemePath() on the WebView. > We would anyway need theme to be stored in EwkView as we have to support 'const char* ewk_view_theme_get(const Evas_Object* ewkView)' - WK2 EFL public API function. So keeping theme path also in WebView does not make much sense.

Created attachment 188552 [details] patch v2 Took comments form Chris into consideration. Fixed theme path restoring.

Comment on attachment 188552 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=188552&action=review LGTM, but shouldnt we add API tests? :) > Source/WebKit2/UIProcess/API/C/efl/WKView.h:52 > + WKViewWebProcessCrashedCallback webProcessCrashed; > + WKViewCallback webProcessDidRelaunch; > }; Is this common style? having a WKClassNameCallback when the callback takes no special arguments?

(In reply to comment #10) > (From update of attachment 188552 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=188552&action=review > > LGTM, but shouldnt we add API tests? :) > > > Source/WebKit2/UIProcess/API/C/efl/WKView.h:52 > > + WKViewWebProcessCrashedCallback webProcessCrashed; > > + WKViewCallback webProcessDidRelaunch; > > }; > > Is this common style? having a WKClassNameCallback when the callback takes no special arguments? As I said in https://bugs.webkit.org/show_bug.cgi?id=109828#c2, it is done for WKPageCallback at least so it is not new.

Created attachment 188603 [details] patch v3 Added API tests

Comment on attachment 188603 [details] patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=188603&action=review > Source/WebKit2/ChangeLog:3 > + [WK2][EFL] Introduce Web Process crash and Web Process relaunch callbacks to WKViewClient Add callbacks to the WKViewClient to handle Web Process crash and relaunch It sounds as you are introducing a crash :-) > Source/WebKit2/ChangeLog:10 > + Providing WKViewClient with Web Process crash and Web Process relaunch > + callbacks brings better design as WebView should not be aware of > + EFL-specific code handling the corresponding events. You should say that you also add an implementation

(In reply to comment #13) > (From update of attachment 188603 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=188603&action=review > > > Source/WebKit2/ChangeLog:3 > > + [WK2][EFL] Introduce Web Process crash and Web Process relaunch callbacks to WKViewClient > > Add callbacks to the WKViewClient to handle Web Process crash and relaunch > > It sounds as you are introducing a crash :-) :-) hopefully not

Created attachment 188860 [details] patch v4 Change log updated.

Comment on attachment 188860 [details] patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=188860&action=review LGTM > Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:59 > + // Check if loading was ongoing, when web process crashed. > + double loadProgress = ewk_view_load_progress_get(ewkView->evasObject()); Wouldnt it be easier to get this directly with the WK C API?

Created attachment 189280 [details] patch v5 Use WKPageGetEstimatedProgress rather than ewk_view_load_progress_get

Comment on attachment 189280 [details] patch v5 View in context: https://bugs.webkit.org/attachment.cgi?id=189280&action=review > Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:72 > + if (!handled) { > + WKEinaSharedString urlString(url); > + WARN("WARNING: The web process experienced a crash on '%s'.\n", static_cast<const char*>(urlString)); Any reason to eina share this? > Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:75 > + // Display an error page > + ewk_view_html_string_load(ewkView->evasObject(), "The web process has crashed.", 0, urlString); I had assumed you got rid of this one as well, if it is not a lot of extra code with the C api

(In reply to comment #18) > (From update of attachment 189280 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189280&action=review > > > Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:72 > > + if (!handled) { > > + WKEinaSharedString urlString(url); > > + WARN("WARNING: The web process experienced a crash on '%s'.\n", static_cast<const char*>(urlString)); > > Any reason to eina share this? WKEinaSharedString just provides convenient way of getting "const char*" from WKURLRef. Eina sharing is overhead but I do not think it is a significant overhead considering obtained code compactness. > > > Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:75 > > + // Display an error page > > + ewk_view_html_string_load(ewkView->evasObject(), "The web process has crashed.", 0, urlString); > > I had assumed you got rid of this one as well, if it is not a lot of extra code with the C api ewk_view_html_string_load will also invoke EwkView::informURLChange() which is needed.

LGTM, I would add a commetn about the informUrl change

Comment on attachment 189280 [details] patch v5 View in context: https://bugs.webkit.org/attachment.cgi?id=189280&action=review No problem with this, I sign off on the change. > Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:61 > + loadProgress = 1; not loadProgress = 0? > Tools/TestWebKitAPI/Tests/WebKit2/efl/WKViewClientWebProcessCallbacks.cpp:41 > +static bool didFinishLoad; > +static bool didCrash; > +static bool didRelaunch; > + > +static WKURLRef urlToPass; > +static WKViewRef viewToPass; > +static const char* dataToPass; For a common test, I would say this should be test local. For EFL tests, you do as you wish I guess.

(In reply to comment #21) > (From update of attachment 189280 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189280&action=review > > No problem with this, I sign off on the change. > > > Source/WebKit2/UIProcess/efl/ViewClientEfl.cpp:61 > > + loadProgress = 1; > > not loadProgress = 0? > Agree, this might look weird but it's a part of WK2 EFL behaviour that I'm keeping untouched in this patch.

Comment on attachment 189280 [details] patch v5 View in context: https://bugs.webkit.org/attachment.cgi?id=189280&action=review r=me as signed off my benjamin >> Tools/TestWebKitAPI/Tests/WebKit2/efl/WKViewClientWebProcessCallbacks.cpp:41 >> +static const char* dataToPass; > > For a common test, I would say this should be test local. > For EFL tests, you do as you wish I guess. Mikhail can you have a look to see if you could make this change? People will look at current tests when writing new ones

Created attachment 191215 [details] patch v6 Re-factored the added API test so that all the test data is local for the test.

Comment on attachment 191215 [details] patch v6 View in context: https://bugs.webkit.org/attachment.cgi?id=191215&action=review > Tools/TestWebKitAPI/Tests/WebKit2/efl/WKViewClientWebProcessCallbacks.cpp:104 > + WKURLRef url = Util::createURLForResource("simple", "html"); ahh, I should have used WKRetainPtr here.. (but I can fix it while committing :) )

Comment on attachment 191215 [details] patch v6 r=me as signed off by ben

Created attachment 191294 [details] to be landed

Committed r144664: <http://trac.webkit.org/changeset/144664>