Bug 108796 - [WK2][EFL] Stop using internal C++ API in ewk_error
Summary: [WK2][EFL] Stop using internal C++ API in ewk_error
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks: 107657
  Show dependency treegraph
 
Reported: 2013-02-03 23:38 PST by Chris Dumez
Modified: 2013-02-18 22:59 PST (History)
11 users (show)

See Also:


Attachments
Patch (3.23 KB, patch)
2013-02-03 23:49 PST, Chris Dumez
beidson: review-
Details | Formatted Diff | Diff
Patch (3.84 KB, patch)
2013-02-04 10:18 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (3.84 KB, patch)
2013-02-04 10:22 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2013-02-03 23:38:40 PST
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.
Comment 1 Chris Dumez 2013-02-03 23:49:04 PST
Created attachment 186314 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2013-02-04 00:29:45 PST
Comment on attachment 186314 [details]
Patch

LGTM
Comment 3 Build Bot 2013-02-04 00:58:11 PST
Comment on attachment 186314 [details]
Patch

Attachment 186314 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16366186
Comment 4 Chris Dumez 2013-02-04 00:58:54 PST
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
Comment 5 Mikhail Pozdnyakov 2013-02-04 01:27:28 PST
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
Comment 6 Brady Eidson 2013-02-04 09:08:26 PST
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.
Comment 7 Brady Eidson 2013-02-04 09:10:31 PST
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.
Comment 8 Chris Dumez 2013-02-04 09:45:49 PST
Thanks for the suggestion. I'll look into checking the error code instead.
Comment 9 Chris Dumez 2013-02-04 10:18:04 PST
Created attachment 186412 [details]
Patch

No longer add any C API and compare error domain/code instead to determine if it is a cancellation.
Comment 10 WebKit Review Bot 2013-02-04 10:20:37 PST
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.
Comment 11 Chris Dumez 2013-02-04 10:22:52 PST
Created attachment 186414 [details]
Patch

Fix style error.
Comment 12 Kenneth Rohde Christiansen 2013-02-04 10:24:47 PST
Comment on attachment 186414 [details]
Patch

LGTM
Comment 13 Brady Eidson 2013-02-04 10:26:37 PST
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?
Comment 14 Brady Eidson 2013-02-04 10:29:54 PST
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();
}

?
Comment 15 Chris Dumez 2013-02-04 10:33:03 PST
(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.
Comment 16 Kenneth Rohde Christiansen 2013-02-04 10:37:13 PST
> 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.
Comment 17 Chris Dumez 2013-02-04 10:40:12 PST
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.
Comment 18 Brady Eidson 2013-02-04 11:01:55 PST
(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?
Comment 19 Brady Eidson 2013-02-04 11:03:47 PST
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.
Comment 20 Chris Dumez 2013-02-04 11:14:36 PST
(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.
Comment 21 Brady Eidson 2013-02-04 11:26:55 PST
(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+.
Comment 22 Chris Dumez 2013-02-04 11:40:04 PST
> 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.
Comment 23 WebKit Review Bot 2013-02-18 22:59:35 PST
Comment on attachment 186414 [details]
Patch

Clearing flags on attachment: 186414

Committed r143297: <http://trac.webkit.org/changeset/143297>
Comment 24 WebKit Review Bot 2013-02-18 22:59:40 PST
All reviewed patches have been landed.  Closing bug.