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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 6044006: Remove wstring from l10n_util. Part 3.... (Closed)

Created:
9 years, 12 months ago by Avi (use Gerrit)
Modified:
9 years, 7 months ago
CC:
chromium-reviews, ncarter (slow), idana, Alpha Left Google, Sergey Ulanov, Raghu Simha, Erik does not do reviews, tim (not reviewing), jam, Aaron Boodman, dmac, pam+watch_chromium.org, awong, garykac, Paweł Hajdan Jr., darin-cc_chromium.org, stuartmorgan+watch_chromium.org
Visibility:
Public.

Description

Remove wstring from l10n_util. Part 3. BUG=9911 TEST=no visible changes; all tests pass Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=70271

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 17
Unified diffs Side-by-side diffs Delta from patch set Stats (+205 lines, -157 lines) Patch
M chrome/browser/autocomplete/keyword_provider.cc View 1 3 chunks +19 lines, -16 lines 4 comments Download
M chrome/browser/autocomplete/search_provider.cc View 1 2 chunks +7 lines, -5 lines 0 comments Download
M chrome/browser/browser_browsertest.cc View 1 1 chunk +8 lines, -4 lines 0 comments Download
M chrome/browser/custom_home_pages_table_model.cc View 1 1 chunk +4 lines, -3 lines 0 comments Download
M chrome/browser/dom_ui/options/about_page_handler.cc View 1 4 chunks +34 lines, -37 lines 0 comments Download
M chrome/browser/extensions/extensions_startup.cc View 1 1 chunk +4 lines, -3 lines 2 comments Download
M chrome/browser/extensions/extensions_ui.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/extensions/pack_extension_job.h View 1 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/extensions/pack_extension_job.cc View 1 2 chunks +17 lines, -11 lines 4 comments Download
M chrome/browser/importer/importer_bridge.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/importer/importer_list.cc View 1 3 chunks +6 lines, -4 lines 0 comments Download
M chrome/browser/notifications/notification_exceptions_table_model.cc View 1 1 chunk +4 lines, -2 lines 0 comments Download
MM chrome/browser/notifications/notification_exceptions_table_model_unittest.cc View 1 2 chunks +11 lines, -5 lines 2 comments Download
M chrome/browser/omnibox_search_hint.cc View 1 2 chunks +3 lines, -2 lines 1 comment Download
M chrome/browser/plugin_exceptions_table_model.cc View 1 1 chunk +4 lines, -2 lines 0 comments Download
M chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc View 1 2 chunks +7 lines, -5 lines 0 comments Download
M chrome/browser/remoting/remoting_setup_flow.cc View 1 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/browser/remoting/setup_flow.cc View 1 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/browser/search_engines/template_url_table_model.cc View 1 2 chunks +11 lines, -7 lines 2 comments Download
M chrome/browser/ssl/ssl_manager.h View 1 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/ssl/ssl_manager.cc View 1 1 chunk +6 lines, -5 lines 0 comments Download
M chrome/browser/sync/sync_setup_flow.cc View 1 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/task_manager/task_manager_browsertest.cc View 1 3 chunks +6 lines, -6 lines 0 comments Download
M chrome/browser/task_manager/task_manager_resource_providers.cc View 1 5 chunks +14 lines, -12 lines 0 comments Download
M chrome/browser/ui/app_modal_dialogs/message_box_handler.cc View 1 2 chunks +14 lines, -9 lines 2 comments Download
M chrome/browser/ui/login/login_prompt.cc View 1 1 chunk +9 lines, -7 lines 0 comments Download
M chrome/browser/ui/toolbar/toolbar_model.cc View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 4 (0 generated)
Avi (use Gerrit)
9 years, 12 months ago (2010-12-29 02:19:31 UTC) #1
Avi (use Gerrit)
With lint fix and merge to today.
9 years, 12 months ago (2010-12-29 15:56:54 UTC) #2
viettrungluu
LGTM with mostly style nits (and one non-style nit). http://codereview.chromium.org/6044006/diff/26001/chrome/browser/autocomplete/keyword_provider.cc File chrome/browser/autocomplete/keyword_provider.cc (right): http://codereview.chromium.org/6044006/diff/26001/chrome/browser/autocomplete/keyword_provider.cc#newcode323 chrome/browser/autocomplete/keyword_provider.cc:323: ...
9 years, 12 months ago (2010-12-29 19:04:14 UTC) #3
Avi (use Gerrit)
9 years, 12 months ago (2010-12-29 19:40:48 UTC) #4 
http://codereview.chromium.org/6044006/diff/26001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/keyword_provider.cc (right):

http://codereview.chromium.org/6044006/diff/26001/chrome/browser/autocomplete...
chrome/browser/autocomplete/keyword_provider.cc:323:
WideToUTF16Hack(element->AdjustedShortNameForLocaleDirection()),
On 2010/12/29 19:04:15, viettrungluu wrote:
> Nit: this line (and the next) should probably be indented 2 more spaces.

Done.

http://codereview.chromium.org/6044006/diff/26001/chrome/browser/autocomplete...
chrome/browser/autocomplete/keyword_provider.cc:427: string16 keyword_desc(
On 2010/12/29 19:04:15, viettrungluu wrote:
> Was there a particular reason for getting rid of the static const? (I don't
> think it matters much either way, but was just wondering....)

Statics are evil. You're not allowed to use them.

http://codereview.chromium.org/6044006/diff/26001/chrome/browser/extensions/e...
File chrome/browser/extensions/extensions_startup.cc (right):

http://codereview.chromium.org/6044006/diff/26001/chrome/browser/extensions/e...
chrome/browser/extensions/extensions_startup.cc:27: crx_path,
output_private_key_path)));
On 2010/12/29 19:04:15, viettrungluu wrote:
> Ditto indentation nit (continuation indents are usually double indents).

Done.

http://codereview.chromium.org/6044006/diff/26001/chrome/browser/extensions/p...
File chrome/browser/extensions/pack_extension_job.cc (right):

http://codereview.chromium.org/6044006/diff/26001/chrome/browser/extensions/p...
chrome/browser/extensions/pack_extension_job.cc:91:
WideToUTF16(base::SysNativeMBToWide(crx_file.value()));
On 2010/12/29 19:04:15, viettrungluu wrote:
> Don't do this. Use WideToUTF16(..._file.ToWStringHack()) (which is horrible,
but
> at least we'll be able to search for it and fix it someday[*]). You'll also be
> able to get rid of the #ifdef.
> 
> [*] One day, I'll convince people that being able to display paths (and thus
> have a method to get them as Unicode) is important, even if strictly speaking
> impossible.

Done.

http://codereview.chromium.org/6044006/diff/26001/chrome/browser/extensions/p...
chrome/browser/extensions/pack_extension_job.cc:95: if (key_file.empty()) {
On 2010/12/29 19:04:15, viettrungluu wrote:
> key_file_string, I guess.

Done.

http://codereview.chromium.org/6044006/diff/26001/chrome/browser/notification...
File
chrome/browser/notifications/notification_exceptions_table_model_unittest.cc
(right):

http://codereview.chromium.org/6044006/diff/26001/chrome/browser/notification...
chrome/browser/notifications/notification_exceptions_table_model_unittest.cc:8:
#include "base/utf_string_conversions.h"
On 2010/12/29 19:04:15, viettrungluu wrote:
> Could you mark this with a "TODO: remove when conversions removed" or similar?

Done.

http://codereview.chromium.org/6044006/diff/26001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_table_model.cc (right):

http://codereview.chromium.org/6044006/diff/26001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_table_model.cc:195: } else {
On 2010/12/29 19:04:15, viettrungluu wrote:
> Nit: else unnecessary. (Style guide prefers |if { ... return ...; } ... return
> ...;| to |if { ... return ...; } else { ... return ...; }|.)

Done.

http://codereview.chromium.org/6044006/diff/26001/chrome/browser/ui/app_modal...
File chrome/browser/ui/app_modal_dialogs/message_box_handler.cc (right):

http://codereview.chromium.org/6044006/diff/26001/chrome/browser/ui/app_modal...
chrome/browser/ui/app_modal_dialogs/message_box_handler.cc:86:
l10n_util::GetStringUTF16(IDS_BEFOREUNLOAD_MESSAGEBOX_TITLE)),
On 2010/12/29 19:04:15, viettrungluu wrote:
> Ditto indentation nit.

Done.

Powered by Google App Engine
This is Rietveld 408576698