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

LocaleRegion API #3027

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Prev Previous commit
Next Next commit
addressing comments
  • Loading branch information
srwei committed Nov 29, 2022
commit bb757b379f632328d82080d64ca19d2d38252785
36 changes: 15 additions & 21 deletions specs/LocaleRegion.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,23 +24,16 @@ options->put_LocaleRegion(m_webviewOption.localeRegion.c_str());
```

```c#
CoreWebView2Environment _webViewEnvironment;
WebViewCreationOptions _creationOptions;
public CreateWebView2Controller(IntPtr parentWindow)
CoreWebView2Environment environment;
CoreWebView2ControllerOptions CreateCoreWebView2ControllerOptions(CoreWebView2Environment environment)

Choose a reason for hiding this comment

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

Method name is a little misleading, since it doesn't just create the options, but sets the LocaleRegion as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could name it CreateAndInitializeCoreWebView2ControllerOptions?

{
CoreWebView2ControllerOptions controllerOptions = new CoreWebView2ControllerOptions();
controllerOptions.LocaleRegion = _creationOptions.localeRegion;
CoreWebView2Controller controller = null;
if (_creationOptions.entry == WebViewCreateEntry.CREATE_WITH_OPTION)
CoreWebView2ControllerOptions ControllerOptions = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CoreWebView2ControllerOptions ControllerOptions = null;
CoreWebView2ControllerOptions controllerOptions = null;

if (LocaleRegion != null)
{
controller = await _webViewEnvironment.CreateCoreWebView2ControllerAsync(parentWindow, options);
ControllerOptions = environment.CreateCoreWebView2ControllerOptions();

Choose a reason for hiding this comment

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

Why is a method needed to create the options?

Copy link
Contributor

Choose a reason for hiding this comment

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

ControllerOptions is an existing WebView2 class not defined in this spec. An app can have multiple instances of a webview2 environment running at once and with just a constructor we wouldn't know which instance should be creating an object and so generally creating objects is done via a CoreWebView2Environment

ControllerOptions.LocaleRegion = LocaleRegion;
}
else
{
controller = await _webViewEnvironment.CreateCoreWebView2ControllerAsync(parentWindow);
}
// update locale with value
SetAppLocale(localeRegion);
return ControllerOptions;
}
```

Expand All @@ -55,14 +48,15 @@ interface ICoreWebView2StagingControllerOptions : IUnknown {
/// 639](https://www.iso.org/iso-639-language-codes.html) and `country` is the
/// 2-letter code from [ISO 3166](https://www.iso.org/standard/72482.html).
///
/// This property will update the environment creation. This is global and immutable,
/// so changes will not be reflected in the existing webviews. They will need to closed
/// and reopened in order to see the changes reflected from using the new creation environment.
/// This property sets the region for a CoreWebView2Environment during its creation.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this sentence. The CoreWebview2Environment already exists; indeed, it's the object that creates the CoreWebView2ControllerOptions! So this property cannot affect the CoreWebview2Environment's creation, because the creation has already happened.

/// Creating a new CoreWebView2Environment object that connects to an already running
/// browser process cannot change the region previously set by an earlier CoreWebView2Environment.
/// The CoreWebView2Environment and all associated webview2 objects will need to closed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what this is saying.

  1. If a browser process has more than one CoreWebView2Environment associated with it, then the first CoreWebView2Environment can change the region at any time to any supported value. If the second CoreWebView2Environment tries to change the region, then the call...
    a. is ignored,
    b. fails with an exception.
  2. If a browser process has more than one CoreWebView2Environment associated with it, then neither can change the region.
  3. If a browser process has more than one CoreWebView2Environment associated with it, and then the first CoreWebView2Environment is destroyed, the second CoreWebView2Environment still cannot change the region (because only the first one can change the region).
  4. If a browser process has more than one CoreWebView2Environment associated with it, and then either CoreWebView2Environment is destroyed, the remaining CoreWebView2Environment can change the region (because there is now only one environment connected).
  5. If a browser process has more than one CoreWebView2Environment associated with it, and the second one tries to change the region and fails, then you must close the second one (and all the webview2 objects associated with the second environment) immediately.
    a. Failure to do so immediately will cause unspecified harm.
    b. If you do not destroy the second environment immediately, the second environment will be non-functional (all methods throw exceptions or something).

///
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still confused about what the region is associated with. This text suggests that it's associated with the browser process. But if that's the case, why is it a property on the controller options?

/// Validation is done on the V8 engine to match on the closest locale
/// from the passed in locale region value. For example, passing in "en_gb"
/// will reflect the "en-GB" locale in V8.
/// If V8 cannot find any matching locale on the input value, it will default
/// Validation processing is done by ICU to match on the closest locale
/// from the passed in locale region value. ICU documentation can be found here
/// (https://source.chromium.org/chromium/chromium/src/+/main:third_party/icu/source/common/unicode/locid.h)
/// If ICU cannot find any matching locale on the input value, it will default
/// to the display language as the locale.
///
/// The default value for LocaleRegion will be depend on the WebView2 language
Expand Down