Wires the locale provided by Fuchsia. #13045

filmil · 2019-10-09T22:39:16Z

The FIDL service fuchsia.intl.PropertyProvider is a service that
the flutter runner can use to obtain information on system preferred
locales.

This change sends a platform message "setLocale" on the channel
"flutter/localization", based on the values provided by the above
mentioned FIDL service.

Credit: most of this was initially written by @kpozin; I ported it
to out-of-tree flutter engine.

Tested:

Compile and publish the unit tests package as shown in
the script below.
In a Fuchsia repository (pointed to by $FUCHSIA_DIR), run
fx serve
fx shell run fuchsia-pkg://fuchsia.com/flutter_runner_tests#meta/flutter_runner_tests.cmx

The script used to update the unit tests.

#!/bin/bash

set -x

# You will need to ensure that $FLUTTER_ENGINE_DIR and $FUCHSIA_DIR
# are set to appropriate values for your machine.
FLUTTER_ENGINE_DIR="${FLUTTER_ENGINE_DIR:-$HOME/fx/flutter/engine/src}"
readonly OUT_DIR="${FLUTTER_ENGINE_DIR}/out"

(
  cd ${FLUTTER_ENGINE_DIR}
  ./flutter/tools/gn --fuchsia --fuchsia-cpu x64 --unoptimized
  ninja -j 100 -C "${OUT_DIR}/fuchsia_debug_unopt_x64"
  cp "${OUT_DIR}/compile_commands.json" "${FLUTTER_ENGINE_DIR}"

  echo "Publishing the tests package"
  "${FLUTTER_ENGINE_DIR}/fuchsia/sdk/linux/tools/pm" publish \
    -a -r $FUCHSIA_DIR/out/release/amber-files \
    -f "${FLUTTER_ENGINE_DIR}/out/fuchsia_debug_unopt_x64/flutter_runner_tests-0.far"
)

chinmaygarde

Other than the license block and that we should just update our ICU version instead of backporting patches, lgtm.

chinmaygarde · 2019-10-10T05:41:38Z

shell/platform/fuchsia/flutter/fuchsia_intl.cc

@@ -0,0 +1,126 @@
+// Copyright 2019 The Flutter Authors. All rights reserved.


Please use the usual Flutter license block like in the files next to this TU.

Will do. I will just assume that your suggested approach is fine.

chinmaygarde · 2019-10-10T05:42:54Z

shell/platform/fuchsia/flutter/fuchsia_intl.cc

+
+using icu::Locale;
+
+// This backfills for the static method icu::Locale::forLanguageTag which is


We should just update the ICU version ourselves. I did it about a year ago so it is definitely a bit old.

OK. But, let me do so in a separate change then.

Do you have a preference for the version? ICU 64 has been released on Oct 3. 64.2 was current before, but it also has a lot of patches that would need to be applied too.

chinmaygarde · 2019-10-10T05:44:18Z

cc @jason-simmons @GaryQian

The current version is 62.1, which is a now 3 major releases behind, given that ICU 65 was released on Oct 3, 2019. Previous update was here: flutter#6097 I was advised to upgrade in the code revie for this PR: flutter#13045 This change brings in the icu::Locale::forLanguageTag(...) function which is directly relevant to the work in the PR, but also updates the library considerably. * TODO(filmil): Release notes (will work with reviewers)

The current version is 62.1, which is a now 3 major releases behind, given that ICU 65 was released on Oct 3, 2019. The version bump required an update to buildroot to fix too strict compilation of ICU. Previous update was here: flutter#6097 I was advised to upgrade in the code revie for this PR: flutter#13045 This change brings in the icu::Locale::forLanguageTag(...) function which is directly relevant to the work in the PR, but also updates the library considerably. * TODO(filmil): Release notes (will work with reviewers) Tested: ```bash #!/bin/bash set -x readonly FLUTTER_ENGINE_DIR="${FLUTTER_ENGINE_DIR:-$HOME/fx/flutter/engine/src}" readonly OUT_DIR="${FLUTTER_ENGINE_DIR}/out" ( cd ${FLUTTER_ENGINE_DIR} ./flutter/tools/gn --unoptimized ninja -j 100 -C "${OUT_DIR}/host_debug_unopt" ./flutter/testing/run_tests.py --type engine ) ```

The current version is 62.1, which is a now 3 major releases behind, given that ICU 65 was released on Oct 3, 2019. The version bump required an update to buildroot to fix too strict compilation of ICU. Previous update was here: flutter#6097 I was advised to upgrade in the code review for this PR: flutter#13045 This change brings in the function `icu::Locale::forLanguageTag(...)` which is directly relevant to the work in the PR, but also updates the library considerably. * TODO(filmil): Release notes (will work with reviewers) Tested: ```bash #!/bin/bash set -x readonly FLUTTER_ENGINE_DIR="${FLUTTER_ENGINE_DIR:-$HOME/fx/flutter/engine/src}" readonly OUT_DIR="${FLUTTER_ENGINE_DIR}/out" ( cd ${FLUTTER_ENGINE_DIR} ./flutter/tools/gn --unoptimized ninja -j 100 -C "${OUT_DIR}/host_debug_unopt" ./flutter/testing/run_tests.py --type engine ) ```

The current version is 62.1, which is a now 3 major releases behind, given that ICU 65 was released on Oct 3, 2019. The version bump required an update to buildroot to fix too strict compilation of ICU. Previous update was here: flutter#6097 I was advised to upgrade in the code review for this PR: flutter#13045 This change brings in the function `icu::Locale::forLanguageTag(...)` which is directly relevant to the work in the PR, but also updates the library considerably. * TODO(filmil): Release notes (will work with reviewers) Tested: ```bash set -x readonly FLUTTER_ENGINE_DIR="${FLUTTER_ENGINE_DIR:-$HOME/fx/flutter/engine/src}" readonly OUT_DIR="${FLUTTER_ENGINE_DIR}/out" ( cd ${FLUTTER_ENGINE_DIR} ./flutter/tools/gn --unoptimized ninja -j 100 -C "${OUT_DIR}/host_debug_unopt" ./flutter/testing/run_tests.py --type engine ) ```

The current version is 62.1, which is a now 3 major releases behind, given that ICU 65 was released on Oct 3, 2019. The version bump required an update to buildroot to fix too strict compilation of ICU. Previous update was here: flutter#6097 I was advised to upgrade in the code review for this PR: flutter#13045 This change brings in the function `icu::Locale::forLanguageTag(...)` which is directly relevant to the work in the PR, but also updates the library considerably. Tested: ```bash set -x readonly FLUTTER_ENGINE_DIR="${FLUTTER_ENGINE_DIR:-$HOME/fx/flutter/engine/src}" readonly OUT_DIR="${FLUTTER_ENGINE_DIR}/out" ( cd ${FLUTTER_ENGINE_DIR} ./flutter/tools/gn --unoptimized ninja -j 100 -C "${OUT_DIR}/host_debug_unopt" ./flutter/testing/run_tests.py --type engine ) ```

The current version is 62.1, which is a now 3 major releases behind, given that ICU 65 was released on Oct 3, 2019. The version bump required an update to buildroot to fix too strict compilation of ICU. Previous update was here: #6097 I was advised to upgrade in the code review for this PR: #13045 This change brings in the function `icu::Locale::forLanguageTag(...)` which is directly relevant to the work in the PR, but also updates the library considerably. Tested: ```bash set -x readonly FLUTTER_ENGINE_DIR="${FLUTTER_ENGINE_DIR:-$HOME/fx/flutter/engine/src}" readonly OUT_DIR="${FLUTTER_ENGINE_DIR}/out" ( cd ${FLUTTER_ENGINE_DIR} ./flutter/tools/gn --unoptimized ninja -j 100 -C "${OUT_DIR}/host_debug_unopt" ./flutter/testing/run_tests.py --type engine ) ```

cbracken · 2019-10-21T18:13:07Z

@filmil looks like the ICU upgrade landed but was reverted. I assume you're working on a re-land before picking this up again?

filmil · 2019-10-21T18:15:13Z

@cbracken My understanding is that build cops needed to re-sequence several ICU landings, and will reapply the ICU upgrade that I did. Once done, they will reapply my change, and I can continue on this. Will let you know when ready.

filmil · 2019-10-22T00:25:04Z

I had a license file merge to do, and this took a while. The current commit seems to validate well on my machine, let's see if the pre-commit hooks agree.

filmil · 2019-10-23T17:17:54Z

@chinmaygarde @cbracken PTAL? This has now passed all checks, whew!

chinmaygarde

LGTM. May I land it?

filmil · 2019-10-23T19:22:39Z

Yes, please.

…

On Wed, Oct 23, 2019 at 12:20 PM Chinmay Garde ***@***.***> wrote: ***@***.**** approved this pull request. LGTM. May I land it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13045?email_source=notifications&email_token=AAB4GMH64AAY3D37ICXKET3QQCPXPA5CNFSM4I7FTKG2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCI7S7PA#pullrequestreview-306130876>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB4GMENQXQBW7T7AOTOS33QQCPXPANCNFSM4I7FTKGQ> .

@kpozin

The FIDL service `fuchsia.intl.PropertyProvider` is a service that the flutter runner can use to obtain information on system preferred locales. This change sends a platform message "setLocale" on the channel "flutter/localization", based on the values provided by the above mentioned FIDL service. Credit: most of this was initially written by @kpozin; I ported it to out-of-tree flutter engine. Tested: 1. Compile and publish the unit tests package as shown in the script below. 2. In a Fuchsia repository (pointed to by `$FUCHSIA_DIR`), run `fx serve` 3. `fx shell run fuchsia-pkg://fuchsia.com/flutter_runner_tests#meta/flutter_runner_tests.cmx` The script used to update the unit tests. ```bash set -x FLUTTER_ENGINE_DIR="${FLUTTER_ENGINE_DIR:-$HOME/fx/flutter/engine/src}" readonly OUT_DIR="${FLUTTER_ENGINE_DIR}/out" ( cd ${FLUTTER_ENGINE_DIR} ./flutter/tools/gn --fuchsia --fuchsia-cpu x64 --unoptimized ninja -j 100 -C "${OUT_DIR}/fuchsia_debug_unopt_x64" cp "${OUT_DIR}/compile_commands.json" "${FLUTTER_ENGINE_DIR}" echo "Publishing the tests package" "${FLUTTER_ENGINE_DIR}/fuchsia/sdk/linux/tools/pm" publish \ -a -r $FUCHSIA_DIR/out/release/amber-files \ -f "${FLUTTER_ENGINE_DIR}/out/fuchsia_debug_unopt_x64/flutter_runner_tests-0.far" ) ```

filmil · 2019-10-23T20:27:23Z

Rebased on top of current HEAD.

git@github.com:flutter/engine.git/compare/195425e04480...400e3b0 git log 195425e..400e3b0 --no-merges --oneline 2019-10-23 fmil@google.com Wires the locale provided by Fuchsia. (flutter/engine#13045) 2019-10-23 chinmaygarde@google.com Add FlutterEngineRunsAOTCompiledDartCode to the embedder API. (flutter/engine#13319) 2019-10-23 50856934+nturgut@users.noreply.github.com [web] [test] Adding firefox install functionality to the test platform (flutter/engine#13272) 2019-10-23 skia-flutter-autoroll@skia.org Roll src/third_party/skia 32803ff74448..6863bb0930cf (7 commits) (flutter/engine#13318) 2019-10-23 bkonyi@google.com Roll src/third_party/dart 5fd6c8a3c1..b359ac0a1e (2 commits) 2019-10-23 iska.kaushik@gmail.com [recipe] Upload opt flutter_tester (flutter/engine#13311) 2019-10-23 jason-simmons@users.noreply.github.com Update the dependencies for the Fuchsia build of flutter_frontend_server (flutter/engine#13316) 2019-10-23 jmccandless@google.com NO_SUGGESTIONS keyboard flag in Android (flutter/engine#13099) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC aaclarke@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

googlebot added the cla: yes label Oct 9, 2019

filmil force-pushed the intl branch 2 times, most recently from b05f16d to 0d30560 Compare October 9, 2019 23:37

chinmaygarde reviewed Oct 10, 2019

View reviewed changes

filmil force-pushed the intl branch 2 times, most recently from e885e16 to b1be359 Compare October 14, 2019 18:37

filmil mentioned this pull request Oct 14, 2019

Upgrades the ICU version to 64.2 #13123

Merged

filmil force-pushed the intl branch from b1be359 to c468aad Compare October 14, 2019 23:36

filmil force-pushed the intl branch from c468aad to 1f3e103 Compare October 15, 2019 01:34

filmil force-pushed the intl branch 3 times, most recently from b9d6479 to c9ee1c3 Compare October 22, 2019 00:24

filmil force-pushed the intl branch 4 times, most recently from 567fed3 to 19edf29 Compare October 23, 2019 00:33

chinmaygarde approved these changes Oct 23, 2019

View reviewed changes

filmil force-pushed the intl branch from 19edf29 to c62564b Compare October 23, 2019 20:26

chinmaygarde merged commit 400e3b0 into flutter:master Oct 23, 2019

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 24, 2019

400e3b0 Wires the locale provided by Fuchsia. (flutter/engine#13045)

0560ddc

engine-flutter-autoroll mentioned this pull request Oct 24, 2019

Roll engine 195425e04480..400e3b00fb4c (8 commits) flutter/flutter#43387

Merged

		@@ -0,0 +1,126 @@
		// Copyright 2019 The Flutter Authors. All rights reserved.


		using icu::Locale;

		// This backfills for the static method icu::Locale::forLanguageTag which is

Wires the locale provided by Fuchsia. #13045

Wires the locale provided by Fuchsia. #13045

Uh oh!

Conversation

filmil commented Oct 9, 2019

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

filmil Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

filmil Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

filmil Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Oct 10, 2019

Uh oh!

cbracken commented Oct 21, 2019

Uh oh!

filmil commented Oct 21, 2019

Uh oh!

filmil commented Oct 22, 2019

Uh oh!

filmil commented Oct 23, 2019

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

filmil commented Oct 23, 2019 via email

Uh oh!

filmil commented Oct 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants