Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(392)

Issue 2665703002: [cronet] Make getAcceptLanguages index a static table instead of the application bundle. (Closed)

Created:
3 years, 10 months ago by lilyhoughton1
Modified:
3 years, 9 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[cronet] Make getAcceptLanguages index a static table instead of the application bundle. First steps to try and decrease initialization time by putting acceptLanguages into a static table. BUG=655689 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2665703002 Cr-Commit-Position: refs/heads/master@{#454660} Committed: https://chromium.googlesource.com/chromium/src/+/fe175d21e8db10f5de5c06fc3ae68788009581f1

Patch Set 1 #

Patch Set 2 : add explicit setter and local var for acceptLanguages #

Total comments: 4

Patch Set 3 : add NSLocale-receiving method #

Total comments: 10

Patch Set 4 : add tests for getAcceptLanguagesFromNSLocale #

Patch Set 5 : address rob's comments #

Total comments: 2

Patch Set 6 : small fixes #

Total comments: 5

Patch Set 7 : fix copyright; remove unused resources #

Patch Set 8 : remove reference to bundle in BUILD.gn #

Patch Set 9 : make script to generate accept languages table #

Patch Set 10 : use NSLocale preferredLanguages instead of currentLocale #

Patch Set 11 : have getAcceptLanguages append the accept language strings from all preferred languages #

Patch Set 12 : fix accept languages for cronet testing to make tests more deterministic #

Total comments: 1

Patch Set 13 : move accept_languages header generation script to more appropriate location #

Unified diffs Side-by-side diffs Delta from patch set Stats (+150 lines, -199 lines) Patch
M components/cronet/ios/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +12 lines, -66 lines 0 comments Download
M components/cronet/ios/Cronet.h View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 0 comments Download
M components/cronet/ios/Cronet.mm View 1 2 3 4 5 6 7 8 9 10 4 chunks +30 lines, -14 lines 0 comments Download
D components/cronet/ios/Resources/Localization/am.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/ar.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/bg.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/bn.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/ca.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/cs.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/da.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/de.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/el.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/en-GB.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/en.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/es-419.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/es.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/et.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/fa.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/fi.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/fil.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/fr.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/gu.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/he.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/hi.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/hr.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/hu.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/id.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/it.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/ja.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/kn.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/ko.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/lt.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/lv.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/ml.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/mr.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/ms.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/nb.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/nl.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/pl.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/pt-BR.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/pt-PT.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/pt.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/ro.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/ru.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/sk.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/sl.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/sr.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/sv.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/sw.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/ta.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/te.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/th.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/tr.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/uk.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/vi.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/zh-Hans.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/zh-Hant.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/Localization/zh.lproj/Localizable.strings View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
D components/cronet/ios/Resources/README View 1 2 3 4 5 6 1 chunk +0 lines, -9 lines 0 comments Download
M components/cronet/ios/test/BUILD.gn View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
A components/cronet/ios/test/cronet_acceptlang_test.mm View 1 2 3 4 5 6 7 8 9 10 1 chunk +57 lines, -0 lines 0 comments Download
M components/cronet/ios/test/start_cronet.mm View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
A components/cronet/tools/generate_accept_languages.py View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +45 lines, -0 lines 0 comments Download

Messages

Total messages: 25 (10 generated)
lilyhoughton
PTAL. I'm not quite sure that the functions I use to get language and region ...
3 years, 10 months ago (2017-01-30 15:35:54 UTC) #4
lilyhoughton
Added a setAcceptLanguages function; if this is worth keeping then we should probably also rename ...
3 years, 10 months ago (2017-01-30 19:42:53 UTC) #5
mef
Please add BUG= https://codereview.chromium.org/2665703002/diff/20001/components/cronet/ios/Cronet.mm File components/cronet/ios/Cronet.mm (right): https://codereview.chromium.org/2665703002/diff/20001/components/cronet/ios/Cronet.mm#newcode118 components/cronet/ios/Cronet.mm:118: + (NSString*)getAcceptLanguages { For testing purposes ...
3 years, 10 months ago (2017-02-01 19:36:04 UTC) #7
lilyhoughton
https://codereview.chromium.org/2665703002/diff/20001/components/cronet/ios/Cronet.mm File components/cronet/ios/Cronet.mm (right): https://codereview.chromium.org/2665703002/diff/20001/components/cronet/ios/Cronet.mm#newcode118 components/cronet/ios/Cronet.mm:118: + (NSString*)getAcceptLanguages { On 2017/02/01 19:36:03, mef wrote: > ...
3 years, 10 months ago (2017-02-01 20:58:30 UTC) #10
robgaunt
https://codereview.chromium.org/2665703002/diff/40001/components/cronet/ios/Cronet.mm File components/cronet/ios/Cronet.mm (right): https://codereview.chromium.org/2665703002/diff/40001/components/cronet/ios/Cronet.mm#newcode118 components/cronet/ios/Cronet.mm:118: + (NSString*)getAcceptLanguagesFromNSLocale:(NSLocale*)locale { On 2017/02/01 20:58:29, lilyhoughton wrote: > ...
3 years, 10 months ago (2017-02-02 18:44:33 UTC) #11
lilyhoughton
https://codereview.chromium.org/2665703002/diff/40001/components/cronet/ios/Cronet.mm File components/cronet/ios/Cronet.mm (right): https://codereview.chromium.org/2665703002/diff/40001/components/cronet/ios/Cronet.mm#newcode118 components/cronet/ios/Cronet.mm:118: + (NSString*)getAcceptLanguagesFromNSLocale:(NSLocale*)locale { On 2017/02/02 18:44:33, robgaunt wrote: > ...
3 years, 10 months ago (2017-02-03 18:36:24 UTC) #12
robgaunt
lgtm, aside from your TODOs - ideally someone who is more familiar with iOS localization ...
3 years, 10 months ago (2017-02-03 18:42:44 UTC) #13
mef
Sylvain: do you know, who is a good person to look at localization usage? Lily: ...
3 years, 10 months ago (2017-02-06 16:21:05 UTC) #14
mef
On 2017/02/06 16:21:05, mef wrote: > Sylvain: do you know, who is a good person ...
3 years, 10 months ago (2017-02-06 16:47:07 UTC) #15
lilyhoughton
https://codereview.chromium.org/2665703002/diff/100001/components/cronet/ios/test/cronet_acceptlang_test.mm File components/cronet/ios/test/cronet_acceptlang_test.mm (right): https://codereview.chromium.org/2665703002/diff/100001/components/cronet/ios/test/cronet_acceptlang_test.mm#newcode1 components/cronet/ios/test/cronet_acceptlang_test.mm:1: // Copyright 2016 The Chromium Authors. All rights reserved. ...
3 years, 10 months ago (2017-02-06 17:54:55 UTC) #16
mef
Argh, I still can't find anybody to comment on the use of NSLocale vs NSLocalizedStringWithDefault ...
3 years, 10 months ago (2017-02-16 19:20:29 UTC) #17
chromium-reviews
You can try asking your question to https://groups.google.com/a/google.com/forum/#!forum/appledev-discuss On Thu, Feb 16, 2017 at 11:20 ...
3 years, 10 months ago (2017-02-16 19:22:05 UTC) #18
mef
lgtm https://codereview.chromium.org/2665703002/diff/210001/components/cronet/ios/BUILD.gn File components/cronet/ios/BUILD.gn (right): https://codereview.chromium.org/2665703002/diff/210001/components/cronet/ios/BUILD.gn#newcode128 components/cronet/ios/BUILD.gn:128: script = "//components/cronet/ios/generate_accept_languages.py" FWIW we currently have all ...
3 years, 9 months ago (2017-03-02 20:22:44 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2665703002/220001
3 years, 9 months ago (2017-03-03 19:11:54 UTC) #22
commit-bot: I haz the power
3 years, 9 months ago (2017-03-03 20:30:04 UTC) #25
Message was sent while issue was closed.
Committed patchset #13 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/fe175d21e8db10f5de5c06fc3ae6...

Powered by Google App Engine
This is Rietveld 408576698