Bug 109828

Summary: [WK2][EFL] Add callbacks to the WKViewClient to handle Web Process crash and relaunch
Product: WebKit Reporter: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Component: WebKit EFLAssignee: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, benjamin, cdumez, cmarcelo, gyuyoung.kim, kenneth, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 107657    
Attachments:
Description Flags
patch
none
patch v2
none
patch v3
none
patch v4
none
patch v5
kenneth: review+
patch v6
kenneth: review+
to be landed none

Description Mikhail Pozdnyakov 2013-02-14 07:00:28 PST
SSIA.
Comment 1 Mikhail Pozdnyakov 2013-02-14 07:19:12 PST
Created attachment 188346 [details]
patch
Comment 2 Chris Dumez 2013-02-14 07:36:27 PST
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 3 Chris Dumez 2013-02-14 07:36:59 PST
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.
Comment 4 Mikhail Pozdnyakov 2013-02-14 09:51:11 PST
(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 5 Mikhail Pozdnyakov 2013-02-14 09:56:22 PST
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.
Comment 6 Chris Dumez 2013-02-14 10:02:12 PST
(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.
Comment 7 Kenneth Rohde Christiansen 2013-02-15 05:20:56 PST
> 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
Comment 8 Mikhail Pozdnyakov 2013-02-15 06:22:29 PST
> 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.
Comment 9 Mikhail Pozdnyakov 2013-02-15 06:39:19 PST
Created attachment 188552 [details]
patch v2

Took comments form Chris into consideration. 
Fixed theme path restoring.
Comment 10 Kenneth Rohde Christiansen 2013-02-15 06:45:13 PST
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?
Comment 11 Chris Dumez 2013-02-15 06:48:34 PST
(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.
Comment 12 Mikhail Pozdnyakov 2013-02-15 11:02:03 PST
Created attachment 188603 [details]
patch v3

Added API tests
Comment 13 Kenneth Rohde Christiansen 2013-02-18 04:56:39 PST
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
Comment 14 Mikhail Pozdnyakov 2013-02-18 05:08:12 PST
(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
Comment 15 Mikhail Pozdnyakov 2013-02-18 05:32:33 PST
Created attachment 188860 [details]
patch v4

Change log updated.
Comment 16 Kenneth Rohde Christiansen 2013-02-20 01:12:08 PST
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?
Comment 17 Mikhail Pozdnyakov 2013-02-20 03:50:54 PST
Created attachment 189280 [details]
patch v5

Use WKPageGetEstimatedProgress rather than ewk_view_load_progress_get
Comment 18 Kenneth Rohde Christiansen 2013-02-20 04:18:11 PST
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
Comment 19 Mikhail Pozdnyakov 2013-02-20 04:31:56 PST
(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.
Comment 20 Kenneth Rohde Christiansen 2013-02-20 04:34:53 PST
LGTM, I would add a commetn about the informUrl change
Comment 21 Benjamin Poulain 2013-02-26 13:32:47 PST
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.
Comment 22 Mikhail Pozdnyakov 2013-02-27 02:37:13 PST
(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 23 Kenneth Rohde Christiansen 2013-03-04 01:09:53 PST
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
Comment 24 Mikhail Pozdnyakov 2013-03-04 05:46:33 PST
Created attachment 191215 [details]
patch v6

Re-factored the added API test so that all the test data is local for the test.
Comment 25 Mikhail Pozdnyakov 2013-03-04 05:49:13 PST
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 26 Kenneth Rohde Christiansen 2013-03-04 12:37:22 PST
Comment on attachment 191215 [details]
patch v6

r=me as signed off by ben
Comment 27 Mikhail Pozdnyakov 2013-03-04 12:43:58 PST
Created attachment 191294 [details]
to be landed
Comment 28 Mikhail Pozdnyakov 2013-03-04 13:40:56 PST
Committed r144664: <http://trac.webkit.org/changeset/144664>