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

Issue 21414: We will sort the encoding menu items according to current used UI language ex... (Closed)

Created:
11 years, 10 months ago by Johnny(Jianning) Ding
Modified:
9 years, 7 months ago
Reviewers:
jungshik at Google
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

We will sort the encoding menu items according to current used UI language except the UTF-8 encoding and the recently selectd encodings. For details, see Fix bug http://code.google.com/p/chromium/issues/detail?id=7647 BUG=7647 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=10077

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Total comments: 34

Patch Set 4 : '' #

Patch Set 5 : '' #

Patch Set 6 : '' #

Patch Set 7 : '' #

Total comments: 12

Patch Set 8 : '' #

Patch Set 9 : '' #

Total comments: 4

Patch Set 10 : '' #

Patch Set 11 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+234 lines, -124 lines) Patch
M chrome/browser/character_encoding.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +24 lines, -5 lines 0 comments Download
M chrome/browser/character_encoding.cc View 1 2 3 4 5 6 7 8 9 10 11 chunks +67 lines, -37 lines 0 comments Download
M chrome/browser/encoding_menu_controller_delegate.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +13 lines, -13 lines 0 comments Download
M chrome/browser/spellchecker.cc View 1 2 3 4 5 6 7 8 9 10 6 chunks +7 lines, -6 lines 0 comments Download
M chrome/common/l10n_util.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +73 lines, -0 lines 0 comments Download
M chrome/common/l10n_util.cc View 1 2 3 4 5 6 7 8 9 10 5 chunks +41 lines, -56 lines 0 comments Download
M chrome/test/automated_ui_tests/automated_ui_tests.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +9 lines, -7 lines 0 comments Download

Messages

Total messages: 10 (0 generated)
Johnny(Jianning) Ding
11 years, 10 months ago (2009-02-17 16:32:38 UTC) #1
jungshik at Google
Thanks a lot for the patch. Please, see comments below. http://codereview.chromium.org/21414/diff/28/34 File chrome/browser/character_encoding.cc (right): http://codereview.chromium.org/21414/diff/28/34#newcode206 ...
11 years, 10 months ago (2009-02-17 20:43:09 UTC) #2
Johnny(Jianning) Ding
Hi Jungshik, I have all your comments, please take another look. Thanks! http://codereview.chromium.org/21414/diff/28/34 File chrome/browser/character_encoding.cc ...
11 years, 10 months ago (2009-02-18 07:29:20 UTC) #3
jungshik at Google
thank you for taking care of my comments. here are a few more (mostly nit-ish). ...
11 years, 10 months ago (2009-02-18 21:31:43 UTC) #4
jungshik at Google
http://codereview.chromium.org/21414/diff/2001/3007 File chrome/common/l10n_util.h (right): http://codereview.chromium.org/21414/diff/2001/3007#newcode253 Line 253: Locale loc(WideToUTF8(locale).c_str()); nit: WideToASCII
11 years, 10 months ago (2009-02-18 21:32:50 UTC) #5
jungshik at Google
http://codereview.chromium.org/21414/diff/2001/3005 File chrome/browser/character_encoding.cc (right): http://codereview.chromium.org/21414/diff/2001/3005#newcode44 Line 44: { IDC_ENCODING_EUCJP, L"EUC-JP", IDS_ENCODING_JAPANESE }, while you're at ...
11 years, 10 months ago (2009-02-18 21:46:01 UTC) #6
Johnny(Jianning) Ding
Thanks for your comments, Jungshik. I have fix them. Please have another look. http://codereview.chromium.org/21414/diff/2001/3005 File ...
11 years, 10 months ago (2009-02-19 16:42:22 UTC) #7
jungshik at Google
LGTM ! Please, take care of minor nits when checking this in. On 2009/02/19 16:42:22, ...
11 years, 10 months ago (2009-02-19 17:51:41 UTC) #8
jungshik at Google
LGTM! ooops. I meant to send this along with LGTM. You're good to go with ...
11 years, 10 months ago (2009-02-19 17:53:09 UTC) #9
Johnny(Jianning) Ding
11 years, 10 months ago (2009-02-20 04:24:38 UTC) #10
all done.

http://codereview.chromium.org/21414/diff/1071/1076
File chrome/browser/character_encoding.cc (right):

http://codereview.chromium.org/21414/diff/1071/1076#newcode144
Line 144: IDC_ENCODING_EUCJP,
On 2009/02/19 17:53:09, Jungshik Shin wrote:
> oops. I should have told you to switch ISO-2022-JP and EUC-JP in this array.
Can
> you switch them when you check in? thanks.

Done.

http://codereview.chromium.org/21414/diff/1071/1077
File chrome/common/l10n_util.cc (right):

http://codereview.chromium.org/21414/diff/1071/1077#newcode209
Line 209: const std::wstring& rhs) {
On 2009/02/19 17:53:09, Jungshik Shin wrote:
> nit: I prefer CompareStringWithCollator to GetCollationResult, but it's up to
> you. 

Done.

Powered by Google App Engine
This is Rietveld 408576698