Move |GetCanonicalEncodingNameByAliasName| to base/i18n

Move |GetCanonicalEncodingNameByAliasName| to base/i18n

This CL moves the encoding-related utility method to base/i18n where
it belongs better and makes itself available directly to
embedder/contents.

It also helps the interface ContentBrowserClient do without
the method of the same name, which makes it simpler.

Committed: https://crrev.com/79df88ae0437499f21dd82976ca24a0c6f2d11c7
Cr-Commit-Position: refs/heads/master@{#441326}

[Jinsuk Kim, 2016-12-15 03:52:23]

jinsukkim@chromium.org changed reviewers:
+ thestig@chromium.org

[Jinsuk Kim, 2016-12-15 03:52:23]

..sort of motivated by the urge to remove TODO added in
https://crrev.com/2254273003 as it's getting close to the end of year.

[Jinsuk Kim, 2016-12-15 04:13:44]

jinsukkim@chromium.org changed reviewers:
+ jshin@chromium.org
- thestig@chromium.org

[jungshik at Google, 2016-12-15 18:35:01]

See the comment below.

https://codereview.chromium.org/2578003002/diff/1/base/i18n/character_encodin...
File base/i18n/character_encoding.cc (right):

https://codereview.chromium.org/2578003002/diff/1/base/i18n/character_encodin...
base/i18n/character_encoding.cc:28: for (size_t i = 0; i <
arraysize(kCanonicalEncodingNames); ++i) {
nit: why don't you use a new C++ for-each syntax here?

https://codereview.chromium.org/2578003002/diff/1/base/i18n/character_encodin...
base/i18n/character_encoding.cc:33: for (size_t i = 0; i < arraysize(standards);
++i) {
ditto

https://codereview.chromium.org/2578003002/diff/1/base/i18n/character_encodin...
base/i18n/character_encoding.cc:36: ucnv_getCanonicalName(alias_name.c_str(),
standards[i], &error_code);
ucnv_getStandardName() has to be used here.

https://codereview.chromium.org/2578003002/diff/1/base/i18n/character_encodin...
File base/i18n/character_encoding_unittest.cc (right):

https://codereview.chromium.org/2578003002/diff/1/base/i18n/character_encodin...
base/i18n/character_encoding_unittest.cc:18: EXPECT_EQ("gb18030",
GetCanonicalEncodingNameByAliasName("GB18030"));
While you're at it, you'd better add a couple of more from the WHATWG encoding
spec. ( https://encoding.spec.whatwg.org/#names-and-labels or
third_party/icu/source/data/convrtrs.txt ).

   EXPECT_EQ("windows-874", GetCanonicalEncodingNameByAliasName("tis-620"));
  EXPECT_EQ("EUC-KR", GetCanonicalEncodingNameByAliasName("ks_c_5601-1987"));


Then, those test will fail because GetCanonicalEncodingNameAliasName uses a
wrong ICU API. It should use ucnv_getStandardName instead of
ucnv_getCanonicalName.

[Jinsuk Kim, 2016-12-16 00:03:10]

https://codereview.chromium.org/2578003002/diff/1/base/i18n/character_encodin...
File base/i18n/character_encoding.cc (right):

https://codereview.chromium.org/2578003002/diff/1/base/i18n/character_encodin...
base/i18n/character_encoding.cc:28: for (size_t i = 0; i <
arraysize(kCanonicalEncodingNames); ++i) {
On 2016/12/15 18:35:01, jungshik at google wrote:
> nit: why don't you use a new C++ for-each syntax here? 

Done.

https://codereview.chromium.org/2578003002/diff/1/base/i18n/character_encodin...
base/i18n/character_encoding.cc:33: for (size_t i = 0; i < arraysize(standards);
++i) {
On 2016/12/15 18:35:01, jungshik at google wrote:
> ditto

Done.

https://codereview.chromium.org/2578003002/diff/1/base/i18n/character_encodin...
base/i18n/character_encoding.cc:36: ucnv_getCanonicalName(alias_name.c_str(),
standards[i], &error_code);
On 2016/12/15 18:35:01, jungshik at google wrote:
> ucnv_getStandardName() has to be used here. 

Done.

https://codereview.chromium.org/2578003002/diff/1/base/i18n/character_encodin...
File base/i18n/character_encoding_unittest.cc (right):

https://codereview.chromium.org/2578003002/diff/1/base/i18n/character_encodin...
base/i18n/character_encoding_unittest.cc:18: EXPECT_EQ("gb18030",
GetCanonicalEncodingNameByAliasName("GB18030"));
On 2016/12/15 18:35:01, jungshik at google wrote:
> While you're at it, you'd better add a couple of more from the WHATWG encoding
> spec. ( https://encoding.spec.whatwg.org/#names-and-labels or
> third_party/icu/source/data/convrtrs.txt ).
> 
>    EXPECT_EQ("windows-874", GetCanonicalEncodingNameByAliasName("tis-620"));
>   EXPECT_EQ("EUC-KR", GetCanonicalEncodingNameByAliasName("ks_c_5601-1987"));
> 
> 
> Then, those test will fail because GetCanonicalEncodingNameAliasName uses a
> wrong ICU API. It should use ucnv_getStandardName instead of
> ucnv_getCanonicalName. 
> 

I don't know all the history, but it means this bug has been present since 2008.
Glad to see it finally spotted and fixed after all these years.

[jungshik at Google, 2016-12-16 01:20:12]

On 2016/12/16 00:03:10, Jinsuk Kim wrote:
>
https://codereview.chromium.org/2578003002/diff/1/base/i18n/character_encodin...
> File base/i18n/character_encoding.cc (right):
> 
>
https://codereview.chromium.org/2578003002/diff/1/base/i18n/character_encodin...
> base/i18n/character_encoding.cc:28: for (size_t i = 0; i <
> arraysize(kCanonicalEncodingNames); ++i) {
> On 2016/12/15 18:35:01, jungshik at google wrote:
> > nit: why don't you use a new C++ for-each syntax here? 
> 
> Done.
> 
>
https://codereview.chromium.org/2578003002/diff/1/base/i18n/character_encodin...
> base/i18n/character_encoding.cc:33: for (size_t i = 0; i <
arraysize(standards);
> ++i) {
> On 2016/12/15 18:35:01, jungshik at google wrote:
> > ditto
> 
> Done.
> 
>
https://codereview.chromium.org/2578003002/diff/1/base/i18n/character_encodin...
> base/i18n/character_encoding.cc:36: ucnv_getCanonicalName(alias_name.c_str(),
> standards[i], &error_code);
> On 2016/12/15 18:35:01, jungshik at google wrote:
> > ucnv_getStandardName() has to be used here. 
> 
> Done.
> 
>
https://codereview.chromium.org/2578003002/diff/1/base/i18n/character_encodin...
> File base/i18n/character_encoding_unittest.cc (right):
> 
>
https://codereview.chromium.org/2578003002/diff/1/base/i18n/character_encodin...
> base/i18n/character_encoding_unittest.cc:18: EXPECT_EQ("gb18030",
> GetCanonicalEncodingNameByAliasName("GB18030"));
> On 2016/12/15 18:35:01, jungshik at google wrote:
> > While you're at it, you'd better add a couple of more from the WHATWG
encoding
> > spec. ( https://encoding.spec.whatwg.org/#names-and-labels or
> > third_party/icu/source/data/convrtrs.txt ).
> > 
> >    EXPECT_EQ("windows-874", GetCanonicalEncodingNameByAliasName("tis-620"));
> >   EXPECT_EQ("EUC-KR",
GetCanonicalEncodingNameByAliasName("ks_c_5601-1987"));
> > 
> > 
> > Then, those test will fail because GetCanonicalEncodingNameAliasName uses a
> > wrong ICU API. It should use ucnv_getStandardName instead of
> > ucnv_getCanonicalName. 
> > 
> 
> I don't know all the history, but it means this bug has been present since
2008.
> Glad to see it finally spotted and fixed after all these years.

Perhaps it didn't matter because the input has been always one of encodings
listed in the hard-coded list or gb18030.  

I'm not sure who would use this function for what. (my memory is fuzzy as to why
this was added in the first place). 

Anyway, the movement was LGTM

[Jinsuk Kim, 2016-12-16 01:43:28]

jinsukkim@chromium.org changed reviewers:
+ dcheng@chromium.org, nasko@chromium.org

[Jinsuk Kim, 2016-12-16 01:43:29]

dcheng@chromium.org: Please review changes in base/BUILD.gn

nasko@chromium.org: Please review changes in content/. A public interface was
updated.

[Jinsuk Kim, 2016-12-16 02:09:53]

jinsukkim@chromium.org changed reviewers:
+ michaelbai@chromium.org, sky@chromium.org

[Jinsuk Kim, 2016-12-16 02:09:54]

michaelbai@chromium.org: Please review changes in android_webview/

sky@chromium.org: Please review changes in chrome/ I need your owner's seal

[sky, 2016-12-16 17:04:47]

chrome LGTM

[michaelbai, 2016-12-16 18:40:45]

android_webview LGTM

[dcheng, 2016-12-20 19:26:12]

https://codereview.chromium.org/2578003002/diff/20001/base/i18n/character_enc...
File base/i18n/character_encoding.cc (right):

https://codereview.chromium.org/2578003002/diff/20001/base/i18n/character_enc...
base/i18n/character_encoding.cc:27: std::string
GetCanonicalEncodingNameByAliasName(const std::string& alias_name) {
Can this return const char* to avoid constructing a std::string()?

[Jinsuk Kim, 2016-12-20 21:14:51]

The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 21:15:31]

Dry run: 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, 2016-12-20 22:55:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 22:55:08]

Dry run: This issue passed the CQ dry run.

[Jinsuk Kim, 2016-12-20 23:11:54]

https://codereview.chromium.org/2578003002/diff/20001/base/i18n/character_enc...
File base/i18n/character_encoding.cc (right):

https://codereview.chromium.org/2578003002/diff/20001/base/i18n/character_enc...
base/i18n/character_encoding.cc:27: std::string
GetCanonicalEncodingNameByAliasName(const std::string& alias_name) {
On 2016/12/20 19:26:12, dcheng wrote:
> Can this return const char* to avoid constructing a std::string()?

Done.

[dcheng, 2017-01-03 21:52:54]

https://codereview.chromium.org/2578003002/diff/40001/base/i18n/character_enc...
File base/i18n/character_encoding.cc (right):

https://codereview.chromium.org/2578003002/diff/40001/base/i18n/character_enc...
base/i18n/character_encoding.cc:30: return alias_name.c_str();
return encoding_name; is safer (there's no guarantee what the lifetime of
|alias_name| is once this function returns.

https://codereview.chromium.org/2578003002/diff/40001/base/i18n/character_enc...
base/i18n/character_encoding.cc:32: const char* standards[3] = {"HTML", "MIME",
"IANA"};
static, and name this kStandards

[Jinsuk Kim, 2017-01-03 23:42:30]

https://codereview.chromium.org/2578003002/diff/40001/base/i18n/character_enc...
File base/i18n/character_encoding.cc (right):

https://codereview.chromium.org/2578003002/diff/40001/base/i18n/character_enc...
base/i18n/character_encoding.cc:30: return alias_name.c_str();
On 2017/01/03 21:52:54, dcheng wrote:
> return encoding_name; is safer (there's no guarantee what the lifetime of
> |alias_name| is once this function returns.

Done.

https://codereview.chromium.org/2578003002/diff/40001/base/i18n/character_enc...
base/i18n/character_encoding.cc:32: const char* standards[3] = {"HTML", "MIME",
"IANA"};
On 2017/01/03 21:52:54, dcheng wrote:
> static, and name this kStandards

Done.

[nasko, 2017-01-04 00:38:07]

content/ LGTM

[dcheng, 2017-01-04 00:47:56]

lgtm

it feels a bit weird in //base, given that it's just two very specific callsites
that need this... but I don't really have any better ideas atm either.

[Jinsuk Kim, 2017-01-04 02:29:41]

The CQ bit was checked by jinsukkim@chromium.org

[Jinsuk Kim, 2017-01-04 02:29:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelbai@chromium.org, jshin@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2578003002/#ps60001
(title: "static/encoding_name")

[commit-bot: I haz the power, 2017-01-04 02:30:07]

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-01-04 03:44:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 03:44:15]

Failed to apply patch for chrome/browser/chrome_content_browser_client.cc:
While running git apply --index -p1;
  error: patch failed: chrome/browser/chrome_content_browser_client.cc:39
  error: chrome/browser/chrome_content_browser_client.cc: patch does not apply

Patch:       chrome/browser/chrome_content_browser_client.cc
Index: chrome/browser/chrome_content_browser_client.cc
diff --git a/chrome/browser/chrome_content_browser_client.cc
b/chrome/browser/chrome_content_browser_client.cc
index
2d7a02d61fe879e4dd3f804febcb773bdd14a7b8..61468804a2956c179c14ae531fb459819a62379b
100644
--- a/chrome/browser/chrome_content_browser_client.cc
+++ b/chrome/browser/chrome_content_browser_client.cc
@@ -14,6 +14,7 @@
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/files/scoped_file.h"
+#include "base/i18n/character_encoding.h"
 #include "base/json/json_reader.h"
 #include "base/lazy_instance.h"
 #include "base/macros.h"
@@ -39,7 +40,6 @@
 #include "chrome/browser/browsing_data/origin_filter_builder.h"
 #include "chrome/browser/browsing_data/registrable_domain_filter_builder.h"
 #include "chrome/browser/budget_service/budget_service_impl.h"
-#include "chrome/browser/character_encoding.h"
 #include "chrome/browser/chrome_content_browser_client_parts.h"
 #include "chrome/browser/chrome_net_benchmarking_message_filter.h"
 #include "chrome/browser/chrome_quota_permission_context.h"
@@ -1411,11 +1411,6 @@ bool
ChromeContentBrowserClient::ShouldAssignSiteForURL(const GURL& url) {
   return !url.SchemeIs(chrome::kChromeNativeScheme);
 }
 
-std::string ChromeContentBrowserClient::GetCanonicalEncodingNameByAliasName(
-    const std::string& alias_name) {
-  return ::GetCanonicalEncodingNameByAliasName(alias_name);
-}
-
 namespace {
 
 bool IsAutoReloadEnabled() {
@@ -2546,8 +2541,8 @@ void ChromeContentBrowserClient::OverrideWebkitPrefs(
 #endif
 
   // Make sure we will set the default_encoding with canonical encoding name.
-  web_prefs->default_encoding = GetCanonicalEncodingNameByAliasName(
-          web_prefs->default_encoding);
+  web_prefs->default_encoding =
+      base::GetCanonicalEncodingNameByAliasName(web_prefs->default_encoding);
   if (web_prefs->default_encoding.empty()) {
     prefs->ClearPref(prefs::kDefaultCharset);
     web_prefs->default_encoding = prefs->GetString(prefs::kDefaultCharset);

[Jinsuk Kim, 2017-01-04 04:48:27]

The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 04:48:40]

Dry run: 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-01-04 05:58:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 05:58:37]

Dry run: This issue passed the CQ dry run.

[Jinsuk Kim, 2017-01-04 06:20:35]

The CQ bit was checked by jinsukkim@chromium.org

[Jinsuk Kim, 2017-01-04 06:20:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelbai@chromium.org, jshin@chromium.org, dcheng@chromium.org,
nasko@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2578003002/#ps80001
(title: "rebased")

[commit-bot: I haz the power, 2017-01-04 06:20:44]

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-01-04 06:26:19]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483510835485240,
"parent_rev": "bc3f7b84c63f10e4e19b7349d7359e6683313189", "commit_rev":
"696e3af162dd3338f227a87e1164a07fba26f1fa"}

[commit-bot: I haz the power, 2017-01-04 06:27:24]

Description was changed from

==========
Move |GetCanonicalEncodingNameByAliasName| to base/i18n

This CL moves the encoding-related utility method to base/i18n where
it belongs better and makes itself available directly to
embedder/contents.

It also helps the interface ContentBrowserClient do without
the method of the same name, which makes it simpler.
==========

to

==========
Move |GetCanonicalEncodingNameByAliasName| to base/i18n

This CL moves the encoding-related utility method to base/i18n where
it belongs better and makes itself available directly to
embedder/contents.

It also helps the interface ContentBrowserClient do without
the method of the same name, which makes it simpler.

Review-Url: https://codereview.chromium.org/2578003002
==========

[commit-bot: I haz the power, 2017-01-04 06:27:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2017-01-04 06:29:54]

Description was changed from

==========
Move |GetCanonicalEncodingNameByAliasName| to base/i18n

This CL moves the encoding-related utility method to base/i18n where
it belongs better and makes itself available directly to
embedder/contents.

It also helps the interface ContentBrowserClient do without
the method of the same name, which makes it simpler.

Review-Url: https://codereview.chromium.org/2578003002
==========

to

==========
Move |GetCanonicalEncodingNameByAliasName| to base/i18n

This CL moves the encoding-related utility method to base/i18n where
it belongs better and makes itself available directly to
embedder/contents.

It also helps the interface ContentBrowserClient do without
the method of the same name, which makes it simpler.

Committed: https://crrev.com/79df88ae0437499f21dd82976ca24a0c6f2d11c7
Cr-Commit-Position: refs/heads/master@{#441326}
==========

[commit-bot: I haz the power, 2017-01-04 06:29:55]

Patchset 5 (id:??) landed as
https://crrev.com/79df88ae0437499f21dd82976ca24a0c6f2d11c7
Cr-Commit-Position: refs/heads/master@{#441326}