Make the NaCl windows 64 bit binaries not depend on chrome targets.

Make the NaCl windows 64 bit binaries not depend on chrome targets. These targets are very simple and used little code from chrome targets. However their dependency on chrome targets was problematic because a lot of code wasn't being built for 64 bit on Windows, and so there were a lot of "dummy" files being added with stub functions and code was also being compiled out in random places for NACL_WIN64.

I've made the NaCl 64 bit windows targets self contained. They do use a few files from common, but those files are self-contained. In the future, we could move these to be in the same 64 bit target as the constants from common. However that won't make a maintenance difference since someone could still introduce link dependencies to other files in common.

Additionally, since we're not using chrome code anymore, we can avoid having both nacl.exe and nacl.dll. nacl.exe is sufficient, and this saves 1.4MB of uncompresed binaries in the installer.

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

[jam, 2011-09-11 16:00:14]

Brad Nelson: please review the gyp changes
Brad Chen: please review the rest. I wasn't sure who to send this to, so feel
free to send all or some parts to others.

[Brad Chen (chromium), 2011-09-11 17:35:37]

This looks like it kills a lot of nasty code so that's wonderful. I would like
to add another reviewer who knows the Windows startup code better, adding Noel
Allen.

Another concern is that this is well tested before it lands. I'm not in a
position to  test on Windows today but I could give it a try tomorrow. Let me
know if you have doubts about what you've tested yourself. It doesn't look to me
like the trybots cover 32- and 64-bit Windows very well so I guess that needs to
be done manually. I imagine the bots are all 64-bit Windows, that's probably
better as it's what is most impacted by this change. 

It would also be good to know how this will impact installer size. Presumably it
eliminates code from the 64-bit DLL. It's worth confirming it won't get bigger.

http://codereview.chromium.org/7863024/diff/8070/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

http://codereview.chromium.org/7863024/diff/8070/chrome/chrome_exe.gypi#newco...
chrome/chrome_exe.gypi:553: 'SubSystem': '2',
What does this mean? I'm not fluent in MSVS settings.

http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_broker_list...
File chrome/nacl/nacl_broker_listener.h (right):

http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_broker_list...
chrome/nacl/nacl_broker_listener.h:6: #define
CHROME_NACL_NACL_BROKER_LISTENER_H_
Is there a naming convention for files that are Windows only, akin to _linux for
linux-only files?

If you want this to be used portably someday a TODO() comment to that effect
would be good.

http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_exe_win_64.cc
File chrome/nacl/nacl_exe_win_64.cc (right):

http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_exe_win_64....
chrome/nacl/nacl_exe_win_64.cc:12: int APIENTRY wWinMain(HINSTANCE instance,
HINSTANCE, wchar_t*, int) {
For my edification, what does the w before WinMain mean here?

http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_main_win_64.cc
File chrome/nacl/nacl_main_win_64.cc (right):

http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_main_win_64...
chrome/nacl/nacl_main_win_64.cc:3: // found in the LICENSE file.
Someplace, perhaps here, it would be nice to have a sketch of how NaCl support
on Windows works, why and how 32-bit is different than 64-bit, etc.

http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_main_win_64...
chrome/nacl/nacl_main_win_64.cc:70: bool debug =
parsed_command_line.HasSwitch(switches::kEnableNaClDebug);
If you are unclear about how this debug support works or how to test it it would
be worth asking Mike Mortensen to review this code.

http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_main_win_64...
chrome/nacl/nacl_main_win_64.cc:89: __declspec(dllexport) int __cdecl
ChromeMain(
Can you add some sort of comment that explains why it is necessary to have a
routine named ChromeMain() here? I'm assuming that the way things are put
together you need a stand-in for the routine in chrome/apps/chrome_main.cc but
it is not obvious why.

What I'm inferring is that this is a stripped down version of chrome.exe that
just knows how to do what the 64-bit NaCl launcher needs. That doesn't
necessarily explain why "ChromeMain" must be reused.

[Brad Chen (chromium), 2011-09-11 17:37:12]

One other detail; from your CL description it sounds like this change might
address the need you noted earlier for something like the nacl_helper on
Windows. If it does not, please add a comment someplace in the Windows code
about what you think should happen next towards that end.

On 2011/09/11 17:35:37, Brad Chen (chromium) wrote:
> This looks like it kills a lot of nasty code so that's wonderful. I would like
> to add another reviewer who knows the Windows startup code better, adding Noel
> Allen.
> 
> Another concern is that this is well tested before it lands. I'm not in a
> position to  test on Windows today but I could give it a try tomorrow. Let me
> know if you have doubts about what you've tested yourself. It doesn't look to
me
> like the trybots cover 32- and 64-bit Windows very well so I guess that needs
to
> be done manually. I imagine the bots are all 64-bit Windows, that's probably
> better as it's what is most impacted by this change. 
> 
> It would also be good to know how this will impact installer size. Presumably
it
> eliminates code from the 64-bit DLL. It's worth confirming it won't get
bigger.
> 
> http://codereview.chromium.org/7863024/diff/8070/chrome/chrome_exe.gypi
> File chrome/chrome_exe.gypi (right):
> 
>
http://codereview.chromium.org/7863024/diff/8070/chrome/chrome_exe.gypi#newco...
> chrome/chrome_exe.gypi:553: 'SubSystem': '2',
> What does this mean? I'm not fluent in MSVS settings.
> 
>
http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_broker_list...
> File chrome/nacl/nacl_broker_listener.h (right):
> 
>
http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_broker_list...
> chrome/nacl/nacl_broker_listener.h:6: #define
> CHROME_NACL_NACL_BROKER_LISTENER_H_
> Is there a naming convention for files that are Windows only, akin to _linux
for
> linux-only files?
> 
> If you want this to be used portably someday a TODO() comment to that effect
> would be good.
> 
>
http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_exe_win_64.cc
> File chrome/nacl/nacl_exe_win_64.cc (right):
> 
>
http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_exe_win_64....
> chrome/nacl/nacl_exe_win_64.cc:12: int APIENTRY wWinMain(HINSTANCE instance,
> HINSTANCE, wchar_t*, int) {
> For my edification, what does the w before WinMain mean here?
> 
>
http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_main_win_64.cc
> File chrome/nacl/nacl_main_win_64.cc (right):
> 
>
http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_main_win_64...
> chrome/nacl/nacl_main_win_64.cc:3: // found in the LICENSE file.
> Someplace, perhaps here, it would be nice to have a sketch of how NaCl support
> on Windows works, why and how 32-bit is different than 64-bit, etc.
> 
>
http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_main_win_64...
> chrome/nacl/nacl_main_win_64.cc:70: bool debug =
> parsed_command_line.HasSwitch(switches::kEnableNaClDebug);
> If you are unclear about how this debug support works or how to test it it
would
> be worth asking Mike Mortensen to review this code.
> 
>
http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_main_win_64...
> chrome/nacl/nacl_main_win_64.cc:89: __declspec(dllexport) int __cdecl
> ChromeMain(
> Can you add some sort of comment that explains why it is necessary to have a
> routine named ChromeMain() here? I'm assuming that the way things are put
> together you need a stand-in for the routine in chrome/apps/chrome_main.cc but
> it is not obvious why.
> 
> What I'm inferring is that this is a stripped down version of chrome.exe that
> just knows how to do what the 64-bit NaCl launcher needs. That doesn't
> necessarily explain why "ChromeMain" must be reused.

[jam, 2011-09-11 22:51:37]

To answer your questions, I had tested this on 64 bit Windows. This code is only
used in 64 bit, so I can test on a vm for 32 bit but it shouldn't make a
difference since this code path/binary isn't used there. And yep, this is the
sort of Windows equivalent to nacl_helper, so this take care of bug 86322.

Your comments made me realize that this could be simplified even further. We
only needed an exe/dll pair because the other Windows code that we reused from
chrome needed this. That's because chrome needs a chrome exe/dll pair for auto
update reasons. However the nacl broker doesn't need this. It had it before
because it shared code that assumed this. But now that we don't need any chrome
code, we can put everything in nacl.exe and not need a nacl.dll. This would
simplify the code and gyp a lot more, and reduce the need to build the installer
code in 64 bit which would reduce the binary size even more. I'll look into
this, so please hold off reviews until I finish it. Thanks!

http://codereview.chromium.org/7863024/diff/8070/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

http://codereview.chromium.org/7863024/diff/8070/chrome/chrome_exe.gypi#newco...
chrome/chrome_exe.gypi:553: 'SubSystem': '2',
On 2011/09/11 17:35:37, Brad Chen (chromium) wrote:
> What does this mean? I'm not fluent in MSVS settings.

This just tells Visual Studio that it's generating a Windows binary, and to look
for wWinMain function. Before when we were using "chrome_exe_target==1", it was
done by line 55 above. since we're not using that anymore, I have to add it
here.

http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_broker_list...
File chrome/nacl/nacl_broker_listener.h (right):

http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_broker_list...
chrome/nacl/nacl_broker_listener.h:6: #define
CHROME_NACL_NACL_BROKER_LISTENER_H_
On 2011/09/11 17:35:37, Brad Chen (chromium) wrote:
> Is there a naming convention for files that are Windows only, akin to _linux
for
> linux-only files?

we add _win at the end. This is used when a file is listed in a target that has
different files for different platforms, so gyp automatically selects the files
for each platform based on that suffix. Since this file is only in a Windows
target, it's unnecessary.

> 
> If you want this to be used portably someday a TODO() comment to that effect
> would be good.

The broker is only used on Windows, so there'd be no need to port this.

http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_exe_win_64.cc
File chrome/nacl/nacl_exe_win_64.cc (right):

http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_exe_win_64....
chrome/nacl/nacl_exe_win_64.cc:12: int APIENTRY wWinMain(HINSTANCE instance,
HINSTANCE, wchar_t*, int) {
On 2011/09/11 17:35:37, Brad Chen (chromium) wrote:
> For my edification, what does the w before WinMain mean here?

This is the entry point that Windows looks for. This matches what's in
chrome/app/chrome_exe_main_win.cc

http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_main_win_64.cc
File chrome/nacl/nacl_main_win_64.cc (right):

http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_main_win_64...
chrome/nacl/nacl_main_win_64.cc:3: // found in the LICENSE file.
On 2011/09/11 17:35:37, Brad Chen (chromium) wrote:
> Someplace, perhaps here, it would be nice to have a sketch of how NaCl support
> on Windows works, why and how 32-bit is different than 64-bit, etc.

that sounds like a good idea. Is there no other documentation in any of the nacl
docs about why a broker is needed on 64 bit windows (I'm not familiar with the
nacl docs)

http://codereview.chromium.org/7863024/diff/8070/chrome/nacl/nacl_main_win_64...
chrome/nacl/nacl_main_win_64.cc:89: __declspec(dllexport) int __cdecl
ChromeMain(
On 2011/09/11 17:35:37, Brad Chen (chromium) wrote:
> Can you add some sort of comment that explains why it is necessary to have a
> routine named ChromeMain() here? I'm assuming that the way things are put
> together you need a stand-in for the routine in chrome/apps/chrome_main.cc but
> it is not obvious why.

The code in MainDllLoader (called from nacl_exe_win_64.cc) loads the associated
dll and looks for "ChromeMain" export.

> 
> What I'm inferring is that this is a stripped down version of chrome.exe that
> just knows how to do what the 64-bit NaCl launcher needs. That doesn't
> necessarily explain why "ChromeMain" must be reused.

[jam, 2011-09-12 06:44:08]

ok, this is now ready for review. I've removed nacl.dll and put everything in
nacl.exe, and now in total this change saves 1.4 MB of uncompressed binaries in
the installer.

[Brad Chen, 2011-09-12 16:52:51]

LGTM for the CC changes. I'd still like to have Noel have a look, and also Brad
Nelson for the gyp changes.

On 2011/09/12 06:44:08, John Abd-El-Malek wrote:
> ok, this is now ready for review. I've removed nacl.dll and put everything in
> nacl.exe, and now in total this change saves 1.4 MB of uncompressed binaries
in
> the installer.

[bradn, 2011-09-12 18:15:08]

LGTM

[noelallen1, 2011-09-12 18:52:39]

One question just for my edification, and a nit you can ignore.
LGTM.

http://codereview.chromium.org/7863024/diff/11018/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

http://codereview.chromium.org/7863024/diff/11018/chrome/chrome_exe.gypi#newc...
chrome/chrome_exe.gypi:11: ['chrome_exe_target==1', {
Question:  This isn't really part of this CL, but does this mean we can get rid
of this block by moving it bellow into the chrome target?  Not sure it gets used
anywhere else anymore.

http://codereview.chromium.org/7863024/diff/11018/chrome/chrome_exe.gypi#newc...
chrome/chrome_exe.gypi:556: 'SubSystem': '2',         # Set /SUBSYSTEM:WINDOWS
nit: Isn't this the default?  Not sure you really had to specify this.

[jam, 2011-09-12 19:53:02]

http://codereview.chromium.org/7863024/diff/11018/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

http://codereview.chromium.org/7863024/diff/11018/chrome/chrome_exe.gypi#newc...
chrome/chrome_exe.gypi:11: ['chrome_exe_target==1', {
On 2011/09/12 18:52:39, noelallen1 wrote:
> Question:  This isn't really part of this CL, but does this mean we can get
rid
> of this block by moving it bellow into the chrome target?  Not sure it gets
used
> anywhere else anymore.

ah yes, you're right, I forgot about it (I cleaned up the other instances but
forgot about it). will take care of this

http://codereview.chromium.org/7863024/diff/11018/chrome/chrome_exe.gypi#newc...
chrome/chrome_exe.gypi:556: 'SubSystem': '2',         # Set /SUBSYSTEM:WINDOWS
On 2011/09/12 18:52:39, noelallen1 wrote:
> nit: Isn't this the default?  Not sure you really had to specify this.

the default is a SUBSYSTEM:CONSOLE