Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2933343002:
Deduplicate Monochrome locale .paks (Closed)
DescriptionDeduplicate Monochrome locale .paks
This CL removes WebView strings from Chrome locale .paks in Monochrome
using a generated resource whitelist. While WebView's locale resource
logic remains the same, Chrome in Monochrome now loads a secondary
locale pack file as a fallback when a string cannot be found in the
primary locale pack file.
This CL reduces binary size by over 400KB.
BUG=724110
Review-Url: https://codereview.chromium.org/2933343002
Cr-Commit-Position: refs/heads/master@{#485635}
Committed: https://chromium.googlesource.com/chromium/src/+/08ba6b7c7e7edb3f7c8851eb7ddc1d2ea64497e6
Patch Set 1 #
Total comments: 28
Patch Set 2 : Addressing comments #
Total comments: 4
Patch Set 3 : Addressing comments #
Total comments: 14
Patch Set 4 : Addressing comments #
Messages
Total messages: 61 (45 generated)
Description was changed from ========== DNS generate de-duplicated monochrome locale paks BUG= ========== to ========== generate de-duplicated monochrome locale paks BUG= ==========
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by zpeng@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== generate de-duplicated monochrome locale paks BUG= ========== to ========== Deduplicate Monochrome locale .paks This CL removes WebView strings from Chrome locale .paks in Monochrome using a generated resource whitelist. While WebView's locale resource logic remains the same, Chrome in Monochrome now loads a secondary locale pack file as a fallback when a string cannot be found in the primary locale pack file. This CL reduces binary size by over 400KB. BUG=724110 ==========
Description was changed from ========== Deduplicate Monochrome locale .paks This CL removes WebView strings from Chrome locale .paks in Monochrome using a generated resource whitelist. While WebView's locale resource logic remains the same, Chrome in Monochrome now loads a secondary locale pack file as a fallback when a string cannot be found in the primary locale pack file. This CL reduces binary size by over 400KB. BUG=724110 ========== to ========== Deduplicate Monochrome locale .paks This CL removes WebView strings from Chrome locale .paks in Monochrome using a generated resource whitelist. While WebView's locale resource logic remains the same, Chrome in Monochrome now loads a secondary locale pack file as a fallback when a string cannot be found in the primary locale pack file. This CL reduces binary size by over 400KB. BUG=724110 ==========
Patchset #1 (id:60001) has been deleted
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
zpeng@chromium.org changed reviewers: + agrieve@chromium.org
Hi Andrew, PTAL. Thanks!
Some nits, but overall looks awesome! lgtm! https://codereview.chromium.org/2933343002/diff/80001/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/2933343002/diff/80001/chrome/android/BUILD.gn... chrome/android/BUILD.gn:731: assert(is_android, nit: No need to assert this. Any file that has android_* targets in it already asserts so (there's an assert in our rules.gni) https://codereview.chromium.org/2933343002/diff/80001/chrome/android/BUILD.gn... chrome/android/BUILD.gn:736: system_webview_pak_whitelist = nit: prefix with _ so as not not confuse with a parameter to action() https://codereview.chromium.org/2933343002/diff/80001/tools/resources/filter_... File tools/resources/filter_resource_whitelist.py (right): https://codereview.chromium.org/2933343002/diff/80001/tools/resources/filter_... tools/resources/filter_resource_whitelist.py:11: USAGE = """filter_resource_whitelist.py [-h] [-i INPUT] [-f FILTER] [-o OUTPUT] nit: Use this as the __doc__ string (put before imports and remove USAGE =. Assign it below using usage=__doc__ https://codereview.chromium.org/2933343002/diff/80001/tools/resources/filter_... tools/resources/filter_resource_whitelist.py:23: nit: remove blank line https://codereview.chromium.org/2933343002/diff/80001/tools/resources/filter_... tools/resources/filter_resource_whitelist.py:30: '-i', '--input', type=argparse.FileType('r'), default=sys.stdin, nit: drop the short form arguments. They just serve to confuse when used. https://codereview.chromium.org/2933343002/diff/80001/tools/resources/filter_... tools/resources/filter_resource_whitelist.py:34: '-f', '--filter', type=argparse.FileType('r'), default=sys.stdin, Seems strange to have two different flags default to stdin. Doesn't look like we use stdin ever, so probably better to just change to required=true https://codereview.chromium.org/2933343002/diff/80001/tools/resources/filter_... tools/resources/filter_resource_whitelist.py:41: '--out-dir', required=True, nit: call this '--output-directory' to be consistent with other scripts (although I recognize there's already some variance, --output-directory is the most common). https://codereview.chromium.org/2933343002/diff/80001/ui/base/resource/resour... File ui/base/resource/resource_bundle_android.cc (right): https://codereview.chromium.org/2933343002/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle_android.cc:39: base::MemoryMappedFile::Region* out_region) { nit: this now doesn't match the other out param (fd_out). https://codereview.chromium.org/2933343002/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle_android.cc:57: int LoadLocalePakFromApk(const std::string& app_locale, nit: might be nice to match the signature of the above method (two outs and returning a bool). Or change the other one to match this one. https://codereview.chromium.org/2933343002/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle_android.cc:62: LOG(WARNING) << "locale_path_within_apk.empty() for locale " nit: Probably worth using ERROR here https://codereview.chromium.org/2933343002/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle_android.cc:74: LOG(ERROR) << "failed to load locale.pak"; nit: just << to the NOTREACHED(). https://codereview.chromium.org/2933343002/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle_android.cc:140: // unnecessary for loading locale resources. nit: say why it's unnecessary. https://codereview.chromium.org/2933343002/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle_android.cc:148: if (!secondary_locale_resources_data_.get()) nit: probably fine to omit this early return since we'll at least have the primary pak?
https://codereview.chromium.org/2933343002/diff/80001/tools/resources/filter_... File tools/resources/filter_resource_whitelist.py (right): https://codereview.chromium.org/2933343002/diff/80001/tools/resources/filter_... tools/resources/filter_resource_whitelist.py:1: #!/usr/bin/env python Also: Add yourself (and me) to the OWNERS file here so that you can make updates to this script if necessary.
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
zpeng@chromium.org changed reviewers: + dpranke@chromium.org, sadrul@chromium.org, thestig@chromium.org
Thanks Andrew! PTAL Hi Dirk, Lei, and Sadrul, PTAL. Thanks! https://codereview.chromium.org/2933343002/diff/80001/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/2933343002/diff/80001/chrome/android/BUILD.gn... chrome/android/BUILD.gn:731: assert(is_android, On 2017/06/29 01:09:53, agrieve wrote: > nit: No need to assert this. Any file that has android_* targets in it already > asserts so (there's an assert in our rules.gni) Done. https://codereview.chromium.org/2933343002/diff/80001/chrome/android/BUILD.gn... chrome/android/BUILD.gn:736: system_webview_pak_whitelist = On 2017/06/29 01:09:53, agrieve wrote: > nit: prefix with _ so as not not confuse with a parameter to action() Done. https://codereview.chromium.org/2933343002/diff/80001/tools/resources/filter_... File tools/resources/filter_resource_whitelist.py (right): https://codereview.chromium.org/2933343002/diff/80001/tools/resources/filter_... tools/resources/filter_resource_whitelist.py:1: #!/usr/bin/env python On 2017/06/29 01:29:25, agrieve wrote: > Also: Add yourself (and me) to the OWNERS file here so that you can make updates > to this script if necessary. Done. https://codereview.chromium.org/2933343002/diff/80001/tools/resources/filter_... tools/resources/filter_resource_whitelist.py:11: USAGE = """filter_resource_whitelist.py [-h] [-i INPUT] [-f FILTER] [-o OUTPUT] On 2017/06/29 01:09:53, agrieve wrote: > nit: Use this as the __doc__ string (put before imports and remove USAGE =. > > Assign it below using usage=__doc__ Done. https://codereview.chromium.org/2933343002/diff/80001/tools/resources/filter_... tools/resources/filter_resource_whitelist.py:23: On 2017/06/29 01:09:53, agrieve wrote: > nit: remove blank line Done. https://codereview.chromium.org/2933343002/diff/80001/tools/resources/filter_... tools/resources/filter_resource_whitelist.py:30: '-i', '--input', type=argparse.FileType('r'), default=sys.stdin, On 2017/06/29 01:09:53, agrieve wrote: > nit: drop the short form arguments. They just serve to confuse when used. Done. https://codereview.chromium.org/2933343002/diff/80001/tools/resources/filter_... tools/resources/filter_resource_whitelist.py:34: '-f', '--filter', type=argparse.FileType('r'), default=sys.stdin, On 2017/06/29 01:09:53, agrieve wrote: > Seems strange to have two different flags default to stdin. Doesn't look like we > use stdin ever, so probably better to just change to required=true Done. https://codereview.chromium.org/2933343002/diff/80001/tools/resources/filter_... tools/resources/filter_resource_whitelist.py:41: '--out-dir', required=True, On 2017/06/29 01:09:53, agrieve wrote: > nit: call this '--output-directory' to be consistent with other scripts > (although I recognize there's already some variance, --output-directory is the > most common). Done. https://codereview.chromium.org/2933343002/diff/80001/ui/base/resource/resour... File ui/base/resource/resource_bundle_android.cc (right): https://codereview.chromium.org/2933343002/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle_android.cc:39: base::MemoryMappedFile::Region* out_region) { On 2017/06/29 01:09:53, agrieve wrote: > nit: this now doesn't match the other out param (fd_out). Done. I put "out" in front of "fd/region" because that's how GetCommonResourcesPackFd etc. do it. https://codereview.chromium.org/2933343002/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle_android.cc:57: int LoadLocalePakFromApk(const std::string& app_locale, On 2017/06/29 01:09:53, agrieve wrote: > nit: might be nice to match the signature of the above method (two outs and > returning a bool). Or change the other one to match this one. Resolved offline. https://codereview.chromium.org/2933343002/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle_android.cc:62: LOG(WARNING) << "locale_path_within_apk.empty() for locale " On 2017/06/29 01:09:53, agrieve wrote: > nit: Probably worth using ERROR here Done. https://codereview.chromium.org/2933343002/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle_android.cc:74: LOG(ERROR) << "failed to load locale.pak"; On 2017/06/29 01:09:53, agrieve wrote: > nit: just << to the NOTREACHED(). Resolved offline. https://codereview.chromium.org/2933343002/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle_android.cc:140: // unnecessary for loading locale resources. On 2017/06/29 01:09:53, agrieve wrote: > nit: say why it's unnecessary. Done. https://codereview.chromium.org/2933343002/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle_android.cc:148: if (!secondary_locale_resources_data_.get()) On 2017/06/29 01:09:53, agrieve wrote: > nit: probably fine to omit this early return since we'll at least have the > primary pak? Resolved offline.
The CQ bit was unchecked by zpeng@chromium.org
Patchset #2 (id:100001) has been deleted
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/29 18:31:21, F wrote: > Thanks Andrew! PTAL > > Hi Dirk, Lei, and Sadrul, PTAL. Thanks! Can you specify what each individual should be reviewing?
Dirk: PTAL for /tools. Thanks! Lei: PTAL for /chrome. Thanks! Sadrul: PTAL for /ui. Thanks!
On 2017/06/29 18:47:08, F wrote: > Dirk: PTAL for /tools. Thanks! > Lei: PTAL for /chrome. Thanks! > Sadrul: PTAL for /ui. Thanks! still lgtm
lgtm https://codereview.chromium.org/2933343002/diff/120001/chrome/app/chrome_main... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/2933343002/diff/120001/chrome/app/chrome_main... chrome/app/chrome_main_delegate.cc:891: // Load secondary locale .pak file if it exists. Do you want to move the newly added code up to line ~879? So the code loads the secondary locale pack right after the primary? https://codereview.chromium.org/2933343002/diff/120001/tools/resources/filter... File tools/resources/filter_resource_whitelist.py (right): https://codereview.chromium.org/2933343002/diff/120001/tools/resources/filter... tools/resources/filter_resource_whitelist.py:22: import os os and re aren't used?
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Lei! PTAL https://codereview.chromium.org/2933343002/diff/120001/chrome/app/chrome_main... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/2933343002/diff/120001/chrome/app/chrome_main... chrome/app/chrome_main_delegate.cc:891: // Load secondary locale .pak file if it exists. On 2017/06/29 19:08:31, Lei Zhang wrote: > Do you want to move the newly added code up to line ~879? So the code loads the > secondary locale pack right after the primary? Done. https://codereview.chromium.org/2933343002/diff/120001/tools/resources/filter... File tools/resources/filter_resource_whitelist.py (right): https://codereview.chromium.org/2933343002/diff/120001/tools/resources/filter... tools/resources/filter_resource_whitelist.py:22: import os On 2017/06/29 19:08:31, Lei Zhang wrote: > os and re aren't used? Done.
rs- lgtm.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:140001) has been deleted
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2933343002/diff/160001/ui/base/resource/resou... File ui/base/resource/resource_bundle.cc (right): https://codereview.chromium.org/2933343002/diff/160001/ui/base/resource/resou... ui/base/resource/resource_bundle.cc:254: std::unique_ptr<DataPack> data_pack(new DataPack(SCALE_FACTOR_100P)); auto data_pack = base::MakeUnique... https://codereview.chromium.org/2933343002/diff/160001/ui/base/resource/resou... ui/base/resource/resource_bundle.cc:259: g_shared_instance_->secondary_locale_resources_data_ = std::move(data_pack); Why do you need to use |g_shared_instance| here? https://codereview.chromium.org/2933343002/diff/160001/ui/base/resource/resou... ui/base/resource/resource_bundle.cc:581: } This looks backwards. Shouldn't you look up in the main data pack before going to the secondary data pack? https://codereview.chromium.org/2933343002/diff/160001/ui/base/resource/resou... ui/base/resource/resource_bundle.cc:610: } else if (secondary_locale_resources_data_.get() && Don't need else. https://codereview.chromium.org/2933343002/diff/160001/ui/base/resource/resou... File ui/base/resource/resource_bundle_android.cc (right): https://codereview.chromium.org/2933343002/diff/160001/ui/base/resource/resou... ui/base/resource/resource_bundle_android.cc:72: std::unique_ptr<DataPack> data_pack(new DataPack(SCALE_FACTOR_100P)); MakeUnique<> https://codereview.chromium.org/2933343002/diff/160001/ui/base/resource/resou... ui/base/resource/resource_bundle_android.cc:75: NOTREACHED(); I think you can just do: NOTREACHED() << "failed to load locale.pak"; https://codereview.chromium.org/2933343002/diff/160001/ui/base/resource/resou... ui/base/resource/resource_bundle_android.cc:150: return std::string(); Should you really return std::string() here, considering loading the primary locales succeeded?
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Sadrul! PTAL https://codereview.chromium.org/2933343002/diff/160001/ui/base/resource/resou... File ui/base/resource/resource_bundle.cc (right): https://codereview.chromium.org/2933343002/diff/160001/ui/base/resource/resou... ui/base/resource/resource_bundle.cc:254: std::unique_ptr<DataPack> data_pack(new DataPack(SCALE_FACTOR_100P)); On 2017/07/04 17:01:15, sadrul wrote: > auto data_pack = base::MakeUnique... Done. https://codereview.chromium.org/2933343002/diff/160001/ui/base/resource/resou... ui/base/resource/resource_bundle.cc:259: g_shared_instance_->secondary_locale_resources_data_ = std::move(data_pack); On 2017/07/04 17:01:15, sadrul wrote: > Why do you need to use |g_shared_instance| here? Oops. Thanks! Done https://codereview.chromium.org/2933343002/diff/160001/ui/base/resource/resou... ui/base/resource/resource_bundle.cc:581: } On 2017/07/04 17:01:14, sadrul wrote: > This looks backwards. Shouldn't you look up in the main data pack before going > to the secondary data pack? Before the change, Chrome would look for string resources in main locale data pack first, and main resource data pack next. With the introduction of secondary locale data pack, Chrome would look for string resources in main locale data pack first, secondary locale data pack next, and then the main resource data pack. The main resource data pack is just a fallback, currently only used for tests. It should not contain any string resources used by Chrome. https://codereview.chromium.org/2933343002/diff/160001/ui/base/resource/resou... ui/base/resource/resource_bundle.cc:610: } else if (secondary_locale_resources_data_.get() && On 2017/07/04 17:01:14, sadrul wrote: > Don't need else. Done. https://codereview.chromium.org/2933343002/diff/160001/ui/base/resource/resou... File ui/base/resource/resource_bundle_android.cc (right): https://codereview.chromium.org/2933343002/diff/160001/ui/base/resource/resou... ui/base/resource/resource_bundle_android.cc:72: std::unique_ptr<DataPack> data_pack(new DataPack(SCALE_FACTOR_100P)); On 2017/07/04 17:01:15, sadrul wrote: > MakeUnique<> Done. https://codereview.chromium.org/2933343002/diff/160001/ui/base/resource/resou... ui/base/resource/resource_bundle_android.cc:75: NOTREACHED(); On 2017/07/04 17:01:15, sadrul wrote: > I think you can just do: > NOTREACHED() << "failed to load locale.pak"; I think NOTREACHED() is a DCHECK that would be no-op in release builds. Using LOG(ERROR) would ensure the error log. https://codereview.chromium.org/2933343002/diff/160001/ui/base/resource/resou... ui/base/resource/resource_bundle_android.cc:150: return std::string(); On 2017/07/04 17:01:15, sadrul wrote: > Should you really return std::string() here, considering loading the primary > locales succeeded? This CL splits Monochrome's Chrome locale paks into two equally important parts. Monochrome needs both to work properly. So I think only loading primary locales is still a failure case.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by zpeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zpeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, agrieve@chromium.org, dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2933343002/#ps180001 (title: "Addressing comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1499786901147560,
"parent_rev": "e01ad954bd943e60d5f9b45f9dfe54357811d7ca", "commit_rev":
"08ba6b7c7e7edb3f7c8851eb7ddc1d2ea64497e6"}
Message was sent while issue was closed.
Description was changed from ========== Deduplicate Monochrome locale .paks This CL removes WebView strings from Chrome locale .paks in Monochrome using a generated resource whitelist. While WebView's locale resource logic remains the same, Chrome in Monochrome now loads a secondary locale pack file as a fallback when a string cannot be found in the primary locale pack file. This CL reduces binary size by over 400KB. BUG=724110 ========== to ========== Deduplicate Monochrome locale .paks This CL removes WebView strings from Chrome locale .paks in Monochrome using a generated resource whitelist. While WebView's locale resource logic remains the same, Chrome in Monochrome now loads a secondary locale pack file as a fallback when a string cannot be found in the primary locale pack file. This CL reduces binary size by over 400KB. BUG=724110 Review-Url: https://codereview.chromium.org/2933343002 Cr-Commit-Position: refs/heads/master@{#485635} Committed: https://chromium.googlesource.com/chromium/src/+/08ba6b7c7e7edb3f7c8851eb7ddc... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:180001) as https://chromium.googlesource.com/chromium/src/+/08ba6b7c7e7edb3f7c8851eb7ddc...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:180001) has been created in https://codereview.chromium.org/2980773002/ by zpeng@chromium.org. The reason for reverting is: Chrome is now missing 5 locale strings. The reason is that these strings are believed to be in WebView locales because they're whitelisted, but since WebView uses multiple whitelists to pack its locale strings, these strings are actually not contained by WebView. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
