Bug 108796 – [WK2][EFL] Stop using internal C++ API in ewk_error

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

There is currently no API to check if a WKError corresponds to a cancellation or not. I believe such API would be useful.

Note that both EFL and Qt ports of WK2 currently expose this information via their public API.

[Chris Dumez]

Created attachment 186314 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 186314 [details]
Patch

LGTM

[Build Bot]

Comment on attachment 186314 [details]
Patch

Attachment 186314 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16366186

[Chris Dumez]

Win-ews warning seems unrelated:
1>ImageDiffCG.cpp
1>..\cg\ImageDiffCG.cpp(37) : fatal error C1083: Cannot open include file: 'wtf/Platform.h': No such file or directory

[Mikhail Pozdnyakov]

Comment on attachment 186314 [details]
Patch

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

looks good.

> Source/WebKit2/Shared/WebError.h:-54
> -    int errorCode() const { return m_platformError.errorCode();; }

heh

[Brady Eidson]

Comment on attachment 186314 [details]
Patch

Why don't we simply add a new error code to WKError.h?

The only thing that makes a cancellation error special is that it's error code means "cancelled"
If we're adding a "WKErrorIsCancellation" API why not also add a "WKErrorIsCannotLoadPlugIn" API?  Or a "WKErrorFrameLoadInterruptedByPolicyChange" API?

Because that'd be crazy when clients can already compare an error's error code to public API constants.

[Brady Eidson]

Also, another objection is that this patch is changing the cross platform C-API solely as an implementation detail of a platform specific API, when a perfectly good alternative already existed.

I had this same objection to some icon database API work within the last week, and that objection still stands.

[Chris Dumez]

Thanks for the suggestion. I'll look into checking the error code instead.

[Chris Dumez]

Created attachment 186412 [details]
Patch

No longer add any C API and compare error domain/code instead to determine if it is a cancellation.

[WebKit Review Bot]

Attachment 186412 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/efl/ewk_error.cpp', u'Source/WebKit2/UIProcess/API/efl/ewk_error_private.h']" exit_code: 1
Source/WebKit2/UIProcess/API/efl/ewk_error_private.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 186414 [details]
Patch

Fix style error.

[Kenneth Rohde Christiansen]

Comment on attachment 186414 [details]
Patch

LGTM

[Brady Eidson]

Comment on attachment 186414 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_error_private.h:46
> -    String domain() const;
> +    WKRetainPtr<WKStringRef> domain() const;

I don't understand the reason behind this change.  A WKStringRef is just a wrapper around WTF::String.  Why the desperation to use the C-API internally when using WTF::String directly would be easier and more efficient?

[Brady Eidson]

Comment on attachment 186412 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_error.cpp:55
> -String EwkError::domain() const
> +WKRetainPtr<WKStringRef> EwkError::domain() const
>  {
> -    WKRetainPtr<WKStringRef> wkDomain(AdoptWK, WKErrorCopyDomain(m_wkError.get()));
> -    return toWTFString(wkDomain.get());
> +    return adoptWK(WKErrorCopyDomain(m_wkError.get()));

Instead of passing WKStringRefs around and going through this dance, why not leave it like:

String EwkError::domain() const
{
    return toImpl(m_wkError.get())->domain();
}

?

[Chris Dumez]

(In reply to comment #14)
> (From update of attachment 186412 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186412&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_error.cpp:55
> > -String EwkError::domain() const
> > +WKRetainPtr<WKStringRef> EwkError::domain() const
> >  {
> > -    WKRetainPtr<WKStringRef> wkDomain(AdoptWK, WKErrorCopyDomain(m_wkError.get()));
> > -    return toWTFString(wkDomain.get());
> > +    return adoptWK(WKErrorCopyDomain(m_wkError.get()));
> 
> Instead of passing WKStringRefs around and going through this dance, why not leave it like:
> 
> String EwkError::domain() const
> {
>     return toImpl(m_wkError.get())->domain();
> }
> 
> ?

That would mean using WebError type directly which is what we are trying to stop doing. As per recent recommendation from Benjamin Poulain, we decided to use the C API inside our EFL API implementation.

[Kenneth Rohde Christiansen]

> Instead of passing WKStringRefs around and going through this dance, why not leave it like:
> 
> String EwkError::domain() const
> {
>     return toImpl(m_wkError.get())->domain();
> }
> 
> ?

We would like to be able to ship our EFL-like API as a separate project, and potentially use the more low-level WK C API directly for others products such as browser/runtime.

[Chris Dumez]

Comment on attachment 186414 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_error_private.h:46
>> +    WKRetainPtr<WKStringRef> domain() const;
> 
> I don't understand the reason behind this change.  A WKStringRef is just a wrapper around WTF::String.  Why the desperation to use the C-API internally when using WTF::String directly would be easier and more efficient?

The domain is only used to compare with other UTF8 strings (as const char*). So I figured it was better to keep it as a WKStringRef and simply use WKStringIsEqualToUTF8CString(). We could keep converting the WKString to a WTF::String as well but then it would uselessly copy the String.

[Brady Eidson]

(In reply to comment #17)
> (From update of attachment 186414 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186414&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_error_private.h:46
> >> +    WKRetainPtr<WKStringRef> domain() const;
> > 
> > I don't understand the reason behind this change.  A WKStringRef is just a wrapper around WTF::String.  Why the desperation to use the C-API internally when using WTF::String directly would be easier and more efficient?
> 
> The domain is only used to compare with other UTF8 strings (as const char*). So I figured it was better to keep it as a WKStringRef and simply use WKStringIsEqualToUTF8CString(). We could keep converting the WKString to a WTF::String as well but then it would uselessly copy the String.

...UNLESS you did a toImpl(error)->domain() as I mentioned in a previous comment.

(In reply to comment #16)
> > Instead of passing WKStringRefs around and going through this dance, why not leave it like:
> > 
> > String EwkError::domain() const
> > {
> >     return toImpl(m_wkError.get())->domain();
> > }
> > 
> > ?
> 
> We would like to be able to ship our EFL-like API as a separate project, and potentially use the more low-level WK C API directly for others products such as browser/runtime.

Is EwkError exposed directly in this EFL-like API, or is it an "impl" class the same way WebError is an impl class for WKError?

[Brady Eidson]

I'm seeing a bit of an impedance mismatch here.

If the Ewk* classes are meant to be exposed as EFL-like API, then they probably have no business exposing WK* API objects, and there's probably no need to work with WK* API objects internally.

[Chris Dumez]

(In reply to comment #19)
> I'm seeing a bit of an impedance mismatch here.
> 
> If the Ewk* classes are meant to be exposed as EFL-like API, then they probably have no business exposing WK* API objects, and there's probably no need to work with WK* API objects internally.

Ewk* classes are not exposed to clients. Those are C++ classes that we use internally for convenience. Our public EFL API is in C and that C/EFL API uses our Ewk classes internally.

Regarding the use of toImpl(error)->domain(). We got patches r- by WK2 owners before because they used internal C++ WK2 API. It was then recommended to us to use the C API instead and stop poking holes through the layers. Also, the C API will hopefully be quite stable and using it would likely mean that our port code is less likely the break if the WK2 generic code is changed.

As Kenneth also commented, we may start having the WK2 EFL API as a separate project and relying only of the C API is needed to achieve this.

[Brady Eidson]

(In reply to comment #20)
> (In reply to comment #19)
> > I'm seeing a bit of an impedance mismatch here.
> > 
> > If the Ewk* classes are meant to be exposed as EFL-like API, then they probably have no business exposing WK* API objects, and there's probably no need to work with WK* API objects internally.
> 
> Ewk* classes are not exposed to clients. Those are C++ classes that we use internally for convenience. Our public EFL API is in C and that C/EFL API uses our Ewk classes internally.

Right, so the Ewk classes are the "impls" for the actual EFL API classes.

And by moving the Ewk classes from generic WTF/WebCore types to WK* types, you're basically getting ready to punch a hole exposing WK* API in the EFL API.

I don't know if this is what you want to do or not.  I can tell you I personally don't think it's a good idea.
 
> Regarding the use of toImpl(error)->domain(). We got patches r- by WK2 owners before because they used internal C++ WK2 API. It was then recommended to us to use the C API instead and stop poking holes through the layers. Also, the C API will hopefully be quite stable and using it would likely mean that our port code is less likely the break if the WK2 generic code is changed.

I wasn't around for those patches, so I don't know specifics.  I suspect situations were slightly different, but maybe it's exactly the same.
 
> As Kenneth also commented, we may start having the WK2 EFL API as a separate project and relying only of the C API is needed to achieve this.

I'm not sure I'm convinced it's "needed" though I concede it would make it much more convenient from a build engineering perspective.

The mechanics of this patch look fine to me, I just think it's a whole bunch of needless object churn.

At this point I'd recommend pinging one of the reviewers who encouraged previous patches to use the WK2 C-API to implement your API to get them to r+.

[Chris Dumez]

> The mechanics of this patch look fine to me, I just think it's a whole bunch of needless object churn.
> 
> At this point I'd recommend pinging one of the reviewers who encouraged previous patches to use the WK2 C-API to implement your API to get them to r+.

Ok, will do. Thanks for taking a look at the patch and pointing me in the right direction.

[WebKit Review Bot]

Comment on attachment 186414 [details]
Patch

Clearing flags on attachment: 186414

Committed r143297: <http://trac.webkit.org/changeset/143297>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.