Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8085026:
Add a metric to Chrome to measure fragmentation of chrome.dll at startup.BUG=98033TEST=None (Closed)
|
Created:
9 years, 2 months ago by robertshield Modified:
9 years, 2 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd a metric to Chrome to measure fragmentation of chrome.dll at startup.
BUG=98033
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=103760
Patch Set 1 #Patch Set 2 : '' #
Total comments: 32
Patch Set 3 : '' #
Total comments: 24
Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 8
Patch Set 6 : '' #
Total comments: 10
Patch Set 7 : '' #
Messages
Total messages: 12 (0 generated)
looks pretty awesome. how about a unit test to make sure it doesn't crash? http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... File chrome/browser/fragmentation_checker_win.cc (right): http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:20: size_t kMaxBufferSize = 1 << 20; const size_t... http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:29: (number_of_extents - 1) * (sizeof(LARGE_INTEGER) * 2); indentation http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:29: (number_of_extents - 1) * (sizeof(LARGE_INTEGER) * 2); is it possible to replace sizeof(LARGE_INTEGER) * 2 with something like RETRIEVAL_POINTERS_BUFFER::Extents? or something silly like that? http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:35: if (!file_util::PathExists(file_path)) { why do this extra check? CreatePlatformFile should fail just fine if the file doesn't exist. if you want to log that, check for PLATFORM_FILE_ERROR_NOT_FOUND when CreatePlatformFile fails. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:56: std::vector<BYTE> retrieval_pointers_buffer; uint8 rather than win-specific BYTE? http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:58: size_t output_size = ComputeRetrievalPointersBufferSize(number_of_extents); do this computation before line 56, then change line 56 to construct the buffer with the proper size: std::vector<uint8> retrieval_pointers_buffer(output_size); http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:65: SetLastError(ERROR_SUCCESS); this is extremely odd. please document why this is needed. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:66: result = DeviceIoControl( MSDN says that the call fails (returns 0) if the buffer is too small, yet that's not how you're using it. have you found that MSDN is lying? if so, please document why you're not following the conventions described in MSDN. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:74: NULL) == TRUE; == TRUE -> != 0 MSDN says it returns zero/non-zero, not FALSE/TRUE. even if that weren't the case, it's safer to compare to FALSE/0 than the converse. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:76: DWORD last_error = GetLastError(); this is extremely odd. according to MSDN, you would only need to call GLE if DeviceIoControl returned 0, yet here you call it without checking the return value of DeviceIoControl. please document why this is needed. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:90: retrieval_pointers_buffer.resize(output_size); replace these two lines with a call to .assign(output_size, 0) http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:93: LOG(ERROR) << "FSCTL_GET_RETRIEVAL_POINTERS failed. gle = " PLOG(ERROR) << "FSCTL_GET_RETRIEVAL_POINTERS failed."; http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:113: file_extent_data->number_of_extents = extent_count; this does (in essence) a uint32 -> int conversion. please make it safe. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:129: FilePath module_path; #include "base/file_path.h" for this http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:136: file_extent_data.virtual_cluster_span); 1) this macro sets the max sample to 1000000, which is a lot less than what virtual_cluster_span can potentially hold. perhaps you should use CUSTOM_COUNTS here with Histogram::kSampleType_MAX? Maybe you want more buckets in that case? 2) you're passing an int64 into something that takes an int. i think you need to compress down to an int somehow. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... File chrome/browser/fragmentation_checker_win.h (right): http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. 2011
Thanks, ptal http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... File chrome/browser/fragmentation_checker_win.cc (right): http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:20: size_t kMaxBufferSize = 1 << 20; On 2011/09/30 18:44:14, grt wrote: > const size_t... Done. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:29: (number_of_extents - 1) * (sizeof(LARGE_INTEGER) * 2); On 2011/09/30 18:44:14, grt wrote: > indentation Done. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:29: (number_of_extents - 1) * (sizeof(LARGE_INTEGER) * 2); On 2011/09/30 18:44:14, grt wrote: > indentation Done. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:35: if (!file_util::PathExists(file_path)) { On 2011/09/30 18:44:14, grt wrote: > why do this extra check? CreatePlatformFile should fail just fine if the file > doesn't exist. if you want to log that, check for PLATFORM_FILE_ERROR_NOT_FOUND > when CreatePlatformFile fails. Done. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:56: std::vector<BYTE> retrieval_pointers_buffer; On 2011/09/30 18:44:14, grt wrote: > uint8 rather than win-specific BYTE? Done, although this file is (and can only be) Windows-only. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:58: size_t output_size = ComputeRetrievalPointersBufferSize(number_of_extents); On 2011/09/30 18:44:14, grt wrote: > do this computation before line 56, then change line 56 to construct the buffer > with the proper size: > std::vector<uint8> retrieval_pointers_buffer(output_size); Done. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:65: SetLastError(ERROR_SUCCESS); On 2011/09/30 18:44:14, grt wrote: > this is extremely odd. please document why this is needed. rejiggered as discussed. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:66: result = DeviceIoControl( On 2011/09/30 18:44:14, grt wrote: > MSDN says that the call fails (returns 0) if the buffer is too small, yet that's > not how you're using it. have you found that MSDN is lying? if so, please > document why you're not following the conventions described in MSDN. I found documentation suggesting using the GLE code rather than the return value, but have updated this to be more conventional. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:74: NULL) == TRUE; On 2011/09/30 18:44:14, grt wrote: > == TRUE -> != 0 > MSDN says it returns zero/non-zero, not FALSE/TRUE. even if that weren't the > case, it's safer to compare to FALSE/0 than the converse. My version of WinBase.h says it returns FALSE/TRUE and I trust that more. Changed to != FALSE. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:76: DWORD last_error = GetLastError(); On 2011/09/30 18:44:14, grt wrote: > this is extremely odd. according to MSDN, you would only need to call GLE if > DeviceIoControl returned 0, yet here you call it without checking the return > value of DeviceIoControl. please document why this is needed. Changed as discussed. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:90: retrieval_pointers_buffer.resize(output_size); On 2011/09/30 18:44:14, grt wrote: > replace these two lines with a call to .assign(output_size, 0) Done. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:93: LOG(ERROR) << "FSCTL_GET_RETRIEVAL_POINTERS failed. gle = " On 2011/09/30 18:44:14, grt wrote: > PLOG(ERROR) << "FSCTL_GET_RETRIEVAL_POINTERS failed."; Done. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:113: file_extent_data->number_of_extents = extent_count; On 2011/09/30 18:44:14, grt wrote: > this does (in essence) a uint32 -> int conversion. please make it safe. Done. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:129: FilePath module_path; On 2011/09/30 18:44:14, grt wrote: > #include "base/file_path.h" for this Done. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:136: file_extent_data.virtual_cluster_span); On 2011/09/30 18:44:14, grt wrote: > 1) this macro sets the max sample to 1000000, which is a lot less than what > virtual_cluster_span can potentially hold. perhaps you should use CUSTOM_COUNTS > here with Histogram::kSampleType_MAX? Maybe you want more buckets in that case? > 2) you're passing an int64 into something that takes an int. i think you need > to compress down to an int somehow. As discussed, while reading the documentation again I realized that the virtual_cluster_span metric isn't actually useful. I'd need to map that to logical addresses on disk which requires querying the volume for information, which requires a volume handle which requires admin. So I'll drop that and leave the number_of_extents as the only metric being reported for now. http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... File chrome/browser/fragmentation_checker_win.h (right): http://codereview.chromium.org/8085026/diff/4001/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. On 2011/09/30 18:44:14, grt wrote: > 2011 sigh
http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... File chrome/browser/fragmentation_checker_unittest_win.cc (right): http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_unittest_win.cc:5: #include "chrome/browser/fragmentation_checker_win.h" shouldn't this be in-order with the other headers? http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... File chrome/browser/fragmentation_checker_win.cc (right): http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:9: #include <algorithm> #include <vector> http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:21: const size_t kMaxBufferSize = 1 << 20; this is waaaaay larger than it needs to be given kMaxExtentCount. do you think it's worth making kMaxExtentCount be the most that CountFileExtents will dish out? if so, the max buffer size could be computed from kMaxExtentCount. http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:24: size_t ComputeRetrievalPointersBufferSize(int number_of_extents) { that an int suffices here for an approximation of the number of extents makes me think that uint32 could safely be replaced with int for all counts of extents (i'm referring to my uint32 -> int comment in the header). http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:26: return sizeof(RETRIEVAL_POINTERS_BUFFER) + sizeof(buffer) + ... http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:46: if (error_code == base::PLATFORM_FILE_OK && !created) { base::PLATFORM_FILE_OPEN dictates that created will never be true, so remove " && !created" http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:49: // Compute an output size capable of holding 16 extents at first. This may may -> will http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:73: extents_guess *= 2; maybe cap this at kMaxExtentCount instead of capping the buffer size, and return kMaxExtentCount when the guess exceeds it? http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:97: LOG(ERROR) << "Failed to open module file to check extents."; may as well log error_code http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... File chrome/browser/fragmentation_checker_win.h (right): http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.h:13: // Returns the count of the number of extents for the file at |file_path|. On Is "the count of the number of" different than "the number of"? http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.h:15: bool CountFileExtents(const FilePath& file_path, uint32* file_extents_count); Style guide says not to use unsigned types just because something should never be negative. Did you chose uint32 because you need a full 32 bits? Is int appropriate? http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.h:18: // of fragments the current module is stored in as well as the total on-disk remove " as well as the total on-disk..."
http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... File chrome/browser/fragmentation_checker_unittest_win.cc (right): http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_unittest_win.cc:5: #include "chrome/browser/fragmentation_checker_win.h" On 2011/10/01 02:49:46, grt wrote: > shouldn't this be in-order with the other headers? Done. http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... File chrome/browser/fragmentation_checker_win.cc (right): http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:9: On 2011/10/01 02:49:46, grt wrote: > #include <algorithm> > #include <vector> Done. http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:21: const size_t kMaxBufferSize = 1 << 20; On 2011/10/01 02:49:46, grt wrote: > this is waaaaay larger than it needs to be given kMaxExtentCount. do you think > it's worth making kMaxExtentCount be the most that CountFileExtents will dish > out? if so, the max buffer size could be computed from kMaxExtentCount. Done, removed the max buffer size, everything is now capped in terms of kMaxExtentCount. http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:24: size_t ComputeRetrievalPointersBufferSize(int number_of_extents) { On 2011/10/01 02:49:46, grt wrote: > that an int suffices here for an approximation of the number of extents makes me > think that uint32 could safely be replaced with int for all counts of extents > (i'm referring to my uint32 -> int comment in the header). Done. http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:26: return sizeof(RETRIEVAL_POINTERS_BUFFER) + On 2011/10/01 02:49:46, grt wrote: > sizeof(buffer) + ... Done. http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:46: if (error_code == base::PLATFORM_FILE_OK && !created) { On 2011/10/01 02:49:46, grt wrote: > base::PLATFORM_FILE_OPEN dictates that created will never be true, so remove " > && !created" Done. http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:49: // Compute an output size capable of holding 16 extents at first. This may On 2011/10/01 02:49:46, grt wrote: > may -> will Done. http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:73: extents_guess *= 2; On 2011/10/01 02:49:46, grt wrote: > maybe cap this at kMaxExtentCount instead of capping the buffer size, and return > kMaxExtentCount when the guess exceeds it? Done. http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.cc:97: LOG(ERROR) << "Failed to open module file to check extents."; On 2011/10/01 02:49:46, grt wrote: > may as well log error_code Done. http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... File chrome/browser/fragmentation_checker_win.h (right): http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.h:13: // Returns the count of the number of extents for the file at |file_path|. On On 2011/10/01 02:49:46, grt wrote: > Is "the count of the number of" different than "the number of"? Yes: http://www.youtube.com/watch?v=kQC82okzTXI http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.h:15: bool CountFileExtents(const FilePath& file_path, uint32* file_extents_count); On 2011/10/01 02:49:46, grt wrote: > Style guide says not to use unsigned types just because something should never > be negative. Did you chose uint32 because you need a full 32 bits? Is int > appropriate? I chose it here because the ioctl returns a dword, so technically I could need the full 32 bits, but that's silly since I cap the max number this function returns, so changed to int. http://codereview.chromium.org/8085026/diff/4006/chrome/browser/fragmentation... chrome/browser/fragmentation_checker_win.h:18: // of fragments the current module is stored in as well as the total on-disk On 2011/10/01 02:49:46, grt wrote: > remove " as well as the total on-disk..." Done.
One last question. (1am, man?) http://codereview.chromium.org/8085026/diff/12005/chrome/browser/fragmentatio... File chrome/browser/fragmentation_checker_win.cc (right): http://codereview.chromium.org/8085026/diff/12005/chrome/browser/fragmentatio... chrome/browser/fragmentation_checker_win.cc:74: if (extents_guess > kMaxExtentCount) { ? *file_extents_count = kMaxExtentCount; return true; ?
http://codereview.chromium.org/8085026/diff/12005/chrome/browser/fragmentatio... File chrome/browser/fragmentation_checker_win.cc (right): http://codereview.chromium.org/8085026/diff/12005/chrome/browser/fragmentatio... chrome/browser/fragmentation_checker_win.cc:112: 1, wdyt about sending up 0 as the count when the count couldn't be reached, so we can see if there's a population for whom this just doesn't work? otherwise we'd lose their results. so, combined with my question above, 0 == can't do it, kMaxExtentCount == fragmented beyond our wildest dreams, and everything in between is a valid number. think that'd be useful?
http://codereview.chromium.org/8085026/diff/12005/chrome/browser/fragmentatio... File chrome/browser/fragmentation_checker_win.cc (right): http://codereview.chromium.org/8085026/diff/12005/chrome/browser/fragmentatio... chrome/browser/fragmentation_checker_win.cc:112: 1, On 2011/10/01 11:04:09, grt wrote: > wdyt about sending up 0 as the count when the count couldn't be reached reached -> read (i.e., CountFileExtents returns false)
http://codereview.chromium.org/8085026/diff/12005/chrome/browser/fragmentatio... File chrome/browser/fragmentation_checker_win.h (right): http://codereview.chromium.org/8085026/diff/12005/chrome/browser/fragmentatio... chrome/browser/fragmentation_checker_win.h:9: #include "base/basictypes.h" nit: you don't need this include here.
Thanks, ptal. http://codereview.chromium.org/8085026/diff/12005/chrome/browser/fragmentatio... File chrome/browser/fragmentation_checker_win.cc (right): http://codereview.chromium.org/8085026/diff/12005/chrome/browser/fragmentatio... chrome/browser/fragmentation_checker_win.cc:74: if (extents_guess > kMaxExtentCount) { On 2011/10/01 10:59:38, grt wrote: > ? > *file_extents_count = kMaxExtentCount; > return true; > ? Done. http://codereview.chromium.org/8085026/diff/12005/chrome/browser/fragmentatio... chrome/browser/fragmentation_checker_win.cc:112: 1, On 2011/10/01 22:34:53, grt wrote: > On 2011/10/01 11:04:09, grt wrote: > > wdyt about sending up 0 as the count when the count couldn't be reached > > reached -> read (i.e., CountFileExtents returns false) Done. http://codereview.chromium.org/8085026/diff/12005/chrome/browser/fragmentatio... chrome/browser/fragmentation_checker_win.cc:112: 1, On 2011/10/01 11:04:09, grt wrote: > wdyt about sending up 0 as the count when the count couldn't be reached, so we > can see if there's a population for whom this just doesn't work? otherwise we'd > lose their results. > > so, combined with my question above, 0 == can't do it, kMaxExtentCount == > fragmented beyond our wildest dreams, and everything in between is a valid > number. > > think that'd be useful? Sounds reasonable. I modified the CountFileExtents() function to return an int, with the only caveat being that there's no way for the caller to tell if the number of extents equals kMaxExtentCount or exceeds it, but I think that's fine. http://codereview.chromium.org/8085026/diff/12005/chrome/browser/fragmentatio... File chrome/browser/fragmentation_checker_win.h (right): http://codereview.chromium.org/8085026/diff/12005/chrome/browser/fragmentatio... chrome/browser/fragmentation_checker_win.h:9: #include "base/basictypes.h" On 2011/10/02 00:16:25, tfarina wrote: > nit: you don't need this include here. Thanks! Removed.
code LGTM. just some final header inclusion tweaks. http://codereview.chromium.org/8085026/diff/13001/chrome/browser/fragmentatio... File chrome/browser/fragmentation_checker_win.cc (right): http://codereview.chromium.org/8085026/diff/13001/chrome/browser/fragmentatio... chrome/browser/fragmentation_checker_win.cc:10: #include <algorithm> no longer needed http://codereview.chromium.org/8085026/diff/13001/chrome/browser/fragmentatio... chrome/browser/fragmentation_checker_win.cc:13: #include "base/at_exit.h" remove http://codereview.chromium.org/8085026/diff/13001/chrome/browser/fragmentatio... chrome/browser/fragmentation_checker_win.cc:14: #include "base/basictypes.h" is this used? http://codereview.chromium.org/8085026/diff/13001/chrome/browser/fragmentatio... chrome/browser/fragmentation_checker_win.cc:15: #include "base/command_line.h" remove http://codereview.chromium.org/8085026/diff/13001/chrome/browser/fragmentatio... chrome/browser/fragmentation_checker_win.cc:17: #include "base/file_util.h" i think you want #include "base/platform_file.h" instead of this
Thanks! http://codereview.chromium.org/8085026/diff/13001/chrome/browser/fragmentatio... File chrome/browser/fragmentation_checker_win.cc (right): http://codereview.chromium.org/8085026/diff/13001/chrome/browser/fragmentatio... chrome/browser/fragmentation_checker_win.cc:10: #include <algorithm> On 2011/10/03 14:33:32, grt wrote: > no longer needed Done. http://codereview.chromium.org/8085026/diff/13001/chrome/browser/fragmentatio... chrome/browser/fragmentation_checker_win.cc:13: #include "base/at_exit.h" On 2011/10/03 14:33:32, grt wrote: > remove Done. http://codereview.chromium.org/8085026/diff/13001/chrome/browser/fragmentatio... chrome/browser/fragmentation_checker_win.cc:14: #include "base/basictypes.h" On 2011/10/03 14:33:32, grt wrote: > is this used? Done. http://codereview.chromium.org/8085026/diff/13001/chrome/browser/fragmentatio... chrome/browser/fragmentation_checker_win.cc:15: #include "base/command_line.h" On 2011/10/03 14:33:32, grt wrote: > remove Done. http://codereview.chromium.org/8085026/diff/13001/chrome/browser/fragmentatio... chrome/browser/fragmentation_checker_win.cc:17: #include "base/file_util.h" On 2011/10/03 14:33:32, grt wrote: > i think you want > #include "base/platform_file.h" > instead of this Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
