Issue 6995121: New NaCl zygote implementation 2

Issue 6995121: New NaCl zygote implementation 2 (Closed)

Created:
9 years, 6 months ago by Brad Chen

Modified:
9 years, 5 months ago

Reviewers:
jam, Evan Martin, commit-bot: I haz the power, Mark Seaborn, darin (slow to review), agl, gregoryd

CC:
chromium-reviews, joi+watch-content_chromium.org

Base URL:
http://git.chromium.org/git/chromium.git@trunk

Visibility:
Public.

Description

New NaCl zygote implementation 2, in which Chrome zygote forks a NaCl helper. This patch can launch earth_c.html with and without the SUID sandbox. It is enabled with the environment variable NACL_NEW_ZYGOTE. BUG=nativeclient:480 TEST=nativeclient in-browser tests on Linux, ChromeOS Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=90681

Patch Set 1 #

Patch Set 2 : Second cut at NaCl-specific process launcher. #

Patch Set 3 : New nacl helper for Linux. Works inside suid sandbox. #

Patch Set 4 : NaCl helper for Chrome Linux zygote #

Total comments: 45

Patch Set 5 : agl feedback #

Patch Set 6 : Responding to reviewer feedback. #

Patch Set 7 : Fix sandbox descriptor passing #

Patch Set 8 : A few final cleanups. #

Patch Set 9 : Fixing include paths as per Chrome DEPS requirements. #

Patch Set 10 : Fixing includes for checkdeps.py #

Total comments: 31

Patch Set 11 : Addressing review feedback from Mark Seaborn #

Patch Set 12 : Moved NaCl code out of content; added zygote_fork_delegate. #

Patch Set 13 : Dropping mods to content/browser/DEPS #

Total comments: 14

Patch Set 14 : Addressing Adam's latest feedback. #

Patch Set 15 : Use IPC::Channel directly instead of ChildProcess #

Total comments: 27

Patch Set 16 : Addressing feedback from John #

Total comments: 9

Patch Set 17 : Addressing feedback from jam@ and gregoryd@ and the bots #

Patch Set 18 : Get IPC channel handle from command line #

Patch Set 19 : Fix regression for non-linux platforms #

Patch Set 20 : Fix regression for non-linux platforms #

Patch Set 21 : Eliminate duplicate code in nacl_main and nacl_helper_linux #

Patch Set 22 : Fixed loading with old zygote #

Patch Set 23 : Enable with command-line flag, pass channel ID from zygote #

Total comments: 20

Patch Set 24 : Addressing more feedback from jam@ #

Total comments: 8

Patch Set 25 : Final revs, for comments from Darin #

Patch Set 26 : Missed a whitespace change. #

Patch Set 27 : Removing and re-adding nacl_listener.cc in attempt to fix error from bot (step 1 - remove) #

Patch Set 28 : Re-adding nacl_listener.cc #

Patch Set 29 : Fix build problem on windows with _linux.h include files #

Patch Set 30 : Follow #define conventions #

Total comments: 35

Patch Set 31 : Addressing feedback from evanm #

Patch Set 32 : Missing include... #

Total comments: 36

Patch Set 33 : Responding to latest feedback from agl@ and evanm@ #

Patch Set 34 : Fixing includes for Windows bots #

Created: 9 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+648 lines, -255 lines)			Patch
M	base/process_util.h	View	1 2 3 4 5	1 chunk	+9 lines, -0 lines	0 comments	Download
M	base/process_util_posix.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25	4 chunks	+31 lines, -4 lines	0 comments	Download
M	chrome/app/chrome_main.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32	3 chunks	+10 lines, -2 lines	0 comments	Download
A	chrome/common/nacl_fork_delegate_linux.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32	1 chunk	+37 lines, -0 lines	0 comments	Download
A	chrome/common/nacl_helper_linux.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30	1 chunk	+46 lines, -0 lines	0 comments	Download
M	chrome/common/nacl_types.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33	2 chunks	+3 lines, -1 line	0 comments	Download
M	chrome/nacl.gypi	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32	3 chunks	+22 lines, -2 lines	0 comments	Download
A	chrome/nacl/nacl_fork_delegate_linux.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32	1 chunk	+113 lines, -0 lines	0 comments	Download
A	chrome/nacl/nacl_helper_linux.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32	1 chunk	+161 lines, -0 lines	0 comments	Download
D	chrome/nacl/nacl_launcher_thread.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 19 20	1 chunk	+0 lines, -33 lines	0 comments	Download
D	chrome/nacl/nacl_launcher_thread.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 19 20	1 chunk	+0 lines, -131 lines	0 comments	Download
A	chrome/nacl/nacl_listener.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32	1 chunk	+31 lines, -0 lines	0 comments	Download
A +	chrome/nacl/nacl_listener.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32	4 chunks	+33 lines, -29 lines	0 comments	Download
M	chrome/nacl/nacl_main.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30	2 chunks	+5 lines, -5 lines	0 comments	Download
M	content/browser/child_process_launcher.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32	2 chunks	+3 lines, -3 lines	0 comments	Download
M	content/browser/zygote_host_linux.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30	2 chunks	+6 lines, -4 lines	0 comments	Download
M	content/browser/zygote_host_linux.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22	3 chunks	+6 lines, -4 lines	0 comments	Download
M	content/browser/zygote_main_linux.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32	13 chunks	+73 lines, -35 lines	0 comments	Download
M	content/common/content_switches.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32	1 chunk	+4 lines, -0 lines	0 comments	Download
M	content/common/content_switches.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32	1 chunk	+4 lines, -0 lines	0 comments	Download
M	content/common/sandbox_methods_linux.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24	2 chunks	+2 lines, -2 lines	0 comments	Download
A	content/common/zygote_fork_delegate_linux.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32	1 chunk	+49 lines, -0 lines	0 comments	Download

Messages

Total messages: 44 (0 generated)

Expand Messages | Collapse Messages

Brad Chen

I think this is ready for your reviews. Things that I would like to take ...

9 years, 6 months ago (2011-06-10 17:02:54 UTC) #1

agl

One general comment: the use of send() and read()/recv() everywhere needs to be wrapped in ...

9 years, 6 months ago (2011-06-10 17:28:03 UTC) #2

Brad Chen

I believe I've responded to almost all the review feedback. One question for Adam below. ...

9 years, 6 months ago (2011-06-14 00:16:01 UTC) #3

agl

http://codereview.chromium.org/6995121/diff/5001/content/browser/zygote_main_linux.cc File content/browser/zygote_main_linux.cc (right): http://codereview.chromium.org/6995121/diff/5001/content/browser/zygote_main_linux.cc#newcode277 content/browser/zygote_main_linux.cc:277: DLOG(INFO) << "suid sandbox NOT active"; On 2011/06/14 00:16:01, ...

9 years, 6 months ago (2011-06-14 14:38:35 UTC) #4

Brad Chen

9 years, 6 months ago (2011-06-14 22:14:01 UTC) #5

jam

NaCl is completely in the chrome layer at this point, i.e. there's no mention of ...

9 years, 6 months ago (2011-06-14 22:33:38 UTC) #6

jam

http://codereview.chromium.org/6995121/diff/16003/chrome/nacl/nacl_helper_linux.cc File chrome/nacl/nacl_helper_linux.cc (right): http://codereview.chromium.org/6995121/diff/16003/chrome/nacl/nacl_helper_linux.cc#newcode48 chrome/nacl/nacl_helper_linux.cc:48: bool FindProxyForUrl(const GURL& url, std::string* proxy_list) { btw this ...

9 years, 6 months ago (2011-06-15 00:35:31 UTC) #7

Brad Chen

Sorry to hear about the problems with the nacl_win64. In this case I'd be happy ...

9 years, 6 months ago (2011-06-15 02:18:40 UTC) #8

Mark Seaborn

Have you checked that all of chrome_browser_tests passes with this? http://codereview.chromium.org/6995121/diff/16003/base/process_util.h File base/process_util.h (right): http://codereview.chromium.org/6995121/diff/16003/base/process_util.h#newcode292 ...

9 years, 6 months ago (2011-06-15 16:09:04 UTC) #9

Brad Chen

Please see responses to Mark's comments below. It will take more time to address John's ...

9 years, 6 months ago (2011-06-15 18:47:47 UTC) #10

Mark Seaborn

On 15 June 2011 11:47, <bradchen@google.com> wrote: > http://codereview.chromium.org/6995121/diff/16003/base/process_util.h > File base/process_util.h (right): > > ...

9 years, 6 months ago (2011-06-15 19:23:26 UTC) #11

On 15 June 2011 11:47, <bradchen@google.com> wrote:

> http://codereview.chromium.org/6995121/diff/16003/base/process_util.h
> File base/process_util.h (right):
>
>
>
http://codereview.chromium.org/6995121/diff/16003/base/process_util.h#newcode292
> base/process_util.h:292: BASE_API bool LaunchAppWithClone(const
> std::vector<std::string>& argv,
>
> Hmm... I'd have to mimic/copy ~100 lines of code from LaunchAppImpl() if
> I didn't create this new variant. That seems much worse than this
> change.
>

No, because you don't need most of the stuff in LaunchAppImpl().  You don't
need any of the stuff about:

 * setting environment
 * /dev/null
 * process groups
 * signal handlers
 * CloseSuperfluousFds

In fact all this extra stuff could break things.  If people add more stuff
in there in the future, it could break in the rather unusual case of using
clone()+CLONE_FS.  Doing clone()+execve() directly would be much better.  I
really think Brett's e-mail is right here, and it doesn't make sense to add
things to 'base' that have only one user.


On 2011/06/15 16:09:04, Mark Seaborn wrote:
>
>> I've not seen MSG_EOR before.  What function is it performing here?
>>
> http://codereview.chromium.**org/6995121/diff/16003/chrome/**
>
nacl/nacl_helper_linux.cc#**newcode154<http://codereview.chromium.org/6995121/diff/16003/chrome/nacl/nacl_helper_linux.cc#newcode154>
> chrome/nacl/nacl_helper_linux.**cc:154: sizeof(kNaClHelperMagic),
> MSG_EOR)) !=
> It is to terminate the message for the UnixDomainSocket::RecvMsg on the
> other side.
>

You shouldn't need this.  You're using Linux SOCK_SEQPACKET sockets which
are inherently message-based.  Chromium uses SOCK_SEQPACKET sockets in
various places without using MSG_EOR.


http://codereview.chromium.org/6995121/diff/16003/content/browser/zygote_main...
> content/browser/zygote_main_**linux.cc:516: base::LaunchAppWithClone(argv,
> fds_to_map, false, &g_naclpid,
> This code works, which suggests I might not be messing up FDs in the
> calling process. We might talk more about this if you'd like.
>
> On 2011/06/15 16:09:04, Mark Seaborn wrote:
>
>> If this takes fds_to_map, and you're passing CLONE_FS, won't this mess up
>> the
>>
> FDs in the calling process?  Surely the LaunchApp function will try to
>> close all
>>
> FDs except those in fds_to_map.
>>
>
Ah, it turns out clone() provides two flags, CLONE_FS (for sharing cwd/root)
and CLONE_FILES (for sharing the FD table).  Since you're just passing
CLONE_FS, this is fine and one process won't mess up the FD table of the
other.  This is safer than I initially thought.

Mark

Brad Chen

John, Adam; I've moved the NaCl code out of content/ using a ZygoteForkDelegate class. Please ...

9 years, 6 months ago (2011-06-17 18:34:27 UTC) #12

agl

http://codereview.chromium.org/6995121/diff/29001/chrome/browser/nacl_fork_delegate.cc File chrome/browser/nacl_fork_delegate.cc (right): http://codereview.chromium.org/6995121/diff/29001/chrome/browser/nacl_fork_delegate.cc#newcode47 chrome/browser/nacl_fork_delegate.cc:47: base::LaunchAppWithClone(argv, fds_to_map, false, &pid_, This looks like Linux specific ...

9 years, 6 months ago (2011-06-20 17:28:55 UTC) #13

Brad Chen

I believe I've addressed all feedback at this point. I haven't had any more detailed ...

9 years, 6 months ago (2011-06-20 22:09:46 UTC) #14

I believe I've addressed all feedback at this point. I haven't had any more
detailed feedback from John yet but will be talking to him soon. The base
changes will require an additional review from somebody, maybe Brett or Darin.

http://codereview.chromium.org/6995121/diff/16003/base/process_util.h
File base/process_util.h (right):

http://codereview.chromium.org/6995121/diff/16003/base/process_util.h#newcode292
base/process_util.h:292: BASE_API bool LaunchAppWithClone(const
std::vector<std::string>& argv,
Today I spent some time trying to use clone/execvp directly. I am getting
mysterious failures in another thread, process_ not defined. After copying a
bunch of code from LaunchAppImpl() it appears to me it is not a simple matter of
clone+execvp, and it would be better to add LaunchAppWithClone() rather than
complicated/duplicate launching functionality in the caller. Of course I will
clear this with an OWNER on base before committing. 

On 2011/06/15 18:47:47, Brad Chen wrote:
> Hmm... I'd have to mimic/copy ~100 lines of code from LaunchAppImpl() if I
> didn't create this new variant. That seems much worse than this change.
> 
> On 2011/06/15 16:09:04, Mark Seaborn wrote:
> > Please bear in mind Brett Wilson's post from January on chromium-dev: 'Base
is
> > "done"';
> >
>
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre....
> > 
> > Calling clone()+execve() is simple enough that you probably don't need to
add
> a
> > new function to 'base' for it.
>

http://codereview.chromium.org/6995121/diff/29001/chrome/browser/nacl_fork_de...
File chrome/browser/nacl_fork_delegate.cc (right):

http://codereview.chromium.org/6995121/diff/29001/chrome/browser/nacl_fork_de...
chrome/browser/nacl_fork_delegate.cc:47: base::LaunchAppWithClone(argv,
fds_to_map, false, &pid_,
I renamed the file to nacl/nacl_fork_delegate_linux.cc.

The code runs in the zygote. When the zygote wants to fork, it first asks the
delegate if it wants to implement fork. If the delegate says "no" then the
zygote does the fork itself. I've added an explanatory comment to the .h file as
I believe is the convention. If that doesn't seem like enough let me know. 

On 2011/06/20 17:28:55, agl wrote:
> This looks like Linux specific code. If so, the filename should have _linux at
> the end.
> 
> Also, I must admit that I'm lost here. Where does this code run?

http://codereview.chromium.org/6995121/diff/29001/chrome/browser/nacl_fork_de...
chrome/browser/nacl_fork_delegate.cc:50: close(fds[1]);
On 2011/06/20 17:28:55, agl wrote:
> HANDLE_EINTR around this.

Done.

http://codereview.chromium.org/6995121/diff/29001/chrome/browser/nacl_fork_de...
chrome/browser/nacl_fork_delegate.cc:52: const int kExpectedLength =
sizeof(kNaClHelperStartupAck);
On 2011/06/20 17:28:55, agl wrote:
> s/int/size_t/

Done.

http://codereview.chromium.org/6995121/diff/29001/chrome/browser/nacl_fork_de...
chrome/browser/nacl_fork_delegate.cc:71: close(fds[0]);
On 2011/06/20 17:28:55, agl wrote:
> HANDLE_EINTR

Done.

http://codereview.chromium.org/6995121/diff/29001/chrome/browser/nacl_fork_de...
chrome/browser/nacl_fork_delegate.cc:75: close(fd_);  // side effect: delegate
process will terminate
On 2011/06/20 17:28:55, agl wrote:
> HANDLE_EINTR()

Done.

http://codereview.chromium.org/6995121/diff/29001/chrome/nacl/nacl_helper_lin...
File chrome/nacl/nacl_helper_linux.cc (right):

http://codereview.chromium.org/6995121/diff/29001/chrome/nacl/nacl_helper_lin...
chrome/nacl/nacl_helper_linux.cc:106: base::mac::ScopedNSAutoreleasePool
autorelease_pool;
Agreed! In the next revision NaClMain() has been deconstructed such that the
scoped pointer stuff is no longer needed.

On 2011/06/20 17:28:55, agl wrote:
> It's odd to see "base::mac::" in a _linux file.

http://codereview.chromium.org/6995121/diff/29001/content/common/zygote_fork_...
File content/common/zygote_fork_delegate.h (right):

http://codereview.chromium.org/6995121/diff/29001/content/common/zygote_fork_...
content/common/zygote_fork_delegate.h:10: 
On 2011/06/20 17:28:55, agl wrote:
> This file need a large number of comments, for each class and each method.

Done.

Brad Chen

I'm sorry this has taken so many iterations. This update incorporates more of John's suggestions ...

9 years, 6 months ago (2011-06-21 17:46:01 UTC) #15

Brad Chen

I checked with Darin and he is okay with the notion of adding LaunchAppWithClone to ...

9 years, 6 months ago (2011-06-21 17:58:17 UTC) #16

jam

http://codereview.chromium.org/6995121/diff/33001/chrome/chrome.gyp File chrome/chrome.gyp (right): http://codereview.chromium.org/6995121/diff/33001/chrome/chrome.gyp#newcode1316 chrome/chrome.gyp:1316: 'browser/renderer_host/render_process_host_dummy.cc', is this needed? http://codereview.chromium.org/6995121/diff/33001/chrome/chrome.gyp#newcode1318 chrome/chrome.gyp:1318: 'app/dummy_main_functions.cc', ditto (and ...

9 years, 6 months ago (2011-06-21 20:54:08 UTC) #17

Brad Chen

PTAL Still working on the bots. http://codereview.chromium.org/6995121/diff/33001/chrome/chrome.gyp File chrome/chrome.gyp (right): http://codereview.chromium.org/6995121/diff/33001/chrome/chrome.gyp#newcode1316 chrome/chrome.gyp:1316: 'browser/renderer_host/render_process_host_dummy.cc', I've dropped ...

9 years, 6 months ago (2011-06-21 23:43:48 UTC) #18

jam

http://codereview.chromium.org/6995121/diff/33005/chrome/chrome.gyp File chrome/chrome.gyp (right): http://codereview.chromium.org/6995121/diff/33005/chrome/chrome.gyp#newcode1346 chrome/chrome.gyp:1346: 'target_name': 'nacl_helper', should this be in chrome\nacl.gyp? http://codereview.chromium.org/6995121/diff/33005/chrome/common/nacl_helper_linux.h File ...

9 years, 6 months ago (2011-06-22 00:49:52 UTC) #19

gregoryd

http://codereview.chromium.org/6995121/diff/33005/chrome/nacl/nacl_fork_delegate_linux.cc File chrome/nacl/nacl_fork_delegate_linux.cc (right): http://codereview.chromium.org/6995121/diff/33005/chrome/nacl/nacl_fork_delegate_linux.cc#newcode42 chrome/nacl/nacl_fork_delegate_linux.cc:42: const char* nacl_zygote_exe = getenv("NACL_NEW_ZYGOTE"); Will we need to ...

9 years, 6 months ago (2011-06-22 12:46:07 UTC) #20

Brad Chen

Good point. The environment variable is a temporary thing, for debugging. Once this is stable/tested/enabled ...

9 years, 6 months ago (2011-06-22 12:50:23 UTC) #21

jam

fyi the standard way of enabling features before they're ready for general use is a ...

9 years, 6 months ago (2011-06-22 16:05:52 UTC) #22

Brad Chen

http://codereview.chromium.org/6995121/diff/33005/chrome/chrome.gyp File chrome/chrome.gyp (right): http://codereview.chromium.org/6995121/diff/33005/chrome/chrome.gyp#newcode1346 chrome/chrome.gyp:1346: 'target_name': 'nacl_helper', On 2011/06/22 00:49:52, John Abd-El-Malek wrote: > ...

9 years, 6 months ago (2011-06-22 16:10:31 UTC) #23

Brad Chen

I believe the latest revision addresses all outstanding reviewer feedback. It appears to me that ...

9 years, 6 months ago (2011-06-23 23:05:12 UTC) #24

jam

just nits http://codereview.chromium.org/6995121/diff/49019/chrome/common/nacl_fork_delegate_linux.h File chrome/common/nacl_fork_delegate_linux.h (right): http://codereview.chromium.org/6995121/diff/49019/chrome/common/nacl_fork_delegate_linux.h#newcode9 chrome/common/nacl_fork_delegate_linux.h:9: #if defined(OS_LINUX) not needed http://codereview.chromium.org/6995121/diff/49019/chrome/common/nacl_fork_delegate_linux.h#newcode24 chrome/common/nacl_fork_delegate_linux.h:24: // ...

9 years, 6 months ago (2011-06-24 00:10:34 UTC) #25

Brad Chen

PTAL http://codereview.chromium.org/6995121/diff/49019/chrome/common/nacl_fork_delegate_linux.h File chrome/common/nacl_fork_delegate_linux.h (right): http://codereview.chromium.org/6995121/diff/49019/chrome/common/nacl_fork_delegate_linux.h#newcode9 chrome/common/nacl_fork_delegate_linux.h:9: #if defined(OS_LINUX) On 2011/06/24 00:10:34, John Abd-El-Malek wrote: ...

9 years, 6 months ago (2011-06-24 00:40:11 UTC) #26

jam

lgtm with these two small changes. thanks for bearing through :) http://codereview.chromium.org/6995121/diff/49019/content/common/zygote_fork_delegate_linux.h File content/common/zygote_fork_delegate_linux.h (right): ...

9 years, 6 months ago (2011-06-24 05:14:04 UTC) #27

Brad Chen

I've made the changes you suggest. Now looking for a base/ reviewer for review of ...

9 years, 6 months ago (2011-06-24 15:37:13 UTC) #28

darin (slow to review)

I just looked at base/* changes. LGTM w/ nits: http://codereview.chromium.org/6995121/diff/48007/base/process_util.h File base/process_util.h (right): http://codereview.chromium.org/6995121/diff/48007/base/process_util.h#newcode293 base/process_util.h:293: ...

9 years, 6 months ago (2011-06-24 16:32:19 UTC) #29

Brad Chen

Replies below. This is headed for the commit queue shortly. http://codereview.chromium.org/6995121/diff/48007/base/process_util.h File base/process_util.h (right): http://codereview.chromium.org/6995121/diff/48007/base/process_util.h#newcode293 ...

9 years, 6 months ago (2011-06-24 16:52:12 UTC) #30

commit-bot: I haz the power

Can't process patch for file chrome/nacl/nacl_listener.cc. A +

9 years, 6 months ago (2011-06-24 16:54:24 UTC) #31

Brad Chen

Patch set 29 fixes a #include file problem for Windows build in chrome/app/chrome_main.cc. John would ...

9 years, 6 months ago (2011-06-24 17:19:03 UTC) #33

Evan Martin

http://codereview.chromium.org/6995121/diff/54001/base/process_util.h File base/process_util.h (right): http://codereview.chromium.org/6995121/diff/54001/base/process_util.h#newcode296 base/process_util.h:296: #endif FYI, in the future it'd be easier to ...

9 years, 6 months ago (2011-06-24 18:57:33 UTC) #34

Brad Chen

Thanks Evan for the careful review. It looks to me like most of these are ...

9 years, 6 months ago (2011-06-24 20:09:17 UTC) #35

Thanks Evan for the careful review. It looks to me like most of these are
relatively easy to fix, although there are a couple cases where things are
as they are because of the content factoring work John is doing.

Would you object to me cleaning things up in a subsequent CL? I have some
family stuff I need to take care of this afternoon. I'm in on Monday, but
will be going on vacation on Tuesday, and I don't want to delay other work
that I'm hoping will happen while I'm gone. I can certainly have the
clean-up CL for you Monday.

Brad

On Fri, Jun 24, 2011 at 11:57 AM, <evan@chromium.org> wrote:

>
>
http://codereview.chromium.**org/6995121/diff/54001/base/**process_util.h<htt...
>
> File base/process_util.h (right):
>
> http://codereview.chromium.**org/6995121/diff/54001/base/**
>
process_util.h#newcode296<http://codereview.chromium.org/6995121/diff/54001/base/process_util.h#newcode296>
> base/process_util.h:296: #endif
> FYI, in the future it'd be easier to land this and review this if you
> landed this new API bits separately from your larger change.
>
> http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>
common/nacl_fork_delegate_**linux.h<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_fork_delegate_linux.h>
> File chrome/common/nacl_fork_**delegate_linux.h (right):
>
> http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>
common/nacl_fork_delegate_**linux.h#newcode12<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_fork_delegate_linux.h#newcode12>
> chrome/common/nacl_fork_**delegate_linux.h:12: class NaClForkDelegate :
> public ZygoteForkDelegate {
> Needs file-level comments describing what this is.
>
> http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>
common/nacl_fork_delegate_**linux.h#newcode22<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_fork_delegate_linux.h#newcode22>
> chrome/common/nacl_fork_**delegate_linux.h:22: virtual void Init(const
> bool sandboxed,
> Please use OVERRIDE on virtual methods that you are overriding.
>
> http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>
common/nacl_helper_linux.h<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_helper_linux.h>
> File chrome/common/nacl_helper_**linux.h (right):
>
> http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>
common/nacl_helper_linux.h#**newcode11<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_helper_linux.h#newcode11>
> chrome/common/nacl_helper_**linux.h:11: #define kNaClHelperStartupAck
> "NACLHELPER_OK"
> Why #define and not char[]?
> Why no comments?
>
> http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>
common/nacl_helper_linux.h#**newcode15<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_helper_linux.h#newcode15>
> chrome/common/nacl_helper_**linux.h:15: // kMagicSandboxIPCDescriptor.
> This will likely regress unless you either are very careful with
> comments in the other code or (better) you make these share a header.
>
> http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>
nacl/nacl_fork_delegate_linux.**cc<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_fork_delegate_linux.cc>
>
> File chrome/nacl/nacl_fork_**delegate_linux.cc (right):
>
> http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>
nacl/nacl_fork_delegate_linux.**cc#newcode19<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_fork_delegate_linux.cc#newcode19>
> chrome/nacl/nacl_fork_**delegate_linux.cc:19: #include
>
> "chrome/common/nacl_fork_**delegate_linux.h"
> This should be the first #include.
>
> http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>
nacl/nacl_helper_linux.cc<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_helper_linux.cc>
> File chrome/nacl/nacl_helper_linux.**cc (right):
>
> http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>
nacl/nacl_helper_linux.cc#**newcode20<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_helper_linux.cc#newcode20>
> chrome/nacl/nacl_helper_linux.**cc:20: #include
>
> "chrome/common/nacl_helper_**linux.h"
> This should be the first #include.
>
> http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>
nacl/nacl_helper_linux.cc#**newcode26<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_helper_linux.cc#newcode26>
> chrome/nacl/nacl_helper_linux.**cc:26: static bool g_suid_sandbox_active;
> Use namespace {} instead of static.
>
> http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>
nacl/nacl_listener.cc<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_listener.cc>
> File chrome/nacl/nacl_listener.cc (right):
>
> http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>
nacl/nacl_listener.cc#newcode8<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_listener.cc#newcode8>
> chrome/nacl/nacl_listener.cc:**8: #include "chrome/nacl/nacl_listener.h"
> This should be first.
>
> http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>
nacl/nacl_listener.h<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_listener.h>
> File chrome/nacl/nacl_listener.h (right):
>
> http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>
nacl/nacl_listener.h#newcode4<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_listener.h#newcode4>
> chrome/nacl/nacl_listener.h:4: #ifndef CHROME_NACL_NACL_LISTENER_H_
> whitespace above this line
>
> http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>
nacl/nacl_listener.h#newcode9<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_listener.h#newcode9>
> chrome/nacl/nacl_listener.h:9: #include "base/memory/scoped_ptr.h"
> These #includes aren't necessary.
>
> http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>
nacl/nacl_main.cc<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_main.cc>
> File chrome/nacl/nacl_main.cc (right):
>
> http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>
nacl/nacl_main.cc#newcode122<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_main.cc#newcode122>
> chrome/nacl/nacl_main.cc:122: NaClListener *listener = new
> NaClListener();
> Star on the left.
>
> Is it intentional that this leaks?
>
> http://codereview.chromium.**org/6995121/diff/54001/**
>
content/browser/zygote_host_**linux.h<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_host_linux.h>
> File content/browser/zygote_host_**linux.h (right):
>
> http://codereview.chromium.**org/6995121/diff/54001/**
>
content/browser/zygote_host_**linux.h#newcode35<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_host_linux.h#newcode35>
> content/browser/zygote_host_**linux.h:35: // Tries to start a renderer
> process.  Returns its pid on success, otherwise
> Comment is now wrong: "renderer process"
>
> http://codereview.chromium.**org/6995121/diff/54001/**
>
content/browser/zygote_main_**linux.cc<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc>
> File content/browser/zygote_main_**linux.cc (right):
>
> http://codereview.chromium.**org/6995121/diff/54001/**
>
content/browser/zygote_main_**linux.cc#newcode97<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc#newcode97>
> content/browser/zygote_main_**linux.cc:97: static std::string
> ExtractArg(std::vector<std::**string> args,
> Use namespace{} instead of static
>
> Why no comments?
>
> http://codereview.chromium.**org/6995121/diff/54001/**
>
content/browser/zygote_main_**linux.cc#newcode98<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc#newcode98>
> content/browser/zygote_main_**linux.cc:98: const std::string swtch) {
> Pass by reference, not by copying
>
> http://codereview.chromium.**org/6995121/diff/54001/**
>
content/browser/zygote_main_**linux.cc#newcode184<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc#newcode184>
> content/browser/zygote_main_**linux.cc:184: return HandleForkRequest(fd,
> pickle, iter, fds, process_type);
> Since HandleForkRequest reaches into the pickle anyway, why not have it
> extract the process type as well?
>
> http://codereview.chromium.**org/6995121/diff/54001/**
>
content/browser/zygote_main_**linux.cc#newcode384<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc#newcode384>
> content/browser/zygote_main_**linux.cc:384: const std::string
> process_type) {
> Pass by reference, not copy
>
> http://codereview.chromium.**org/6995121/diff/54001/**
>
content/browser/zygote_main_**linux.cc#newcode768<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc#newcode768>
> content/browser/zygote_main_**linux.cc:768: // initialize the fork helper
> This comment is uninformative.  I suggest deleting it.
>
>
>
http://codereview.chromium.**org/6995121/<http://codereview.chromium.org/6995...
>

Evan Martin

How about having whoever is relying on this finish up this change on your behalf? ...

9 years, 6 months ago (2011-06-24 20:11:26 UTC) #36

How about having whoever is relying on this finish up this change on your
behalf?

On 2011/06/24 20:09:17, Brad Chen wrote:
> Thanks Evan for the careful review. It looks to me like most of these are
> relatively easy to fix, although there are a couple cases where things are
> as they are because of the content factoring work John is doing.
> 
> Would you object to me cleaning things up in a subsequent CL? I have some
> family stuff I need to take care of this afternoon. I'm in on Monday, but
> will be going on vacation on Tuesday, and I don't want to delay other work
> that I'm hoping will happen while I'm gone. I can certainly have the
> clean-up CL for you Monday.
> 
> Brad
> 
> On Fri, Jun 24, 2011 at 11:57 AM, <mailto:evan@chromium.org> wrote:
> 
> >
> >
>
http://codereview.chromium.**org/6995121/diff/54001/base/**process_util.h%3Ch...>
> >
> > File base/process_util.h (right):
> >
> > http://codereview.chromium.**org/6995121/diff/54001/base/**
> >
>
process_util.h#newcode296<http://codereview.chromium.org/6995121/diff/54001/base/process_util.h#newcode296>
> > base/process_util.h:296: #endif
> > FYI, in the future it'd be easier to land this and review this if you
> > landed this new API bits separately from your larger change.
> >
> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
> >
>
common/nacl_fork_delegate_**linux.h<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_fork_delegate_linux.h>
> > File chrome/common/nacl_fork_**delegate_linux.h (right):
> >
> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
> >
>
common/nacl_fork_delegate_**linux.h#newcode12<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_fork_delegate_linux.h#newcode12>
> > chrome/common/nacl_fork_**delegate_linux.h:12: class NaClForkDelegate :
> > public ZygoteForkDelegate {
> > Needs file-level comments describing what this is.
> >
> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
> >
>
common/nacl_fork_delegate_**linux.h#newcode22<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_fork_delegate_linux.h#newcode22>
> > chrome/common/nacl_fork_**delegate_linux.h:22: virtual void Init(const
> > bool sandboxed,
> > Please use OVERRIDE on virtual methods that you are overriding.
> >
> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
> >
>
common/nacl_helper_linux.h<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_helper_linux.h>
> > File chrome/common/nacl_helper_**linux.h (right):
> >
> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
> >
>
common/nacl_helper_linux.h#**newcode11<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_helper_linux.h#newcode11>
> > chrome/common/nacl_helper_**linux.h:11: #define kNaClHelperStartupAck
> > "NACLHELPER_OK"
> > Why #define and not char[]?
> > Why no comments?
> >
> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
> >
>
common/nacl_helper_linux.h#**newcode15<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_helper_linux.h#newcode15>
> > chrome/common/nacl_helper_**linux.h:15: // kMagicSandboxIPCDescriptor.
> > This will likely regress unless you either are very careful with
> > comments in the other code or (better) you make these share a header.
> >
> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
> >
>
nacl/nacl_fork_delegate_linux.**cc<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_fork_delegate_linux.cc>
> >
> > File chrome/nacl/nacl_fork_**delegate_linux.cc (right):
> >
> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
> >
>
nacl/nacl_fork_delegate_linux.**cc#newcode19<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_fork_delegate_linux.cc#newcode19>
> > chrome/nacl/nacl_fork_**delegate_linux.cc:19: #include
> >
> > "chrome/common/nacl_fork_**delegate_linux.h"
> > This should be the first #include.
> >
> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
> >
>
nacl/nacl_helper_linux.cc<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_helper_linux.cc>
> > File chrome/nacl/nacl_helper_linux.**cc (right):
> >
> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
> >
>
nacl/nacl_helper_linux.cc#**newcode20<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_helper_linux.cc#newcode20>
> > chrome/nacl/nacl_helper_linux.**cc:20: #include
> >
> > "chrome/common/nacl_helper_**linux.h"
> > This should be the first #include.
> >
> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
> >
>
nacl/nacl_helper_linux.cc#**newcode26<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_helper_linux.cc#newcode26>
> > chrome/nacl/nacl_helper_linux.**cc:26: static bool g_suid_sandbox_active;
> > Use namespace {} instead of static.
> >
> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
> >
>
nacl/nacl_listener.cc<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_listener.cc>
> > File chrome/nacl/nacl_listener.cc (right):
> >
> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
> >
>
nacl/nacl_listener.cc#newcode8<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_listener.cc#newcode8>
> > chrome/nacl/nacl_listener.cc:**8: #include "chrome/nacl/nacl_listener.h"
> > This should be first.
> >
> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
> >
>
nacl/nacl_listener.h<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_listener.h>
> > File chrome/nacl/nacl_listener.h (right):
> >
> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
> >
>
nacl/nacl_listener.h#newcode4<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_listener.h#newcode4>
> > chrome/nacl/nacl_listener.h:4: #ifndef CHROME_NACL_NACL_LISTENER_H_
> > whitespace above this line
> >
> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
> >
>
nacl/nacl_listener.h#newcode9<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_listener.h#newcode9>
> > chrome/nacl/nacl_listener.h:9: #include "base/memory/scoped_ptr.h"
> > These #includes aren't necessary.
> >
> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
> >
>
nacl/nacl_main.cc<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_main.cc>
> > File chrome/nacl/nacl_main.cc (right):
> >
> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
> >
>
nacl/nacl_main.cc#newcode122<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_main.cc#newcode122>
> > chrome/nacl/nacl_main.cc:122: NaClListener *listener = new
> > NaClListener();
> > Star on the left.
> >
> > Is it intentional that this leaks?
> >
> > http://codereview.chromium.**org/6995121/diff/54001/**
> >
>
content/browser/zygote_host_**linux.h<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_host_linux.h>
> > File content/browser/zygote_host_**linux.h (right):
> >
> > http://codereview.chromium.**org/6995121/diff/54001/**
> >
>
content/browser/zygote_host_**linux.h#newcode35<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_host_linux.h#newcode35>
> > content/browser/zygote_host_**linux.h:35: // Tries to start a renderer
> > process.  Returns its pid on success, otherwise
> > Comment is now wrong: "renderer process"
> >
> > http://codereview.chromium.**org/6995121/diff/54001/**
> >
>
content/browser/zygote_main_**linux.cc<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc>
> > File content/browser/zygote_main_**linux.cc (right):
> >
> > http://codereview.chromium.**org/6995121/diff/54001/**
> >
>
content/browser/zygote_main_**linux.cc#newcode97<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc#newcode97>
> > content/browser/zygote_main_**linux.cc:97: static std::string
> > ExtractArg(std::vector<std::**string> args,
> > Use namespace{} instead of static
> >
> > Why no comments?
> >
> > http://codereview.chromium.**org/6995121/diff/54001/**
> >
>
content/browser/zygote_main_**linux.cc#newcode98<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc#newcode98>
> > content/browser/zygote_main_**linux.cc:98: const std::string swtch) {
> > Pass by reference, not by copying
> >
> > http://codereview.chromium.**org/6995121/diff/54001/**
> >
>
content/browser/zygote_main_**linux.cc#newcode184<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc#newcode184>
> > content/browser/zygote_main_**linux.cc:184: return HandleForkRequest(fd,
> > pickle, iter, fds, process_type);
> > Since HandleForkRequest reaches into the pickle anyway, why not have it
> > extract the process type as well?
> >
> > http://codereview.chromium.**org/6995121/diff/54001/**
> >
>
content/browser/zygote_main_**linux.cc#newcode384<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc#newcode384>
> > content/browser/zygote_main_**linux.cc:384: const std::string
> > process_type) {
> > Pass by reference, not copy
> >
> > http://codereview.chromium.**org/6995121/diff/54001/**
> >
>
content/browser/zygote_main_**linux.cc#newcode768<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc#newcode768>
> > content/browser/zygote_main_**linux.cc:768: // initialize the fork helper
> > This comment is uninformative.  I suggest deleting it.
> >
> >
> >
>
http://codereview.chromium.**org/6995121/%3Chttp://codereview.chromium.org/69...>
> >

Brad Chen

I'd rather finish it myself. I will plan on taking care of it on Monday ...

9 years, 6 months ago (2011-06-24 20:15:19 UTC) #37

I'd rather finish it myself. I will plan on taking care of it on Monday or
over the weekend. If you're not around to review it do you mind Darin or
Brett looking over my responses to your feedback?

Thanks,
Brad

On Fri, Jun 24, 2011 at 1:11 PM, <evan@chromium.org> wrote:

> How about having whoever is relying on this finish up this change on your
> behalf?
>
>
> On 2011/06/24 20:09:17, Brad Chen wrote:
>
>> Thanks Evan for the careful review. It looks to me like most of these are
>> relatively easy to fix, although there are a couple cases where things are
>> as they are because of the content factoring work John is doing.
>>
>
>  Would you object to me cleaning things up in a subsequent CL? I have some
>> family stuff I need to take care of this afternoon. I'm in on Monday, but
>> will be going on vacation on Tuesday, and I don't want to delay other work
>> that I'm hoping will happen while I'm gone. I can certainly have the
>> clean-up CL for you Monday.
>>
>
>  Brad
>>
>
>  On Fri, Jun 24, 2011 at 11:57 AM, <mailto:evan@chromium.org> wrote:
>>
>
>  >
>> >
>>
>
> http://codereview.chromium.****org/6995121/diff/54001/base/****
> process_util.h%3Chttp://codere**view.chromium.org/6995121/**
>
diff/54001/base/process_util.h<http://codereview.chromium.org/6995121/diff/54001/base/process_util.h>
> **>
>
>  >
>> > File base/process_util.h (right):
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/base/**
>> >
>>
>
> process_util.h#newcode296<http**://codereview.chromium.org/**
>
6995121/diff/54001/base/**process_util.h#newcode296<http://codereview.chromium.org/6995121/diff/54001/base/process_util.h#newcode296>
> >
>
>  > base/process_util.h:296: #endif
>> > FYI, in the future it'd be easier to land this and review this if you
>> > landed this new API bits separately from your larger change.
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/chrome/****
>> >
>>
>
> common/nacl_fork_delegate_****linux.h<http://codereview.**
> chromium.org/6995121/diff/**54001/chrome/common/nacl_fork_**
>
delegate_linux.h<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_fork_delegate_linux.h>
> >
>
>  > File chrome/common/nacl_fork_****delegate_linux.h (right):
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/chrome/****
>> >
>>
>
> common/nacl_fork_delegate_****linux.h#newcode12<http://**
> codereview.chromium.org/**6995121/diff/54001/chrome/**
>
common/nacl_fork_delegate_**linux.h#newcode12<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_fork_delegate_linux.h#newcode12>
> >
>
>  > chrome/common/nacl_fork_****delegate_linux.h:12: class NaClForkDelegate
>> :
>> > public ZygoteForkDelegate {
>> > Needs file-level comments describing what this is.
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/chrome/****
>> >
>>
>
> common/nacl_fork_delegate_****linux.h#newcode22<http://**
> codereview.chromium.org/**6995121/diff/54001/chrome/**
>
common/nacl_fork_delegate_**linux.h#newcode22<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_fork_delegate_linux.h#newcode22>
> >
>
>  > chrome/common/nacl_fork_****delegate_linux.h:22: virtual void
>> Init(const
>> > bool sandboxed,
>> > Please use OVERRIDE on virtual methods that you are overriding.
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/chrome/****
>> >
>>
>
> common/nacl_helper_linux.h<htt**p://codereview.chromium.org/**
>
6995121/diff/54001/chrome/**common/nacl_helper_linux.h<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_helper_linux.h>
> >
>
>  > File chrome/common/nacl_helper_****linux.h (right):
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/chrome/****
>> >
>>
>
> common/nacl_helper_linux.h#****newcode11<http://codereview.**
> chromium.org/6995121/diff/**54001/chrome/common/nacl_**
>
helper_linux.h#newcode11<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_helper_linux.h#newcode11>
> >
>
>  > chrome/common/nacl_helper_****linux.h:11: #define kNaClHelperStartupAck
>> > "NACLHELPER_OK"
>> > Why #define and not char[]?
>> > Why no comments?
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/chrome/****
>> >
>>
>
> common/nacl_helper_linux.h#****newcode15<http://codereview.**
> chromium.org/6995121/diff/**54001/chrome/common/nacl_**
>
helper_linux.h#newcode15<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_helper_linux.h#newcode15>
> >
>
>  > chrome/common/nacl_helper_****linux.h:15: //
>> kMagicSandboxIPCDescriptor.
>> > This will likely regress unless you either are very careful with
>> > comments in the other code or (better) you make these share a header.
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/chrome/****
>> >
>>
>
> nacl/nacl_fork_delegate_linux.****cc<http://codereview.**
> chromium.org/6995121/diff/**54001/chrome/nacl/nacl_fork_**
>
delegate_linux.cc<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_fork_delegate_linux.cc>
> >
>
>  >
>> > File chrome/nacl/nacl_fork_****delegate_linux.cc (right):
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/chrome/****
>> >
>>
>
> nacl/nacl_fork_delegate_linux.****cc#newcode19<http://**
> codereview.chromium.org/**6995121/diff/54001/chrome/**
>
nacl/nacl_fork_delegate_linux.**cc#newcode19<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_fork_delegate_linux.cc#newcode19>
> >
>
>  > chrome/nacl/nacl_fork_****delegate_linux.cc:19: #include
>> >
>> > "chrome/common/nacl_fork_****delegate_linux.h"
>> > This should be the first #include.
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/chrome/****
>> >
>>
>
> nacl/nacl_helper_linux.cc<http**://codereview.chromium.org/**
>
6995121/diff/54001/chrome/**nacl/nacl_helper_linux.cc<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_helper_linux.cc>
> >
>
>  > File chrome/nacl/nacl_helper_linux.****cc (right):
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/chrome/****
>> >
>>
>
> nacl/nacl_helper_linux.cc#****newcode20<http://codereview.**
> chromium.org/6995121/diff/**54001/chrome/nacl/nacl_helper_**
>
linux.cc#newcode20<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_helper_linux.cc#newcode20>
> >
>
>  > chrome/nacl/nacl_helper_linux.****cc:20: #include
>> >
>> > "chrome/common/nacl_helper_****linux.h"
>> > This should be the first #include.
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/chrome/****
>> >
>>
>
> nacl/nacl_helper_linux.cc#****newcode26<http://codereview.**
> chromium.org/6995121/diff/**54001/chrome/nacl/nacl_helper_**
>
linux.cc#newcode26<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_helper_linux.cc#newcode26>
> >
>
>  > chrome/nacl/nacl_helper_linux.****cc:26: static bool
>> g_suid_sandbox_active;
>> > Use namespace {} instead of static.
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/chrome/****
>> >
>>
>
> nacl/nacl_listener.cc<http://**codereview.chromium.org/**
>
6995121/diff/54001/chrome/**nacl/nacl_listener.cc<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_listener.cc>
> >
>
>  > File chrome/nacl/nacl_listener.cc (right):
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/chrome/****
>> >
>>
>
> nacl/nacl_listener.cc#**newcode8<http://codereview.**
>
chromium.org/6995121/diff/**54001/chrome/nacl/nacl_**listener.cc#newcode8<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_listener.cc#newcode8>
> >
>
>  > chrome/nacl/nacl_listener.cc:****8: #include
>> "chrome/nacl/nacl_listener.h"
>> > This should be first.
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/chrome/****
>> >
>>
>
> nacl/nacl_listener.h<http://**codereview.chromium.org/**
>
6995121/diff/54001/chrome/**nacl/nacl_listener.h<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_listener.h>
> >
>
>  > File chrome/nacl/nacl_listener.h (right):
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/chrome/****
>> >
>>
>
> nacl/nacl_listener.h#newcode4<**http://codereview.chromium.**
>
org/6995121/diff/54001/chrome/**nacl/nacl_listener.h#newcode4<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_listener.h#newcode4>
> >
>
>  > chrome/nacl/nacl_listener.h:4: #ifndef CHROME_NACL_NACL_LISTENER_H_
>> > whitespace above this line
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/chrome/****
>> >
>>
>
> nacl/nacl_listener.h#newcode9<**http://codereview.chromium.**
>
org/6995121/diff/54001/chrome/**nacl/nacl_listener.h#newcode9<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_listener.h#newcode9>
> >
>
>  > chrome/nacl/nacl_listener.h:9: #include "base/memory/scoped_ptr.h"
>> > These #includes aren't necessary.
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/chrome/****
>> >
>>
>
> nacl/nacl_main.cc<http://**codereview.chromium.org/**
>
6995121/diff/54001/chrome/**nacl/nacl_main.cc<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_main.cc>
> >
>
>  > File chrome/nacl/nacl_main.cc (right):
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/chrome/****
>> >
>>
>
> nacl/nacl_main.cc#newcode122<h**ttp://codereview.chromium.org/**
>
6995121/diff/54001/chrome/**nacl/nacl_main.cc#newcode122<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_main.cc#newcode122>
> >
>
>  > chrome/nacl/nacl_main.cc:122: NaClListener *listener = new
>> > NaClListener();
>> > Star on the left.
>> >
>> > Is it intentional that this leaks?
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/**
>> >
>>
>
> content/browser/zygote_host_****linux.h<http://codereview.**
>
chromium.org/6995121/diff/**54001/content/browser/zygote_**host_linux.h<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_host_linux.h>
> >
>
>  > File content/browser/zygote_host_****linux.h (right):
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/**
>> >
>>
>
> content/browser/zygote_host_****linux.h#newcode35<http://**
> codereview.chromium.org/**6995121/diff/54001/content/**
>
browser/zygote_host_linux.h#**newcode35<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_host_linux.h#newcode35>
> >
>
>  > content/browser/zygote_host_****linux.h:35: // Tries to start a
>> renderer
>> > process.  Returns its pid on success, otherwise
>> > Comment is now wrong: "renderer process"
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/**
>> >
>>
>
> content/browser/zygote_main_****linux.cc<http://codereview.**
>
chromium.org/6995121/diff/**54001/content/browser/zygote_**main_linux.cc<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc>
> >
>
>  > File content/browser/zygote_main_****linux.cc (right):
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/**
>> >
>>
>
> content/browser/zygote_main_****linux.cc#newcode97<http://**
> codereview.chromium.org/**6995121/diff/54001/content/**
>
browser/zygote_main_linux.cc#**newcode97<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc#newcode97>
> >
>
>  > content/browser/zygote_main_****linux.cc:97: static std::string
>> > ExtractArg(std::vector<std::****string> args,
>> > Use namespace{} instead of static
>> >
>> > Why no comments?
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/**
>> >
>>
>
> content/browser/zygote_main_****linux.cc#newcode98<http://**
> codereview.chromium.org/**6995121/diff/54001/content/**
>
browser/zygote_main_linux.cc#**newcode98<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc#newcode98>
> >
>
>  > content/browser/zygote_main_****linux.cc:98: const std::string swtch) {
>> > Pass by reference, not by copying
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/**
>> >
>>
>
> content/browser/zygote_main_****linux.cc#newcode184<http://**
> codereview.chromium.org/**6995121/diff/54001/content/**
>
browser/zygote_main_linux.cc#**newcode184<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc#newcode184>
> >
>
>  > content/browser/zygote_main_****linux.cc:184: return
>> HandleForkRequest(fd,
>> > pickle, iter, fds, process_type);
>> > Since HandleForkRequest reaches into the pickle anyway, why not have it
>> > extract the process type as well?
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/**
>> >
>>
>
> content/browser/zygote_main_****linux.cc#newcode384<http://**
> codereview.chromium.org/**6995121/diff/54001/content/**
>
browser/zygote_main_linux.cc#**newcode384<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc#newcode384>
> >
>
>  > content/browser/zygote_main_****linux.cc:384: const std::string
>> > process_type) {
>> > Pass by reference, not copy
>> >
>> > http://codereview.chromium.****org/6995121/diff/54001/**
>> >
>>
>
> content/browser/zygote_main_****linux.cc#newcode768<http://**
> codereview.chromium.org/**6995121/diff/54001/content/**
>
browser/zygote_main_linux.cc#**newcode768<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc#newcode768>
> >
>
>  > content/browser/zygote_main_****linux.cc:768: // initialize the fork
>> helper
>> > This comment is uninformative.  I suggest deleting it.
>> >
>> >
>> >
>>
>
> http://codereview.chromium.****org/6995121/%3Chttp://coderevi**
> ew.chromium.org/6995121/ <http://codereview.chromium.org/6995121/>>
>
>> >
>>
>
>
>
>
http://codereview.chromium.**org/6995121/<http://codereview.chromium.org/6995...
>

Evan Martin

Yeah, that's fine. I also intend to be here Monday. On Fri, Jun 24, 2011 ...

9 years, 6 months ago (2011-06-24 22:12:06 UTC) #38

Yeah, that's fine.  I also intend to be here Monday.

On Fri, Jun 24, 2011 at 1:15 PM, Brad Chen <bradchen@google.com> wrote:
> I'd rather finish it myself. I will plan on taking care of it on Monday or
> over the weekend. If you're not around to review it do you mind Darin or
> Brett looking over my responses to your feedback?
> Thanks,
> Brad
>
> On Fri, Jun 24, 2011 at 1:11 PM, <evan@chromium.org> wrote:
>>
>> How about having whoever is relying on this finish up this change on your
>> behalf?
>>
>> On 2011/06/24 20:09:17, Brad Chen wrote:
>>>
>>> Thanks Evan for the careful review. It looks to me like most of these are
>>> relatively easy to fix, although there are a couple cases where things
>>> are
>>> as they are because of the content factoring work John is doing.
>>
>>> Would you object to me cleaning things up in a subsequent CL? I have some
>>> family stuff I need to take care of this afternoon. I'm in on Monday, but
>>> will be going on vacation on Tuesday, and I don't want to delay other
>>> work
>>> that I'm hoping will happen while I'm gone. I can certainly have the
>>> clean-up CL for you Monday.
>>
>>> Brad
>>
>>> On Fri, Jun 24, 2011 at 11:57 AM, <mailto:evan@chromium.org> wrote:
>>
>>> >
>>> >
>>
>>
>>
http://codereview.chromium.**org/6995121/diff/54001/base/**process_util.h%3Ch...>
>>>
>>> >
>>> > File base/process_util.h (right):
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/base/**
>>> >
>>
>>
>>
process_util.h#newcode296<http://codereview.chromium.org/6995121/diff/54001/base/process_util.h#newcode296>
>>>
>>> > base/process_util.h:296: #endif
>>> > FYI, in the future it'd be easier to land this and review this if you
>>> > landed this new API bits separately from your larger change.
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>>> >
>>
>>
>>
common/nacl_fork_delegate_**linux.h<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_fork_delegate_linux.h>
>>>
>>> > File chrome/common/nacl_fork_**delegate_linux.h (right):
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>>> >
>>
>>
>>
common/nacl_fork_delegate_**linux.h#newcode12<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_fork_delegate_linux.h#newcode12>
>>>
>>> > chrome/common/nacl_fork_**delegate_linux.h:12: class NaClForkDelegate :
>>> > public ZygoteForkDelegate {
>>> > Needs file-level comments describing what this is.
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>>> >
>>
>>
>>
common/nacl_fork_delegate_**linux.h#newcode22<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_fork_delegate_linux.h#newcode22>
>>>
>>> > chrome/common/nacl_fork_**delegate_linux.h:22: virtual void Init(const
>>> > bool sandboxed,
>>> > Please use OVERRIDE on virtual methods that you are overriding.
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>>> >
>>
>>
>>
common/nacl_helper_linux.h<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_helper_linux.h>
>>>
>>> > File chrome/common/nacl_helper_**linux.h (right):
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>>> >
>>
>>
>>
common/nacl_helper_linux.h#**newcode11<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_helper_linux.h#newcode11>
>>>
>>> > chrome/common/nacl_helper_**linux.h:11: #define kNaClHelperStartupAck
>>> > "NACLHELPER_OK"
>>> > Why #define and not char[]?
>>> > Why no comments?
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>>> >
>>
>>
>>
common/nacl_helper_linux.h#**newcode15<http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_helper_linux.h#newcode15>
>>>
>>> > chrome/common/nacl_helper_**linux.h:15: // kMagicSandboxIPCDescriptor.
>>> > This will likely regress unless you either are very careful with
>>> > comments in the other code or (better) you make these share a header.
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>>> >
>>
>>
>>
nacl/nacl_fork_delegate_linux.**cc<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_fork_delegate_linux.cc>
>>>
>>> >
>>> > File chrome/nacl/nacl_fork_**delegate_linux.cc (right):
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>>> >
>>
>>
>>
nacl/nacl_fork_delegate_linux.**cc#newcode19<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_fork_delegate_linux.cc#newcode19>
>>>
>>> > chrome/nacl/nacl_fork_**delegate_linux.cc:19: #include
>>> >
>>> > "chrome/common/nacl_fork_**delegate_linux.h"
>>> > This should be the first #include.
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>>> >
>>
>>
>>
nacl/nacl_helper_linux.cc<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_helper_linux.cc>
>>>
>>> > File chrome/nacl/nacl_helper_linux.**cc (right):
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>>> >
>>
>>
>>
nacl/nacl_helper_linux.cc#**newcode20<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_helper_linux.cc#newcode20>
>>>
>>> > chrome/nacl/nacl_helper_linux.**cc:20: #include
>>> >
>>> > "chrome/common/nacl_helper_**linux.h"
>>> > This should be the first #include.
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>>> >
>>
>>
>>
nacl/nacl_helper_linux.cc#**newcode26<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_helper_linux.cc#newcode26>
>>>
>>> > chrome/nacl/nacl_helper_linux.**cc:26: static bool
>>> > g_suid_sandbox_active;
>>> > Use namespace {} instead of static.
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>>> >
>>
>>
>>
nacl/nacl_listener.cc<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_listener.cc>
>>>
>>> > File chrome/nacl/nacl_listener.cc (right):
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>>> >
>>
>>
>>
nacl/nacl_listener.cc#newcode8<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_listener.cc#newcode8>
>>>
>>> > chrome/nacl/nacl_listener.cc:**8: #include
>>> > "chrome/nacl/nacl_listener.h"
>>> > This should be first.
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>>> >
>>
>>
>>
nacl/nacl_listener.h<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_listener.h>
>>>
>>> > File chrome/nacl/nacl_listener.h (right):
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>>> >
>>
>>
>>
nacl/nacl_listener.h#newcode4<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_listener.h#newcode4>
>>>
>>> > chrome/nacl/nacl_listener.h:4: #ifndef CHROME_NACL_NACL_LISTENER_H_
>>> > whitespace above this line
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>>> >
>>
>>
>>
nacl/nacl_listener.h#newcode9<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_listener.h#newcode9>
>>>
>>> > chrome/nacl/nacl_listener.h:9: #include "base/memory/scoped_ptr.h"
>>> > These #includes aren't necessary.
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>>> >
>>
>>
>>
nacl/nacl_main.cc<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_main.cc>
>>>
>>> > File chrome/nacl/nacl_main.cc (right):
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/chrome/**
>>> >
>>
>>
>>
nacl/nacl_main.cc#newcode122<http://codereview.chromium.org/6995121/diff/54001/chrome/nacl/nacl_main.cc#newcode122>
>>>
>>> > chrome/nacl/nacl_main.cc:122: NaClListener *listener = new
>>> > NaClListener();
>>> > Star on the left.
>>> >
>>> > Is it intentional that this leaks?
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/**
>>> >
>>
>>
>>
content/browser/zygote_host_**linux.h<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_host_linux.h>
>>>
>>> > File content/browser/zygote_host_**linux.h (right):
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/**
>>> >
>>
>>
>>
content/browser/zygote_host_**linux.h#newcode35<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_host_linux.h#newcode35>
>>>
>>> > content/browser/zygote_host_**linux.h:35: // Tries to start a renderer
>>> > process.  Returns its pid on success, otherwise
>>> > Comment is now wrong: "renderer process"
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/**
>>> >
>>
>>
>>
content/browser/zygote_main_**linux.cc<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc>
>>>
>>> > File content/browser/zygote_main_**linux.cc (right):
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/**
>>> >
>>
>>
>>
content/browser/zygote_main_**linux.cc#newcode97<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc#newcode97>
>>>
>>> > content/browser/zygote_main_**linux.cc:97: static std::string
>>> > ExtractArg(std::vector<std::**string> args,
>>> > Use namespace{} instead of static
>>> >
>>> > Why no comments?
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/**
>>> >
>>
>>
>>
content/browser/zygote_main_**linux.cc#newcode98<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc#newcode98>
>>>
>>> > content/browser/zygote_main_**linux.cc:98: const std::string swtch) {
>>> > Pass by reference, not by copying
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/**
>>> >
>>
>>
>>
content/browser/zygote_main_**linux.cc#newcode184<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc#newcode184>
>>>
>>> > content/browser/zygote_main_**linux.cc:184: return
>>> > HandleForkRequest(fd,
>>> > pickle, iter, fds, process_type);
>>> > Since HandleForkRequest reaches into the pickle anyway, why not have it
>>> > extract the process type as well?
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/**
>>> >
>>
>>
>>
content/browser/zygote_main_**linux.cc#newcode384<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc#newcode384>
>>>
>>> > content/browser/zygote_main_**linux.cc:384: const std::string
>>> > process_type) {
>>> > Pass by reference, not copy
>>> >
>>> > http://codereview.chromium.**org/6995121/diff/54001/**
>>> >
>>
>>
>>
content/browser/zygote_main_**linux.cc#newcode768<http://codereview.chromium.org/6995121/diff/54001/content/browser/zygote_main_linux.cc#newcode768>
>>>
>>> > content/browser/zygote_main_**linux.cc:768: // initialize the fork
>>> > helper
>>> > This comment is uninformative.  I suggest deleting it.
>>> >
>>> >
>>> >
>>
>>
>>
http://codereview.chromium.**org/6995121/%3Chttp://codereview.chromium.org/69...>
>>>
>>> >
>>
>>
>>
>> http://codereview.chromium.org/6995121/
>
>

Brad Chen

I have addressed Evan's feedback. PTAL http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_fork_delegate_linux.h File chrome/common/nacl_fork_delegate_linux.h (right): http://codereview.chromium.org/6995121/diff/54001/chrome/common/nacl_fork_delegate_linux.h#newcode12 chrome/common/nacl_fork_delegate_linux.h:12: class NaClForkDelegate : ...

9 years, 6 months ago (2011-06-25 22:14:50 UTC) #39

Evan Martin

This LGTM, I just have some nit comments below. http://codereview.chromium.org/6995121/diff/67002/chrome/common/nacl_fork_delegate_linux.h File chrome/common/nacl_fork_delegate_linux.h (right): http://codereview.chromium.org/6995121/diff/67002/chrome/common/nacl_fork_delegate_linux.h#newcode23 chrome/common/nacl_fork_delegate_linux.h:23: ...

9 years, 6 months ago (2011-06-27 18:49:32 UTC) #40

agl

http://codereview.chromium.org/6995121/diff/67002/chrome/nacl/nacl_fork_delegate_linux.cc File chrome/nacl/nacl_fork_delegate_linux.cc (right): http://codereview.chromium.org/6995121/diff/67002/chrome/nacl/nacl_fork_delegate_linux.cc#newcode68 chrome/nacl/nacl_fork_delegate_linux.cc:68: } else { nit: there shouldn't be an else{} ...

9 years, 5 months ago (2011-06-27 18:59:32 UTC) #41

Brad Chen

Addressing feedback from evanm and agl. http://codereview.chromium.org/6995121/diff/67002/chrome/common/nacl_fork_delegate_linux.h File chrome/common/nacl_fork_delegate_linux.h (right): http://codereview.chromium.org/6995121/diff/67002/chrome/common/nacl_fork_delegate_linux.h#newcode23 chrome/common/nacl_fork_delegate_linux.h:23: virtual void Init(const ...

9 years, 5 months ago (2011-06-27 22:06:24 UTC) #42

Brad Chen

9 years, 5 months ago (2011-06-28 05:42:32 UTC) #44

Tragically while the default trybots were happy the clang bots and a couple
others were not, and the change was rightfully reverted. My git-foo is not
strong enough to know out how to carry this CL forward, so I am instantiating
the changes in a new CL, then I will layer the Clang changes on top of them. See
http://codereview.chromium.org/7276026

Expand Messages | Collapse Messages