Bug 145269 – Add a _WKWebKitContentExtensionStore initializer that takes a path

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Brian Weinstein]

We need a _WKWebKitContentExtensionStore initializer that takes a path

[Brian Weinstein]

Created attachment 253529 [details]
[PATCH] Fix

I’m not sure if UserContentExtensionStore::storeWithPath is correct in terms of memory management, but the rest should be good.

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:57:  Declaration has space between type name and * in UserContentExtensionStore *store  [whitespace/declaration] [3]
Total errors found: 1 in 5 files


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

[Alex Christensen]

Comment on attachment 253529 [details]
[PATCH] Fix

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

> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:57
> +    UserContentExtensionStore *store = new UserContentExtensionStore(storePath);

This is definitely a memory leak.

[Alex Christensen]

You'll probably need to use UserContentExtensionStore.wrapper directly and maybe an adoptNS somewhere to make refcounting work right.  I probably shouldn't be the final reviewer on this patch.

[Anders Carlsson]

Comment on attachment 253529 [details]
[PATCH] Fix

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

>> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:57
>> +    UserContentExtensionStore *store = new UserContentExtensionStore(storePath);
> 
> This is definitely a memory leak.

This should return a RefPtr<UserContentExtensionStore>.

> Source/WebKit2/UIProcess/API/Cocoa/_WKUserContentExtensionStore.h:36
> ++ (instancetype)storeWithPath:(NSURL *)path;

This should be storeWithURL: Alternatively just make an initializer. Can this fail? If so, maybe it should take an NSError out parameter as well?

[Brian Weinstein]

(In reply to comment #5)
> Comment on attachment 253529 [details]
> [PATCH] Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253529&action=review
> 
> >> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:57
> >> +    UserContentExtensionStore *store = new UserContentExtensionStore(storePath);
> > 
> > This is definitely a memory leak.
> 
> This should return a RefPtr<UserContentExtensionStore>.

Will fix.

> 
> > Source/WebKit2/UIProcess/API/Cocoa/_WKUserContentExtensionStore.h:36
> > ++ (instancetype)storeWithPath:(NSURL *)path;
> 
> This should be storeWithURL: Alternatively just make an initializer. Can
> this fail? If so, maybe it should take an NSError out parameter as well?

(In reply to comment #5)
> Comment on attachment 253529 [details]
> [PATCH] Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253529&action=review
> 
> >> Source/WebKit2/UIProcess/API/APIUserContentExtensionStore.cpp:57
> >> +    UserContentExtensionStore *store = new UserContentExtensionStore(storePath);
> > 
> > This is definitely a memory leak.
> 
> This should return a RefPtr<UserContentExtensionStore>.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/_WKUserContentExtensionStore.h:36
> > ++ (instancetype)storeWithPath:(NSURL *)path;
> 
> This should be storeWithURL: Alternatively just make an initializer. Can
> this fail? If so, maybe it should take an NSError out parameter as well?

This should not fail. I’ll rename it to storeWithURL.

[Brian Weinstein]

Created attachment 253533 [details]
[PATCH] Fix v2

[WebKit Commit Bot]

Comment on attachment 253533 [details]
[PATCH] Fix v2

Clearing flags on attachment: 253533

Committed r184726: <http://trac.webkit.org/changeset/184726>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.