Enable warning 4389 as an error on windows builds. This will make...

Enable warning 4389 as an error on windows builds.  This will make
windows builds more similar to linux/mac, which already treat signed/
unsigned equality comparisons as warnings (and hence errors).

BUG=44471
TEST=none

[Mike Belshe, 2010-05-19 16:04:01]

Since this is mostly a bunch of casting, you seemed like the appropriate
reviewer :-)

[Mike Belshe, 2010-05-19 17:07:20]

http://codereview.chromium.org/2081007/diff/11002/30033
File net/tools/dump_cache/cache_dumper.cc (left):

http://codereview.chromium.org/2081007/diff/11002/30033#oldcode91
net/tools/dump_cache/cache_dumper.cc:91: CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL,
0);
nit: this is a ride-along-fix.

[Peter Kasting, 2010-05-19 23:17:36]

There are an enormous number of nits below.  They can be broken down into a few
categories:

* Change copyright dates on modified files to 2010.

* 0U is much more common than 0u.  If you really want to use 0u instead, you can
ignore these, I'm merely suggesting this for consistency.

* For CHECKs and the like that need to e.g. cast NULL, we normally use
"CHECK(foo != NULL)" instead of using a cast.  Again, this is up to you,
although on this one I feel more strongly that the other form is more readable.

* Various ridealong fixes.  In most cases, I'd like to see these happen even if
you don't do them now, and I'd be happy to do some myself if you want.

* Other.

http://codereview.chromium.org/2081007/diff/37002/18002
File app/os_exchange_data_provider_win.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18002#newcode1
app/os_exchange_data_provider_win.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18002#newcode87
app/os_exchange_data_provider_win.cc:87: int cursor_;
Ridealong fix: Changing this to size_t will remove a few static_casts.

http://codereview.chromium.org/2081007/diff/37002/18002#newcode131
app/os_exchange_data_provider_win.cc:131: CloneFormatEtc(contents_.at(cursor_),
&elements_array[index]);
Ridealong fix: contents_[cursor_] would be both more typical and faster (not
that that matters).

http://codereview.chromium.org/2081007/diff/37002/18003
File app/win/window_impl.h (right):

http://codereview.chromium.org/2081007/diff/37002/18003#newcode1
app/win/window_impl.h:1: // Copyright (c) 2009 The Chromium Authors. All rights
reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18003#newcode68
app/win/window_impl.h:68: DCHECK_EQ((class_style & CS_GLOBALCLASS), 0u);
Nit: "0U" is the significantly-more-common form in Chrome code.
Ridealong fix: (EXPECTED, ACTUAL).

http://codereview.chromium.org/2081007/diff/37002/18004
File base/crypto/scoped_capi_types.h (right):

http://codereview.chromium.org/2081007/diff/37002/18004#newcode67
base/crypto/scoped_capi_types.h:67: CHECK_EQ(static_cast<CAPIHandle>(NULL),
handle_);
Nit: CHECK(handle_ == NULL) is both more common and IMO more readable.

http://codereview.chromium.org/2081007/diff/37002/18005
File base/event_trace_consumer_win_unittest.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18005#newcode1
base/event_trace_consumer_win_unittest.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18005#newcode235
base/event_trace_consumer_win_unittest.cc:235: ASSERT_NE(0u,
TestConsumer::events_.size());
Nit: "0U" is the significantly-more-common form in Chrome code.

http://codereview.chromium.org/2081007/diff/37002/18006
File base/event_trace_controller_win_unittest.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18006#newcode1
base/event_trace_controller_win_unittest.cc:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18006#newcode65
base/event_trace_controller_win_unittest.cc:65: EXPECT_NE(0u,
p->Wnode.BufferSize);
Nit: "0U" is the significantly-more-common form in Chrome code.  (5 places)

http://codereview.chromium.org/2081007/diff/37002/18007
File base/event_trace_provider_win_unittest.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18007#newcode1
base/event_trace_provider_win_unittest.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18007#newcode91
base/event_trace_provider_win_unittest.cc:91:
EXPECT_NE(static_cast<ULONG>(ERROR_SUCCESS), provider.Register());
Odd, why does this need a cast but things like ASSERT_EQ() in the function above
don't?

Nit: EXPECT_FALSE(provider.Register() == ERROR_SUCCESS) might be clearer?  No
real preference.

http://codereview.chromium.org/2081007/diff/37002/18008
File base/file_util_win.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18008#newcode661
base/file_util_win.cc:661: if (::ReadFile(file, data, size, &read, NULL) &&
Ridealong fix: The following is shorter.  I don't know if you can omit the cast
on the return without triggering a warning, but if so, I'd do it:

if (::ReadFile(...) && static_cast<int>(read) == size)
  return read;
return -1;

You could even go further:

return (::ReadFile(...) && static_cast<int>(read) == size) ? read : -1;

http://codereview.chromium.org/2081007/diff/37002/18008#newcode689
base/file_util_win.cc:689: return static_cast<int>(written);
Ridealong fix: Similar to above, I'd omit the cast on the return here if
possible.

http://codereview.chromium.org/2081007/diff/37002/18009
File base/iat_patch.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18009#newcode1
base/iat_patch.cc:1: // Copyright (c) 2006-2008 The Chromium Authors. All rights
reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18009#newcode181
base/iat_patch.cc:181: DCHECK_EQ(static_cast<DWORD>(NO_ERROR), error);
Nit: DCHECK(error == NO_ERROR) is both more common and IMO more readable.

http://codereview.chromium.org/2081007/diff/37002/18010
File base/lock_impl_win.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18010#newcode1
base/lock_impl_win.cc:1: // Copyright (c) 2006-2008 The Chromium Authors. All
rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18010#newcode32
base/lock_impl_win.cc:32: DCHECK_NE(owning_thread_id_, 0u);
Nit: "0U" is the significantly-more-common form in Chrome code.  (2 places)

Ridealong fix: (EXPECTED, ACTUAL) (2 places)

http://codereview.chromium.org/2081007/diff/37002/18011
File base/scoped_bstr_win_unittest.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18011#newcode1
base/scoped_bstr_win_unittest.cc:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18011#newcode12
base/scoped_bstr_win_unittest.cc:12: uint32 test1_len = arraysize(kTestString1)
- 1;
Ridealong fix: These should be size_t rather than uint32.

http://codereview.chromium.org/2081007/diff/37002/18011#newcode18
base/scoped_bstr_win_unittest.cc:18: EXPECT_TRUE(b.Length() == 0);
Ridealong fix: A lot of these could be EXPECT_EQ(0U, ...); (many places)

http://codereview.chromium.org/2081007/diff/37002/18011#newcode41
base/scoped_bstr_win_unittest.cc:41: EXPECT_TRUE(lstrcmpW(b2, kTestString1) ==
0);
Ridealong fix: I don't understand why "lstrcmpW" instead of "lstrcmp", but
"lstrlen" (below) instead of "lstrlenW".  Seems like these should be consistent.

http://codereview.chromium.org/2081007/diff/37002/18011#newcode55
base/scoped_bstr_win_unittest.cc:55: EXPECT_TRUE(lstrlen(b2) ==
static_cast<int>(test1_len));
Nit: It seems like if we must cast, we ought to cast lstrlen()'s result to a
size_t.

http://codereview.chromium.org/2081007/diff/37002/18012
File base/thread_local.h (right):

http://codereview.chromium.org/2081007/diff/37002/18012#newcode1
base/thread_local.h:1: // Copyright (c) 2006-2008 The Chromium Authors. All
rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18012#newcode62
base/thread_local.h:62: typedef unsigned long SlotType;
You may want to get someone familiar with this to review this particular change;
I don't know the ramifications.

Perhaps SlotType should be a size_t or something.

http://codereview.chromium.org/2081007/diff/37002/18014
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18014#newcode1
chrome/app/chrome_dll_main.cc:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18014#newcode528
chrome/app/chrome_dll_main.cc:528: DCHECK_NE(browser_pid, 0u);
Nit: "0U" is the significantly-more-common form in Chrome code.
Ridealong fix: (EXPECTED, ACTUAL).

http://codereview.chromium.org/2081007/diff/37002/18015
File chrome/browser/automation/ui_controls_win.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18015#newcode1
chrome/browser/automation/ui_controls_win.cc:1: // Copyright (c) 2006-2008 The
Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18015#newcode231
chrome/browser/automation/ui_controls_win.cc:231: UINT rv = ::SendInput(i,
input, sizeof(INPUT));
Ridealong simplification:

if (::SendInput(...) != i)
  return false;

http://codereview.chromium.org/2081007/diff/37002/18016
File chrome/browser/debugger/devtools_remote_listen_socket_unittest.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18016#newcode1
chrome/browser/debugger/devtools_remote_listen_socket_unittest.cc:1: //
Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18017
File chrome/browser/extensions/extension_uitest.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18017#newcode1
chrome/browser/extensions/extension_uitest.cc:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18017#newcode158
chrome/browser/extensions/extension_uitest.cc:158: EXPECT_EQ(result,
"tabs.remove");
Ridealong fix: (EXPECTED, ACTUAL) (several places)

http://codereview.chromium.org/2081007/diff/37002/18017#newcode160
chrome/browser/extensions/extension_uitest.cc:160: result = "";
Ridealong fix: result.clear();

http://codereview.chromium.org/2081007/diff/37002/18017#newcode162
chrome/browser/extensions/extension_uitest.cc:162: EXPECT_NE(result, "");
Ridealong fix: EXPECT_FALSE(result.empty());

http://codereview.chromium.org/2081007/diff/37002/18017#newcode164
chrome/browser/extensions/extension_uitest.cc:164: int callback_id = 0xBAADF00D;
Nit: If we just used "123456789", we could avoid the cast below.

http://codereview.chromium.org/2081007/diff/37002/18017#newcode249
chrome/browser/extensions/extension_uitest.cc:249: EXPECT_NE(args.find("42"),
std::string::npos);
Ridealong fix: (EXPECTED, ACTUAL)

http://codereview.chromium.org/2081007/diff/37002/18018
File chrome/browser/importer/importer_unittest.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18018#newcode125
chrome/browser/importer/importer_unittest.cc:125: const int path_size;
Ridealong fix part 1: Changing this to size_t, along with the ridealong fix
below, will allow us to avoid casting.

http://codereview.chromium.org/2081007/diff/37002/18018#newcode187
chrome/browser/importer/importer_unittest.cc:187: for (int k = 0; k <
list[i].path_size; ++k)
Ridealong fix part 2: int k -> size_t k

http://codereview.chromium.org/2081007/diff/37002/18019
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18019#newcode1
chrome/browser/sync/syncable/syncable.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18019#newcode588
chrome/browser/sync/syncable/syncable.cc:588: DCHECK(1 == num_erased);
Ridealong fix: DCHECK_EQ (2 places)

http://codereview.chromium.org/2081007/diff/37002/18019#newcode594
chrome/browser/sync/syncable/syncable.cc:594:
DCHECK(!entry->ref(UNIQUE_CLIENT_TAG).empty() == (num_erased > 0));
Nit: Maybe

DCHECK_EQ(entry->ref(UNIQUE_CLIENT_TAG).empty(), !num_erased);

Or

bool erased_something = (kernel_->client_tag_index->erase(entry) != 0);
DCHECK_EQ(!entry->ref(UNIQUE_CLIENT_TAG).empty(), erased_something);

http://codereview.chromium.org/2081007/diff/37002/18020
File chrome/browser/views/select_profile_dialog.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18020#newcode1
chrome/browser/views/select_profile_dialog.cc:1: // Copyright (c) 2006-2008 The
Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18020#newcode86
chrome/browser/views/select_profile_dialog.cc:86: int index =
profile_combobox_->selected_item();
Ridealong fix: It is probably too much to ask you to go change the combobox
methods to use size_ts instead of ints.  But we could at least make this a bit
simpler by copying the GetItemAt() fix I suggest below, and a few other tweaks:

size_t index = profile_combobox_->selected_item();
DCHECK_LE(index, profiles_.size());
if (index == profiles_.size())
  NewProfileDialog::RunDialog();
else
  UserDataManager::Get()->LaunchChromeForProfile(profiles_[index]);
return true;

http://codereview.chromium.org/2081007/diff/37002/18020#newcode119
chrome/browser/views/select_profile_dialog.cc:119: DCHECK(index >= 0 && index <=
static_cast<int>(profiles_.size()));
Ridealong fix: Using a temp would avoid both casts and simplify the DCHECK:

size_t index_size_t = index;
DCHECK_LE(index_size_t, profiles_.size());
return (index_size_t == profiles_.size()) ?
    ...;

http://codereview.chromium.org/2081007/diff/37002/18021
File chrome_frame/chrome_active_document.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18021#newcode853
chrome_frame/chrome_active_document.cc:853: if
(SUCCEEDED(travel_log->GetTravelEntry(browser_service, TLOG_BACK, NULL))) {
Ridealong fix: This file can't make up its mind about {} on one-line conditional
bodies.  It seems like we should just remove the ones that are present.

http://codereview.chromium.org/2081007/diff/37002/18021#newcode876
chrome_frame/chrome_active_document.cc:876: if (cmd ==
static_cast<UINT>(context_menu_IDC_BACK)) {
Nit: Instead of this, why not just make these three constants UINTs in
chrome_imported_resources.*?

http://codereview.chromium.org/2081007/diff/37002/18021#newcode904
chrome_frame/chrome_active_document.cc:904: if (frame_cmd_target)
Ridealong fix: {}

http://codereview.chromium.org/2081007/diff/37002/18022
File chrome_frame/chrome_frame_activex.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18022#newcode131
chrome_frame/chrome_frame_activex.cc:131: DCHECK_EQ(0u, onmessage_.size());
Nit: "0U" is the significantly-more-common form in Chrome code.  (5 places)

http://codereview.chromium.org/2081007/diff/37002/18023
File chrome_frame/chrome_launcher.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18023#newcode1
chrome_frame/chrome_launcher.cc:1: // Copyright (c) 2009 The Chromium Authors.
All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18023#newcode68
chrome_frame/chrome_launcher.cc:68: int num_sanitized_switches = 0;
Ridealong fix: Changing this to a size_t would obviate the need to cast below.

http://codereview.chromium.org/2081007/diff/37002/18024
File chrome_frame/function_stub_unittest.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18024#newcode124
chrome_frame/function_stub_unittest.cc:124: EXPECT_NE(0u,
::VirtualQuery(stub_->code(), &info, sizeof(info)));
Nit: "0U" is the significantly-more-common form in Chrome code.  (2 places)

http://codereview.chromium.org/2081007/diff/37002/18025
File chrome_frame/html_utils.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18025#newcode1
chrome_frame/html_utils.cc:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18025#newcode351
chrome_frame/html_utils.cc:351: return "";
Ridealong fix: "" -> std::string()

http://codereview.chromium.org/2081007/diff/37002/18025#newcode352
chrome_frame/html_utils.cc:352: } else {
Ridealong fix: No else after return

http://codereview.chromium.org/2081007/diff/37002/18025#newcode353
chrome_frame/html_utils.cc:353: DCHECK(ret.length() ==
static_cast<size_t>(lstrlenA(ret.c_str())));
Nit: If we really want to do this, it seems like the following is clearer:

DCHECK_EQ(std::string::npos, ret.find('\0'));

http://codereview.chromium.org/2081007/diff/37002/18025#newcode360
chrome_frame/html_utils.cc:360: net::HttpUtil::HeadersIterator it(
Ridealong fix: This function could be made significantly simpler using
GetSpecificHeader() out of net_util.h instead of iterating through headers
ourselves.

http://codereview.chromium.org/2081007/diff/37002/18026
File chrome_frame/test/exception_barrier_unittest.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18026#newcode32
chrome_frame/test/exception_barrier_unittest.cc:32: ASSERT_NE(0u,
::VirtualQuery(&info, &info, sizeof(info)));
Nit: "0U" is the significantly-more-common form in Chrome code.  (3 places)

http://codereview.chromium.org/2081007/diff/37002/18027
File chrome_frame/test/html_util_unittests.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18027#newcode1
chrome_frame/test/html_util_unittests.cc:1: // Copyright (c) 2006-2009 The
Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18027#newcode295
chrome_frame/test/html_util_unittests.cc:295: EXPECT_NE(0u, ua.length());
Nit: "0U" is the significantly-more-common form in Chrome code.  (2 places)

http://codereview.chromium.org/2081007/diff/37002/18028
File chrome_frame/test/perf/chrome_frame_perftest.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18028#newcode1
chrome_frame/test/perf/chrome_frame_perftest.cc:1: // Copyright (c) 2006-2009
The Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18029
File chrome_frame/test/util_unittests.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18029#newcode1
chrome_frame/test/util_unittests.cc:1: // Copyright (c) 2006-2009 The Chromium
Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18029#newcode27
chrome_frame/test/util_unittests.cc:27: EXPECT_NE(high, 0u);
Nit: "0U" is the significantly-more-common form in Chrome code.  (2 places)
Ridealong fix: (EXPECTED, ACTUAL).  (2 places)

http://codereview.chromium.org/2081007/diff/37002/18030
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18030#newcode1
chrome_frame/urlmon_url_request.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18030#newcode99
chrome_frame/urlmon_url_request.cc:99: if (binding_) {
Ridealong fix: Some places in this file use braces on one-line bodies and some
don't... we should perhaps just remove all the ones that do.

http://codereview.chromium.org/2081007/diff/37002/18030#newcode122
chrome_frame/urlmon_url_request.cc:122: DCHECK_EQ(0u, pending_read_size_);
Nit: "0U" is the significantly-more-common form in Chrome code.

http://codereview.chromium.org/2081007/diff/37002/18030#newcode1127
chrome_frame/urlmon_url_request.cc:1127: DCHECK_EQ(1u, n);
Nit: "1U" is the significantly-more-common form in Chrome code.

http://codereview.chromium.org/2081007/diff/37002/18031
File gfx/icon_util.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18031#newcode1
gfx/icon_util.cc:1: // Copyright (c) 2006-2008 The Chromium Authors. All rights
reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18031#newcode179
gfx/icon_util.cc:179: DCHECK_EQ((*p & 0xff000000), 0u);
Nit: "0U" is the significantly-more-common form in Chrome code.
Ridealong fix: (EXPECTED, ACTUAL).

http://codereview.chromium.org/2081007/diff/37002/18031#newcode224
gfx/icon_util.cc:224: int bitmap_count = static_cast<int>(bitmaps.size());
Ridealong fix: We could save a number of different casts in this file by e.g.
using a size_t here, using (and returning) a size_t in
ComputeIconFileBufferSize(), etc.

http://codereview.chromium.org/2081007/diff/37002/18032
File net/base/listen_socket_unittest.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18032#newcode1
net/base/listen_socket_unittest.cc:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18033
File net/base/network_change_notifier_win.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18033#newcode1
net/base/network_change_notifier_win.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18033#newcode54
net/base/network_change_notifier_win.cc:54:
CHECK_EQ(static_cast<DWORD>(ERROR_IO_PENDING), ret);
Nit: CHECK(ret == ERROR_IO_PENDING) is both more common and IMO more readable.

http://codereview.chromium.org/2081007/diff/37002/18034
File net/tools/dump_cache/cache_dumper.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18034#newcode1
net/tools/dump_cache/cache_dumper.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18034#newcode96
net/tools/dump_cache/cache_dumper.cc:96: return entry_ != INVALID_HANDLE_VALUE;
This should be "return true;" now; or else:

if (entry_ == INVALID_HANDLE_VALUE)
  wprintf(L"CreateFileW (%s) failed: %d\n", file.c_str(), GetLastError());
return entry_ != INVALID_HANDLE_VALUE;

http://codereview.chromium.org/2081007/diff/37002/18034#newcode150
net/tools/dump_cache/cache_dumper.cc:150: int len;
Ridealong fix: If you make this a size_t, you don't need the cast below.

http://codereview.chromium.org/2081007/diff/37002/18035
File net/tools/dump_cache/upgrade.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18035#newcode1
net/tools/dump_cache/upgrade.cc:1: // Copyright (c) 2008 The Chromium Authors.
All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18035#newcode47
net/tools/dump_cache/upgrade.cc:47: int32 buffer_bytes;
Ridealong fix: I believe by making buffer_bytes be a uint32, you can remove a
number of signedness conversions (and maybe some casts) in this file.  You'll
also need to change DoReadData() and DoGetKey() to take size_ts, DWORDs, or
something similar.

http://codereview.chromium.org/2081007/diff/37002/18036
File sandbox/src/file_policy_test.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18036#newcode1
sandbox/src/file_policy_test.cc:1: // Copyright (c) 2006-2010 The Chromium
Authors. All rights reserved.
Nit: Just 2010

http://codereview.chromium.org/2081007/diff/37002/18036#newcode387
sandbox/src/file_policy_test.cc:387: ASSERT_NE(::GetTempPath(MAX_PATH,
temp_directory), 0u);
Nit: "0U" is the significantly-more-common form in Chrome code.  (Many places)
Ridealong fix: (EXPECTED, ACTUAL).  (Many places)

http://codereview.chromium.org/2081007/diff/37002/18037
File sandbox/src/interception_unittest.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18037#newcode1
sandbox/src/interception_unittest.cc:1: // Copyright (c) 2006-2010 The Chromium
Authors. All rights reserved.
Nit: Just 2010

http://codereview.chromium.org/2081007/diff/37002/18037#newcode40
sandbox/src/interception_unittest.cc:40: ASSERT_NE(0u, wcslen(dll->dll_name));
Nit: "0U" is the significantly-more-common form in Chrome code.  (Many places)

http://codereview.chromium.org/2081007/diff/37002/18038
File sandbox/src/job_unittest.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18038#newcode1
sandbox/src/job_unittest.cc:1: // Copyright (c) 2006-2008 The Chromium Authors.
All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18038#newcode23
sandbox/src/job_unittest.cc:23: ASSERT_NE(reinterpret_cast<HANDLE>(NULL),
job_handle);
Nit: ASSERT(job_handle != NULL) is both more common and IMO more readable. 
(Many places).

http://codereview.chromium.org/2081007/diff/37002/18039
File sandbox/src/restricted_token_unittest.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18039#newcode1
sandbox/src/restricted_token_unittest.cc:1: // Copyright (c) 2006-2008 The
Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/37002/18040
File sandbox/src/win_utils_unittest.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18040#newcode17
sandbox/src/win_utils_unittest.cc:17: ASSERT_NE(::GetTempPath(MAX_PATH,
temp_directory), 0u);
Nit: "0U" is the significantly-more-common form in Chrome code.  (2 places)
Ridealong fix: (EXPECTED, ACTUAL).  (2 places)

http://codereview.chromium.org/2081007/diff/37002/18040#newcode40
sandbox/src/win_utils_unittest.cc:40: EXPECT_TRUE(INVALID_HANDLE_VALUE != dir);
Ridealong fix: EXPECT_NE(INVALID_HANDLE_VALUE, dir);

http://codereview.chromium.org/2081007/diff/37002/18041
File views/controls/table/table_view.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18041#newcode373
views/controls/table/table_view.cc:373: DCHECK_EQ(0u,
all_columns_.count(col.id));
Nit: "0U" is the significantly-more-common form in Chrome code.

[Mike Belshe, 2010-05-20 22:04:09]

http://codereview.chromium.org/2081007/diff/37002/18002
File app/os_exchange_data_provider_win.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18002#newcode87
app/os_exchange_data_provider_win.cc:87: int cursor_;
On 2010/05/19 23:17:37, Peter Kasting wrote:
> Ridealong fix: Changing this to size_t will remove a few static_casts.

Done.

http://codereview.chromium.org/2081007/diff/37002/18002#newcode131
app/os_exchange_data_provider_win.cc:131: CloneFormatEtc(contents_.at(cursor_),
&elements_array[index]);
skip
On 2010/05/19 23:17:37, Peter Kasting wrote:
> Ridealong fix: contents_[cursor_] would be both more typical and faster (not
> that that matters).

http://codereview.chromium.org/2081007/diff/37002/18004
File base/crypto/scoped_capi_types.h (right):

http://codereview.chromium.org/2081007/diff/37002/18004#newcode67
base/crypto/scoped_capi_types.h:67: CHECK_EQ(static_cast<CAPIHandle>(NULL),
handle_);
On 2010/05/19 23:17:37, Peter Kasting wrote:
> Nit: CHECK(handle_ == NULL) is both more common and IMO more readable.

OK.

http://codereview.chromium.org/2081007/diff/37002/18007
File base/event_trace_provider_win_unittest.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18007#newcode91
base/event_trace_provider_win_unittest.cc:91:
EXPECT_NE(static_cast<ULONG>(ERROR_SUCCESS), provider.Register());
Good observation.  I verified the discrepancy does exist.

It's the template usage which is causing the error; it appears that the ASSERT_*
functions don't use the templates quite the way the EXPECT_* functions do.

This indicates that there is a chance our EXPECT_* macros can be reworked to
avoid this silly casting problem altogether.


On 2010/05/19 23:17:37, Peter Kasting wrote:
> Odd, why does this need a cast but things like ASSERT_EQ() in the function
above
> don't?
> 
> Nit: EXPECT_FALSE(provider.Register() == ERROR_SUCCESS) might be clearer?  No
> real preference.

ok - except used EXPECT_TRUE(x != NULL);

http://codereview.chromium.org/2081007/diff/37002/18008
File base/file_util_win.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18008#newcode661
base/file_util_win.cc:661: if (::ReadFile(file, data, size, &read, NULL) &&
On 2010/05/19 23:17:37, Peter Kasting wrote:
> Ridealong fix: The following is shorter.  I don't know if you can omit the
cast
> on the return without triggering a warning, but if so, I'd do it:
> 
> if (::ReadFile(...) && static_cast<int>(read) == size)
>   return read;
> return -1;
> 
> You could even go further:
> 
> return (::ReadFile(...) && static_cast<int>(read) == size) ? read : -1;

Done.

http://codereview.chromium.org/2081007/diff/37002/18008#newcode689
base/file_util_win.cc:689: return static_cast<int>(written);
On 2010/05/19 23:17:37, Peter Kasting wrote:
> Ridealong fix: Similar to above, I'd omit the cast on the return here if
> possible.

Done.

http://codereview.chromium.org/2081007/diff/37002/18009
File base/iat_patch.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18009#newcode181
base/iat_patch.cc:181: DCHECK_EQ(static_cast<DWORD>(NO_ERROR), error);
On 2010/05/19 23:17:37, Peter Kasting wrote:
> Nit: DCHECK(error == NO_ERROR) is both more common and IMO more readable.

Done.

http://codereview.chromium.org/2081007/diff/37002/18015
File chrome/browser/automation/ui_controls_win.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18015#newcode231
chrome/browser/automation/ui_controls_win.cc:231: UINT rv = ::SendInput(i,
input, sizeof(INPUT));
On 2010/05/19 23:17:37, Peter Kasting wrote:
> Ridealong simplification:
> 
> if (::SendInput(...) != i)
>   return false;

Done.

http://codereview.chromium.org/2081007/diff/37002/18017
File chrome/browser/extensions/extension_uitest.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18017#newcode249
chrome/browser/extensions/extension_uitest.cc:249: EXPECT_NE(args.find("42"),
std::string::npos);
On 2010/05/19 23:17:37, Peter Kasting wrote:
> Ridealong fix: (EXPECTED, ACTUAL)

Done.

http://codereview.chromium.org/2081007/diff/37002/18018
File chrome/browser/importer/importer_unittest.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18018#newcode125
chrome/browser/importer/importer_unittest.cc:125: const int path_size;
nice!

On 2010/05/19 23:17:37, Peter Kasting wrote:
> Ridealong fix part 1: Changing this to size_t, along with the ridealong fix
> below, will allow us to avoid casting.

http://codereview.chromium.org/2081007/diff/37002/18018#newcode187
chrome/browser/importer/importer_unittest.cc:187: for (int k = 0; k <
list[i].path_size; ++k)
On 2010/05/19 23:17:37, Peter Kasting wrote:
> Ridealong fix part 2: int k -> size_t k

Done.

http://codereview.chromium.org/2081007/diff/37002/18019
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18019#newcode588
chrome/browser/sync/syncable/syncable.cc:588: DCHECK(1 == num_erased);
On 2010/05/19 23:17:37, Peter Kasting wrote:
> Ridealong fix: DCHECK_EQ (2 places)

Done.

http://codereview.chromium.org/2081007/diff/37002/18019#newcode594
chrome/browser/sync/syncable/syncable.cc:594:
DCHECK(!entry->ref(UNIQUE_CLIENT_TAG).empty() == (num_erased > 0));
On 2010/05/19 23:17:37, Peter Kasting wrote:
> Nit: Maybe
> 
> DCHECK_EQ(entry->ref(UNIQUE_CLIENT_TAG).empty(), !num_erased);
> 
> Or
> 
> bool erased_something = (kernel_->client_tag_index->erase(entry) != 0);
> DCHECK_EQ(!entry->ref(UNIQUE_CLIENT_TAG).empty(), erased_something);

Done.

http://codereview.chromium.org/2081007/diff/37002/18020
File chrome/browser/views/select_profile_dialog.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18020#newcode86
chrome/browser/views/select_profile_dialog.cc:86: int index =
profile_combobox_->selected_item();
On 2010/05/19 23:17:37, Peter Kasting wrote:
> Ridealong fix: It is probably too much to ask you to go change the combobox
> methods to use size_ts instead of ints.  But we could at least make this a bit
> simpler by copying the GetItemAt() fix I suggest below, and a few other
tweaks:

I fear this just moves the casts to the callers of these functions :-)

> 
> size_t index = profile_combobox_->selected_item();
> DCHECK_LE(index, profiles_.size());
> if (index == profiles_.size())
>   NewProfileDialog::RunDialog();
> else
>   UserDataManager::Get()->LaunchChromeForProfile(profiles_[index]);
> return true;

Done

http://codereview.chromium.org/2081007/diff/37002/18020#newcode119
chrome/browser/views/select_profile_dialog.cc:119: DCHECK(index >= 0 && index <=
static_cast<int>(profiles_.size()));
On 2010/05/19 23:17:37, Peter Kasting wrote:
> Ridealong fix: Using a temp would avoid both casts and simplify the DCHECK:
> 
> size_t index_size_t = index;
> DCHECK_LE(index_size_t, profiles_.size());
> return (index_size_t == profiles_.size()) ?
>     ...;

Done.

http://codereview.chromium.org/2081007/diff/37002/18021
File chrome_frame/chrome_active_document.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18021#newcode876
chrome_frame/chrome_active_document.cc:876: if (cmd ==
static_cast<UINT>(context_menu_IDC_BACK)) {
On 2010/05/19 23:17:37, Peter Kasting wrote:
> Nit: Instead of this, why not just make these three constants UINTs in
> chrome_imported_resources.*?

This looked like a good idea, so I did try.  But then I had to put UINTs into
the header file, and it chokes because we don't include the windows headers
there.  I don't want to try to sort that out.

http://codereview.chromium.org/2081007/diff/37002/18023
File chrome_frame/chrome_launcher.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18023#newcode68
chrome_frame/chrome_launcher.cc:68: int num_sanitized_switches = 0;
On 2010/05/19 23:17:37, Peter Kasting wrote:
> Ridealong fix: Changing this to a size_t would obviate the need to cast below.

Done.

http://codereview.chromium.org/2081007/diff/37002/18025
File chrome_frame/html_utils.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18025#newcode351
chrome_frame/html_utils.cc:351: return "";
On 2010/05/19 23:17:37, Peter Kasting wrote:
> Ridealong fix: "" -> std::string()

Done.

http://codereview.chromium.org/2081007/diff/37002/18025#newcode352
chrome_frame/html_utils.cc:352: } else {
On 2010/05/19 23:17:37, Peter Kasting wrote:
> Ridealong fix: No else after return

Done.

http://codereview.chromium.org/2081007/diff/37002/18025#newcode353
chrome_frame/html_utils.cc:353: DCHECK(ret.length() ==
static_cast<size_t>(lstrlenA(ret.c_str())));
On 2010/05/19 23:17:37, Peter Kasting wrote:
> Nit: If we really want to do this, it seems like the following is clearer:
> 
> DCHECK_EQ(std::string::npos, ret.find('\0'));

I'm not sure that is 100% equivalent?

http://codereview.chromium.org/2081007/diff/37002/18033
File net/base/network_change_notifier_win.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18033#newcode54
net/base/network_change_notifier_win.cc:54:
CHECK_EQ(static_cast<DWORD>(ERROR_IO_PENDING), ret);
On 2010/05/19 23:17:37, Peter Kasting wrote:
> Nit: CHECK(ret == ERROR_IO_PENDING) is both more common and IMO more readable.

Done.

[Peter Kasting, 2010-05-21 22:21:14]

Added tommi so he can answer my question below.

Let me know if you don't do the header deletion below, so I can.

http://codereview.chromium.org/2081007/diff/37002/18021
File chrome_frame/chrome_active_document.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18021#newcode876
chrome_frame/chrome_active_document.cc:876: if (cmd ==
static_cast<UINT>(context_menu_IDC_BACK)) {
On 2010/05/20 22:04:10, Mike Belshe wrote:
> On 2010/05/19 23:17:37, Peter Kasting wrote:
> > Nit: Instead of this, why not just make these three constants UINTs in
> > chrome_imported_resources.*?
> 
> This looked like a good idea, so I did try.  But then I had to put UINTs into
> the header file, and it chokes because we don't include the windows headers
> there.  I don't want to try to sort that out.

This is actually the only file that #includes that header.  So the easiest fix
is to simply delete the header and declare the externs in this file.  Voila,
problem solved!

http://codereview.chromium.org/2081007/diff/37002/18025
File chrome_frame/html_utils.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18025#newcode353
chrome_frame/html_utils.cc:353: DCHECK(ret.length() ==
static_cast<size_t>(lstrlenA(ret.c_str())));
On 2010/05/20 22:04:10, Mike Belshe wrote:
> On 2010/05/19 23:17:37, Peter Kasting wrote:
> > Nit: If we really want to do this, it seems like the following is clearer:
> > 
> > DCHECK_EQ(std::string::npos, ret.find('\0'));
> 
> I'm not sure that is 100% equivalent?

Hmm, I guess there are two ways the two could differ, this way and if |ret|
contains multibyte UTF8.  tommi: What was the intent of this check?  Should we
be doing IsStringASCII()?

http://codereview.chromium.org/2081007/diff/23003/54008
File base/event_trace_provider_win_unittest.cc (right):

http://codereview.chromium.org/2081007/diff/23003/54008#newcode91
base/event_trace_provider_win_unittest.cc:91: EXPECT_TRUE(provider.Register() !=
NULL);
Nit: Since Register() returns a ULONG, shouldn't this be ERROR_SUCCESS instead
of NULL?

http://codereview.chromium.org/2081007/diff/23003/54011
File base/lock_impl_win.cc (right):

http://codereview.chromium.org/2081007/diff/23003/54011#newcode1
base/lock_impl_win.cc:1: // Copyright (c) 2006-2008 The Chromium Authors. All
rights reserved.
Nit: 2010

http://codereview.chromium.org/2081007/diff/23003/54020
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/2081007/diff/23003/54020#newcode594
chrome/browser/sync/syncable/syncable.cc:594: bool erased_something = num_erased
> 0;
Nit: This isn't really what I was suggesting, I think it actually made things
worse... how about just:

num_erased = kernel_->client_tag_index->erase(entry);
DCHECK_EQ(entry->ref(UNIQUE_CLIENT_TAG).empty(), !num_erased);

[Mike Belshe, 2010-05-24 20:08:19]

http://codereview.chromium.org/2081007/diff/37002/18021
File chrome_frame/chrome_active_document.cc (right):

http://codereview.chromium.org/2081007/diff/37002/18021#newcode876
chrome_frame/chrome_active_document.cc:876: if (cmd ==
static_cast<UINT>(context_menu_IDC_BACK)) {
On 2010/05/21 22:21:14, Peter Kasting wrote:
> On 2010/05/20 22:04:10, Mike Belshe wrote:
> > On 2010/05/19 23:17:37, Peter Kasting wrote:
> > > Nit: Instead of this, why not just make these three constants UINTs in
> > > chrome_imported_resources.*?
> > 
> > This looked like a good idea, so I did try.  But then I had to put UINTs
into
> > the header file, and it chokes because we don't include the windows headers
> > there.  I don't want to try to sort that out.
> 
> This is actually the only file that #includes that header.  So the easiest fix
> is to simply delete the header and declare the externs in this file.  Voila,
> problem solved!

Based on your suggestion I tried this, but I ended up having to back it out. 
The IDC_xx macros are redefined from other windows headers or something.  I
believe the reason this is an external file is simply to avoid some whacky
include ordering.

http://codereview.chromium.org/2081007/diff/23003/54008
File base/event_trace_provider_win_unittest.cc (right):

http://codereview.chromium.org/2081007/diff/23003/54008#newcode91
base/event_trace_provider_win_unittest.cc:91: EXPECT_TRUE(provider.Register() !=
NULL);
On 2010/05/21 22:21:15, Peter Kasting wrote:
> Nit: Since Register() returns a ULONG, shouldn't this be ERROR_SUCCESS instead
> of NULL?

Done.

http://codereview.chromium.org/2081007/diff/23003/54020
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/2081007/diff/23003/54020#newcode594
chrome/browser/sync/syncable/syncable.cc:594: bool erased_something = num_erased
> 0;
On 2010/05/21 22:21:15, Peter Kasting wrote:
> Nit: This isn't really what I was suggesting, I think it actually made things
> worse... how about just:
> 
> num_erased = kernel_->client_tag_index->erase(entry);
> DCHECK_EQ(entry->ref(UNIQUE_CLIENT_TAG).empty(), !num_erased);

Done.

[Peter Kasting, 2010-05-25 00:04:39]

Somehow, tommi didn't get added before.

tommi, can you say what the DCHECK at the end of
html_utils.cc:GetDefaultUserAgent() was designed to catch?  See the discussion
at http://codereview.chromium.org/2081007/diff/37002/18025#newcode353 .

[tommi1, 2010-05-25 01:17:12]

sorry about the delay (today is a holiday and codereview kept giving me
server errors).
IIRC I put the check there initially to make sure my math wasn't off in
buffer size calculations.


On Mon, May 24, 2010 at 8:04 PM, <pkasting@chromium.org> wrote:

> Somehow, tommi didn't get added before.
>
> tommi, can you say what the DCHECK at the end of
> html_utils.cc:GetDefaultUserAgent() was designed to catch?  See the
> discussion
> at http://codereview.chromium.org/2081007/diff/37002/18025#newcode353 .
>
>
> http://codereview.chromium.org/2081007/show
>

[Peter Kasting, 2010-05-25 03:23:10]

On Mon, May 24, 2010 at 6:16 PM, Tommi <tommi@google.com> wrote:

> sorry about the delay (today is a holiday and codereview kept giving me
> server errors).
> IIRC I put the check there initially to make sure my math wasn't off in
> buffer size calculations.
>

Can you say more?  I'm not sure what kind of math failure this could catch.
 Should we remove it entirely?


> http://codereview.chromium.org/2081007/show
>>
>
>

[tommi (sloooow) - chröme, 2010-05-25 03:58:00]

Yes, it can be removed.  It's a sanity check for the .resize() call just
above.  The DCHECK should really be there.  Not sure how it made its way
down :)

On Mon, May 24, 2010 at 11:22 PM, Peter Kasting <pkasting@chromium.org>wrote:

> On Mon, May 24, 2010 at 6:16 PM, Tommi <tommi@google.com> wrote:
>
>> sorry about the delay (today is a holiday and codereview kept giving me
>> server errors).
>> IIRC I put the check there initially to make sure my math wasn't off in
>> buffer size calculations.
>>
>
> Can you say more?  I'm not sure what kind of math failure this could catch.
>  Should we remove it entirely?
>
>
>>  http://codereview.chromium.org/2081007/show
>>>
>>
>>
>

[Peter Kasting, 2010-05-25 06:06:51]

LGTM

http://codereview.chromium.org/2081007/diff/75004/96026
File chrome_frame/html_utils.cc (right):

http://codereview.chromium.org/2081007/diff/75004/96026#newcode345
chrome_frame/html_utils.cc:345: ret.resize(size - sizeof(char));  // NOLINT
Nit: sizeof(char) is 1 by definition, and looks pretty weird... can we just
change this to 1 while we're here?

http://codereview.chromium.org/2081007/diff/75004/96026#newcode354
chrome_frame/html_utils.cc:354: DCHECK(ret.length() ==
static_cast<size_t>(lstrlenA(ret.c_str())));
Per tommi, let's remove this.