Bug 109828 – [WK2][EFL] Add callbacks to the WKViewClient to handle Web Process crash and relaunch

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

SSIA.

[Mikhail Pozdnyakov]

Created attachment 188346 [details]
patch

[Chris Dumez]

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.

[Chris Dumez]

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.

[Mikhail Pozdnyakov]

(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

[Mikhail Pozdnyakov]

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.

[Chris Dumez]

(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.

[Kenneth Rohde Christiansen]

> 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

[Mikhail Pozdnyakov]

> 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.

[Mikhail Pozdnyakov]

Created attachment 188552 [details]
patch v2

Took comments form Chris into consideration. 
Fixed theme path restoring.

[Kenneth Rohde Christiansen]

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?

[Chris Dumez]

(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.

[Mikhail Pozdnyakov]

Created attachment 188603 [details]
patch v3

Added API tests

[Kenneth Rohde Christiansen]

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

[Mikhail Pozdnyakov]

(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

[Mikhail Pozdnyakov]

Created attachment 188860 [details]
patch v4

Change log updated.

[Kenneth Rohde Christiansen]

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?

[Mikhail Pozdnyakov]

Created attachment 189280 [details]
patch v5

Use WKPageGetEstimatedProgress rather than ewk_view_load_progress_get

[Kenneth Rohde Christiansen]

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

[Mikhail Pozdnyakov]

(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.

[Kenneth Rohde Christiansen]

LGTM, I would add a commetn about the informUrl change

[Benjamin Poulain]

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.

[Mikhail Pozdnyakov]

(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.

[Kenneth Rohde Christiansen]

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

[Mikhail Pozdnyakov]

Created attachment 191215 [details]
patch v6

Re-factored the added API test so that all the test data is local for the test.

[Mikhail Pozdnyakov]

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 :) )

[Kenneth Rohde Christiansen]

Comment on attachment 191215 [details]
patch v6

r=me as signed off by ben

[Mikhail Pozdnyakov]

Created attachment 191294 [details]
to be landed

[Mikhail Pozdnyakov]

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