Remove dependency from NaCl code on chrome_constants.

Remove dependency from NaCl code on chrome_constants.

This is part of an effort to componentize NaCl code.
BUG=244791
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207806

[yael.aharon1, 2013-05-31 19:28:08]

This is a small step to reduce the dependencies between NaCl code and the chrome
layer.
The overall work was discussed in
https://groups.google.com/a/chromium.org/forum/?fromgroups#%21topic/pepper-de...
Can you please review?
thanks!

[dmichael (off chromium), 2013-05-31 19:42:18]

I don't think I'm an owner in any of these places... looks mostly good to me,
just some nits

https://codereview.chromium.org/16154005/diff/1/chrome/browser/nacl_host/nacl...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/16154005/diff/1/chrome/browser/nacl_host/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:1076: static const wchar_t
kNaClAppName[] = L"nacl64";
nit: static doesn't really add anything here.

https://codereview.chromium.org/16154005/diff/1/chrome/browser/nacl_host/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:1079: LOG(ERROR) << "NaCl process
launch failed: could not resolve module";
FWIW, this log used to only be emitted from this file (but it looks to me like
it's appropriate to emit it in both cases, as you have done).

https://codereview.chromium.org/16154005/diff/1/chrome/browser/nacl_host/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:1082: exe_path =
module_path.DirName().Append(kNaClAppName);
optional nit: Seems like you could just stick the literal here. We don't seem to
be using it anywhere else in the function. Or...  if we want to use the constant
to make it more self documenting, you can stick it right before you use it.

https://codereview.chromium.org/16154005/diff/1/chrome/browser/nacl_host/nacl...
File chrome/browser/nacl_host/nacl_process_host.h (right):

https://codereview.chromium.org/16154005/diff/1/chrome/browser/nacl_host/nacl...
chrome/browser/nacl_host/nacl_process_host.h:118: bool
GetNaclAppPath(base::FilePath& exe_path);
style nit: We use pointers for out-params in Chromium
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu...

(you get used to it...  it does help with knowing at a call-site that an
argument can change).

[Mark Seaborn, 2013-05-31 19:42:29]

https://codereview.chromium.org/16154005/diff/1/chrome/browser/nacl_host/nacl...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/16154005/diff/1/chrome/browser/nacl_host/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:1075: {
Style is that '{' goes on previous line

https://codereview.chromium.org/16154005/diff/1/chrome/browser/nacl_host/nacl...
File chrome/browser/nacl_host/nacl_process_host.h (right):

https://codereview.chromium.org/16154005/diff/1/chrome/browser/nacl_host/nacl...
chrome/browser/nacl_host/nacl_process_host.h:116: // Get path to NaCl loader on
local disk if possible.
Nit: "disk" -> "filesystem", since a lot of systems don't have disks these days
:-)

https://codereview.chromium.org/16154005/diff/1/chrome/browser/nacl_host/nacl...
chrome/browser/nacl_host/nacl_process_host.h:118: bool
GetNaclAppPath(base::FilePath& exe_path);
This should be spelled as "NaCl", not "Nacl".  Can you make this
"GetNaCl64ExePath"?  "App" is vague, and this really refers specifically to
nacl64.exe.

[Lei Zhang, 2013-05-31 19:45:48]

https://codereview.chromium.org/16154005/diff/1/chrome/browser/nacl_host/nacl...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/16154005/diff/1/chrome/browser/nacl_host/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:1076: static const wchar_t
kNaClAppName[] = L"nacl64";
Is this Windows-only code? If not, then it won't work with FilePath::Append()
below.

[Mark Seaborn, 2013-05-31 19:51:13]

https://codereview.chromium.org/16154005/diff/1/chrome/browser/nacl_host/nacl...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/16154005/diff/1/chrome/browser/nacl_host/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:1076: static const wchar_t
kNaClAppName[] = L"nacl64";
On 2013/05/31 19:45:48, Lei Zhang wrote:
> Is this Windows-only code? If not, then it won't work with FilePath::Append()
> below.

Yes, this is Windows-only.  It's inside "#if defined(OS_WIN)".

https://codereview.chromium.org/16154005/diff/1/chrome/browser/nacl_host/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:1085: 
Nit: make the whitespace after "#if" and before "#endif" consistent.

https://codereview.chromium.org/16154005/diff/1/chrome/browser/nacl_host/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:1085: 
Nit: make the whitespace after "#if" and before "#endif" consistent

[yael.aharon1, 2013-05-31 20:04:50]

On 2013/05/31 19:45:48, Lei Zhang wrote:
>
https://codereview.chromium.org/16154005/diff/1/chrome/browser/nacl_host/nacl...
> File chrome/browser/nacl_host/nacl_process_host.cc (right):
> 
>
https://codereview.chromium.org/16154005/diff/1/chrome/browser/nacl_host/nacl...
> chrome/browser/nacl_host/nacl_process_host.cc:1076: static const wchar_t
> kNaClAppName[] = L"nacl64";
> Is this Windows-only code? If not, then it won't work with FilePath::Append()
> below.
Yes, this is windows only code, and it is guarded by #if defined(OS_WIN)

[yael.aharon1, 2013-05-31 20:11:14]

Thank you for all your comments and suggestions. I addressed all of them in the
second version of the patch.

[dmichael (off chromium), 2013-05-31 22:06:32]

lgtm

[Lei Zhang, 2013-05-31 22:12:38]

lgtm++

[commit-bot: I haz the power, 2013-05-31 22:51:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yael.aharon@intel.com/16154005/1006

[yael.aharon1, 2013-06-20 18:56:36]

This patch did not compile for Windows, since I made a wrong assumption about a
relationship between NaClProcessHost and NaClBrokerHost.
To fix the build I needed to move the new method to NaClBrowser.
Are you still ok with this change?

[Lei Zhang, 2013-06-20 19:26:25]

On 2013/06/20 18:56:36, yael.aharon1 wrote:
> This patch did not compile for Windows, since I made a wrong assumption about
a
> relationship between NaClProcessHost and NaClBrokerHost.
> To fix the build I needed to move the new method to NaClBrowser.
> Are you still ok with this change?

I defer the answer to someone with more NaCl expertise.

[yael.aharon1, 2013-06-20 19:30:38]

On 2013/06/20 19:26:25, Lei Zhang wrote:
> On 2013/06/20 18:56:36, yael.aharon1 wrote:
> > This patch did not compile for Windows, since I made a wrong assumption
about
> a
> > relationship between NaClProcessHost and NaClBrokerHost.
> > To fix the build I needed to move the new method to NaClBrowser.
> > Are you still ok with this change?
> 
> I defer the answer to someone with more NaCl expertise.

Mark Seaborn, since dmichael is gardening today, can you please review the last
change I made?

[Mark Seaborn, 2013-06-20 19:53:49]

LGTM

[commit-bot: I haz the power, 2013-06-20 19:58:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yael.aharon@intel.com/16154005/34001

[commit-bot: I haz the power, 2013-06-21 13:16:36]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-06-21 13:44:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yael.aharon@intel.com/16154005/34001

[commit-bot: I haz the power, 2013-06-21 14:20:41]

Change committed as 207806