Don't use PathService for the location of nacl_helper and nacl_helper_bootstrap

Separate FILE_NACL_HELPER and FILE_NACL_HELPER_BOOTSTRAP to their own file.
    
This is in preparation for moving NaCl code out of chrome layer and into components.
BUG=244791
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208053

[yael.aharon1, 2013-06-21 03:00:59]

This CL was split off from https://codereview.chromium.org/16881004/ .
Can you please review?

[Mark Seaborn, 2013-06-21 13:53:26]

https://codereview.chromium.org/17198013/diff/9/chrome/app/chrome_main_delega...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/17198013/diff/9/chrome/app/chrome_main_delega...
chrome/app/chrome_main_delegate.cc:481: #if defined(OS_POSIX) &&
!defined(OS_MACOSX)
This can be
#if !defined(DISABLE_NACL) && defined(OS_LINUX)

nacl_helper_bootstrap is very Linux-specific.  It's not likely to work on
another Unix without a lot of modification.

https://codereview.chromium.org/17198013/diff/9/chrome/app/nacl_fork_delegate...
File chrome/app/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/17198013/diff/9/chrome/app/nacl_fork_delegate...
chrome/app/nacl_fork_delegate_linux.cc:54: if
(!PathService::Get(nacl::FILE_NACL_HELPER, &helper_exe)) {
FWIW, I don't understand why FILE_NACL_HELPER and FILE_NACL_HELPER_BOOTSTRAP
exist in PathService.  What is this level of indirection needed for?

Why don't we change this code here to do the following?

  if (!PathService::Get(base::DIR_MODULE, &dir)) ...
  base::FilePath path = dir.Append("nacl_helper");

Then we can eliminate all this complexity.

The pathname of nacl64.exe (which you refactored earlier) isn't in PathService.

https://codereview.chromium.org/17198013/diff/9/chrome/common/nacl_paths.h
File chrome/common/nacl_paths.h (right):

https://codereview.chromium.org/17198013/diff/9/chrome/common/nacl_paths.h#ne...
chrome/common/nacl_paths.h:16: PATH_START = 9000,
Where does 9000 come from?  What stops this from clashing?

https://codereview.chromium.org/17198013/diff/9/chrome/common/nacl_paths.h#ne...
chrome/common/nacl_paths.h:18: #if defined(OS_POSIX) && !defined(OS_MACOSX)
Better to use OS_LINUX here too

[yael.aharon1, 2013-06-21 15:13:03]

On 2013/06/21 13:53:26, Mark Seaborn wrote:
>
https://codereview.chromium.org/17198013/diff/9/chrome/app/chrome_main_delega...
> File chrome/app/chrome_main_delegate.cc (right):
> 
>
https://codereview.chromium.org/17198013/diff/9/chrome/app/chrome_main_delega...
> chrome/app/chrome_main_delegate.cc:481: #if defined(OS_POSIX) &&
> !defined(OS_MACOSX)
> This can be
> #if !defined(DISABLE_NACL) && defined(OS_LINUX)
> 
> nacl_helper_bootstrap is very Linux-specific.  It's not likely to work on
> another Unix without a lot of modification.
> 
>
https://codereview.chromium.org/17198013/diff/9/chrome/app/nacl_fork_delegate...
> File chrome/app/nacl_fork_delegate_linux.cc (right):
> 
>
https://codereview.chromium.org/17198013/diff/9/chrome/app/nacl_fork_delegate...
> chrome/app/nacl_fork_delegate_linux.cc:54: if
> (!PathService::Get(nacl::FILE_NACL_HELPER, &helper_exe)) {
> FWIW, I don't understand why FILE_NACL_HELPER and FILE_NACL_HELPER_BOOTSTRAP
> exist in PathService.  What is this level of indirection needed for?
> 
> Why don't we change this code here to do the following?
> 
>   if (!PathService::Get(base::DIR_MODULE, &dir)) ...
>   base::FilePath path = dir.Append("nacl_helper");
> 
> Then we can eliminate all this complexity.
> 
> The pathname of nacl64.exe (which you refactored earlier) isn't in
PathService.
> 
> https://codereview.chromium.org/17198013/diff/9/chrome/common/nacl_paths.h
> File chrome/common/nacl_paths.h (right):
> 
>
https://codereview.chromium.org/17198013/diff/9/chrome/common/nacl_paths.h#ne...
> chrome/common/nacl_paths.h:16: PATH_START = 9000,
> Where does 9000 come from?  What stops this from clashing?
> 
>
https://codereview.chromium.org/17198013/diff/9/chrome/common/nacl_paths.h#ne...
> chrome/common/nacl_paths.h:18: #if defined(OS_POSIX) && !defined(OS_MACOSX)
> Better to use OS_LINUX here too

I am not sure how to guarantee that PATH_START will not clash with a future
PathService handler, so I sent this question to chromium-dev.
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zKPmAh--Na0

I like your suggestion of not using PathService at all. I updated the patch to
not use it.

[Mark Seaborn, 2013-06-21 15:28:56]

Please update the commit message to reflect the change.  Then LGTM, thanks.

[yael.aharon, 2013-06-21 15:33:07]

On 2013/06/21 15:28:56, Mark Seaborn wrote:
> Please update the commit message to reflect the change.  Then LGTM, thanks.

Thanks! Updated the commit message.
jam@, cpu@ can you please do OWNER review?

[Mark Seaborn, 2013-06-21 15:45:53]

On 21 June 2013 08:33, <yael.aharon@intel.com> wrote:

> Reviewers: Mark Seaborn, jam, cpu, yael.aharon1,
>
> Message:
> On 2013/06/21 15:28:56, Mark Seaborn wrote:
>
>> Please update the commit message to reflect the change.  Then LGTM,
>> thanks.
>>
>
> Thanks! Updated the commit message.
>

FYI, you don't need to change the "Subject" field in Rietveld:  changing
that only has the effect of fragmenting the e-mail thread.  You only need
to change "Description":  that is what is used as the commit message.

Right now, the Description omits the subject line "Don't use PathService
for the location of nacl_helper and nacl_helper_bootstrap".  I think you
want to add that line into the Description.

You might also want to say why you're removing this use of PathService --
i.e. to remove an apparently unnecessary indirection, and to avoid the enum
ID allocation problem.

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/groups/opt_out.

[Mark Seaborn, 2013-06-21 15:45:54]

On 21 June 2013 08:33, <yael.aharon@intel.com> wrote:

> Reviewers: Mark Seaborn, jam, cpu, yael.aharon1,
>
> Message:
> On 2013/06/21 15:28:56, Mark Seaborn wrote:
>
>> Please update the commit message to reflect the change.  Then LGTM,
>> thanks.
>>
>
> Thanks! Updated the commit message.
>

FYI, you don't need to change the "Subject" field in Rietveld:  changing
that only has the effect of fragmenting the e-mail thread.  You only need
to change "Description":  that is what is used as the commit message.

Right now, the Description omits the subject line "Don't use PathService
for the location of nacl_helper and nacl_helper_bootstrap".  I think you
want to add that line into the Description.

You might also want to say why you're removing this use of PathService --
i.e. to remove an apparently unnecessary indirection, and to avoid the enum
ID allocation problem.

Cheers,
Mark

[jam, 2013-06-21 16:14:33]

Mark: I think the paths should all come from a path provider, that makes it easy
to look for these objects and see all the binaries that we lookup. if we don't
use it, like this patch, this task becomes very difficult.

the pathservice enum issue is something that existed before this change, so no
need to worry about it here.

[yael.aharon1, 2013-06-21 19:55:58]

I brought the first patch back, and simplified the #ifdef as Mark suggested.
I hope we can have an agreement on this :)

[jam, 2013-06-21 20:47:57]

lgtm

[commit-bot: I haz the power, 2013-06-21 20:53:24]

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

[commit-bot: I haz the power, 2013-06-22 02:36:32]

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

[commit-bot: I haz the power, 2013-06-22 15:05:43]

Change committed as 208053