Bug 165255

Summary: Make _WKFullscreenDelegate available to users of the WebKit2 C-API.
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: New BugsAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, commit-queue
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
andersca: review+
Patch for landing
jer.noble: commit-queue-
Patch for landing
none
Patch for landing none

Description Jer Noble 2016-12-01 09:20:26 PST
Make _WKFullscreenDelegate available to users of the WebKit2 C-API.
Comment 1 Jer Noble 2016-12-01 09:21:43 PST
Created attachment 295854 [details]
Patch
Comment 2 Jer Noble 2016-12-01 12:50:04 PST
Created attachment 295881 [details]
Patch
Comment 3 Jer Noble 2016-12-01 13:49:30 PST
The GTK error is in WebCore::jsSubtleCryptoPrototypeFunctionUnwrapKey(), unrelated to this patch.
Comment 4 Anders Carlsson 2016-12-01 14:09:12 PST
Comment on attachment 295881 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/mac/WKPagePrivateMac.h:59
> +WK_EXPORT id <_WKFullscreenDelegate> WKPageGetFullscreenDelegate(WKPageRef page);

If possible, please use __weak id here.
Comment 5 Jer Noble 2017-01-06 14:34:34 PST
(In reply to comment #4)
> Comment on attachment 295881 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295881&action=review
> 
> > Source/WebKit2/UIProcess/API/C/mac/WKPagePrivateMac.h:59
> > +WK_EXPORT id <_WKFullscreenDelegate> WKPageGetFullscreenDelegate(WKPageRef page);
> 
> If possible, please use __weak id here.

Doesn't seem to be possible, as __weak only make sense when compiling under ARC, and WKPagePrivateMac.mm doesn't. Adding __weak here just generates a warning.
Comment 6 Jer Noble 2017-01-10 17:38:12 PST
(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 295881 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=295881&action=review
> > 
> > > Source/WebKit2/UIProcess/API/C/mac/WKPagePrivateMac.h:59
> > > +WK_EXPORT id <_WKFullscreenDelegate> WKPageGetFullscreenDelegate(WKPageRef page);
> > 
> > If possible, please use __weak id here.
> 
> Doesn't seem to be possible, as __weak only make sense when compiling under
> ARC, and WKPagePrivateMac.mm doesn't. Adding __weak here just generates a
> warning.

We're already storing the delegate in a WeakObjCPtr, but there doesn't seem to a way to mark the return value of a C function as __weak (or a parameter as __weak) without generating a compiler error.

I guess I could do something like:

#if !defined WEAK
#if __has_feature(objc_arc)
#define WEAK _weak
#else
#define WEAK
#endif

and then mark up the methods as WK_EXPORT WEAK id <...>; Does that seem right? We don't seem to use __weak in any of the other C-style APIs.
Comment 7 Jer Noble 2017-01-17 10:20:15 PST
Created attachment 299039 [details]
Patch for landing
Comment 8 Jer Noble 2017-01-17 10:57:31 PST
Created attachment 299044 [details]
Patch for landing
Comment 9 Jer Noble 2017-01-23 17:24:49 PST
Created attachment 299558 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2017-01-24 11:57:13 PST
Comment on attachment 299558 [details]
Patch for landing

Clearing flags on attachment: 299558

Committed r211094: <http://trac.webkit.org/changeset/211094>