Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
177843009:
Make GetUserName() return more specific error code, so that we could know how the function failed. (Closed)
|
Created:
6 years, 10 months ago by yao Modified:
6 years, 9 months ago CC:
chromium-reviews, Alexei Svitkine (slow) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMake GetUserName() return more specific error code, so that we could know how the function failed.
However, a lot user at under version 33 or lower, which means they don't auto-update Chrome? I'm not sure if this new piece of code will get to the affected users.
BUG=334675
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255985
Patch Set 1 #Patch Set 2 : Add CHECK for debugging. #
Total comments: 2
Patch Set 3 : build #Patch Set 4 : Build. #
Total comments: 4
Patch Set 5 : Apply Check directly to the function. #
Total comments: 4
Patch Set 6 : Address comments #Messages
Total messages: 26 (0 generated)
The comments below apply throughout the file. I'm not sure why I was asked to review this... did you mean to ask someone else? https://codereview.chromium.org/177843009/diff/40001/rlz/win/lib/process_info.cc File rlz/win/lib/process_info.cc (right): https://codereview.chromium.org/177843009/diff/40001/rlz/win/lib/process_info... rlz/win/lib/process_info.cc:39: CHECK(0) Please try to compile code before sending for a review. Please put semicolons at end of each line of code. Don't bother doing anything else once you hit a "CHECK". Perhaps clearer would be: CHECK_NE(process_handle, 0); https://codereview.chromium.org/177843009/diff/40001/rlz/win/lib/process_info... rlz/win/lib/process_info.cc:40: return ::GetLastError() Note that IF you used a semicolon (on line 39), since this is NOT Python, you would run the return statement all the time, and never reach line 42.
https://codereview.chromium.org/177843009/diff/140001/rlz/win/lib/process_inf... File rlz/win/lib/process_info.cc (right): https://codereview.chromium.org/177843009/diff/140001/rlz/win/lib/process_inf... rlz/win/lib/process_info.cc:47: } From the docs, I don't believe getting a handle to yourself can fail. So I think you can replace all the above with something like: CHECK(::OpenProcessToken(::GetCurrentProcess(), TOKEN_QUERY, &token)); I think you can do something similar for calls to ::GetTokenInformation() and ::LookupAccountSidW() below. https://codereview.chromium.org/177843009/diff/140001/rlz/win/lib/process_inf... rlz/win/lib/process_info.cc:66: } Can probably replace lines 62-66 with: CHECK(token_user_bytes.get()); Similar comment for line 83 below. I don't think you need a |zero| local variable.
https://codereview.chromium.org/177843009/diff/140001/rlz/win/lib/process_inf... File rlz/win/lib/process_info.cc (right): https://codereview.chromium.org/177843009/diff/140001/rlz/win/lib/process_inf... rlz/win/lib/process_info.cc:47: } On 2014/02/28 19:34:00, Roger Tawa wrote: > From the docs, I don't believe getting a handle to yourself can fail. So I > think you can replace all the above with something like: > > CHECK(::OpenProcessToken(::GetCurrentProcess(), TOKEN_QUERY, &token)); > > I think you can do something similar for calls to ::GetTokenInformation() and > ::LookupAccountSidW() below. Done. https://codereview.chromium.org/177843009/diff/140001/rlz/win/lib/process_inf... rlz/win/lib/process_info.cc:66: } On 2014/02/28 19:34:00, Roger Tawa wrote: > Can probably replace lines 62-66 with: > > CHECK(token_user_bytes.get()); > > Similar comment for line 83 below. I don't think you need a |zero| local > variable. Done.
lgtm after addressing two comments below. https://codereview.chromium.org/177843009/diff/180001/rlz/win/lib/process_inf... File rlz/win/lib/process_info.cc (right): https://codereview.chromium.org/177843009/diff/180001/rlz/win/lib/process_inf... rlz/win/lib/process_info.cc:55: token_user_size, &token_user_size2)) Remove extra space, add missing ; at end. https://codereview.chromium.org/177843009/diff/180001/rlz/win/lib/process_inf... rlz/win/lib/process_info.cc:65: CHECK(0); You can replace lines 64-65 with: CHECK(token_user);
Note: also missing a ; at end of line 69.
Hi Roger, It now passes the test rlz_unittests. Thanks, Yiyao https://codereview.chromium.org/177843009/diff/180001/rlz/win/lib/process_inf... File rlz/win/lib/process_info.cc (right): https://codereview.chromium.org/177843009/diff/180001/rlz/win/lib/process_inf... rlz/win/lib/process_info.cc:55: token_user_size, &token_user_size2)) On 2014/03/01 14:54:58, Roger Tawa wrote: > Remove extra space, add missing ; at end. Done. https://codereview.chromium.org/177843009/diff/180001/rlz/win/lib/process_inf... rlz/win/lib/process_info.cc:65: CHECK(0); On 2014/03/01 14:54:58, Roger Tawa wrote: > You can replace lines 64-65 with: > > CHECK(token_user); Done.
The CQ bit was checked by yiyaoliu@google.com
The CQ bit was unchecked by yiyaoliu@google.com
The CQ bit was checked by yiyaoliu@google.com
The CQ bit was unchecked by yiyaoliu@google.com
The CQ bit was checked by yiyaoliu@google.com
The CQ bit was unchecked by yiyaoliu@google.com
The CQ bit was checked by yiyaoliu@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yiyaoliu@chromium.org/177843009/270001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yiyaoliu@chromium.org/177843009/270001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yiyaoliu@chromium.org/177843009/270001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by yiyaoliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yiyaoliu@chromium.org/177843009/270001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yiyaoliu@chromium.org/177843009/270001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yiyaoliu@chromium.org/177843009/270001
Message was sent while issue was closed.
Change committed as 255985 |
