Created attachment 186314 [details] Patch

Comment on attachment 186314 [details] Patch LGTM

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

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

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.

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

Created attachment 186412 [details] Patch No longer add any C API and compare error domain/code instead to determine if it is a cancellation.

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.

Created attachment 186414 [details] Patch Fix style error.

Comment on attachment 186414 [details] Patch LGTM

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 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(); } ?

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

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

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

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.

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

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

> 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 on attachment 186414 [details] Patch Clearing flags on attachment: 186414 Committed r143297: <http://trac.webkit.org/changeset/143297>

All reviewed patches have been landed. Closing bug.