Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 10033015: Fix ProcessOutputWatcherTest.OutputWatcher under ASAN (Closed)

Created:
6 years, 5 months ago by tbarzic
Modified:
2 months, 3 weeks ago
Reviewers:
oshima
CC:
chromium-reviews, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org
Visibility:
Public.

Description

Fix ProcessOutputWatcherTest.OutputWatcher under ASAN String length was being set using arraysize of string that had an extra character. BUG=122700 TEST=ProcessOutputWatcher.OutputWatcher under ASAN Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=131703

Patch Set 1 #

Patch Set 2 : rebase #

Patch Set 3 : remove extra line #

Total comments: 2

Patch Set 4 : . #

Total comments: 5

Patch Set 5 : . #

Total comments: 1

Patch Set 6 : comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+66 lines, -54 lines) Patch
M chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc View 1 2 3 4 5 4 chunks +66 lines, -54 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
oshima
http://codereview.chromium.org/10033015/diff/3001/chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc File chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc (right): http://codereview.chromium.org/10033015/diff/3001/chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc#newcode131 chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc:131: arraysize(line_with_null_char) - 1, Maybe I'm dumb, but sorry I ...
6 years, 5 months ago (2012-04-10 00:58:11 UTC) #1
tonibarzic
http://codereview.chromium.org/10033015/diff/3001/chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc File chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc (right): http://codereview.chromium.org/10033015/diff/3001/chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc#newcode131 chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc:131: arraysize(line_with_null_char) - 1, On 2012/04/10 00:58:12, oshima wrote: > ...
6 years, 5 months ago (2012-04-10 01:16:59 UTC) #2
tbarzic
Hey oshima, can you take another look at this. I modified the test not to ...
6 years, 5 months ago (2012-04-10 17:28:24 UTC) #3
tbarzic
https://chromiumcodereview.appspot.com/10033015/diff/6/chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc File chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc (left): https://chromiumcodereview.appspot.com/10033015/diff/6/chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc#oldcode118 chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc:118: // TODO(tbarzic): We don't support stderr anymore, so this ...
6 years, 5 months ago (2012-04-10 17:30:05 UTC) #4
oshima
http://codereview.chromium.org/10033015/diff/6/chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc File chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc (right): http://codereview.chromium.org/10033015/diff/6/chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc#newcode161 chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc:161: test_cases.push_back(TestCase("a", false)); true?
6 years, 5 months ago (2012-04-10 18:23:03 UTC) #5
tbarzic
https://chromiumcodereview.appspot.com/10033015/diff/6/chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc File chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc (right): https://chromiumcodereview.appspot.com/10033015/diff/6/chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc#newcode161 chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc:161: test_cases.push_back(TestCase("a", false)); On 2012/04/10 18:23:03, oshima wrote: > true? ...
6 years, 5 months ago (2012-04-10 18:46:40 UTC) #6
oshima
6 years, 5 months ago (2012-04-10 18:55:00 UTC) #7
LGTM

https://chromiumcodereview.appspot.com/10033015/diff/7001/chrome/browser/chro...
File chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10033015/diff/7001/chrome/browser/chro...
chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc:161:
test_cases.push_back(TestCase("a", true));
If you intention was to send null, and then make sure next one works (with
false), please mention that in the comment. It's not so obvious and whoever may
have to maintain this code in the future will get confused.

Powered by Google App Engine
This is Rietveld 408576698