GH-196 Browser Customization

[Mrnikbobjeff]

Description of Change

Implemented some Features required for GH- #196.

API Changes

List all API changes here (or just put None), example:

Added:

public readonly struct EssentialsColor : IEquatable<EssentialsColor>
{
        public byte R { get; }
        public byte G { get; }
        public byte B { get; }
        public byte A { get; }
}

public readonly partial struct BrowserLaunchOptions : IEquatable<BrowserLaunchOptions>
{
        public EssentialsColor? PreferredTitleColor { get; }

        public EssentialsColor? PreferredBackgroundColor { get; }

        public EssentialsColor? PrefferedControlColor { get; }

        public bool? ShouldShowTitle { get; }
}

Changed:

• Browser.OpenAsync(string uri, BrowserLaunchOptions options = default) // Added default parameter

Behavioral Changes

None. All values are nullable. If they are not set as they are by default no changes are made.

PR Checklist

• Has tests (if omitted, state reason in description)
• Has samples (if omitted, state reason in description)
• Rebased on top of master at time of PR
• Changes adhere to coding standard
• Updated documentation (see walkthrough)

[Mrnikbobjeff]

We should really consider dropping .Net Standard 1.x support. System.Drawing.Color had to be manually approximated to have a cross platfrorm argb representation.

[jamesmontemagno]

No need for a specific color, we should use System.Drawing.Color

[Mrnikbobjeff]

Which is not available in .net standard 1.6, that is why I had to implement this abstraction. System.drawing.color became available in .Net Standard 2.0

[jamesmontemagno]

We should limit the API to only .NET Standard 2.0. We do not want to add a new type here.

[mattleibow]

We can create the color type in essentials for netstandard1.0:

#if NETSTANDARD_1_0
public struct Color {
    // ...
}
#else
[assembly: TypeForward(System.Drawing.Color)]
#endif

How this works:

If the user is building with PCL/NetStd10, then he compiles against the local color and is happy. If he targets NetStd20, then he gets the real deal.

At runtime, the OS will use the real type - in fact, the code in NetStd10 will never, ever get run, so we don't even need a real implementation!

We can literally pull this in for NetStd10: https://github.com/dotnet/corefx/blob/master/src/System.Drawing.Primitives/ref/System.Drawing.Primitives.cs

[mattleibow]

I think you are on the right track, and the code is quite clean 😄

There are a couple of comments:

• BrowserLaunchMode should be inside BrowserLaunchOptions - see related comments
• We need to decide how we are going to do the EssentialsColor/Color thing

[mattleibow]

We must not use default parameters, rather create an overload:

Task OpenAsync(string uri) =>
    OpenAsync(uri, null);
Task OpenAsync(string uri, BrowserLaunchMode launchMode) =>
    OpenAsync(uri, new BrowserLaunchOptions(launchMode));
Task OpenAsync(string uri, BrowserLaunchOptions options) =>
    PlatformOpenAsync(uri, options);

I don't think there will be an overload that takes both BrowserLaunchMode AND BrowserLaunchOptions - what is the case where you will set colors and then also say ignore those colors?

[Mrnikbobjeff]

There is none, I will change this in the next commit.

[jamesmontemagno]

This api should only be visibile in .net standard 2.0/Android/iOS/UWP

You can use

#if NETSTANDARD2_0
#endif

around it to factor it out.

[jamesmontemagno]

So, System.Drawing.Color is not available in Xamarin.iOS, but instead OpenTK, so for now this work will be delayed until that is fixed. For tracking here: dotnet/macios#5273

[mattleibow]

Based on the response:

Xamarin.iOS does not "conform" to NetStandard in the sense that the Xamarin.iOS.dll is expected to include all the dlls needed for all the NetStandard APIs. Instead Xamarin.iOS provides the Facades with the type forwarders needed to find the actual dlls required for specific types.

I would say this is what we are going to have to do. An we can add the OpenTK-1 reference in the NuGet. It should be "OK" because it is a "system" dll for iOS.

[Mrnikbobjeff]

Would it also be possible to type forward to a type forward? So just skip referencing OpenTK and forward Color to the Xamarin.iOS type forward

[mattleibow]

@jamesmontemagno @Mrnikbobjeff I would really like to do that, but I have one concern: dotnet/macios#5275

We need to test somethings:

• if we use Color, can we live with the extra overhead - we probably will have to
• if we don't use any is-using-Color APIs, will the OpenTK be stripped entirely, or will the mere existence of of the reference cause the dll to be pulled in?
• if we use Color, how far does the ripple effect go, is this a bug that we need to escalate on the mac/ios teams?

[mattleibow]

@jamesmontemagno I think we can just go ahead and depend on OpenTK for iOS - Xamarin.Forms already references it so we can't really add any more. Sure, Bob the iOS-only developer may not be 100% happy, but it shouldn't affect him that much. And, I don't think anything is going to change anytime soon with iOS and the linker.

[jamesmontemagno]

In this regard I am doing some color work and will set it up, so please wait until I have that in a branch for you to pull up.

[mattleibow]

NICE! Looking forward to seeing cool things happen in this world!

[ghost]

OPS Build status updates of commit 245a157:

🕙 Preparing: average preparing time is 54 sec(s)

[ghost]

OPS Build status updates of commit 245a157:

🕙 Incremental building: average incremental building time is 24 sec(s)

[ghost]

OPS Build status updates of commit 245a157:

✅ Validation status: passed

File	Status	Preview URL	Details
Xamarin.Essentials/Browser/Browser.android.cs	✅Succeeded
Xamarin.Essentials/Browser/Browser.ios.cs	✅Succeeded
Xamarin.Essentials/Browser/Browser.netstandard.cs	✅Succeeded
Xamarin.Essentials/Browser/Browser.shared.cs	✅Succeeded
Xamarin.Essentials/Browser/Browser.uwp.cs	✅Succeeded

For more details, please refer to the build report.

Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.

[jamesmontemagno]

Rebased off 1.1.0 so system.drawing.color should be good to use now with opentk on iOS

[Mrnikbobjeff]

Hey, so I was doing some work for type forwarding and I would love to have clarification on how I am supposed to implement this. @jamesmontemagno said that this should only be available on netstandard 2.0. I suppose you don't mean ifdefing the entire static class right? If so I am good to go, otherwise some clarification for the shared class would be nice :)

[mattleibow]

You are going to have to #if the class and create a dummy type so we don't have to worry about method signatures. Nobody will actually ever get netstandard1.0 in their app - unless they are using PCL. But, I don't think that is actually possible as the new IDEs can't open those projects anymore.

You probably can do something like this:

#if NETSTANDARD1_0
// a dummy type for .NET Standard 1.0
public readonly partial struct BrowserLaunchOptions { }
#else
// the real type for all the other platforms
public readonly partial struct BrowserLaunchOptions : … {
    ...
}
#endif

@jamesmontemagno does this look fine to you? Not super pretty, but this way we don't have #if all over the show.

Another way might be to create a dummy Color type that is wrapped in a #if. All the code stays in it's beautiful place, but for the netstandard1.0 target, we just include this:

#if NETSTANDARD1_0
namespace System.Drawing {
    public struct Color {
    }
}
#endif

That may cause issues if someone builds against netstandard1.0, and then uses it in an app. The runtime will look for color in our assembly, but we could also do this:

#if NETSTANDARD1_0
namespace System.Drawing {
    public struct Color {
    }
}
#else
[assembly: TypeForwardedTo(typeof(Color))]
#endif

That should fix all cases. We don't have to provide any implementation for Color, or we can just add the A R G B properties to be friendly... It may need testing, but I can't see any issues as this is what netstandard.dll is built on.

My main concern here is that we are going to write bad code just to satisfy the compilers and things, that is why I am pushing a separate, smaller file that we can just delete when we finally go golden - and nothing breaks anyone.

[ghost]

OPS Build status updates of commit 289f7b3:

🕙 Preparing: average preparing time is 54 sec(s)

[ghost]

OPS Build status updates of commit 289f7b3:

🕙 Incremental building: average incremental building time is 23 sec(s)

[ghost]

OPS Build status updates of commit 289f7b3:

✅ Validation status: passed

File	Status	Preview URL	Details
Xamarin.Essentials/Browser/Browser.android.cs	✅Succeeded
Xamarin.Essentials/Browser/Browser.ios.cs	✅Succeeded
Xamarin.Essentials/Browser/Browser.netstandard.cs	✅Succeeded
Xamarin.Essentials/Browser/Browser.shared.cs	✅Succeeded
Xamarin.Essentials/Browser/Browser.uwp.cs	✅Succeeded
Xamarin.Essentials/Types/PlatformExtensions/ColorExtensions.shared.cs	✅Succeeded
Xamarin.Essentials/Xamarin.Essentials.csproj	✅Succeeded

For more details, please refer to the build report.

Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.

[ghost]

OPS Build status updates of commit 6e49126:

🕙 Preparing: average preparing time is 54 sec(s)

[ghost]

OPS Build status updates of commit 6e49126:

🕙 Incremental building: average incremental building time is 22 sec(s)

[ghost]

OPS Build status updates of commit 6e49126:

✅ Validation status: passed

File	Status	Preview URL	Details
Xamarin.Essentials/Browser/Browser.android.cs	✅Succeeded
Xamarin.Essentials/Browser/Browser.ios.cs	✅Succeeded
Xamarin.Essentials/Browser/Browser.netstandard.cs	✅Succeeded
Xamarin.Essentials/Browser/Browser.shared.cs	✅Succeeded
Xamarin.Essentials/Browser/Browser.uwp.cs	✅Succeeded
Xamarin.Essentials/Types/PlatformExtensions/ColorExtensions.shared.cs	✅Succeeded
Xamarin.Essentials/Xamarin.Essentials.csproj	✅Succeeded

For more details, please refer to the build report.

Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.

[mattleibow]

Looking good!

[mattleibow]

Rather use the full name instead of the using: [assembly: TypeForwardedTo(typeof(System.Drawing;.Color))]

I think there are rules about where usings can go...

[Mrnikbobjeff]

Stylecop did not complain. If I remove the using the android build fails, not sure why.

[mattleibow]

@jamesmontemagno Do we want a "default" option here? If follows the rules, but is this maybe not useful? I think our original idea that "Default" === "SystemPreferred", but we used a different name to help discoverability? Not sure about this one...

[jamesmontemagno]

I don't think we wan't default here.

SystemPreferred != Default as Default would be launching URI. This is a special case in general and should be the default. I think that we should pass in the LaunchMode as a parameter and LaunchOptions as another. I would hate to create another class just to do systempreferred or external if I don't need any of the options.

[Mrnikbobjeff]

I can revert to the three parameter style I adopted originally for the internal implementation and just pass null. For the public api there will be 6 overloads in total, one for just string/uri, one for uri+Launchoptions and the last 2 for all three.

[ghost]

OPS Build status updates of commit cb3c9ba:

🕙 Preparing: average preparing time is 54 sec(s)

[ghost]

OPS Build status updates of commit cb3c9ba:

🕙 Incremental building: average incremental building time is 22 sec(s)

[ghost]

OPS Build status updates of commit cb3c9ba:

✅ Validation status: passed

File	Status	Preview URL	Details
Xamarin.Essentials/Browser/Browser.android.cs	✅Succeeded
Xamarin.Essentials/Browser/Browser.ios.cs	✅Succeeded
Xamarin.Essentials/Browser/Browser.netstandard.cs	✅Succeeded
Xamarin.Essentials/Browser/Browser.shared.cs	✅Succeeded
Xamarin.Essentials/Browser/Browser.uwp.cs	✅Succeeded
Xamarin.Essentials/Types/PlatformExtensions/ColorExtensions.shared.cs	✅Succeeded
Xamarin.Essentials/Xamarin.Essentials.csproj	✅Succeeded

For more details, please refer to the build report.

Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.

[jamesmontemagno]

Any reason this is IEquatable? Doesn't seem like it needs to be at all since it is a class.... should it be a struct?

[mattleibow]

Probably shod be class with no interfaces. Not sure if this is official, but I go with POCO classes in, read-only structs out...

[Mrnikbobjeff]

See my other comment. While all current features use the style @mattleibow describes I think it would be wortwhile investigating when we should differ from this. Even though this is an async method it could be that a passed value type does not have to be boxed. Then, having this as a value type would IMHO be preferrable due to the lessened heap allocation. All contained properties are value types as well, which is not the case for any other in parameter for the public api. This still is a remnant from when this was a struct in the original pr. I can remove it once we clear this up :)