Top-level domain-based default encoding

Top-level domain-based default encoding

Default text encoding is determined based on the top
level domain (*.jp, *.uk, etc) for the pages missing
encoding information. Used Chromium's resource
(components/strings/components_locale_settings_*.xtb) and
Android system locale list for { TLD : encoding } mapping.

Mapping for countries whose legacy, default encoding is
not obvious (ge-Georgian, id-Indonesian, etc) are not
included. They are likely to be UTF-8, handled by
UTF-8 detector to be introduced soon.

BUG=583564
Committed: https://crrev.com/5146788484915fc4e364a330d3a948ed7c03cd8f
Cr-Commit-Position: refs/heads/master@{#382963}

[Jinsuk Kim, 2016-02-24 07:17:33]

jinsukkim@chromium.org changed reviewers:
+ aelias@chromium.org, esprehn@chromium.org

[aelias_OOO_until_Jul13, 2016-02-26 09:07:24]

https://codereview.chromium.org/1725283002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp
(right):

https://codereview.chromium.org/1725283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp:50: map =
new WTF::HashMap<String, WTF::TextEncoding>();
Hmm, in Chromium we would use something like LAZY_INSTANCE_INITIALIZER for this,
but it seems there's no Blink equivalent and I find this mutable static object
that's never deleted kind of sketchy.

Given that the performance advantage of HashMap is really negligible here --
indeed the cost of constructing it might overwhelm it -- how about just having a
simple const array of pairs of const char*s, and doing a linear search on it? 
You would create the WTF::TextEncoding object after the search, not before.

https://codereview.chromium.org/1725283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp:156: return
TextResourceDecoder::create(m_mimeType, encodingFromDomain.isValid() ?
encodingFromDomain : settings->defaultTextEncodingName(),
settings->usesEncodingDetector());
Looks OK, can you add a test verifying (for one or two of the domains, doesn't
need to be all) that the domain-based one is used instead of the
settings->defaultTextEncodingName()?

[esprehn, 2016-02-26 09:43:14]

I think you want std::lower_bound instead.

https://codereview.chromium.org/1725283002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp
(right):

https://codereview.chromium.org/1725283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp:46: static
WTF::HashMap<String, WTF::TextEncoding>* getEncodingMap()
if you use a Map you should return by const ref here

https://codereview.chromium.org/1725283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp:50: map =
new WTF::HashMap<String, WTF::TextEncoding>();
This should use DEFINE_STATIC_LOCAL and map.isEmpty() for the init path

https://codereview.chromium.org/1725283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp:122:
map->add(String(encodingMap[i].domain), encodingMap[i].encoding);
lets not do this, make this method into something like findEncoding(String) and
use std::lower_bound

https://codereview.chromium.org/1725283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp:130:
url.host().split(String(".", 1), tokens);
you don't need to do String(".", 1), just pass "." and it'll implicitly
construct, or better yet do split('.', tokens) which will take the split(UChar
separator, ...) fast path.

https://codereview.chromium.org/1725283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp:135: return
map->get(tld);
lets use std::lower_bound on a static list instead, you can construct a fresh
TextEncoding on return.

[Jinsuk Kim, 2016-03-02 03:46:08]

https://codereview.chromium.org/1725283002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp
(right):

https://codereview.chromium.org/1725283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp:46: static
WTF::HashMap<String, WTF::TextEncoding>* getEncodingMap()
On 2016/02/26 09:43:13, esprehn wrote:
> if you use a Map you should return by const ref here

I chose to use a simple array. Please see if it looks okay.

https://codereview.chromium.org/1725283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp:50: map =
new WTF::HashMap<String, WTF::TextEncoding>();
On 2016/02/26 09:07:24, aelias wrote:
> Hmm, in Chromium we would use something like LAZY_INSTANCE_INITIALIZER for
this,
> but it seems there's no Blink equivalent and I find this mutable static object
> that's never deleted kind of sketchy.
> 
> Given that the performance advantage of HashMap is really negligible here --
> indeed the cost of constructing it might overwhelm it -- how about just having
a
> simple const array of pairs of const char*s, and doing a linear search on it? 
> You would create the WTF::TextEncoding object after the search, not before.

I agree. It got complicated that I had wished. Rewrote with an array with a
linear search.

https://codereview.chromium.org/1725283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp:50: map =
new WTF::HashMap<String, WTF::TextEncoding>();
On 2016/02/26 09:43:13, esprehn wrote:
> This should use DEFINE_STATIC_LOCAL and map.isEmpty() for the init path

Please see the reply above.

https://codereview.chromium.org/1725283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp:122:
map->add(String(encodingMap[i].domain), encodingMap[i].encoding);
On 2016/02/26 09:43:13, esprehn wrote:
> lets not do this, make this method into something like findEncoding(String)
and
> use std::lower_bound

Rewrote it with an array.

https://codereview.chromium.org/1725283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp:130:
url.host().split(String(".", 1), tokens);
On 2016/02/26 09:43:13, esprehn wrote:
> you don't need to do String(".", 1), just pass "." and it'll implicitly
> construct, or better yet do split('.', tokens) which will take the split(UChar
> separator, ...) fast path.

Better. Thanks. Done.

https://codereview.chromium.org/1725283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp:135: return
map->get(tld);
On 2016/02/26 09:43:13, esprehn wrote:
> lets use std::lower_bound on a static list instead, you can construct a fresh
> TextEncoding on return.

Rewrote it with an array.

https://codereview.chromium.org/1725283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp:156: return
TextResourceDecoder::create(m_mimeType, encodingFromDomain.isValid() ?
encodingFromDomain : settings->defaultTextEncodingName(),
settings->usesEncodingDetector());
On 2016/02/26 09:07:24, aelias wrote:
> Looks OK, can you add a test verifying (for one or two of the domains, doesn't
> need to be all) that the domain-based one is used instead of the
> settings->defaultTextEncodingName()?

Added tests.

[Jinsuk Kim, 2016-03-04 05:54:09]

- Used Vector methods more suitable for the job:
 s/v.size() > 0/v.isEmpty()/
 s/v[v.size() - 1]/v.last()/

- Exported TextResourceDecoderBuilder so that the newly added
TextResourceDecoderBuildTest can find the class and link as expected on
component build.

[aelias_OOO_until_Jul13, 2016-03-04 06:53:12]

non-owner lgtm modulo comment

https://codereview.chromium.org/1725283002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/TextResourceDecoderBuilderTest.cpp
(right):

https://codereview.chromium.org/1725283002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/TextResourceDecoderBuilderTest.cpp:30:
EXPECT_EQ(WTF::Latin1Encoding(),
defaultEncodingForURL("http://www.itojun.org/paper/keio-doctor97.html"));
Since this real URL is actually not in Latin1 it's a bit confusing to test that
we choose Latin1 for it, I'd suggest picking a different example.

[Jinsuk Kim, 2016-03-07 01:28:00]

https://codereview.chromium.org/1725283002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/TextResourceDecoderBuilderTest.cpp
(right):

https://codereview.chromium.org/1725283002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/TextResourceDecoderBuilderTest.cpp:30:
EXPECT_EQ(WTF::Latin1Encoding(),
defaultEncodingForURL("http://www.itojun.org/paper/keio-doctor97.html"));
On 2016/03/04 06:53:12, aelias wrote:
> Since this real URL is actually not in Latin1 it's a bit confusing to test
that
> we choose Latin1 for it, I'd suggest picking a different example.

Done.

[esprehn, 2016-03-21 22:56:25]

lgtm, but I'd rather you used a struct instead of math on the char* array.

https://codereview.chromium.org/1725283002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp
(right):

https://codereview.chromium.org/1725283002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp:45: static
const char* legacyEncodings[] = {
I'd name this into an array of structs, so you could access the short name and
the long name separately without the + 1 below.

[Jinsuk Kim, 2016-03-22 06:08:19]

The CQ bit was checked by jinsukkim@chromium.org

[Jinsuk Kim, 2016-03-22 06:08:20]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/1725283002/#ps60001
(title: "addressed comments")

[Jinsuk Kim, 2016-03-22 06:08:23]

The CQ bit was unchecked by jinsukkim@chromium.org

[Jinsuk Kim, 2016-03-22 21:51:53]

https://codereview.chromium.org/1725283002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp
(right):

https://codereview.chromium.org/1725283002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/TextResourceDecoderBuilder.cpp:45: static
const char* legacyEncodings[] = {
On 2016/03/21 22:56:25, esprehn wrote:
> I'd name this into an array of structs, so you could access the short name and
> the long name separately without the + 1 below.

Done. Also used anonymous namespace to limit the scope of the struct defined for
this array.

[esprehn, 2016-03-22 23:04:20]

lgtm

[esprehn, 2016-03-22 23:04:36]

btw where are the tests?

[Jinsuk Kim, 2016-03-22 23:07:51]

On 2016/03/22 23:04:36, esprehn wrote:
> btw where are the tests?

Added it back.

[Jinsuk Kim, 2016-03-22 23:09:13]

On 2016/03/22 23:07:51, Jinsuk wrote:
> On 2016/03/22 23:04:36, esprehn wrote:
> > btw where are the tests?
> 
> Added it back.

Please take another look at crrev.com/1721373002 that handles the UTF-8 decoder
mentioned in the patch description. Thanks.

[Jinsuk Kim, 2016-03-23 23:01:32]

The CQ bit was checked by jinsukkim@chromium.org

[Jinsuk Kim, 2016-03-23 23:01:32]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1725283002/#ps120001
(title: "rebased")

[commit-bot: I haz the power, 2016-03-23 23:01:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1725283002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1725283002/120001

[commit-bot: I haz the power, 2016-03-23 23:08:55]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-23 23:10:18]

Description was changed from

==========
Top-level domain-based default encoding

Default text encoding is determined based on the top
level domain (*.jp, *.uk, etc) for the pages missing
encoding information. Used Chromium's resource
(components/strings/components_locale_settings_*.xtb) and
Android system locale list for { TLD : encoding } mapping.

Mapping for countries whose legacy, default encoding is
not obvious (ge-Georgian, id-Indonesian, etc) are not
included. They are likely to be UTF-8, handled by
UTF-8 detector to be introduced soon.

BUG=583564
==========

to

==========
Top-level domain-based default encoding

Default text encoding is determined based on the top
level domain (*.jp, *.uk, etc) for the pages missing
encoding information. Used Chromium's resource
(components/strings/components_locale_settings_*.xtb) and
Android system locale list for { TLD : encoding } mapping.

Mapping for countries whose legacy, default encoding is
not obvious (ge-Georgian, id-Indonesian, etc) are not
included. They are likely to be UTF-8, handled by
UTF-8 detector to be introduced soon.

BUG=583564

Committed: https://crrev.com/5146788484915fc4e364a330d3a948ed7c03cd8f
Cr-Commit-Position: refs/heads/master@{#382963}
==========

[commit-bot: I haz the power, 2016-03-23 23:10:19]

Patchset 7 (id:??) landed as
https://crrev.com/5146788484915fc4e364a330d3a948ed7c03cd8f
Cr-Commit-Position: refs/heads/master@{#382963}