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

Remove remaining asserts from PR 6237 #6686

Merged
merged 1 commit into from
May 3, 2022

Conversation

buyaa-n
Copy link
Contributor

@buyaa-n buyaa-n commented Apr 29, 2022

Removed remaining Debug.Asserts added in PR

Issues Fixed

Fixes # #6616

@buyaa-n buyaa-n requested review from mattleibow and PureWeen April 29, 2022 20:53
Comment on lines 18 to 19
case BrowserLaunchMode.SystemPreferred:
System.Diagnostics.Debug.Assert(!OperatingSystem.IsIOSVersionAtLeast(11));
await LaunchSafariViewController(uri, options);
Copy link
Member

Choose a reason for hiding this comment

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

What happens if you run this code on iOS 11? Will it crash? If so, then we need to update the switch to maybe be an if and then make it also check version. If SystemPreferred && OSVersion(11) then use safari, else use external

Copy link
Contributor Author

@buyaa-n buyaa-n May 2, 2022

Choose a reason for hiding this comment

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

What happens if you run this code on iOS 11? Will it crash?

I do not have a device setup to test iOS, anyway all these warnings annotation/attribute based

After removing the Assert there is no warning here anymore, I think it is because we increased the browser version to support ios11.0+, I can check that when I got access to my desktop EDIT: The API has unsupported ios11.0, I was mistaken with supported, also I see the analyzer is disabled at project level, that is why I didn't see a warning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The method LaunchSafariViewController(Uri uri, BrowserLaunchOptions options) below annotated with UnsupportedOSPlatform("ios11.0") because it is unconditionally calling SFSafariViewController constructor unsupported from iOS 11.0, I see there is a constructor supported from 11.0+
image
So the correct handling is probably conditionally call those constructors, but I do not know what to pass for SFSafariViewControllerConfiguration parameter

System.Diagnostics.Debug.Assert(OperatingSystem.IsIOSVersionAtLeast(11));
#pragma warning disable CA1416 // TODO: 'UIDragInteraction.Delegate', 'UIView.Interactions' is only supported on: 'ios' 11.0 and later, 'maccatalyst' 11.0 and later
Copy link
Member

Choose a reason for hiding this comment

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

Will it crash on iOS 10? If so, maybe we need to wrap this whole block - and related - in a OS version check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will it crash on iOS 10?

Annotated with SupportedOSPlatform("ios11.0")

maybe we need to wrap this whole block - and related - in a OS version check.

In the current code _dragAndDropDelegate is set only within ios11 block, therefore the null check kind of enough if that logic will not change.

if (Forms.IsiOS11OrNewer && recognizer is DragGestureRecognizer)
{
dragFound = true;
_dragAndDropDelegate = _dragAndDropDelegate ?? new DragAndDropDelegate();
if (uIDragInteraction == null)
{
var interaction = new UIDragInteraction(_dragAndDropDelegate);
interaction.Enabled = true;
_renderer.NativeView.AddInteraction(interaction);
}
}
if (Forms.IsiOS11OrNewer && recognizer is DropGestureRecognizer)
{
dropFound = true;
_dragAndDropDelegate = _dragAndDropDelegate ?? new DragAndDropDelegate();

This is actually perfect example of when we should Assert the platform check

@Eilon Eilon added the area-infrastructure CI, Maestro / Coherency, upstream dependencies/versions label May 2, 2022
Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

The TODOs need to be followed up on by code owners... and the essentials ones are me :)

@mattleibow mattleibow merged commit c5db893 into dotnet:main May 3, 2022
@mattleibow mattleibow added this to the 6.0.300 milestone May 3, 2022
@mattleibow mattleibow self-assigned this May 3, 2022
@buyaa-n buyaa-n deleted the remove_asserts branch May 3, 2022 16:13
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
@samhouts samhouts added the fixed-in-6.0.300-rc.3 Look for this fix in 6.0.300-rc.3! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-infrastructure CI, Maestro / Coherency, upstream dependencies/versions fixed-in-6.0.300-rc.3 Look for this fix in 6.0.300-rc.3!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants