Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2127993002:
[chromedriver] Handle missing WCHAN field in output from ps. (Closed)
Description[chromedriver] Handle missing WCHAN field in output from ps.
Accounts for issue where ps command isn't returning a - when a field is blank, throwing off the tokenization.
This update takes last field as the app name (instead of absolute index of [8]) and allows for 1 missing field.
BUG=chromedriver:1378
Committed: https://crrev.com/9ac8817f503a25ed86db93584813e8b6e5c7f6a8
Cr-Commit-Position: refs/heads/master@{#406296}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Addressing CR feedback #Patch Set 3 : Removing extraneous whitespace after comment #Messages
Total messages: 27 (17 generated)
Description was changed from ========== Fix for https://bugs.chromium.org/p/chromedriver/issues/detail?id=1378# Accounts for issue where ps command isn't returning a - when a field is blank, throwing off the tokenization. This update takes last field as the app name (instead of absolute index of [8]) and allows for 1 missing field. BUG=1378 ========== to ========== Fix for https://bugs.chromium.org/p/chromedriver/issues/detail?id=1378# Accounts for issue where ps command isn't returning a - when a field is blank, throwing off the tokenization. This update takes last field as the app name (instead of absolute index of [8]) and allows for 1 missing field. BUG= ==========
szurgotc@amazon.com changed reviewers: + frankf@chromium.org, samuong@chromium.org
(Actually adding reviewers) We've run multiple test passes internally, and have had success with no internal issues.
szurgotc@amazon.com changed reviewers: + gmanikpure@chromium.org
Adding @gmanikpure as he marked the issue in question as valid and available.
samuong@chromium.org changed reviewers: - frankf@chromium.org, gmanikpure@chromium.org
https://codereview.chromium.org/2127993002/diff/1/AUTHORS File AUTHORS (right): https://codereview.chromium.org/2127993002/diff/1/AUTHORS#newcode719 AUTHORS:719: Chris Szrugot <szurgotc@amazon.com> can you put your name further up the file, so it's in alphabetic order? https://codereview.chromium.org/2127993002/diff/1/chrome/test/chromedriver/ch... File chrome/test/chromedriver/chrome/adb_impl.cc (right): https://codereview.chromium.org/2127993002/diff/1/chrome/test/chromedriver/ch... chrome/test/chromedriver/chrome/adb_impl.cc:211: if (tokens.size() < 8) we still want to skip records when size > 9, so: tokens.size() != 8 && tokens.size() != 9 https://codereview.chromium.org/2127993002/diff/1/chrome/test/chromedriver/ch... chrome/test/chromedriver/chrome/adb_impl.cc:213: if (tokens[tokens.size() - 1] == process_name) { can you add a comment above this line along the lines of: "The ps command on Android M+ does not always output a value for WCHAN, so the process name might appear in the 8th or 9th column. Use the right-most column for process name."
Updated based on CR feedback.
Description was changed from ========== Fix for https://bugs.chromium.org/p/chromedriver/issues/detail?id=1378# Accounts for issue where ps command isn't returning a - when a field is blank, throwing off the tokenization. This update takes last field as the app name (instead of absolute index of [8]) and allows for 1 missing field. BUG= ========== to ========== [chromedriver] Handle missing WCHAN field in output from ps. Accounts for issue where ps command isn't returning a - when a field is blank, throwing off the tokenization. This update takes last field as the app name (instead of absolute index of [8]) and allows for 1 missing field. BUG=chromedriver:1378 ==========
i've tweaked the subject/description a bit, lgtm
The CQ bit was checked by szurgotc@amazon.com 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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by szurgotc@amazon.com 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 szurgotc@amazon.com
The patchset sent to the CQ was uploaded after l-g-t-m from samuong@chromium.org Link to the patchset: https://codereview.chromium.org/2127993002/#ps40001 (title: "Removing extraneous whitespace after comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [chromedriver] Handle missing WCHAN field in output from ps. Accounts for issue where ps command isn't returning a - when a field is blank, throwing off the tokenization. This update takes last field as the app name (instead of absolute index of [8]) and allows for 1 missing field. BUG=chromedriver:1378 ========== to ========== [chromedriver] Handle missing WCHAN field in output from ps. Accounts for issue where ps command isn't returning a - when a field is blank, throwing off the tokenization. This update takes last field as the app name (instead of absolute index of [8]) and allows for 1 missing field. BUG=chromedriver:1378 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [chromedriver] Handle missing WCHAN field in output from ps. Accounts for issue where ps command isn't returning a - when a field is blank, throwing off the tokenization. This update takes last field as the app name (instead of absolute index of [8]) and allows for 1 missing field. BUG=chromedriver:1378 ========== to ========== [chromedriver] Handle missing WCHAN field in output from ps. Accounts for issue where ps command isn't returning a - when a field is blank, throwing off the tokenization. This update takes last field as the app name (instead of absolute index of [8]) and allows for 1 missing field. BUG=chromedriver:1378 Committed: https://crrev.com/9ac8817f503a25ed86db93584813e8b6e5c7f6a8 Cr-Commit-Position: refs/heads/master@{#406296} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9ac8817f503a25ed86db93584813e8b6e5c7f6a8 Cr-Commit-Position: refs/heads/master@{#406296} |
