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

Issue 2713773002: ShowThreadNames tool to get thread ID/name pairs in a Chrome process (Closed)

Created:
3 years, 10 months ago by chengx
Modified:
3 years, 10 months ago
Reviewers:
stanisc, brucedawson
CC:
chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

ShowThreadNames tool to get thread ID/name pairs in a Chrome process This tool is designed to test the usage of the SetThreadDescription WinAPI in Chrome. In Chrome, the SetThreadDescription API has been enabled to set thread names. However, since there is no tool support to retrieve thread names set by GetThreadDescription, we will still rely on SetNameInternal function in platform_thread_win.cc to set thread names. Despite this, we need a tool to demo the SetThreadDescription API works, even without the debugger to be present. This tool incorporates the GetThreadDescription API trying to get names of all threads in a process specified by its ID. If the thread names have been set by SetThreadDescription API call like in Chrome, all thread ID/name pairs are returned. BUG=684203 Review-Url: https://codereview.chromium.org/2713773002 Cr-Commit-Position: refs/heads/master@{#452617} Committed: https://chromium.googlesource.com/chromium/src/+/b97c0922116dbeb62d81790daf475495a399d264

Patch Set 1 #

Total comments: 22

Patch Set 2 : Address CR feedback. #

Total comments: 10

Patch Set 3 : Add support for getting error msg. #

Total comments: 10

Patch Set 4 : Fix nits. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+353 lines, -0 lines) Patch
A tools/win/ShowThreadNames/.gitignore View 1 1 chunk +4 lines, -0 lines 0 comments Download
A tools/win/ShowThreadNames/ReadMe.txt View 1 1 chunk +61 lines, -0 lines 0 comments Download
A tools/win/ShowThreadNames/ShowThreadNames.cc View 1 2 3 1 chunk +142 lines, -0 lines 0 comments Download
A tools/win/ShowThreadNames/ShowThreadNames.sln View 1 chunk +28 lines, -0 lines 0 comments Download
A tools/win/ShowThreadNames/ShowThreadNames.vcxproj View 1 1 chunk +118 lines, -0 lines 0 comments Download

Messages

Total messages: 54 (44 generated)
chengx
PTAL~
3 years, 10 months ago (2017-02-23 01:35:02 UTC) #10
brucedawson
The description needs a bit of scrubbing, and you also need to add a .gitignore ...
3 years, 10 months ago (2017-02-23 01:57:26 UTC) #13
stanisc
https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNames/ShowThreadNames.cc File tools/win/ShowThreadNames/ShowThreadNames.cc (right): https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNames/ShowThreadNames.cc#newcode21 tools/win/ShowThreadNames/ShowThreadNames.cc:21: // The GetThreadDescription API is available since Windows 10, ...
3 years, 10 months ago (2017-02-23 02:29:59 UTC) #19
chengx
I have addressed all the comments in the new patch set. PTAL~ https://codereview.chromium.org/2713773002/diff/80001/tools/win/ShowThreadNames/ReadMe.txt File tools/win/ShowThreadNames/ReadMe.txt ...
3 years, 10 months ago (2017-02-23 03:26:04 UTC) #28
stanisc
lgtm with some nits https://codereview.chromium.org/2713773002/diff/140001/tools/win/ShowThreadNames/ShowThreadNames.cc File tools/win/ShowThreadNames/ShowThreadNames.cc (right): https://codereview.chromium.org/2713773002/diff/140001/tools/win/ShowThreadNames/ShowThreadNames.cc#newcode24 tools/win/ShowThreadNames/ShowThreadNames.cc:24: // in Chrome. Binding SetThreadDescription ...
3 years, 10 months ago (2017-02-23 19:19:45 UTC) #41
brucedawson
https://codereview.chromium.org/2713773002/diff/180001/tools/win/ShowThreadNames/ShowThreadNames.cc File tools/win/ShowThreadNames/ShowThreadNames.cc (right): https://codereview.chromium.org/2713773002/diff/180001/tools/win/ShowThreadNames/ShowThreadNames.cc#newcode24 tools/win/ShowThreadNames/ShowThreadNames.cc:24: // The reason why this API is binded in ...
3 years, 10 months ago (2017-02-23 19:33:12 UTC) #42
brucedawson
lgtm with some minor nits. https://codereview.chromium.org/2713773002/diff/180001/tools/win/ShowThreadNames/ShowThreadNames.cc File tools/win/ShowThreadNames/ShowThreadNames.cc (right): https://codereview.chromium.org/2713773002/diff/180001/tools/win/ShowThreadNames/ShowThreadNames.cc#newcode137 tools/win/ShowThreadNames/ShowThreadNames.cc:137: } Put a line ...
3 years, 10 months ago (2017-02-23 19:37:32 UTC) #43
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2713773002/200001
3 years, 10 months ago (2017-02-23 20:43:58 UTC) #50
chengx
I have addressed all the feedback in the new patch set. Thanks for reviewing this ...
3 years, 10 months ago (2017-02-23 20:45:06 UTC) #51
commit-bot: I haz the power
3 years, 10 months ago (2017-02-23 20:49:30 UTC) #54
Message was sent while issue was closed.
Committed patchset #4 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/b97c0922116dbeb62d81790daf47...

Powered by Google App Engine
This is Rietveld 408576698