[cronet] Make getAcceptLanguages index a static table instead of the application bundle.

[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

[lilyhoughton1, 2017-01-30 15:31:00]

Description was changed from

==========
[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=
==========

to

==========
[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=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[lilyhoughton, 2017-01-30 15:32:49]

Description was changed from

==========
[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=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
[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=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[lilyhoughton, 2017-01-30 15:32:49]

lilyhoughton@chromium.org changed reviewers:
+ mef@chromium.org

[lilyhoughton, 2017-01-30 15:35:54]

PTAL.

I'm not quite sure that the functions I use to get language and region are
correct - in particular, they make one of the tests fail if the system language
is not en-US.  However, I'm not confident that that test is correct, as it
doesn't seem to have any provisions of setting the language.  It simply verifies
that getAcceptLanguages always returns en-US.  (Test is TEST_F(HttpTest,
NSURLSessionAcceptLanguage)).

[lilyhoughton, 2017-01-30 19:42:53]

Added a setAcceptLanguages function; if this is worth keeping then we should
probably also rename getAcceptLanguages to something more obvious?  maybe
getSystemDefaultAcceptLanguages or something?

[mef, 2017-02-01 19:36:03]

mef@chromium.org changed reviewers:
+ robgaunt@google.com, sdefresne@chromium.org

[mef, 2017-02-01 19:36:04]

Please add BUG=

https://codereview.chromium.org/2665703002/diff/20001/components/cronet/ios/C...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2665703002/diff/20001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:118: + (NSString*)getAcceptLanguages {
For testing purposes I'd suggest to have a method (maybe separate) that takes
NSLocale, so we could verify that it does the right thing.

https://codereview.chromium.org/2665703002/diff/20001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:120: NSDictionary* acceptLangs = @{
Given that we need to pass it to set_accept_language as std::string it could
make sense to use C++ data structures. Not sure about memory and cpu efficiency
of either though, could be faster to scan a static array instead of creating
NSDictionary or std::map.

https://codereview.chromium.org/2665703002/diff/20001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:177: // TODO(lilyhoughton) reasonably sure this
is wrong
Yeah, I'm adding Sylvain and Rob to suggest proper way.

[lilyhoughton, 2017-02-01 20:11:48]

Description was changed from

==========
[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=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
[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
==========

[lilyhoughton, 2017-02-01 20:58:29]

lilyhoughton@chromium.org changed reviewers:
+ lilyhoughton@chromium.org

[lilyhoughton, 2017-02-01 20:58:30]

https://codereview.chromium.org/2665703002/diff/20001/components/cronet/ios/C...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2665703002/diff/20001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:118: + (NSString*)getAcceptLanguages {
On 2017/02/01 19:36:03, mef wrote:
> For testing purposes I'd suggest to have a method (maybe separate) that takes
> NSLocale, so we could verify that it does the right thing.

Done.

https://codereview.chromium.org/2665703002/diff/40001/components/cronet/ios/C...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2665703002/diff/40001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:118: +
(NSString*)getAcceptLanguagesFromNSLocale:(NSLocale*)locale {
Started writing tests for this, but I don't know how to make it visible to the
tests without making it public.

[robgaunt, 2017-02-02 18:44:33]

https://codereview.chromium.org/2665703002/diff/40001/components/cronet/ios/C...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2665703002/diff/40001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:118: +
(NSString*)getAcceptLanguagesFromNSLocale:(NSLocale*)locale {
On 2017/02/01 20:58:29, lilyhoughton wrote:
> Started writing tests for this, but I don't know how to make it visible to the
> tests without making it public.

One trick is to use a category to make it visible to the tests.

So, in your Test.m file, you add

@interface Cronet (ExposedForTesting)
+ (NSString *)getAcceptLanguagesFromNSLocal:(NSLocale *)locale
@end

and now you can call that method in your Test.m file.

https://codereview.chromium.org/2665703002/diff/40001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:177: // TODO(lilyhoughton) reasonably sure this
is wrong
I am not an expert on localization, so I'm not sure how much help I can be...

I just read over the NSLocale docs, does [NSLocale preferredLanguages] give you
what you're looking for?
https://developer.apple.com/reference/foundation/nslocale/1856129-preferredla...

https://codereview.chromium.org/2665703002/diff/40001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:179: stringWithFormat:@"%@-%@", [[NSLocale
currentLocale] languageCode],
s/[NSLocale currentLocale]/locale

https://codereview.chromium.org/2665703002/diff/40001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:184: : acceptLangs[lang] ? acceptLangs[lang] :
@"en-US,en";
Tip: you can use the ?: operator.

acceptLangs[region] ?: acceptLangs[lang] ?: @"en-US,en"

https://codereview.chromium.org/2665703002/diff/40001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:192: +
(void)setAcceptLanguages:(NSString*)acceptLanguages {
This new method isn't called from anywhere or exposed in the public header.

[lilyhoughton, 2017-02-03 18:36:24]

https://codereview.chromium.org/2665703002/diff/40001/components/cronet/ios/C...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2665703002/diff/40001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:118: +
(NSString*)getAcceptLanguagesFromNSLocale:(NSLocale*)locale {
On 2017/02/02 18:44:33, robgaunt wrote:
> On 2017/02/01 20:58:29, lilyhoughton wrote:
> > Started writing tests for this, but I don't know how to make it visible to
the
> > tests without making it public.
> 
> One trick is to use a category to make it visible to the tests.
> 
> So, in your Test.m file, you add
> 
> @interface Cronet (ExposedForTesting)
> + (NSString *)getAcceptLanguagesFromNSLocal:(NSLocale *)locale
> @end
> 
> and now you can call that method in your Test.m file.

Done.

https://codereview.chromium.org/2665703002/diff/40001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:179: stringWithFormat:@"%@-%@", [[NSLocale
currentLocale] languageCode],
On 2017/02/02 18:44:33, robgaunt wrote:
> s/[NSLocale currentLocale]/locale

Done.

https://codereview.chromium.org/2665703002/diff/40001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:184: : acceptLangs[lang] ? acceptLangs[lang] :
@"en-US,en";
On 2017/02/02 18:44:33, robgaunt wrote:
> Tip: you can use the ?: operator.
> 
> acceptLangs[region] ?: acceptLangs[lang] ?: @"en-US,en"

Done.

https://codereview.chromium.org/2665703002/diff/40001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:192: +
(void)setAcceptLanguages:(NSString*)acceptLanguages {
On 2017/02/02 18:44:33, robgaunt wrote:
> This new method isn't called from anywhere or exposed in the public header.

Done.

[robgaunt, 2017-02-03 18:42:44]

lgtm, aside from your TODOs - ideally someone who is more familiar with iOS
localization can look this over

https://codereview.chromium.org/2665703002/diff/80001/components/cronet/ios/C...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2665703002/diff/80001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:190: +
(void)setAcceptLanguages:(NSString*)acceptLanguages {
[self checkNotStarted];

https://codereview.chromium.org/2665703002/diff/80001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:255: gAcceptLanguages ? gAcceptLanguages : [self
getAcceptLanguages]));
gAcceptLanguages ?: [self getAcceptLanguages]

[mef, 2017-02-06 16:21:05]

Sylvain: do you know, who is a good person to look at localization usage?

Lily: You should also remove cronet/ios/Resources as we don't need them any
more.

https://codereview.chromium.org/2665703002/diff/100001/components/cronet/ios/...
File components/cronet/ios/test/cronet_acceptlang_test.mm (right):

https://codereview.chromium.org/2665703002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_acceptlang_test.mm:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
nit: 2017

https://codereview.chromium.org/2665703002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_acceptlang_test.mm:29:
localeWithLocaleIdentifier:@"ja-JP"]];
did you mean to specify just 'ja'?

[mef, 2017-02-06 16:47:07]

On 2017/02/06 16:21:05, mef wrote:
> Sylvain: do you know, who is a good person to look at localization usage?
> 
> Lily: You should also remove cronet/ios/Resources as we don't need them any
> more.
I think 'bundle_data("cronet_framework_resources") should be removed from
ios/BUILD.gn 
> 
>
https://codereview.chromium.org/2665703002/diff/100001/components/cronet/ios/...
> File components/cronet/ios/test/cronet_acceptlang_test.mm (right):
> 
>
https://codereview.chromium.org/2665703002/diff/100001/components/cronet/ios/...
> components/cronet/ios/test/cronet_acceptlang_test.mm:1: // Copyright 2016 The
> Chromium Authors. All rights reserved.
> nit: 2017
> 
>
https://codereview.chromium.org/2665703002/diff/100001/components/cronet/ios/...
> components/cronet/ios/test/cronet_acceptlang_test.mm:29:
> localeWithLocaleIdentifier:@"ja-JP"]];
> did you mean to specify just 'ja'?

[lilyhoughton, 2017-02-06 17:54:55]

https://codereview.chromium.org/2665703002/diff/100001/components/cronet/ios/...
File components/cronet/ios/test/cronet_acceptlang_test.mm (right):

https://codereview.chromium.org/2665703002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_acceptlang_test.mm:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2017/02/06 16:21:05, mef wrote:
> nit: 2017

Done.

https://codereview.chromium.org/2665703002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_acceptlang_test.mm:29:
localeWithLocaleIdentifier:@"ja-JP"]];
On 2017/02/06 16:21:05, mef wrote:
> did you mean to specify just 'ja'?

No, this is testing the behavior that if a language is specified with a region
that is not supported, it correctly defaults to just that language without any
region.

[mef, 2017-02-16 19:20:29]

Argh, I still can't find anybody to comment on the use of NSLocale vs
NSLocalizedStringWithDefault value.

https://codereview.chromium.org/2665703002/diff/100001/components/cronet/ios/...
File components/cronet/ios/test/cronet_acceptlang_test.mm (right):

https://codereview.chromium.org/2665703002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_acceptlang_test.mm:29:
localeWithLocaleIdentifier:@"ja-JP"]];
On 2017/02/06 17:54:55, lilyhoughton wrote:
> On 2017/02/06 16:21:05, mef wrote:
> > did you mean to specify just 'ja'?
> 
> No, this is testing the behavior that if a language is specified with a region
> that is not supported, it correctly defaults to just that language without any
> region.

Acknowledged.

[chromium-reviews, 2017-02-16 19:22:05]

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 AM, <mef@chromium.org> wrote:

> Argh, I still can't find anybody to comment on the use of NSLocale vs
> NSLocalizedStringWithDefault value.
>
>
> 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#newcode29
> components/cronet/ios/test/cronet_acceptlang_test.mm:29:
> localeWithLocaleIdentifier:@"ja-JP"]];
> On 2017/02/06 17:54:55, lilyhoughton wrote:
> > On 2017/02/06 16:21:05, mef wrote:
> > > did you mean to specify just 'ja'?
> >
> > No, this is testing the behavior that if a language is specified with
> a region
> > that is not supported, it correctly defaults to just that language
> without any
> > region.
>
> Acknowledged.
>
> https://codereview.chromium.org/2665703002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mef, 2017-03-02 20:22:44]

lgtm

https://codereview.chromium.org/2665703002/diff/210001/components/cronet/ios/...
File components/cronet/ios/BUILD.gn (right):

https://codereview.chromium.org/2665703002/diff/210001/components/cronet/ios/...
components/cronet/ios/BUILD.gn:128: script =
"//components/cronet/ios/generate_accept_languages.py"
FWIW we currently have all scripts in components/cronet/tools directory.

[lilyhoughton, 2017-03-03 19:11:26]

The CQ bit was checked by lilyhoughton@chromium.org

[lilyhoughton, 2017-03-03 19:11:27]

The patchset sent to the CQ was uploaded after l-g-t-m from robgaunt@google.com,
mef@chromium.org
Link to the patchset: https://codereview.chromium.org/2665703002/#ps220001
(title: "move accept_languages header generation script to more appropriate
location")

[commit-bot: I haz the power, 2017-03-03 19:11:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 20:29:24]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1488568286440900,
"parent_rev": "1021ed20bd0720753b316270561c691e26f93cf7", "commit_rev":
"fe175d21e8db10f5de5c06fc3ae68788009581f1"}

[commit-bot: I haz the power, 2017-03-03 20:30:02]

Description was changed from

==========
[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
==========

to

==========
[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/+/fe175d21e8db10f5de5c06fc3ae6...
==========

[commit-bot: I haz the power, 2017-03-03 20:30:04]

Committed patchset #13 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/fe175d21e8db10f5de5c06fc3ae6...