NaCl-Chrome integration - step 1

First step towards NaCl-Chrome integration:1. NaCl plugin becomes a built-in plugin in Chrome and runs in the renderer process.2. Most of the changes are related to launching the NaCl process (that loads and runs the NaCl module) and establishing the initial communication between that process and the NaCl plugin.3. Command line flag "--internal-nacl" is required to enable the built-in NaCl plugin. NaCl still cannot run in Chrome sandbox, the flag automatically disables the sandboxCommitted: http://src.chromium.org/viewvc/chrome?view=rev&revision=27315

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=27324

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=27397

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=27445

[jam, 2009-07-02 22:24:09]

http://codereview.chromium.org/153002/diff/1014/2026
File build/build_config.h (right):

http://codereview.chromium.org/153002/diff/1014/2026#newcode20
Line 20: #define NACL_WINDOWS 0
why does this need to be done in the base header, i.e. why can't nacl code use
the same ifdefs as the rest of the code?

http://codereview.chromium.org/153002/diff/1014/2028
File chrome/app/chrome_dll.vcproj (right):

http://codereview.chromium.org/153002/diff/1014/2028#newcode59
Line 59: AdditionalDependencies="urlmon.lib
$(OutDir)\obj\global_intermediate\tcmalloc\libcmt.lib npGoogleNaClPlugin.lib
nonnacl_util.lib nonnacl_srpc.lib nrd_xfer.lib platform_util.lib gio.lib
google_nacl_npruntime.lib google_nacl_imc_c.lib sel.lib nacl_xdr.lib
ncvalidate.lib container.lib platform_qual_lib.lib platform.lib expiration.lib"
do you have numbers on how much bigger chrome.dll is after this?

http://codereview.chromium.org/153002/diff/1014/2028#newcode62
Line 62:
AdditionalLibraryDirectories=""E:\projects\nacl3\src\googleclient\native_client\scons-out\dbg-win-x86-32\obj\src\trusted\plugin";"E:\projects\nacl3\src\googleclient\native_client\scons-out\dbg-win-x86-32\lib""
I presume this won't get checked in like this?

http://codereview.chromium.org/153002/diff/1014/2027
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/153002/diff/1014/2027#newcode484
Line 484: rv = SelLdrMain(main_params);
Chrome engineers won't knwo what SelLdr is, so beter to use kNaClProcess and
NaClMain.

http://codereview.chromium.org/153002/diff/1014/2030
File chrome/browser/plugin_service.cc (right):

http://codereview.chromium.org/153002/diff/1014/2030#newcode160
Line 160: SelLdrProcessHost* sel_ldr_host = new SelLdrProcessHost();
It doesn't look like you use anything from PluginService, so the calling code
should just do this without going through PluginService.

http://codereview.chromium.org/153002/diff/1014/2036
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/153002/diff/1014/2036#newcode294
Line 294: if (browser_command_line.HasSwitch(kInternalNativeClient)) {
Nothing stops this from being put in the switch_names array.

http://codereview.chromium.org/153002/diff/1014/2035
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/153002/diff/1014/2035#newcode269
Line 269: IPC_MESSAGE_HANDLER_DELAY_REPLY(ViewHostMsg_LaunchSelLdr,
OnLaunchSelLdr)
no need to use IPC_MESSAGE_HANDLER_DELAY_REPLY, that's only used when you can't
send the reply in your dispatch method (i.e. you need to go to another thread).

http://codereview.chromium.org/153002/diff/1014/2031
File chrome/browser/sandbox_policy.cc (right):

http://codereview.chromium.org/153002/diff/1014/2031#newcode365
Line 365: type = ChildProcessInfo::SELLDR_PROCESS;
as above, please use NACL_PROCESS

http://codereview.chromium.org/153002/diff/1014/2033
File chrome/browser/selldr_process_host.cc (right):

http://codereview.chromium.org/153002/diff/1014/2033#newcode43
Line 43: #include "webkit/glue/plugins/plugin_constants_win.h"
please go through the above list and remove all the unnecessary includes.

http://codereview.chromium.org/153002/diff/1014/2033#newcode55
Line 55: PLUGIN_PROCESS,
shouldn't this be NACL_PROCESS?

http://codereview.chromium.org/153002/diff/1014/2033#newcode56
Line 56: PluginService::GetInstance()->resource_dispatcher_host()) {
I think it'd be clearer if the constructor takes a ResourceDispatcherHost
pointer, without going through PluginService.

http://codereview.chromium.org/153002/diff/1014/2033#newcode62
Line 62: CancelRequestsForProcess(GetProcessId());
this is done automatically by ChildProcessHost.

http://codereview.chromium.org/153002/diff/1014/2033#newcode65
Line 65: bool SelLdrProcessHost::Launch(ResourceMessageFilter*
renderer_msg_filter,
no need for RMF and IPC::Message pointers.  the plugin code does this since the
answer isn't always returned right away.

http://codereview.chromium.org/153002/diff/1014/2033#newcode129
Line 129: switches::kEnableStatsTable,
please only add the flags that you need propagated.

http://codereview.chromium.org/153002/diff/1014/2033#newcode180
Line 180: descriptor, reinterpret_cast<int>(channel));
spacing is off, it should be 4 spaces

http://codereview.chromium.org/153002/diff/1014/2033#newcode181
Line 181: msg->set_unblock(true);
are you sure you want to do this? :)

http://codereview.chromium.org/153002/diff/1014/2033#newcode217
Line 217: return
CPBrowsingContextManager::Instance()->ToURLRequestContext(request_id);
this should just return NULL.

http://codereview.chromium.org/153002/diff/1014/2032
File chrome/browser/selldr_process_host.h (right):

http://codereview.chromium.org/153002/diff/1014/2032#newcode18
Line 18: #include "chrome/browser/renderer_host/resource_message_filter.h"
We try to avoid adding unnecessary includes in order to not slow down the build
any further.  Almost all the includes above aren't needed.  As for
ResourceMessageFilter, you can just forward declare it.

http://codereview.chromium.org/153002/diff/1014/2032#newcode43
Line 43: private:
nit: no line after private

http://codereview.chromium.org/153002/diff/1014/2032#newcode47
Line 47: const nacl::Handle handle);
nit: need 4 spaces (please review the chromium coding guidelines)

http://codereview.chromium.org/153002/diff/1014/2032#newcode66
Line 66: DISALLOW_EVIL_CONSTRUCTORS(SelLdrProcessHost);
this is deprecated, use DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/153002/diff/1014/2039
File chrome/common/selldr_messages_internal.h (right):

http://codereview.chromium.org/153002/diff/1014/2039#newcode7
Line 7: #include "webkit/glue/webcursor.h"
take out the includes you don't need

http://codereview.chromium.org/153002/diff/1014/2045
File chrome/plugin/sel_ldr_launcher_chrome.cc (right):

http://codereview.chromium.org/153002/diff/1014/2045#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
nit: copyright on new files should be 2009

http://codereview.chromium.org/153002/diff/1014/2045#newcode7
Line 7: // TODO(gregoryd): integrate this if in gyp
note: in general, please move all these new files into chrome\nacl. 
chrome\plugins is for npapi plugins.  this code is for hosting the selldr which
isn't npapi

http://codereview.chromium.org/153002/diff/1014/2045#newcode23
Line 23: if (!RenderThread::current()->Send(new ViewHostMsg_LaunchSelLdr(
This is confusing: code in chrome/plugin calling RenderThread.  Only in the
renderer plugin will RenderThread::current() return anything but NULL.  Which
process is this running in?  I presume the renderer, in which case it should be
located in chrome\renderer

http://codereview.chromium.org/153002/diff/1014/2048
File chrome/plugin/sel_main.cc (right):

http://codereview.chromium.org/153002/diff/1014/2048#newcode3
Line 3: * All rights reserved.
please use the new copyright header, also right year

http://codereview.chromium.org/153002/diff/1014/2046
File chrome/plugin/selldr_thread.cc (right):

http://codereview.chromium.org/153002/diff/1014/2046#newcode26
Line 26: #include "webkit/glue/webkit_glue.h"
nit: too many includes

http://codereview.chromium.org/153002/diff/1014/2046#newcode56
Line 56: ChildThread::Init();
no need to override this function (and CleanUp) if they're not needed

http://codereview.chromium.org/153002/diff/1014/2047
File chrome/plugin/selldr_thread.h (right):

http://codereview.chromium.org/153002/diff/1014/2047#newcode34
Line 34: 
nit: too many extra lines in this file

http://codereview.chromium.org/153002/diff/1014/2021
File webkit/glue/plugins/plugin_list_win.cc (right):

http://codereview.chromium.org/153002/diff/1014/2021#newcode405
Line 405: if (use_internal_nacl_) {
note: you can do all this by calling PluginList::RegisterInternalPlugin in the
browser code.  This would allow you to have the flag in chrome_switches and not
need use_internal_nacl_

[gregoryd, 2009-07-16 01:20:07]

Handled all comments, still need to check the impact on chrome.dll size. The CL
now includes the gyp integration, Mac/Linux support is still missing. Going to
add chromium-reviews as the next step.

http://codereview.chromium.org/153002/diff/1014/2026
File build/build_config.h (right):

http://codereview.chromium.org/153002/diff/1014/2026#newcode20
Line 20: #define NACL_WINDOWS 0
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> why does this need to be done in the base header, i.e. why can't nacl code use
> the same ifdefs as the rest of the code?  

Moved it into gyp file (nacl section in chrome.gyp), will consider adapting
Chrome defines later.

http://codereview.chromium.org/153002/diff/1014/2028
File chrome/app/chrome_dll.vcproj (right):

http://codereview.chromium.org/153002/diff/1014/2028#newcode62
Line 62:
AdditionalLibraryDirectories=""E:\projects\nacl3\src\googleclient\native_client\scons-out\dbg-win-x86-32\obj\src\trusted\plugin";"E:\projects\nacl3\src\googleclient\native_client\scons-out\dbg-win-x86-32\lib""
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> I presume this won't get checked in like this?

Removed the file from the CL - replaced by gyp

http://codereview.chromium.org/153002/diff/1014/2027
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/153002/diff/1014/2027#newcode484
Line 484: rv = SelLdrMain(main_params);
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> Chrome engineers won't knwo what SelLdr is, so beter to use kNaClProcess and
> NaClMain.

Done.

http://codereview.chromium.org/153002/diff/1014/2030
File chrome/browser/plugin_service.cc (right):

http://codereview.chromium.org/153002/diff/1014/2030#newcode160
Line 160: SelLdrProcessHost* sel_ldr_host = new SelLdrProcessHost();
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> It doesn't look like you use anything from PluginService, so the calling code
> should just do this without going through PluginService.

Done.

http://codereview.chromium.org/153002/diff/1014/2036
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/153002/diff/1014/2036#newcode294
Line 294: if (browser_command_line.HasSwitch(kInternalNativeClient)) {
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> Nothing stops this from being put in the switch_names array.

Done.

http://codereview.chromium.org/153002/diff/1014/2035
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/153002/diff/1014/2035#newcode269
Line 269: IPC_MESSAGE_HANDLER_DELAY_REPLY(ViewHostMsg_LaunchSelLdr,
OnLaunchSelLdr)
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> no need to use IPC_MESSAGE_HANDLER_DELAY_REPLY, that's only used when you
can't
> send the reply in your dispatch method (i.e. you need to go to another
thread). 
> 

Done.

http://codereview.chromium.org/153002/diff/1014/2031
File chrome/browser/sandbox_policy.cc (right):

http://codereview.chromium.org/153002/diff/1014/2031#newcode365
Line 365: type = ChildProcessInfo::SELLDR_PROCESS;
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> as above, please use NACL_PROCESS

Done.

http://codereview.chromium.org/153002/diff/1014/2033
File chrome/browser/selldr_process_host.cc (right):

http://codereview.chromium.org/153002/diff/1014/2033#newcode43
Line 43: #include "webkit/glue/plugins/plugin_constants_win.h"
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> please go through the above list and remove all the unnecessary includes.

Done.

http://codereview.chromium.org/153002/diff/1014/2033#newcode55
Line 55: PLUGIN_PROCESS,
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> shouldn't this be NACL_PROCESS?

Done.

http://codereview.chromium.org/153002/diff/1014/2033#newcode56
Line 56: PluginService::GetInstance()->resource_dispatcher_host()) {
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> I think it'd be clearer if the constructor takes a ResourceDispatcherHost
> pointer, without going through PluginService.

Done.

http://codereview.chromium.org/153002/diff/1014/2033#newcode62
Line 62: CancelRequestsForProcess(GetProcessId());
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> this is done automatically by ChildProcessHost.

Done.

http://codereview.chromium.org/153002/diff/1014/2033#newcode65
Line 65: bool SelLdrProcessHost::Launch(ResourceMessageFilter*
renderer_msg_filter,
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> no need for RMF and IPC::Message pointers.  the plugin code does this since
the
> answer isn't always returned right away.

We need RMF to get the renderer process handle - we use it to duplicate handle
to the newly created pipe (see line 206).

http://codereview.chromium.org/153002/diff/1014/2033#newcode129
Line 129: switches::kEnableStatsTable,
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> please only add the flags that you need propagated.

Removed plugin flags, will need to look deeper into supporting the remaining
ones (added a TODO)

http://codereview.chromium.org/153002/diff/1014/2033#newcode180
Line 180: descriptor, reinterpret_cast<int>(channel));
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> spacing is off, it should be 4 spaces

Done.

http://codereview.chromium.org/153002/diff/1014/2033#newcode181
Line 181: msg->set_unblock(true);
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> are you sure you want to do this? :)

Done.

http://codereview.chromium.org/153002/diff/1014/2033#newcode217
Line 217: return
CPBrowsingContextManager::Instance()->ToURLRequestContext(request_id);
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> this should just return NULL.

Done.

http://codereview.chromium.org/153002/diff/1014/2032
File chrome/browser/selldr_process_host.h (right):

http://codereview.chromium.org/153002/diff/1014/2032#newcode18
Line 18: #include "chrome/browser/renderer_host/resource_message_filter.h"
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> We try to avoid adding unnecessary includes in order to not slow down the
build
> any further.  Almost all the includes above aren't needed.  As for
> ResourceMessageFilter, you can just forward declare it.

Done.

http://codereview.chromium.org/153002/diff/1014/2032#newcode43
Line 43: private:
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> nit: no line after private

Done.

http://codereview.chromium.org/153002/diff/1014/2032#newcode47
Line 47: const nacl::Handle handle);
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> nit: need 4 spaces (please review the chromium coding guidelines)

Done.

http://codereview.chromium.org/153002/diff/1014/2032#newcode66
Line 66: DISALLOW_EVIL_CONSTRUCTORS(SelLdrProcessHost);
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> this is deprecated, use DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/153002/diff/1014/2039
File chrome/common/selldr_messages_internal.h (right):

http://codereview.chromium.org/153002/diff/1014/2039#newcode7
Line 7: #include "webkit/glue/webcursor.h"
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> take out the includes you don't need

Done.

http://codereview.chromium.org/153002/diff/1014/2045
File chrome/plugin/sel_ldr_launcher_chrome.cc (right):

http://codereview.chromium.org/153002/diff/1014/2045#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> nit: copyright on new files should be 2009

Done.

http://codereview.chromium.org/153002/diff/1014/2045#newcode7
Line 7: // TODO(gregoryd): integrate this if in gyp
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> note: in general, please move all these new files into chrome\nacl. 
> chrome\plugins is for npapi plugins.  this code is for hosting the selldr
which
> isn't npapi

Done.

http://codereview.chromium.org/153002/diff/1014/2045#newcode23
Line 23: if (!RenderThread::current()->Send(new ViewHostMsg_LaunchSelLdr(
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> This is confusing: code in chrome/plugin calling RenderThread.  Only in the
> renderer plugin will RenderThread::current() return anything but NULL.  Which
> process is this running in?  I presume the renderer, in which case it should
be
> located in chrome\renderer

This is indeed running in the renderer process - it's actually part of the NaCl
plugin code. This code was moved back into NaCl tree - it was here to workaround
build issues.

http://codereview.chromium.org/153002/diff/1014/2048
File chrome/plugin/sel_main.cc (right):

http://codereview.chromium.org/153002/diff/1014/2048#newcode3
Line 3: * All rights reserved.
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> please use the new copyright header, also right year

Done.

http://codereview.chromium.org/153002/diff/1014/2046
File chrome/plugin/selldr_thread.cc (right):

http://codereview.chromium.org/153002/diff/1014/2046#newcode26
Line 26: #include "webkit/glue/webkit_glue.h"
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> nit: too many includes

Done.

http://codereview.chromium.org/153002/diff/1014/2046#newcode56
Line 56: ChildThread::Init();
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> no need to override this function (and CleanUp) if they're not needed

Done.

http://codereview.chromium.org/153002/diff/1014/2047
File chrome/plugin/selldr_thread.h (right):

http://codereview.chromium.org/153002/diff/1014/2047#newcode34
Line 34: 
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> nit: too many extra lines in this file

Done.

http://codereview.chromium.org/153002/diff/1014/2021
File webkit/glue/plugins/plugin_list_win.cc (right):

http://codereview.chromium.org/153002/diff/1014/2021#newcode405
Line 405: if (use_internal_nacl_) {
On 2009/07/02 22:24:09, John Abd-El-Malek wrote:
> note: you can do all this by calling PluginList::RegisterInternalPlugin in the
> browser code.  This would allow you to have the flag in chrome_switches and
not
> need use_internal_nacl_

Done, but had to call PluginList::RegisterInternalPlugin in the renderer
(renderer_process.cc), otherwise the plugin cannot be found.

[gregoryd, 2009-07-16 01:44:19]

Adding chromium-reviews and native-client-reviews

[jam, 2009-07-24 00:02:16]

http://codereview.chromium.org/153002/diff/3060/3083
File chrome/browser/nacl_process_host.cc (right):

http://codereview.chromium.org/153002/diff/3060/3083#newcode27
Line 27: const int descriptor,
nit: spacing is off.  same in other files too

http://codereview.chromium.org/153002/diff/3060/3083#newcode34
Line 34: //ReplyToRenderer(renderer_msg_filter, nacl::kInvalidHandle,
reply_msg);
forgot to remove all the "//ReplyToRender" lines?

http://codereview.chromium.org/153002/diff/3060/3083#newcode50
Line 50: // TODO(gregoryd): linux support
In the meantime, you'll need to add ifdefs to get this code to compile on other
platforms.

http://codereview.chromium.org/153002/diff/3060/3083#newcode71
Line 71: // build command line for nacl, we have to quote the plugin's path to
deal
update comment

http://codereview.chromium.org/153002/diff/3060/3083#newcode85
Line 85: // TODO(gregoryd): check which flags of those below can be supported.
this should be done before checkin..  right away things like kUserAgent,
kUserDataDir, kMemoryProfiling, kUseLowFraHeapCrt, kEnableStatsTable don't look
like they make sense for nacl processes

http://codereview.chromium.org/153002/diff/3060/3082
File chrome/browser/nacl_process_host.h (right):

http://codereview.chromium.org/153002/diff/3060/3082#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
nit: just 2009 since it's a new file (other files too)

http://codereview.chromium.org/153002/diff/3060/3082#newcode46
Line 46: 
nit: extra line

http://codereview.chromium.org/153002/diff/3060/3075
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/153002/diff/3060/3075#newcode958
Line 958: int /* handle - one side of a socket pair */)
based on the last integration meeting, the agreement was that there'd be a
chrome ipc channel between the renderer and nacl process.

is your plan to check this as is and switch it later?

http://codereview.chromium.org/153002/diff/3060/3088
File chrome/nacl/nacl_main.cc (right):

http://codereview.chromium.org/153002/diff/3060/3088#newcode66
Line 66: // TODO(gregoryd): - do we need tests here (see PluginMain)
not really, you can take out this todo.

http://codereview.chromium.org/153002/diff/3060/3089
File chrome/nacl/nacl_thread.cc (right):

http://codereview.chromium.org/153002/diff/3060/3089#newcode32
Line 32: // TODO(gregoryd): this needs to be called from a different thread (?)
which thread are you thinking of?

http://codereview.chromium.org/153002/diff/3060/3090
File chrome/nacl/nacl_thread.h (right):

http://codereview.chromium.org/153002/diff/3060/3090#newcode5
Line 5: #ifndef CHROME_PLUGIN_NACL_THREAD_H_
update path

http://codereview.chromium.org/153002/diff/3060/3090#newcode12
Line 12: #include "webkit/glue/plugins/plugin_lib.h"
these plugin includes aren't needed.

http://codereview.chromium.org/153002/diff/3060/3090#newcode22
Line 22: // Returns the one plugin thread.
update comment

[gregoryd, 2009-09-03 23:12:07]

Mac build still fails due to a gyp problem that will be fixed soon.

http://codereview.chromium.org/153002/diff/3060/3083
File chrome/browser/nacl_process_host.cc (right):

http://codereview.chromium.org/153002/diff/3060/3083#newcode27
Line 27: const int descriptor,
On 2009/07/24 00:02:17, John Abd-El-Malek wrote:
> nit: spacing is off.  same in other files too

Done.

http://codereview.chromium.org/153002/diff/3060/3083#newcode34
Line 34: //ReplyToRenderer(renderer_msg_filter, nacl::kInvalidHandle,
reply_msg);
On 2009/07/24 00:02:17, John Abd-El-Malek wrote:
> forgot to remove all the "//ReplyToRender" lines?

Done.

http://codereview.chromium.org/153002/diff/3060/3083#newcode50
Line 50: // TODO(gregoryd): linux support
On 2009/07/24 00:02:17, John Abd-El-Malek wrote:
> In the meantime, you'll need to add ifdefs to get this code to compile on
other
> platforms.

Done.

http://codereview.chromium.org/153002/diff/3060/3083#newcode71
Line 71: // build command line for nacl, we have to quote the plugin's path to
deal
On 2009/07/24 00:02:17, John Abd-El-Malek wrote:
> update comment

Done.

http://codereview.chromium.org/153002/diff/3060/3082
File chrome/browser/nacl_process_host.h (right):

http://codereview.chromium.org/153002/diff/3060/3082#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
On 2009/07/24 00:02:17, John Abd-El-Malek wrote:
> nit: just 2009 since it's a new file (other files too)

Done.

http://codereview.chromium.org/153002/diff/3060/3082#newcode46
Line 46: 
On 2009/07/24 00:02:17, John Abd-El-Malek wrote:
> nit: extra line

Done.

http://codereview.chromium.org/153002/diff/3060/3075
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/153002/diff/3060/3075#newcode958
Line 958: int /* handle - one side of a socket pair */)
On 2009/07/24 00:02:17, John Abd-El-Malek wrote:
> based on the last integration meeting, the agreement was that there'd be a
> chrome ipc channel between the renderer and nacl process.
> 
> is your plan to check this as is and switch it later?

It will be done at the next stage, it'll require a lot of changes

http://codereview.chromium.org/153002/diff/3060/3088
File chrome/nacl/nacl_main.cc (right):

http://codereview.chromium.org/153002/diff/3060/3088#newcode66
Line 66: // TODO(gregoryd): - do we need tests here (see PluginMain)
On 2009/07/24 00:02:17, John Abd-El-Malek wrote:
> not really, you can take out this todo.

Done.

http://codereview.chromium.org/153002/diff/3060/3090
File chrome/nacl/nacl_thread.h (right):

http://codereview.chromium.org/153002/diff/3060/3090#newcode5
Line 5: #ifndef CHROME_PLUGIN_NACL_THREAD_H_
On 2009/07/24 00:02:17, John Abd-El-Malek wrote:
> update path

Done.

http://codereview.chromium.org/153002/diff/3060/3090#newcode12
Line 12: #include "webkit/glue/plugins/plugin_lib.h"
On 2009/07/24 00:02:17, John Abd-El-Malek wrote:
> these plugin includes aren't needed.

Done.

http://codereview.chromium.org/153002/diff/3060/3090#newcode22
Line 22: // Returns the one plugin thread.
On 2009/07/24 00:02:17, John Abd-El-Malek wrote:
> update comment

Done.

[gregoryd, 2009-09-16 00:21:54]

This change now builds successfully on all platforms.

[gregoryd, 2009-09-16 23:56:13]

Looks like a minor fix in our gyp fixed the problem. The trybots are running now
- waiting for your LGTM to submit.

[jam, 2009-09-17 19:17:57]

almost there, just a few comments.

also re "--no-sandbox", ideally it'd be set automatically if "--internal-nacl"
is also set.  This can be done in RenderProcessBrowserHost.

http://codereview.chromium.org/153002/diff/25001/26025
File chrome/browser/nacl_process_host.cc (right):

http://codereview.chromium.org/153002/diff/25001/26025#newcode87
Line 87: const nacl::Handle handle) {
nit spacing

http://codereview.chromium.org/153002/diff/25001/26030
File chrome/nacl/nacl_thread.cc (right):

http://codereview.chromium.org/153002/diff/25001/26030#newcode31
Line 31: const nacl::FileDescriptor handle) {
nit: spacing

http://codereview.chromium.org/153002/diff/25001/26003
File webkit/glue/plugins/plugin_list.cc (right):

http://codereview.chromium.org/153002/diff/25001/26003#newcode370
Line 370: g_singleton.Pointer()->RegisterInternalPlugin(nacl_plugin_info);
instead of g_singleton.Pointer(), you can just do Singleton().  we used to do
the former before when the constructor had side effects, but that's not the case
anymore

http://codereview.chromium.org/153002/diff/25001/26002
File webkit/glue/plugins/plugin_list.h (right):

http://codereview.chromium.org/153002/diff/25001/26002#newcode161
Line 161: bool ShouldLoadPluginCommon(const WebPluginInfo& info);
the standard way we do this is to have the shared function be called X, and the
platform specific ones be called PlatformX (i.e. PlatformInit).  so this should
be called ShouldLoadPlugin, and the existing functions should be renamed
PlatformShouldLoadPlugin

http://codereview.chromium.org/153002/diff/25001/26004
File webkit/glue/plugins/plugin_list_win.cc (right):

http://codereview.chromium.org/153002/diff/25001/26004#newcode321
Line 321: return ShouldLoadPluginCommon(info);
the way that ShouldLoadPlugin is called should be changed to be consistent with
the rest of the code.

PluginList should call ShouldLoadPlugin which is the one defined in
plugin_list.cc.  When it's done, it should call PlatformShouldLoadPlugin, which
is in the platform files (don't forget about the .mm file, which isn't in this
change).  That way you don't have to call ShouldLoadPlugin three times in each
of the platform code.

http://codereview.chromium.org/153002/diff/25001/26006
File webkit/glue/plugins/webplugin_delegate_impl.cc (right):

http://codereview.chromium.org/153002/diff/25001/26006#newcode99
Line 99: if (!(quirks_ && PLUGIN_QUIRK_DONT_CREATE_DUMMY_WINDOW)) {
I think you mean one "&" instead of two.

http://codereview.chromium.org/153002/diff/25001/26007
File webkit/glue/plugins/webplugin_delegate_impl.h (right):

http://codereview.chromium.org/153002/diff/25001/26007#newcode131
Line 131: // Called from the constructor, sets quirks that are used on all
platforms.
this function is unnecessary, it sets a quirk that's only used in the windows
implementation, so it should just be set in the windows constructor.

http://codereview.chromium.org/153002/diff/25001/26010
File webkit/glue/plugins/webplugin_delegate_impl_win.cc (right):

http://codereview.chromium.org/153002/diff/25001/26010#newcode355
Line 355: // change when a windowless plugin instance changes the cursor
nit: in all this comment, no whitespace needed at the end of lines

[jam, 2009-09-17 19:26:59]

http://codereview.chromium.org/153002/diff/25001/26003
File webkit/glue/plugins/plugin_list.cc (right):

http://codereview.chromium.org/153002/diff/25001/26003#newcode177
Line 177: // platform-specific LoadInternalPlugins() function.
LoadInternalPlugins doesn't exist anymore, so this comment should be updated

http://codereview.chromium.org/153002/diff/25001/26002
File webkit/glue/plugins/plugin_list.h (right):

http://codereview.chromium.org/153002/diff/25001/26002#newcode164
Line 164: void LoadInternalPlugins(std::vector<WebPluginInfo>* plugins);
Is this from a merge with an earlier version?  This function doesn't exist
anymore.

[gregoryd, 2009-09-17 20:40:29]

http://codereview.chromium.org/153002/diff/25001/26025
File chrome/browser/nacl_process_host.cc (right):

http://codereview.chromium.org/153002/diff/25001/26025#newcode87
Line 87: const nacl::Handle handle) {
On 2009/09/17 19:17:57, John Abd-El-Malek wrote:
> nit spacing

Done.

http://codereview.chromium.org/153002/diff/25001/26030
File chrome/nacl/nacl_thread.cc (right):

http://codereview.chromium.org/153002/diff/25001/26030#newcode31
Line 31: const nacl::FileDescriptor handle) {
On 2009/09/17 19:17:57, John Abd-El-Malek wrote:
> nit: spacing

Done.

http://codereview.chromium.org/153002/diff/25001/26003
File webkit/glue/plugins/plugin_list.cc (right):

http://codereview.chromium.org/153002/diff/25001/26003#newcode177
Line 177: // platform-specific LoadInternalPlugins() function.
On 2009/09/17 19:26:59, John Abd-El-Malek wrote:
> LoadInternalPlugins doesn't exist anymore, so this comment should be updated

Done.

http://codereview.chromium.org/153002/diff/25001/26003#newcode370
Line 370: g_singleton.Pointer()->RegisterInternalPlugin(nacl_plugin_info);
On 2009/09/17 19:17:57, John Abd-El-Malek wrote:
> instead of g_singleton.Pointer(), you can just do Singleton().  we used to do
> the former before when the constructor had side effects, but that's not the
case
> anymore

Done.

http://codereview.chromium.org/153002/diff/25001/26002
File webkit/glue/plugins/plugin_list.h (right):

http://codereview.chromium.org/153002/diff/25001/26002#newcode161
Line 161: bool ShouldLoadPluginCommon(const WebPluginInfo& info);
On 2009/09/17 19:17:57, John Abd-El-Malek wrote:
> the standard way we do this is to have the shared function be called X, and
the
> platform specific ones be called PlatformX (i.e. PlatformInit).  so this
should
> be called ShouldLoadPlugin, and the existing functions should be renamed
> PlatformShouldLoadPlugin

Done.

http://codereview.chromium.org/153002/diff/25001/26002#newcode164
Line 164: void LoadInternalPlugins(std::vector<WebPluginInfo>* plugins);
On 2009/09/17 19:26:59, John Abd-El-Malek wrote:
> Is this from a merge with an earlier version?  This function doesn't exist
> anymore.

Done.

http://codereview.chromium.org/153002/diff/25001/26004
File webkit/glue/plugins/plugin_list_win.cc (right):

http://codereview.chromium.org/153002/diff/25001/26004#newcode321
Line 321: return ShouldLoadPluginCommon(info);
On 2009/09/17 19:17:57, John Abd-El-Malek wrote:
> the way that ShouldLoadPlugin is called should be changed to be consistent
with
> the rest of the code.
> 
> PluginList should call ShouldLoadPlugin which is the one defined in
> plugin_list.cc.  When it's done, it should call PlatformShouldLoadPlugin,
which
> is in the platform files (don't forget about the .mm file, which isn't in this
> change).  That way you don't have to call ShouldLoadPlugin three times in each
> of the platform code.

Done.

http://codereview.chromium.org/153002/diff/25001/26006
File webkit/glue/plugins/webplugin_delegate_impl.cc (right):

http://codereview.chromium.org/153002/diff/25001/26006#newcode99
Line 99: if (!(quirks_ && PLUGIN_QUIRK_DONT_CREATE_DUMMY_WINDOW)) {
On 2009/09/17 19:17:57, John Abd-El-Malek wrote:
> I think you mean one "&" instead of two.

Done.

http://codereview.chromium.org/153002/diff/25001/26007
File webkit/glue/plugins/webplugin_delegate_impl.h (right):

http://codereview.chromium.org/153002/diff/25001/26007#newcode131
Line 131: // Called from the constructor, sets quirks that are used on all
platforms.
On 2009/09/17 19:17:57, John Abd-El-Malek wrote:
> this function is unnecessary, it sets a quirk that's only used in the windows
> implementation, so it should just be set in the windows constructor.

A condition with this flag exists in a shared file (webplugin_delegate_impl.c),
so it doesn't look like windows-specific.

http://codereview.chromium.org/153002/diff/25001/26010
File webkit/glue/plugins/webplugin_delegate_impl_win.cc (right):

http://codereview.chromium.org/153002/diff/25001/26010#newcode355
Line 355: // change when a windowless plugin instance changes the cursor
On 2009/09/17 19:17:57, John Abd-El-Malek wrote:
> nit: in all this comment, no whitespace needed at the end of lines

Cannot see any whitespaces in the code, maybe the diff shows them as part of the
layout difference.

[jam, 2009-09-17 21:37:08]

http://codereview.chromium.org/153002/diff/26038/25019
File webkit/glue/plugins/webplugin_delegate_impl.h (right):

http://codereview.chromium.org/153002/diff/26038/25019#newcode132
Line 132: void SetSharedQuirks();
This function is not needed at all.  What I'm trying to say is that the quirk
should be added in webplugin_delegate_impl_win.cc, in the constructor, just like
all the other quirks are currently added.  No need for a function, and no need
to call it or define it in any of the other files.

[gregoryd, 2009-09-17 21:51:20]

I found that this flag has an effect also on Linux - see
webplugin_delegate_impl.cc line 99. If this is the case, it looks that it
won't be safe to set it on Windows only, therefore it needs to be set in a
shared function. However, if you know it won't have any effect on Linux, it
should be safe to remove the function.


On Thu, Sep 17, 2009 at 14:37, <jam@chromium.org> wrote:

>
> http://codereview.chromium.org/153002/diff/26038/25019
> File webkit/glue/plugins/webplugin_delegate_impl.h (right):
>
> http://codereview.chromium.org/153002/diff/26038/25019#newcode132
> Line 132: void SetSharedQuirks();
> This function is not needed at all.  What I'm trying to say is that the
> quirk should be added in webplugin_delegate_impl_win.cc, in the
> constructor, just like all the other quirks are currently added.  No
> need for a function, and no need to call it or define it in any of the
> other files.
>
>
> http://codereview.chromium.org/153002
>

[gregoryd, 2009-09-17 22:21:43]

http://codereview.chromium.org/153002/diff/26038/25019
File webkit/glue/plugins/webplugin_delegate_impl.h (right):

http://codereview.chromium.org/153002/diff/26038/25019#newcode132
Line 132: void SetSharedQuirks();
On 2009/09/17 21:37:08, John Abd-El-Malek wrote:
> This function is not needed at all.  What I'm trying to say is that the quirk
> should be added in webplugin_delegate_impl_win.cc, in the constructor, just
like
> all the other quirks are currently added.  No need for a function, and no need
> to call it or define it in any of the other files.

Done.

[jam, 2009-09-17 22:26:38]

lgtm

[darin (slow to review), 2009-09-18 03:52:26]

the tree was closed... i saw that this was landing... here's some more review
feedback :-)

http://codereview.chromium.org/153002/diff/29003/29014
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/153002/diff/29003/29014#newcode29
Line 29: #include "chrome/browser/nacl_process_host.h"
nit: please sort include headers

http://codereview.chromium.org/153002/diff/29003/29014#newcode588
Line 588: nacl_host->Launch(this, channel_descriptor, handle);
how is the lifetime of nacl_host managed?  looking over the Launch method, I
don't see anything that frees the memory of the NaClProcessHost.  this looks
suspiciously like a memory leak.

http://codereview.chromium.org/153002/diff/29003/29013
File chrome/browser/renderer_host/resource_message_filter.h (right):

http://codereview.chromium.org/153002/diff/29003/29013#newcode158
Line 158: void OnLaunchNaCl(const int channel_descriptor,
nit: const is extraneous here since the argument is copied by value.  our
convention is to leave off extraneous modifiers like this.

http://codereview.chromium.org/153002/diff/29003/29020
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/153002/diff/29003/29020#newcode75
Line 75: const wchar_t kNaClProcess[]                 = L"nacl";
nit: it looks like there was an effort made to line up the values in a column. 
this breaks that.

http://codereview.chromium.org/153002/diff/29003/29020#newcode371
Line 371: const wchar_t kInternalNaCl[]                   = L"internal-nacl";
ditto... although this file is already pretty inconsistent :(

http://codereview.chromium.org/153002/diff/29003/29027
File chrome/common/nacl_messages_internal.h (right):

http://codereview.chromium.org/153002/diff/29003/29027#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
nit: 2009

http://codereview.chromium.org/153002/diff/29003/29027#newcode6
Line 6: #include "chrome/common/nacl_types.h"
nit: please sort includes

http://codereview.chromium.org/153002/diff/29003/29027#newcode13
Line 13: IPC_MESSAGE_CONTROL2(NaClProcessMsg_StartSelLdr,
It is quite contrary to google style to use abbreviations like this.  It assumes
a lot of your reader that they have to know what SelLdr is.  That would be a
very incomprehensible string to me if I were new to the code base.

Maybe the term SelLdr should remain an implementation detail?  How about just
calling this message NaClProcessMsg_Start :)

http://codereview.chromium.org/153002/diff/29003/29035
File chrome/common/nacl_types.h (right):

http://codereview.chromium.org/153002/diff/29003/29035#newcode9
Line 9: namespace nacl {
why is the nacl namespace being defined in chrome/common?  it seems out of place
to do so here.

http://codereview.chromium.org/153002/diff/29003/29035#newcode12
Line 12: #define NATIVE_HANDLE(desc) (static_cast<HANDLE>(desc))
can we avoid macros like this?  maybe an inline function instead?

  inline HANDLE ToNativeHandle(const FileDescriptor& desc) {
    return desc;
  }

and then for posix:

  inline int ToNativeHandle(const FileDescriptor& desc) {
    return desc.fd;
  }

http://codereview.chromium.org/153002/diff/29003/29019
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/153002/diff/29003/29019#newcode1073
Line 1073: int /* channel number, usually 5 */,
Q: is the "usually 5" comment helpful?

http://codereview.chromium.org/153002/diff/29003/29029
File chrome/nacl/nacl_main.cc (right):

http://codereview.chromium.org/153002/diff/29003/29029#newcode21
Line 21: #if defined(OS_WIN)
nit: any reason why there are two OS_WIN sections to include headers?  why not
just have this one here?

http://codereview.chromium.org/153002/diff/29003/29029#newcode43
Line 43: parsed_command_line.command_line_string();
nit: line continuations usually get indented 4 spaces as you do a couple lines
above.

http://codereview.chromium.org/153002/diff/29003/29004
File webkit/glue/plugins/plugin_lib.h (right):

http://codereview.chromium.org/153002/diff/29003/29004#newcode64
Line 64: bool internal() { return internal_; }
it might be nice to define what internal means here.  it is pretty mysterious
given the context of this file.

http://codereview.chromium.org/153002/diff/29003/29006
File webkit/glue/plugins/plugin_list.cc (right):

http://codereview.chromium.org/153002/diff/29003/29006#newcode18
Line 18: #define kNpGoogleNaClPlugin (FILE_PATH_LITERAL("npgooglenaclplugin"))
nit: please avoid defining macros for things like this.  this can be a static
FilePath::CharType pointer.

http://codereview.chromium.org/153002/diff/29003/29010
File webkit/glue/plugins/webplugin_delegate_impl_win.cc (right):

http://codereview.chromium.org/153002/diff/29003/29010#newcode332
Line 332: if (windowless_ && !(quirks_ & PLUGIN_QUIRK_DONT_CREATE_DUMMY_WINDOW))
{
why do we need this quirk if we can just check plugin_lib()->internal()?  isn't
the fact that the plugin is internal enough to know that we cannot create a
dummy window?  if we did so, then we would not need to mention the nacl mime
type in this file, which would be a win for maintainability.

[jam, 2009-09-18 17:58:50]

Sorry for the late reply, but as Marshall pointed out in another review, the
changes to plugin_list aren't needed.  This should reuse the
RegisterInternalPlugin which takes in the function pointers.

[jam, 2009-09-18 20:30:27]

hey guys, so it looks like Darin has a few small comments that should be
addressed.  I think it'd be better if they're done before checkin (usually
when these things are left later for a follow-up patch, they're forgotten).
 Since Gregory is on vacation, can someone from the NaCl team address these
comments before checkin?  Or it can wait until Gregory is back next week.
Here's a patch with my last comment (the one re not needing to change
plugin_list) addressed and also turning on --no-sandbox automatically when
--internal-nacl is turned on: http://codereview.chromium.org/212020  I had
to make changes to the nacl_entry_points files in natiev_client.

Thanks

On Fri, Sep 18, 2009 at 10:58 AM, <jam@chromium.org> wrote:

> Sorry for the late reply, but as Marshall pointed out in another review,
> the
> changes to plugin_list aren't needed.  This should reuse the
> RegisterInternalPlugin which takes in the function pointers.
>
>
> http://codereview.chromium.org/153002
>

[gregoryd, 2009-09-24 18:28:45]

The new revision of this CL can be found at
http://codereview.chromium.org/219023

http://codereview.chromium.org/153002/diff/29003/29014
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/153002/diff/29003/29014#newcode29
Line 29: #include "chrome/browser/nacl_process_host.h"
On 2009/09/18 03:52:26, darin wrote:
> nit: please sort include headers

Done.

http://codereview.chromium.org/153002/diff/29003/29014#newcode588
Line 588: nacl_host->Launch(this, channel_descriptor, handle);
On 2009/09/18 03:52:26, darin wrote:
> how is the lifetime of nacl_host managed?  looking over the Launch method, I
> don't see anything that frees the memory of the NaClProcessHost.  this looks
> suspiciously like a memory leak.

Fixed. Will consider implementing a NaClService similar to PluginService and
WorkerService in the future, but at this point a simple solution seems
sufficient.

http://codereview.chromium.org/153002/diff/29003/29013
File chrome/browser/renderer_host/resource_message_filter.h (right):

http://codereview.chromium.org/153002/diff/29003/29013#newcode158
Line 158: void OnLaunchNaCl(const int channel_descriptor,
On 2009/09/18 03:52:26, darin wrote:
> nit: const is extraneous here since the argument is copied by value.  our
> convention is to leave off extraneous modifiers like this.

Done.

http://codereview.chromium.org/153002/diff/29003/29020
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/153002/diff/29003/29020#newcode75
Line 75: const wchar_t kNaClProcess[]                 = L"nacl";
On 2009/09/18 03:52:26, darin wrote:
> nit: it looks like there was an effort made to line up the values in a column.

> this breaks that.

Done.

http://codereview.chromium.org/153002/diff/29003/29020#newcode371
Line 371: const wchar_t kInternalNaCl[]                   = L"internal-nacl";
On 2009/09/18 03:52:26, darin wrote:
> ditto... although this file is already pretty inconsistent :(

Done.

http://codereview.chromium.org/153002/diff/29003/29027
File chrome/common/nacl_messages_internal.h (right):

http://codereview.chromium.org/153002/diff/29003/29027#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
On 2009/09/18 03:52:26, darin wrote:
> nit: 2009

Done.

http://codereview.chromium.org/153002/diff/29003/29027#newcode6
Line 6: #include "chrome/common/nacl_types.h"
On 2009/09/18 03:52:26, darin wrote:
> nit: please sort includes

Done.

http://codereview.chromium.org/153002/diff/29003/29027#newcode13
Line 13: IPC_MESSAGE_CONTROL2(NaClProcessMsg_StartSelLdr,
On 2009/09/18 03:52:26, darin wrote:
> It is quite contrary to google style to use abbreviations like this.  It
assumes
> a lot of your reader that they have to know what SelLdr is.  That would be a
> very incomprehensible string to me if I were new to the code base.
> 
> Maybe the term SelLdr should remain an implementation detail?  How about just
> calling this message NaClProcessMsg_Start :)

Done.

http://codereview.chromium.org/153002/diff/29003/29035
File chrome/common/nacl_types.h (right):

http://codereview.chromium.org/153002/diff/29003/29035#newcode9
Line 9: namespace nacl {
On 2009/09/18 03:52:26, darin wrote:
> why is the nacl namespace being defined in chrome/common?  it seems out of
place
> to do so here.

This was first added to the nacl directory, but then the check_deps phase of the
build failed. The right way to solve this will be to add a definition for
base::FileDescriptor on Windows. Adding a TODO meanwhile, will add it to this CL
if the time allows.

http://codereview.chromium.org/153002/diff/29003/29035#newcode12
Line 12: #define NATIVE_HANDLE(desc) (static_cast<HANDLE>(desc))
On 2009/09/18 03:52:26, darin wrote:
> can we avoid macros like this?  maybe an inline function instead?
> 
>   inline HANDLE ToNativeHandle(const FileDescriptor& desc) {
>     return desc;
>   }
> 
> and then for posix:
> 
>   inline int ToNativeHandle(const FileDescriptor& desc) {
>     return desc.fd;
>   }
> 

Will be fixed together with the change to base::FileDescriptor

http://codereview.chromium.org/153002/diff/29003/29019
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/153002/diff/29003/29019#newcode1073
Line 1073: int /* channel number, usually 5 */,
On 2009/09/18 03:52:26, darin wrote:
> Q: is the "usually 5" comment helpful?

Done.

http://codereview.chromium.org/153002/diff/29003/29029
File chrome/nacl/nacl_main.cc (right):

http://codereview.chromium.org/153002/diff/29003/29029#newcode21
Line 21: #if defined(OS_WIN)
On 2009/09/18 03:52:26, darin wrote:
> nit: any reason why there are two OS_WIN sections to include headers?  why not
> just have this one here?

Done.

http://codereview.chromium.org/153002/diff/29003/29029#newcode43
Line 43: parsed_command_line.command_line_string();
On 2009/09/18 03:52:26, darin wrote:
> nit: line continuations usually get indented 4 spaces as you do a couple lines
> above.

Done.

http://codereview.chromium.org/153002/diff/29003/29004
File webkit/glue/plugins/plugin_lib.h (right):

http://codereview.chromium.org/153002/diff/29003/29004#newcode64
Line 64: bool internal() { return internal_; }
On 2009/09/18 03:52:26, darin wrote:
> it might be nice to define what internal means here.  it is pretty mysterious
> given the context of this file.

Extended the comment at line 96

http://codereview.chromium.org/153002/diff/29003/29006
File webkit/glue/plugins/plugin_list.cc (right):

http://codereview.chromium.org/153002/diff/29003/29006#newcode18
Line 18: #define kNpGoogleNaClPlugin (FILE_PATH_LITERAL("npgooglenaclplugin"))
On 2009/09/18 03:52:26, darin wrote:
> nit: please avoid defining macros for things like this.  this can be a static
> FilePath::CharType pointer.

Done.

http://codereview.chromium.org/153002/diff/29003/29010
File webkit/glue/plugins/webplugin_delegate_impl_win.cc (right):

http://codereview.chromium.org/153002/diff/29003/29010#newcode332
Line 332: if (windowless_ && !(quirks_ & PLUGIN_QUIRK_DONT_CREATE_DUMMY_WINDOW))
{
On 2009/09/18 03:52:26, darin wrote:
> why do we need this quirk if we can just check plugin_lib()->internal()? 
isn't
> the fact that the plugin is internal enough to know that we cannot create a
> dummy window?  if we did so, then we would not need to mention the nacl mime
> type in this file, which would be a win for maintainability.

Done.

[jam, 2009-09-25 17:35:27]

Hi, this is missing the changes I had suggested/made regarding not needing to
change plugin_list since RegisterInternalPlugin should be used.

http://codereview.chromium.org/153002/diff/29003/29014
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/153002/diff/29003/29014#newcode588
Line 588: nacl_host->Launch(this, channel_descriptor, handle);
On 2009/09/24 18:28:45, gregoryd wrote:
> On 2009/09/18 03:52:26, darin wrote:
> > how is the lifetime of nacl_host managed?  looking over the Launch method, I
> > don't see anything that frees the memory of the NaClProcessHost.  this looks
> > suspiciously like a memory leak.
> 
> Fixed. Will consider implementing a NaClService similar to PluginService and
> WorkerService in the future, but at this point a simple solution seems
> sufficient.

Note: PluginService/WorkerService aren't there to avoid memory leaks (all child
process host objects delete themselves when the child process goes away). 
They're there since each plugin/worker process can support more than one
instance, so the service figures out when to reuse vs create a new process.  So
if each NaCl process will host just one NaCl module, a service isn't necessary.

[gregoryd, 2009-09-25 18:35:29]

Removed the changes to plugin_list files. 

Regarding NaClService - it just seems as a good idea to have a centralized place
to manage all NaCl processes. We are also considering running several instances
of NaCl modules in a single process in the future.

[jam, 2009-09-25 19:36:06]

http://codereview.chromium.org/153002/diff/29003/29014
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/153002/diff/29003/29014#newcode588
Line 588: nacl_host->Launch(this, channel_descriptor, handle);
On 2009/09/24 18:28:45, gregoryd wrote:
> On 2009/09/18 03:52:26, darin wrote:
> > how is the lifetime of nacl_host managed?  looking over the Launch method, I
> > don't see anything that frees the memory of the NaClProcessHost.  this looks
> > suspiciously like a memory leak.
> 
> Fixed.

There is no leak as long as the child process goes away since ChildProcessHost
objects delete themselves when the child process dies.  See
ChildProcessHost::OnChildDied().  So to ensure proper termination, the internal
plugin in the renderer process just has to send a message over the IMC channel
telling the process to exit.  BTW I don't see ViewHostMsg_LaunchNaCl sent
anywhere, is that in a different change?

This fix is incorrect because RenderProcessHost is not threadsafe, so modifying
the member vector could result in it being accessed on the UI thread at the same
time.  Also, there are race conditions since the NaClProcessHost object is being
deleted on two threads.

http://codereview.chromium.org/153002/diff/41001/41014
File chrome/browser/nacl_process_host.cc (right):

http://codereview.chromium.org/153002/diff/41001/41014#newcode170
Line 170: channel);
nit: spacing is off

http://codereview.chromium.org/153002/diff/41001/41003
File webkit/glue/plugins/plugin_lib.h (right):

http://codereview.chromium.org/153002/diff/41001/41003#newcode97
Line 97: // renderer process.
internal plugins are ones that are implemented inside chrome.dll, but they do
run in separate processes (unless code is added to prevent this like in NaCl's
case).  for example, the default plugin.

[gregoryd, 2009-09-25 19:54:29]

http://codereview.chromium.org/153002/diff/29003/29014
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/153002/diff/29003/29014#newcode588
Line 588: nacl_host->Launch(this, channel_descriptor, handle);
I suspected something like this exists but could not find it. Will revert the
related changes and upload. The Launch message is sent from the plugin code that
is in native_client repository.

[gregoryd, 2009-09-25 20:28:15]

Uploaded the new version

http://codereview.chromium.org/153002/diff/41001/41014
File chrome/browser/nacl_process_host.cc (right):

http://codereview.chromium.org/153002/diff/41001/41014#newcode170
Line 170: channel);
On 2009/09/25 19:36:06, John Abd-El-Malek wrote:
> nit: spacing is off

Done.

http://codereview.chromium.org/153002/diff/41001/41003
File webkit/glue/plugins/plugin_lib.h (right):

http://codereview.chromium.org/153002/diff/41001/41003#newcode97
Line 97: // renderer process.
On 2009/09/25 19:36:06, John Abd-El-Malek wrote:
> internal plugins are ones that are implemented inside chrome.dll, but they do
> run in separate processes (unless code is added to prevent this like in NaCl's
> case).  for example, the default plugin.

Updated the comment

[jam, 2009-09-25 20:48:16]

http://codereview.chromium.org/153002/diff/29003/29014
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/153002/diff/29003/29014#newcode588
Line 588: nacl_host->Launch(this, channel_descriptor, handle);
On 2009/09/25 19:54:29, gregoryd wrote:
> I suspected something like this exists but could not find it. Will revert the
> related changes and upload. The Launch message is sent from the plugin code
that
> is in native_client repository.

sorry for continuing to drag this on, but your comment above led me to
src\native_client\src\trusted\nonnacl_util\sel_ldr_launcher_chrome.cc

This breaks the abstractions we have in-place with regard to code in src\
reaching out to chrome.  It also introduces circular dependencies, since
src\chrome depends on src\nativecleint and vice versa.  The DEPS file in
src\nativeclient should have entries to enforce that it doesn't try to include
files outside of that module.

perhaps when RegisterInternalNaClPlugin() is called, Chrome passes in an
implementation of an object that has methods for starting a process.  That
implementation lives in src\chrome and is able to send IPCs etc.

[gregoryd, 2009-09-26 00:20:42]

Done. The change in chrome depends on a change to native_client (
http://codereview.chromium.org/245015/show) that will be submitted once NaCl
tree reopens.

On Fri, Sep 25, 2009 at 13:48, <jam@chromium.org> wrote:

>
> http://codereview.chromium.org/153002/diff/29003/29014
> File chrome/browser/renderer_host/resource_message_filter.cc (right):
>
> http://codereview.chromium.org/153002/diff/29003/29014#newcode588
> Line 588: nacl_host->Launch(this, channel_descriptor, handle);
> On 2009/09/25 19:54:29, gregoryd wrote:
>
>> I suspected something like this exists but could not find it. Will
>>
> revert the
>
>> related changes and upload. The Launch message is sent from the plugin
>>
> code that
>
>> is in native_client repository.
>>
>
> sorry for continuing to drag this on, but your comment above led me to
> src\native_client\src\trusted\nonnacl_util\sel_ldr_launcher_chrome.cc
>
> This breaks the abstractions we have in-place with regard to code in
> src\ reaching out to chrome.  It also introduces circular dependencies,
> since src\chrome depends on src\nativecleint and vice versa.  The DEPS
> file in src\nativeclient should have entries to enforce that it doesn't
> try to include files outside of that module.
>
> perhaps when RegisterInternalNaClPlugin() is called, Chrome passes in an
> implementation of an object that has methods for starting a process.
> That implementation lives in src\chrome and is able to send IPCs etc.
>
>
> http://codereview.chromium.org/153002
>

[jam, 2009-09-26 00:43:54]

lgtm

[darin (slow to review), 2009-09-26 03:45:13]

LGTM2.  please address these nits before committing:

http://codereview.chromium.org/153002/diff/45032/44057
File chrome/nacl/nacl_thread.cc (right):

http://codereview.chromium.org/153002/diff/45032/44057#newcode7
Line 7: #include "build/build_config.h"
nit: it isn't necessary to include build_config.h here.

http://codereview.chromium.org/153002/diff/45032/44060
File chrome/nacl/nacl_thread.h (right):

http://codereview.chromium.org/153002/diff/45032/44060#newcode26
Line 26: void OnStartSelLdr(const int channel_descriptor,
nit: there's no benefit to passing these parameters with "const" since they are
passed by value instead of by reference.  we generally avoid the extraneous
const in cases like this since it only adds more noise.

http://codereview.chromium.org/153002/diff/45032/44060#newcode30
Line 30: scoped_ptr<NotificationService> notification_service_;
this member variable appears to be unused.  can it be removed?

http://codereview.chromium.org/153002/diff/45032/44060#newcode32
Line 32: DISALLOW_EVIL_CONSTRUCTORS(NaClThread);
nit: new code should use DISALLOW_COPY_AND_ASSIGN instead.

http://codereview.chromium.org/153002/diff/45032/44058
File chrome/nacl/sel_main.cc (right):

http://codereview.chromium.org/153002/diff/45032/44058#newcode5
Line 5: #include "build/build_config.h"
nit: build_config.h does not seem necessary here.

http://codereview.chromium.org/153002/diff/45032/44033
File webkit/glue/plugins/plugin_lib.h (right):

http://codereview.chromium.org/153002/diff/45032/44033#newcode96
Line 96: bool internal_;  // True for plugins that are built-in into chrome.dll
we also compile this same code into numerous other binaries (e.g.,
test_shell.exe), so this comment shouldn't be specific about chrome.dll.