Bug 158095 - Expose content extension failure error codes in SPI
Summary: Expose content extension failure error codes in SPI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-25 14:49 PDT by Alex Christensen
Modified: 2016-05-27 11:50 PDT (History)
2 users (show)

See Also:


Attachments
Patch (1.91 KB, patch)
2016-05-25 14:49 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (13.17 KB, patch)
2016-05-25 16:36 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (13.20 KB, patch)
2016-05-25 17:05 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (12.38 KB, patch)
2016-05-26 16:04 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (12.71 KB, patch)
2016-05-27 11:29 PDT, Alex Christensen
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2016-05-25 14:49:24 PDT
Expose content extension failure error codes in SPI
Comment 1 Alex Christensen 2016-05-25 14:49:51 PDT
Created attachment 279820 [details]
Patch
Comment 2 Alex Christensen 2016-05-25 16:36:35 PDT
Created attachment 279836 [details]
Patch
Comment 3 Alex Christensen 2016-05-25 17:05:42 PDT
Created attachment 279841 [details]
Patch
Comment 4 Sam Weinig 2016-05-26 15:51:06 PDT
Comment on attachment 279841 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/_WKUserContentExtensionStore.h:45
> +// These correspond with the error codes in UserContentExtensionStore::Error.
> +typedef NS_ENUM(NSInteger, _WKUserContentExtensionStoreErrorCode) {

Instead of requiring these to stay in sync, we should explicitly convert from one to the other with a switch statement like we do for other errors.  Also, SPI headers should not refer to internals.

I also think this should be an extension to WKErrorCode and we should switch to using the WKError domain.  I believe you do that by using constants of type WKErrorCode, but am not sure.  I'll let Anders say.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/_WKUserContentExtensionStore.mm:68
> +        EXPECT_EQ(error.code, _WKUserContentExtensionStoreErrorCode::CompileFailed);

This should test the domain as well.
Comment 5 Alex Christensen 2016-05-26 16:04:52 PDT
Created attachment 279920 [details]
Patch
Comment 6 Alex Christensen 2016-05-26 16:09:58 PDT
(In reply to comment #4)
> I also think this should be an extension to WKErrorCode and we should switch
> to using the WKError domain.  I believe you do that by using constants of
> type WKErrorCode, but am not sure.  I'll let Anders say.
I disagree and have implemented it as a separate domain.  If Anders tells me otherwise, I'll change it.
Comment 7 Anders Carlsson 2016-05-27 11:25:00 PDT
Comment on attachment 279920 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/_WKUserContentExtensionStore.h:50
> +    LookupFailed = 1,
> +    VersionMismatch,
> +    CompileFailed,
> +    RemoveFailed,

These should be prefixed correctly,

_WKUserContentExtensionStoreErrorLookupFailed, _WKUserContentExtensionStoreErrorVersionMismatch etc.
Comment 8 Alex Christensen 2016-05-27 11:29:54 PDT
Created attachment 279976 [details]
Patch
Comment 9 Anders Carlsson 2016-05-27 11:33:17 PDT
Comment on attachment 279976 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/_WKUserContentExtensionStore.h:44
> +WK_EXTERN NSString * const _ContentExtensionsDomain WK_AVAILABLE(WK_MAC_TBA, WK_IOS_TBA);

Should be _WKUserContentExtensionsDomain.

> Source/WebKit2/UIProcess/API/Cocoa/_WKUserContentExtensionStore.mm:87
> +            auto code = toWKUserContentExtensionStoreErrorCode(API::UserContentExtensionStore::Error::CompileFailed);
> +            rawHandler(nil, [NSError errorWithDomain:_ContentExtensionsDomain code:code userInfo:userInfo]);

No need to use toWKUserContentExtensionStoreErrorCode here - just pass the WebKit code directly.

> Source/WebKit2/UIProcess/API/Cocoa/_WKUserContentExtensionStore.mm:126
> +            auto code = toWKUserContentExtensionStoreErrorCode(API::UserContentExtensionStore::Error::RemoveFailed);
> +            rawHandler([NSError errorWithDomain:_ContentExtensionsDomain code:code userInfo:userInfo]);

No need to use toWKUserContentExtensionStoreErrorCode here - just pass the WebKit code directly.
Comment 10 Alex Christensen 2016-05-27 11:50:01 PDT
Passing the WebKit code doesn't like converting an enum to an NSInteger, so I just used the new API codes.

http://trac.webkit.org/changeset/201457