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

Issue 6624039: libchromeos: add pipe support to Process (Closed)

Created:
9 years, 9 months ago by kmixter1
Modified:
9 years, 7 months ago
Reviewers:
petkov
CC:
chromium-os-reviews_chromium.org
Visibility:
Public.

Description

libchromeos: add pipe support to Process Change-Id: Ic09957371b72b4649144320cc504d207e9b7d37b BUG=chromium-os:11814 TEST=unit tests Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=4694200

Patch Set 1 #

Total comments: 13

Patch Set 2 : respond to petkov review, add pipe map check #

Total comments: 18

Patch Set 3 : respond to petkov #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+189 lines, -11 lines) Patch
M chromeos/process.h View 1 2 5 chunks +31 lines, -5 lines 0 comments Download
M chromeos/process.cc View 1 2 4 chunks +81 lines, -6 lines 1 comment Download
M chromeos/process_mock.h View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M chromeos/process_test.cc View 1 2 5 chunks +75 lines, -0 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
kmixter1
9 years, 9 months ago (2011-03-05 02:31:01 UTC) #1
petkov
Thanks for the nicely sized CL :-) http://codereview.chromium.org/6624039/diff/1/chromeos/process.cc File chromeos/process.cc (right): http://codereview.chromium.org/6624039/diff/1/chromeos/process.cc#newcode48 chromeos/process.cc:48: void ProcessImpl::RedirectUsingPipe(int ...
9 years, 9 months ago (2011-03-07 19:05:46 UTC) #2
kmixter1
PTAL - and see added code. http://codereview.chromium.org/6624039/diff/1/chromeos/process.cc File chromeos/process.cc (right): http://codereview.chromium.org/6624039/diff/1/chromeos/process.cc#newcode48 chromeos/process.cc:48: void ProcessImpl::RedirectUsingPipe(int child_fd, ...
9 years, 9 months ago (2011-03-09 21:29:45 UTC) #3
petkov
http://codereview.chromium.org/6624039/diff/3001/chromeos/process.cc File chromeos/process.cc (right): http://codereview.chromium.org/6624039/diff/3001/chromeos/process.cc#newcode90 chromeos/process.cc:90: if (i->second.child_fd_ != i->first && given that you just ...
9 years, 9 months ago (2011-03-09 21:55:54 UTC) #4
kmixter1
PTAL http://codereview.chromium.org/6624039/diff/3001/chromeos/process.cc File chromeos/process.cc (right): http://codereview.chromium.org/6624039/diff/3001/chromeos/process.cc#newcode90 chromeos/process.cc:90: if (i->second.child_fd_ != i->first && Added a precondition ...
9 years, 9 months ago (2011-03-09 23:24:06 UTC) #5
petkov
9 years, 9 months ago (2011-03-09 23:32:32 UTC) #6
LGTM

http://codereview.chromium.org/6624039/diff/9001/chromeos/process.cc
File chromeos/process.cc (right):

http://codereview.chromium.org/6624039/diff/9001/chromeos/process.cc#newcode132
chromeos/process.cc:132: // then close our ends if they differ.
then close our ends.

Powered by Google App Engine
This is Rietveld 408576698