Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2563483004:
Add more detailed JumpListIcons folder's move operation metric to UMA (Closed)
DescriptionAdd more detailed JumpListIcon folder's move operation metric to UMA
JumpListIcons(Old) folders are getting unexpectively huge for some users.
This may be caused by file operation failures.
After analyzing those folders' problematic move operation metric
by landing crrev.com/2522163003/, we find that it is necessary
to dig into every major step in base::move() operation for better
understanding the root cause.
Added code to record more detailed JumpListIcons folder's move operation
status to UMA.
Will revert this CL after gathering user data for further analysis.
BUG=179576
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Committed: https://crrev.com/fb00ce9a44dda016d9de73318e51d49ad6a14d20
Cr-Commit-Position: refs/heads/master@{#437756}
Patch Set 1 #Patch Set 2 : Mark the old histogram as <obselete> #
Total comments: 16
Patch Set 3 : Fix CopyDirectory(), more detailed UMA record and fix some nits #
Total comments: 7
Patch Set 4 : Move one included header file #Messages
Total messages: 45 (34 generated)
Description was changed from ========== Format xml file Log more detailed user data for base::move() operation BUG=179576 ========== to ========== Format xml file Log more detailed user data for base::move() operation BUG=179576 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...
Description was changed from ========== Format xml file Log more detailed user data for base::move() operation BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Add more detailed JumpListIcon folder's move operation metric to UMA JumpListIcons(Old) folders are getting unexpectively huge for some users. This may be caused by file operation failures. After analyzing those folders' problematic move operation metric by landing crrev.com/2522163003/, we find that it is necessary to dig into every major step in base::move() operation for better understanding the root cause. Added code to record more detailed JumpListIcons folder's move operation status to UMA. Will revert this CL after gathering user data for further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Add more detailed JumpListIcon folder's move operation metric to UMA JumpListIcons(Old) folders are getting unexpectively huge for some users. This may be caused by file operation failures. After analyzing those folders' problematic move operation metric by landing crrev.com/2522163003/, we find that it is necessary to dig into every major step in base::move() operation for better understanding the root cause. Added code to record more detailed JumpListIcons folder's move operation status to UMA. Will revert this CL after gathering user data for further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Add more detailed JumpListIcon folder's move operation metric to UMA JumpListIcons(Old) folders are getting unexpectively huge for some users. This may be caused by file operation failures. After analyzing those folders' problematic move operation metric by landing crrev.com/2522163003/, we find that it is necessary to dig into every major step in base::move() operation for better understanding the root cause. Added code to record more detailed JumpListIcons folder's move operation status to UMA. Will revert this CL after gathering user data for further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Add more detailed JumpListIcon folder's move operation metric to UMA JumpListIcons(Old) folders are getting unexpectively huge for some users. This may be caused by file operation failures. After analyzing those folders' problematic move operation metric by landing crrev.com/2522163003/, we find that it is necessary to dig into every major step in base::move() operation for better understanding the root cause. Added code to record more detailed JumpListIcons folder's move operation status to UMA. Will revert this CL after gathering user data for further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Add more detailed JumpListIcon folder's move operation metric to UMA JumpListIcons(Old) folders are getting unexpectively huge for some users. This may be caused by file operation failures. After analyzing those folders' problematic move operation metric by landing crrev.com/2522163003/, we find that it is necessary to dig into every major step in base::move() operation for better understanding the root cause. Added code to record more detailed JumpListIcons folder's move operation status to UMA. Will revert this CL after gathering user data for further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Add more detailed JumpListIcon folder's move operation metric to UMA JumpListIcons(Old) folders are getting unexpectively huge for some users. This may be caused by file operation failures. After analyzing those folders' problematic move operation metric by landing crrev.com/2522163003/, we find that it is necessary to dig into every major step in base::move() operation for better understanding the root cause. Added code to record more detailed JumpListIcons folder's move operation status to UMA. Will revert this CL after gathering user data for further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Add more detailed JumpListIcon folder's move operation metric to UMA JumpListIcons(Old) folders are getting unexpectively huge for some users. This may be caused by file operation failures. After analyzing those folders' problematic move operation metric by landing crrev.com/2522163003/, we find that it is necessary to dig into every major step in base::move() operation for better understanding the root cause. Added code to record more detailed JumpListIcons folder's move operation status to UMA. Will revert this CL after gathering user data for further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Add more detailed JumpListIcon folder's move operation metric to UMA JumpListIcons(Old) folders are getting unexpectively huge for some users. This may be caused by file operation failures. After analyzing those folders' problematic move operation metric by landing crrev.com/2522163003/, we find that it is necessary to dig into every major step in base::move() operation for better understanding the root cause. Added code to record more detailed JumpListIcons folder's move operation status to UMA. Will revert this CL after gathering user data for further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Add more detailed JumpListIcon folder's move operation metric to UMA JumpListIcons(Old) folders are getting unexpectively huge for some users. This may be caused by file operation failures. After analyzing those folders' problematic move operation metric by landing crrev.com/2522163003/, we find that it is necessary to dig into every major step in base::move() operation for better understanding the root cause. Added code to record more detailed JumpListIcons folder's move operation status to UMA. Will revert this CL after gathering user data for further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Add more detailed JumpListIcon folder's move operation metric to UMA JumpListIcons(Old) folders are getting unexpectively huge for some users. This may be caused by file operation failures. After analyzing those folders' problematic move operation metric by landing crrev.com/2522163003/, we find that it is necessary to dig into every major step in base::move() operation for better understanding the root cause. Added code to record more detailed JumpListIcons folder's move operation status to UMA. Will revert this CL after gathering user data for further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Add more detailed JumpListIcon folder's move operation metric to UMA JumpListIcons(Old) folders are getting unexpectively huge for some users. This may be caused by file operation failures. After analyzing those folders' problematic move operation metric by landing crrev.com/2522163003/, we find that it is necessary to dig into every major step in base::move() operation for better understanding the root cause. Added code to record more detailed JumpListIcons folder's move operation status to UMA. Will revert this CL after gathering user data for further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
chengx@chromium.org changed reviewers: + brucedawson@chromium.org, gab@chromium.org, isherman@chromium.org
This CL is ready for review. PTAL~
It looks like you're changing the meaning of the existing histogram buckets. Is that correct? If so, please mark the old histogram as <obsolete>, and create a new histogram.
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 to run a CQ dry run
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...
On 2016/12/08 23:20:49, Ilya Sherman wrote: > It looks like you're changing the meaning of the existing histogram buckets. Is > that correct? If so, please mark the old histogram as <obsolete>, and create a > new histogram. Yes, I was reusing the old histogram. Now I have marked the old one obsolete and created a new one. Thanks!
Metrics LGTM, thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2563483004/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2563483004/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:233: // This function is mostly copied from //base for UMA debugging purpose. // This function is an exact copy of the one in //base used for UMA debugging of DeleteFile() below. https://codereview.chromium.org/2563483004/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:264: // This function is mostly copied from //base for UMA debugging purpose. // This function is a copy of the one in //base augmented for UMA debugging purposes. (i.e. "mostly" copied leaves it unclear whether logic was modified or not) https://codereview.chromium.org/2563483004/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:267: bool DeleteFileTemp(const base::FilePath& path, naming this "DeleteFile" is fine IMO, it won't be in base:: so it's easy to know you're referring to the right one. https://codereview.chromium.org/2563483004/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:281: return DeleteFileRecursive(path.DirName(), path.BaseName().value(), Not worried about this failing? https://codereview.chromium.org/2563483004/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:290: !SetFileAttributes(path.value().c_str(), Since we now have methods in file scope, some in base: and some from Windows, let's make it clear which methods are from MSDN by prefixing with :: ::GetFileAttributes() ::SetFileAttributes() ::RemoveDirectory() https://codereview.chromium.org/2563483004/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:292: return false; Not worried about this failing? https://codereview.chromium.org/2563483004/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:296: return !!::DeleteFile(path.value().c_str()); #include <windows.h> for ::DeleteFile() and other constants/methods above. https://codereview.chromium.org/2563483004/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:355: move_operation_status |= MoveOperationResult::COPY_FILE_FAILED; This is copying a directory, not a "file", right? s/COPY_FILE_FAILED/COPY_DIRECTORY_FAILED/ ?
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 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...
PTAL~ Thanks! In the new patch, I addressed all the comments. In addition, I added CopyDirectoryTemp() function which is mostly copied from CopyDirectory() in //base. This function in //base has issues as pointed out at line 359 in the new patch, which is presumably fixed here. I need UMA data to verify though. I have to rename CopyDirectory() to CopyDirectoryTemp(), because CopyDirectory() is in anonymous namespace of base::, so the compiler gets confused if I keep the same name. https://codereview.chromium.org/2563483004/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2563483004/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:233: // This function is mostly copied from //base for UMA debugging purpose. On 2016/12/09 19:12:12, gab wrote: > // This function is an exact copy of the one in //base used for UMA debugging of > DeleteFile() below. Yes, you are right. It is an exact copy. I copy it because it is in anonymous namespace in //base, making it inaccessible from outside. https://codereview.chromium.org/2563483004/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:264: // This function is mostly copied from //base for UMA debugging purpose. On 2016/12/09 19:12:12, gab wrote: > // This function is a copy of the one in //base augmented for UMA debugging > purposes. > > (i.e. "mostly" copied leaves it unclear whether logic was modified or not) Done. https://codereview.chromium.org/2563483004/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:267: bool DeleteFileTemp(const base::FilePath& path, On 2016/12/09 19:12:12, gab wrote: > naming this "DeleteFile" is fine IMO, it won't be in base:: so it's easy to know > you're referring to the right one. Done. https://codereview.chromium.org/2563483004/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:281: return DeleteFileRecursive(path.DirName(), path.BaseName().value(), On 2016/12/09 19:12:12, gab wrote: > Not worried about this failing? I don't think it will be hit in my case. But I add UMA here for full coverage in the new patch. Thanks for reminder. https://codereview.chromium.org/2563483004/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:290: !SetFileAttributes(path.value().c_str(), On 2016/12/09 19:12:12, gab wrote: > Since we now have methods in file scope, some in base: and some from Windows, > let's make it clear which methods are from MSDN by prefixing with :: > > ::GetFileAttributes() > ::SetFileAttributes() > ::RemoveDirectory() Done. https://codereview.chromium.org/2563483004/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:292: return false; On 2016/12/09 19:12:12, gab wrote: > Not worried about this failing? Just added UMA for this. Thanks for reminder. https://codereview.chromium.org/2563483004/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:296: return !!::DeleteFile(path.value().c_str()); On 2016/12/09 19:12:12, gab wrote: > #include <windows.h> for ::DeleteFile() and other constants/methods above. Done. https://codereview.chromium.org/2563483004/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:355: move_operation_status |= MoveOperationResult::COPY_FILE_FAILED; On 2016/12/09 19:12:13, gab wrote: > This is copying a directory, not a "file", right? > > s/COPY_FILE_FAILED/COPY_DIRECTORY_FAILED/ ? Yes, you are right.
lgtm w/ comments, good luck! https://codereview.chromium.org/2563483004/diff/40001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2563483004/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:8: #include "base/bind.h" Empty line between system headers and Chrome headers. https://codereview.chromium.org/2563483004/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:361: // the from_path, e.g., parent-child relationship in terms of folder, it |to_path| , |from_path| (i.e. vars in ||) https://codereview.chromium.org/2563483004/diff/40001/chrome/browser/win/jump... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2563483004/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.h:15: #include "base/files/file_enumerator.h" Only required in .cc https://codereview.chromium.org/2563483004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2563483004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:91588: +</enum> Many of these are impossible given the way one implies the others, is it worth having all 64? Worst case I think if we miss a real one it wouldn't be labeled in the dashboards but the raw number would still show up so we'd know to add it.
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...
Patchset #4 (id:60001) has been deleted
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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...
https://codereview.chromium.org/2563483004/diff/40001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2563483004/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:8: #include "base/bind.h" On 2016/12/09 23:17:28, gab wrote: > Empty line between system headers and Chrome headers. Done. https://codereview.chromium.org/2563483004/diff/40001/chrome/browser/win/jump... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2563483004/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.h:15: #include "base/files/file_enumerator.h" On 2016/12/09 23:17:28, gab wrote: > Only required in .cc Done. https://codereview.chromium.org/2563483004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2563483004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:91588: +</enum> On 2016/12/09 23:17:28, gab wrote: > Many of these are impossible given the way one implies the others, is it worth > having all 64? Worst case I think if we miss a real one it wouldn't be labeled > in the dashboards but the raw number would still show up so we'd know to add it. Acknowledged.
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 isherman@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2563483004/#ps80001 (title: "Move one included header file")
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": 80001, "attempt_start_ts": 1481346197686330,
"parent_rev": "e3916da3937d887e5d0b2ea9f12401a39415196f", "commit_rev":
"903ca46fe7d53c798d0ae558f43d7e86dcfcbc3d"}
Message was sent while issue was closed.
Description was changed from ========== Add more detailed JumpListIcon folder's move operation metric to UMA JumpListIcons(Old) folders are getting unexpectively huge for some users. This may be caused by file operation failures. After analyzing those folders' problematic move operation metric by landing crrev.com/2522163003/, we find that it is necessary to dig into every major step in base::move() operation for better understanding the root cause. Added code to record more detailed JumpListIcons folder's move operation status to UMA. Will revert this CL after gathering user data for further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Add more detailed JumpListIcon folder's move operation metric to UMA JumpListIcons(Old) folders are getting unexpectively huge for some users. This may be caused by file operation failures. After analyzing those folders' problematic move operation metric by landing crrev.com/2522163003/, we find that it is necessary to dig into every major step in base::move() operation for better understanding the root cause. Added code to record more detailed JumpListIcons folder's move operation status to UMA. Will revert this CL after gathering user data for further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Review-Url: https://codereview.chromium.org/2563483004 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add more detailed JumpListIcon folder's move operation metric to UMA JumpListIcons(Old) folders are getting unexpectively huge for some users. This may be caused by file operation failures. After analyzing those folders' problematic move operation metric by landing crrev.com/2522163003/, we find that it is necessary to dig into every major step in base::move() operation for better understanding the root cause. Added code to record more detailed JumpListIcons folder's move operation status to UMA. Will revert this CL after gathering user data for further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Review-Url: https://codereview.chromium.org/2563483004 ========== to ========== Add more detailed JumpListIcon folder's move operation metric to UMA JumpListIcons(Old) folders are getting unexpectively huge for some users. This may be caused by file operation failures. After analyzing those folders' problematic move operation metric by landing crrev.com/2522163003/, we find that it is necessary to dig into every major step in base::move() operation for better understanding the root cause. Added code to record more detailed JumpListIcons folder's move operation status to UMA. Will revert this CL after gathering user data for further analysis. BUG=179576 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/fb00ce9a44dda016d9de73318e51d49ad6a14d20 Cr-Commit-Position: refs/heads/master@{#437756} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/fb00ce9a44dda016d9de73318e51d49ad6a14d20 Cr-Commit-Position: refs/heads/master@{#437756} |
