Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2577163002:
A follow-up nit fix for jumplist file operations (Closed)
DescriptionA follow-up nit fix for jumplist file operations
After checking in the CL which tries to address the Jumplisticon
performance bug, grt@ pointed out there are a few nits need to be
fixed, including inconsistent code change in different OS files,
unnecessary code and comments.
I copy and paste the comments from grt@ in line with the code in
this CL and reply direclty there.
BUG=40407
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Committed: https://crrev.com/5f1289df7c7dabe8cc73704108a1f433d645a62c
Cr-Commit-Position: refs/heads/master@{#439005}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fix nits #
Messages
Total messages: 26 (19 generated)
Description was changed from ========== Address a few nits Merge branch 'master' of https://chromium.googlesource.com/chromium/src into jumplistfollowupgrt Return real start time rather than pesudo zero fix nits fix nits Add process start time to task manager Format code Add CpuTime to task manager BUG= ========== to ========== Address a few nits Merge branch 'master' of https://chromium.googlesource.com/chromium/src into jumplistfollowupgrt Return real start time rather than pesudo zero fix nits fix nits Add process start time to task manager Format code Add CpuTime to task manager BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Address a few nits Merge branch 'master' of https://chromium.googlesource.com/chromium/src into jumplistfollowupgrt Return real start time rather than pesudo zero fix nits fix nits Add process start time to task manager Format code Add CpuTime to task manager BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== A follow-up nit fix for jumplist file operations After checking in the CL which tries to address the Jumplisticon performance bug, grt@ pointed out there are a few nits need to be fixed, including consistent code change for different OS, unnecessary code and comments. I copy and paste the comments from grt@ in line with the code in this CL and reply direclty there. BUG=40407 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== A follow-up nit fix for jumplist file operations After checking in the CL which tries to address the Jumplisticon performance bug, grt@ pointed out there are a few nits need to be fixed, including consistent code change for different OS, unnecessary code and comments. I copy and paste the comments from grt@ in line with the code in this CL and reply direclty there. BUG=40407 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== A follow-up nit fix for jumplist file operations After checking in the CL which tries to address the Jumplisticon performance bug, grt@ pointed out there are a few nits need to be fixed, including consistent code change for different OS, unnecessary code and comments. I copy and paste the comments from grt@ in line with the code in this CL and reply direclty there. BUG=40407 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== A follow-up nit fix for jumplist file operations After checking in the CL which tries to address the Jumplisticon performance bug, grt@ pointed out there are a few nits need to be fixed, including consistent code change for different OS, unnecessary code and comments. I copy and paste the comments from grt@ in line with the code in this CL and reply direclty there. BUG=40407 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== A follow-up nit fix for jumplist file operations After checking in the CL which tries to address the Jumplisticon performance bug, grt@ pointed out there are a few nits need to be fixed, including consistent code change for different OS, unnecessary code and comments. I copy and paste the comments from grt@ in line with the code in this CL and reply direclty there. BUG=40407 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
chengx@chromium.org changed reviewers: + brucedawson@chromium.org, dcheng@chromium.org, gab@chromium.org, grt@chromium.org
PTAL~ https://codereview.chromium.org/2577163002/diff/1/base/files/file_util_win.cc File base/files/file_util_win.cc (left): https://codereview.chromium.org/2577163002/diff/1/base/files/file_util_win.cc... base/files/file_util_win.cc:181: // result in a false negative, e.g. on C:\bar\Foo versus C:\bar\FooOld. grt@ nit: i think this comment is unnecessary. the FilePath API should be used because it's the right thing. otherwise, why is this one use of IsParent so important that this comment is justified in comparison to every other use of IsParent in the codebase? https://codereview.chromium.org/2577163002/diff/1/base/files/file_util_win.cc... base/files/file_util_win.cc:183: real_from_path.IsParent(real_to_path)) { grt@ this "optimization" seems wrong to me -- either a FilePath is treated like a string (as was with the old code), or it's treated as a FilePath (as it is with the call to IsParent). i think it's a mistake to mix the two like this. https://codereview.chromium.org/2577163002/diff/1/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc (left): https://codereview.chromium.org/2577163002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:254: int folder_operation_status = FolderOperationResult::SUCCESS; grt@ nit: always use an unsigned type to hold a bitfield -- uint32_t would be good here https://codereview.chromium.org/2577163002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:256: if (base::PathExists(icon_dir_old) && !base::DeleteFile(icon_dir_old, true)) { grt@ base::PathExists is unnecessary -- DeleteFile returns true if the item didn't exist (and is documented as such).
Description was changed from ========== A follow-up nit fix for jumplist file operations After checking in the CL which tries to address the Jumplisticon performance bug, grt@ pointed out there are a few nits need to be fixed, including consistent code change for different OS, unnecessary code and comments. I copy and paste the comments from grt@ in line with the code in this CL and reply direclty there. BUG=40407 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== A follow-up nit fix for jumplist file operations After checking in the CL which tries to address the Jumplisticon performance bug, grt@ pointed out there are a few nits need to be fixed, including inconsistent code change in different OS files, unnecessary code and comments. I copy and paste the comments from grt@ in line with the code in this CL and reply direclty there. BUG=40407 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
lgtm, thanks Greg
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm with nits. thanks for the cleanup! https://codereview.chromium.org/2577163002/diff/1/base/files/file_util_posix.cc File base/files/file_util_posix.cc (right): https://codereview.chromium.org/2577163002/diff/1/base/files/file_util_posix.... base/files/file_util_posix.cc:279: if (real_to_path == real_from_path || real_from_path.IsParent(real_to_path)) { nit: omit braces for consistency with the conditional above https://codereview.chromium.org/2577163002/diff/1/base/files/file_util_win.cc File base/files/file_util_win.cc (right): https://codereview.chromium.org/2577163002/diff/1/base/files/file_util_win.cc... base/files/file_util_win.cc:180: if (real_to_path == real_from_path || real_from_path.IsParent(real_to_path)) { nit: no braces
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chengx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, grt@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2577163002/#ps20001 (title: "Fix nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481853843669230,
"parent_rev": "f0448935d5643a00566b87303f884acbeabda451", "commit_rev":
"2434b64eacb7b9365222d1b59baf0e0196462d03"}
Message was sent while issue was closed.
Description was changed from ========== A follow-up nit fix for jumplist file operations After checking in the CL which tries to address the Jumplisticon performance bug, grt@ pointed out there are a few nits need to be fixed, including inconsistent code change in different OS files, unnecessary code and comments. I copy and paste the comments from grt@ in line with the code in this CL and reply direclty there. BUG=40407 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== A follow-up nit fix for jumplist file operations After checking in the CL which tries to address the Jumplisticon performance bug, grt@ pointed out there are a few nits need to be fixed, including inconsistent code change in different OS files, unnecessary code and comments. I copy and paste the comments from grt@ in line with the code in this CL and reply direclty there. BUG=40407 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Review-Url: https://codereview.chromium.org/2577163002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== A follow-up nit fix for jumplist file operations After checking in the CL which tries to address the Jumplisticon performance bug, grt@ pointed out there are a few nits need to be fixed, including inconsistent code change in different OS files, unnecessary code and comments. I copy and paste the comments from grt@ in line with the code in this CL and reply direclty there. BUG=40407 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Review-Url: https://codereview.chromium.org/2577163002 ========== to ========== A follow-up nit fix for jumplist file operations After checking in the CL which tries to address the Jumplisticon performance bug, grt@ pointed out there are a few nits need to be fixed, including inconsistent code change in different OS files, unnecessary code and comments. I copy and paste the comments from grt@ in line with the code in this CL and reply direclty there. BUG=40407 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/5f1289df7c7dabe8cc73704108a1f433d645a62c Cr-Commit-Position: refs/heads/master@{#439005} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5f1289df7c7dabe8cc73704108a1f433d645a62c Cr-Commit-Position: refs/heads/master@{#439005} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
