Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

icrate method skips for WebKit #327

Closed
wants to merge 3 commits into from
Closed

icrate method skips for WebKit #327

wants to merge 3 commits into from

Conversation

silvanshade
Copy link
Contributor

I made an initial attempt at autogenerating WebKit using the header-translator and this PR includes the modifications to the translation-config.toml I needed. I haven't tried to use the generated output yet so I haven't included that in this PR.

I'm interesting in using this for a tauri project I'm working on that requires interfacing with the platform-specific webview (which, in the case of macos, involves WKWebView.

More generally, I'm interested in #174, specifically for wry, tao, and tauri, but potentially other projects as well if you're still looking for help with that.

@silvanshade silvanshade changed the title Webkit skips icrate method skips for WebKit Jan 9, 2023
Copy link
Owner

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'm definitely interested in supporting this framework as well!

The current setup is a bit bespoke, so feel free to point out and fix things that are annoying about the tool!

I haven't tried to use the generated output yet so I haven't included that in this PR.

I think I'd prefer to have that before I merge this PR (will require an accompanying PR in icrate-generated, which is, yeah, annoying).

potentially other projects as well if you're still looking for help with that.

Definitely, though probably not quite yet, I think it's a little too early for icrate for us to do that ;) (it's a difficult balance between "when is icrate ready" and "when will it cause users the least pain when needing to upgrade").

crates/header-translator/translation-config.toml Outdated Show resolved Hide resolved
crates/header-translator/translation-config.toml Outdated Show resolved Hide resolved
crates/header-translator/translation-config.toml Outdated Show resolved Hide resolved
@silvanshade
Copy link
Contributor Author

silvanshade commented Jan 11, 2023

Okay, I managed to get this working finally, after tracking down the remaining issues and adding the method skips. I also verified that the generated bindings work and was able to completely replace all uses of msg_send! with actual method calls in my tauri project. Once I double check the added configuration (I'm not entirely sure all of the skips I added are necessary yet) I'll update this and submit an accompanying PR for the generated output.

@silvanshade
Copy link
Contributor Author

@madsmtm It seems a lot of the remaining method skip directives I needed to add were due to the compiler giving errors due to MessageReceiver not being implemented for Option<Allocated<T>>. I was looking at adding this instance and was wondering if this is the correct way to do it:

impl<T: Message + ?Sized> Sealed for Option<Allocated<T>> {}

unsafe impl<T: Message + ?Sized> MessageReceiver for Option<Allocated<T>> {
    type __Inner = T;

    #[inline]
    fn __as_raw_receiver(self) -> *mut Object {
        Allocated::option_into_ptr(self).cast()
    }
}

If so, I'll go ahead and add this to the PR and update the config.

@madsmtm
Copy link
Owner

madsmtm commented Jan 11, 2023

Could you give an example of a generated method that causes the error?

@silvanshade
Copy link
Contributor Author

Could you give an example of a generated method that causes the error?

Sure. Actually looking at the generated output just now there aren't as many as I thought, and they are all related to init methods for various DOM event classes. When I was trying to generate the bindings, it looked like I was getting errors on all of the methods for those classes (which I elided below). But I think just skipping those init methods would be enough. I also note that most of these classes and methods are listed as deprecated too, but since we aren't really excluding those in principle at this point, might as well try to fix it for completeness sake.

Note that this is the generated output after I added those impls (I also had to add up to 16 argument impls for EncodeArguments and MessageArguments).

extern_methods!(
    unsafe impl DOMEvent {
        #[method(initEvent:canBubbleArg:cancelableArg:)]
        pub unsafe fn initEvent_canBubbleArg_cancelableArg(
            this: Option<Allocated<Self>>,
            eventTypeArg: Option<&NSString>,
            canBubbleArg: bool,
            cancelableArg: bool,
        );
    }
);
extern_methods!(
    unsafe impl DOMKeyboardEvent {
        #[method(initKeyboardEvent:canBubble:cancelable:view:keyIdentifier:location:ctrlKey:altKey:shiftKey:metaKey:altGraphKey:)]
        pub unsafe fn initKeyboardEvent_canBubble_cancelable_view_keyIdentifier_location_ctrlKey_altKey_shiftKey_metaKey_altGraphKey(
            this: Option<Allocated<Self>>,
            type_: Option<&NSString>,
            canBubble: bool,
            cancelable: bool,
            view: Option<&DOMAbstractView>,
            keyIdentifier: Option<&NSString>,
            location: c_uint,
            ctrlKey: bool,
            altKey: bool,
            shiftKey: bool,
            metaKey: bool,
            altGraphKey: bool,
        );

        #[method(initKeyboardEvent:canBubble:cancelable:view:keyIdentifier:location:ctrlKey:altKey:shiftKey:metaKey:)]
        pub unsafe fn initKeyboardEvent_canBubble_cancelable_view_keyIdentifier_location_ctrlKey_altKey_shiftKey_metaKey(
            this: Option<Allocated<Self>>,
            type_: Option<&NSString>,
            canBubble: bool,
            cancelable: bool,
            view: Option<&DOMAbstractView>,
            keyIdentifier: Option<&NSString>,
            location: c_uint,
            ctrlKey: bool,
            altKey: bool,
            shiftKey: bool,
            metaKey: bool,
        );

        #[method(initKeyboardEvent:canBubble:cancelable:view:keyIdentifier:keyLocation:ctrlKey:altKey:shiftKey:metaKey:altGraphKey:)]
        pub unsafe fn initKeyboardEvent_canBubble_cancelable_view_keyIdentifier_keyLocation_ctrlKey_altKey_shiftKey_metaKey_altGraphKey(
            this: Option<Allocated<Self>>,
            type_: Option<&NSString>,
            canBubble: bool,
            cancelable: bool,
            view: Option<&DOMAbstractView>,
            keyIdentifier: Option<&NSString>,
            keyLocation: c_uint,
            ctrlKey: bool,
            altKey: bool,
            shiftKey: bool,
            metaKey: bool,
            altGraphKey: bool,
        );

        #[method(initKeyboardEvent:canBubble:cancelable:view:keyIdentifier:keyLocation:ctrlKey:altKey:shiftKey:metaKey:)]
        pub unsafe fn initKeyboardEvent_canBubble_cancelable_view_keyIdentifier_keyLocation_ctrlKey_altKey_shiftKey_metaKey(
            this: Option<Allocated<Self>>,
            type_: Option<&NSString>,
            canBubble: bool,
            cancelable: bool,
            view: Option<&DOMAbstractView>,
            keyIdentifier: Option<&NSString>,
            keyLocation: c_uint,
            ctrlKey: bool,
            altKey: bool,
            shiftKey: bool,
            metaKey: bool,
        );
    }
);
extern_methods!(
    unsafe impl DOMMouseEvent {
        #[method(initMouseEvent:canBubble:cancelable:view:detail:screenX:screenY:clientX:clientY:ctrlKey:altKey:shiftKey:metaKey:button:relatedTarget:)]
        pub unsafe fn initMouseEvent_canBubble_cancelable_view_detail_screenX_screenY_clientX_clientY_ctrlKey_altKey_shiftKey_metaKey_button_relatedTarget(
            this: Option<Allocated<Self>>,
            type_: Option<&NSString>,
            canBubble: bool,
            cancelable: bool,
            view: Option<&DOMAbstractView>,
            detail: c_int,
            screenX: c_int,
            screenY: c_int,
            clientX: c_int,
            clientY: c_int,
            ctrlKey: bool,
            altKey: bool,
            shiftKey: bool,
            metaKey: bool,
            button: c_ushort,
            relatedTarget: Option<&DOMEventTarget>,
        );
    }
);
extern_methods!(
    unsafe impl DOMMutationEvent {
        #[method(initMutationEvent:canBubble:cancelable:relatedNode:prevValue:newValue:attrName:attrChange:)]
        pub unsafe fn initMutationEvent_canBubble_cancelable_relatedNode_prevValue_newValue_attrName_attrChange(
            this: Option<Allocated<Self>>,
            type_: Option<&NSString>,
            canBubble: bool,
            cancelable: bool,
            relatedNode: Option<&DOMNode>,
            prevValue: Option<&NSString>,
            newValue: Option<&NSString>,
            attrName: Option<&NSString>,
            attrChange: c_ushort,
        );
    }
);
extern_methods!(
    unsafe impl DOMOverflowEvent {
        #[method(initOverflowEvent:horizontalOverflow:verticalOverflow:)]
        pub unsafe fn initOverflowEvent_horizontalOverflow_verticalOverflow(
            this: Option<Allocated<Self>>,
            orient: c_ushort,
            horizontalOverflow: bool,
            verticalOverflow: bool,
        );
    }
);
extern_methods!(
    unsafe impl DOMUIEvent {
        #[method(initUIEvent:canBubble:cancelable:view:detail:)]
        pub unsafe fn initUIEvent_canBubble_cancelable_view_detail(
            this: Option<Allocated<Self>>,
            type_: Option<&NSString>,
            canBubble: bool,
            cancelable: bool,
            view: Option<&DOMAbstractView>,
            detail: c_int,
        );
    }
);
extern_methods!(
    unsafe impl DOMWheelEvent {
        #[method(initWheelEvent:wheelDeltaY:view:screenX:screenY:clientX:clientY:ctrlKey:altKey:shiftKey:metaKey:)]
        pub unsafe fn initWheelEvent_wheelDeltaY_view_screenX_screenY_clientX_clientY_ctrlKey_altKey_shiftKey_metaKey(
            this: Option<Allocated<Self>>,
            wheelDeltaX: c_int,
            wheelDeltaY: c_int,
            view: Option<&DOMAbstractView>,
            screenX: c_int,
            screenY: c_int,
            clientX: c_int,
            clientY: c_int,
            ctrlKey: bool,
            altKey: bool,
            shiftKey: bool,
            metaKey: bool,
        );
    }
);

@madsmtm
Copy link
Owner

madsmtm commented Jan 12, 2023

Ah, okay.

So this is a bit gnarly, but the gist is that some methods like init, alloc, new, newWithParam:, and so on belong to a "family", which describe the calling convention of said method.

It just so happens that this property, the "family", and therefore the calling convention, is always* possible to know just by looking at the selector. Which is what I use in msg_send_id! and what header-translator uses to determine when to use #[method_id(...)] vs #[method(...)].

Notice the asterisk above? The thing is, the init-family has a special case for non-pointer return values, in which case it falls back to just being a regular method.

In your case, the methods return void, but header-translator still thinks it's dealing with an init method, so it happily emits the this: Option<Allocated<Self>>. This is wrong, since that will consume the item, making it impossible to use afterwards, and would also causing a leak - which is exactly why the type-system later catches it.

I'll have a look at fixing it, essentially we should just output #[method(...)] and &self instead, give me a day or two (and just keep them disabled for now).

See method families and semantics of init in clang's documentation of ARC for more details.

When I was trying to generate the bindings, it looked like I was getting errors on all of the methods for those classes

Yeah, that's an issue with extern_methods! that's possible to fix, but has a certain probability of degrading other diagnostics. I might try to do it at some point though.

@silvanshade
Copy link
Contributor Author

By the way, I was able to update this some and add support for JavaScriptCore.

There are some things I needed to skip from that framework (mentioned in the config) having to do with errors about bool not supporting encode. I read the docs about Encode and saw handling about bool and BOOL but wasn't really sure how to resolve the issue. (Also trying to compile with those definitions changed manually to BOOL gave some warnings about the struct not being FFI safe IIRC, so that's another potential issue).

Adding Security still seems problematic since the header-translator doesn't even finish processing and gives several errors along the way, so will have to revisit that later.

I also note that the PR is failing on the "compiler UI" tests here but I'm not sure what to do about that.

@madsmtm
Copy link
Owner

madsmtm commented Jan 14, 2023

Adding Security still seems problematic since the header-translator doesn't even finish processing and gives several errors along the way, so will have to revisit that later.

I think I would actually like to not include support for JavaScriptCore just quite yet, if that's feasible? Mostly because that includes a large pure-C API, much in the same way as CoreFoundation, which header-translator is not yet really equipped to handle.

By the way, I was able to update this some and add support for JavaScriptCore.

There are some things I needed to skip from that framework (mentioned in the config) having to do with errors about bool not supporting encode. I read the docs about Encode and saw handling about bool and BOOL but wasn't really sure how to resolve the issue. (Also trying to compile with those definitions changed manually to BOOL gave some warnings about the struct not being FFI safe IIRC, so that's another potential issue).

Yeah, I can understand why you're confused; the problem is that the function typedefs used in those structs (JSObjectHasPropertyCallback, JSObjectSetPropertyCallback, JSObjectDeletePropertyCallback and JSObjectHasInstanceCallback) return C99's bool instead of Objective-C's BOOL, which, again, outside what header-translator is really set up to handle just yet. So special-casing it is fine for now.

I also note that the PR is failing on the "compiler UI" tests here but I'm not sure what to do about that.

I suspect in this case it's caused by a nightly compiler update which changes diagnostics. If you run:

TRYBUILD=overwrite cargo +nightly run --features=run --bin test-ui

The test cases will be automatically updated. If you want, you can make a separate PR for updating this, otherwise I'll just do it myself soon-ish.

@silvanshade
Copy link
Contributor Author

I think I would actually like to not include support for JavaScriptCore just quite yet, if that's feasible? Mostly because that includes a large pure-C API, much in the same way as CoreFoundation, which header-translator is not yet really equipped to handle.

Yeah that's fine. I think some parts of Tauri may need access to the javascript context stuff which was the main motivation for including it (aside for completeness) but I think that usage is limited enough that we could just deal with it through some separate manual definitions for now. I'll update the PR and remove that for now.

@silvanshade
Copy link
Contributor Author

Once #342 and #343 are resolved I'll rebase again and then create a separate PR for the generated output.

@madsmtm
Copy link
Owner

madsmtm commented Jan 15, 2023

Do you want to try rebasing this PR now that the linked issues have been merged?

(And just keep this and the generated output in one PR, again, sorry for the confusion earlier)

@silvanshade
Copy link
Contributor Author

Do you want to try rebasing this PR now that the linked issues have been merged?

(And just keep this and the generated output in one PR, again, sorry for the confusion earlier)

Okay, just rebased.

Copy link
Owner

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, have a few concerns regarding mostly the added support for 16-argument selectors.

crates/header-translator/translation-config.toml Outdated Show resolved Hide resolved
crates/header-translator/translation-config.toml Outdated Show resolved Hide resolved
crates/objc2-encode/src/encode.rs Show resolved Hide resolved
crates/objc2/src/message/mod.rs Show resolved Hide resolved
crates/objc2/src/message/mod.rs Show resolved Hide resolved
crates/icrate/src/WebKit/mod.rs Outdated Show resolved Hide resolved
@silvanshade silvanshade deleted the webkit-skips branch January 18, 2023 00:54
@silvanshade silvanshade restored the webkit-skips branch January 18, 2023 19:17
@silvanshade silvanshade mentioned this pull request Jan 18, 2023
@madsmtm
Copy link
Owner

madsmtm commented Jan 18, 2023

Replaced by #355

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants