Skip to content

[Foundation] Add NSProxy stub in order to workaround bug 59247 - #2693

Merged
spouliot merged 2 commits into
dotnet:xcode9from
dalexsoto:NSProxy
Sep 14, 2017
Merged

[Foundation] Add NSProxy stub in order to workaround bug 59247#2693
spouliot merged 2 commits into
dotnet:xcode9from
dalexsoto:NSProxy

Conversation

@dalexsoto

@dalexsoto dalexsoto commented Sep 14, 2017

Copy link
Copy Markdown
Member

https://bugzilla.xamarin.com/show_bug.cgi?id=59247

We need to have a better solution that removes all what NSProxy does not responds to.
Right now this works because NSObject implements NSObject protocol just like NSProxy, but
NSProxy responds to far less selectors hence doing it internal so it is not a breaking change
when we provide the correct fix.

https://bugzilla.xamarin.com/show_bug.cgi?id=59247

We need to have a better solution that removes all what NSProxy does not responds to.
Right now this works because NSObject implements NSObject protocol just like NSProxy, but
NSProxy responds to far less selectors hence doing it internal so it is not a breaking change
when we provide the correct fix.

@rolfbjarne rolfbjarne left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great idea about making it internal.

But maybe we can teach the linker about this, so that it automatically keeps NSProxy when WKNavigationDelegate is used? That would automatically solve the bug without customers ever seeing the problem (and thus not having to track down the solution as well).

@monojenkins

Copy link
Copy Markdown
Contributor

Build success

@spouliot spouliot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's do the linker change in a separate PR. In fact it should not need any linker change, just do

[Preserve (typeof (NSProxy))]
on the WK*Delegate type (or member).

Next, double check that the generator keeps it - otherwise put that in a (manual) file with a partial class.

@rolfbjarne rolfbjarne left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm OK with linker change in a different PR.

We need to keep NSProxy (avoid linking it) if WKNavigationDelegate or IWKNavigationDelegate
is used. Unfortunately [Preserve] can't help us here because we do not generate partial
interfaces rigth now, so we know WKWebView will be there and can hold a reference to it.

@spouliot spouliot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

note that, even with partial, someone using the interface IWKNavigationDelegate would not get the preserve.

@monojenkins

Copy link
Copy Markdown
Contributor

Build failure

@spouliot

Copy link
Copy Markdown
Contributor

aborted (3 hours)

@spouliot

Copy link
Copy Markdown
Contributor

build

@monojenkins

Copy link
Copy Markdown
Contributor

Build success

@spouliot
spouliot merged commit 46c6a7d into dotnet:xcode9 Sep 14, 2017
@dalexsoto
dalexsoto deleted the NSProxy branch September 18, 2017 15:45
mandel-macaque added a commit to mandel-macaque/xamarin-macios that referenced this pull request Aug 23, 2023
Bump maccore to bing the changes for governance and some others:

* Merge 'xcode15' into 'main'. (dotnet#2690)
* [Submission] Add NotaryTool Task (dotnet#2689)
* [XmlDocSync] Remove the XmlDocSync reference from the project. (dotnet#2693)
* [mlaunch] Refactor project structure (dotnet#2694)
* [certificates] Update la_dev_apple.p12, la_distr_apple.p12 and la_mac_installer_distr.p12 (dotnet#2697)
* [Governance] Update the false positives files based on the last runs. (dotnet#2698)

Full idff: https://github.com/xamarin/maccore/compare/fd15821e6780a7439df467c69eb511a046adfa06...63fd14954ca3328219c5f79c754f0b329091535c
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.

6 participants