Move chrome/nacl to components/nacl.

Move chrome/nacl to components/nacl.

This is part of an effort to componentize NaCl code.
Files that belong in the broker process (Windows only) are placed in components/nacl/broker.
Files that belong in the loader process are placed in components/nacl/loader.
Files that belong in the zygote process are placed in components/nacl/zygote.
Some files are shared between the broker, loader and the browser processes. These files are placed in components/nacl/common.

BUG=244791

[yael.aharon1, 2013-06-14 03:29:48]

This patch moves chrome/nacl files to components/nacl/nacl. There are 2 levels
called "nacl" because later on, chrome/browser/nacl_host files will be moved to
components/nacl/nacl_host etc.
Can you please look at the new location of the files and let me know what you
think of that?

[jam, 2013-06-14 19:37:40]

On 2013/06/14 03:29:48, yael.aharon1 wrote:
> This patch moves chrome/nacl files to components/nacl/nacl. There are 2 levels
> called "nacl" because later on, chrome/browser/nacl_host files will be moved
to
> components/nacl/nacl_host etc.
> Can you please look at the new location of the files and let me know what you
> think of that?

You need to get an owner from src/components, I suggest joi.
You also need an owner from current nacl code.

I think components/nacl/nacl is something to be avoided. You should come up with
a more descriptive name for the second directory.

[Jói, 2013-06-14 19:43:44]

I reviewed components/nacl.gypi and components/nacl/OWNERS which both LGTM.

For the review of the rest (under components/nacl/), I'd prefer to defer to one
of the folks in components/nacl/OWNERS.

Cheers,
Jói

[yael.aharon1, 2013-06-14 19:50:32]

Is there a naming convention in the project for something like this? How about
components/nacl/nacl_process?

[Jói, 2013-06-14 19:56:14]

On 2013/06/14 19:50:32, yael.aharon1 wrote:
> Is there a naming convention in the project for something like this? How about
> components/nacl/nacl_process?

Do you mean the code currently under //components/nacl/nacl (in this patch) is
code that runs in a NaCl-specific process?  The normal naming convention would
be to leave it as it currently is in the patch - subdirectories per process are
usually just named for the process (e.g. //components/autofill/browser for
browser-process-specific code).

[Mark Seaborn, 2013-06-14 20:02:20]

On 14 June 2013 12:50, <yael.aharon@chromium.org> wrote:

> Is there a naming convention in the project for something like this? How
> about
> components/nacl/nacl_process?
>

There are two types of NaCl process:

 * The "NaCl loader" process (kNaClLoaderProcess[] = "nacl-loader"), which
is sandboxed and hosts untrusted code.
 * The "NaCl broker" process (kNaClBrokerProcess[] = "nacl-broker"), which
is only used on 64-bit Windows when chrome.exe is a 32-bit executable.
This process is not sandboxed.

So a better directory name than "nacl/nacl/" would be "nacl/loader/".
("nacl/nacl_loader" seems too redundant.)

However, "nacl_broker_listener.{cc,h}" doesn't belong in "nacl/loader/", so
you could create "nacl/broker/" for that.

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-14 20:02:21]

On 14 June 2013 12:50, <yael.aharon@chromium.org> wrote:

> Is there a naming convention in the project for something like this? How
> about
> components/nacl/nacl_process?
>

There are two types of NaCl process:

 * The "NaCl loader" process (kNaClLoaderProcess[] = "nacl-loader"), which
is sandboxed and hosts untrusted code.
 * The "NaCl broker" process (kNaClBrokerProcess[] = "nacl-broker"), which
is only used on 64-bit Windows when chrome.exe is a 32-bit executable.
This process is not sandboxed.

So a better directory name than "nacl/nacl/" would be "nacl/loader/".
("nacl/nacl_loader" seems too redundant.)

However, "nacl_broker_listener.{cc,h}" doesn't belong in "nacl/loader/", so
you could create "nacl/broker/" for that.

Mark

[yael.aharon1, 2013-06-14 20:53:50]

On 2013/06/14 20:02:21, Mark Seaborn wrote:
> On 14 June 2013 12:50, <mailto:yael.aharon@chromium.org> wrote:
> 
> > Is there a naming convention in the project for something like this? How
> > about
> > components/nacl/nacl_process?
> >
> 
> There are two types of NaCl process:
> 
>  * The "NaCl loader" process (kNaClLoaderProcess[] = "nacl-loader"), which
> is sandboxed and hosts untrusted code.
>  * The "NaCl broker" process (kNaClBrokerProcess[] = "nacl-broker"), which
> is only used on 64-bit Windows when chrome.exe is a 32-bit executable.
> This process is not sandboxed.
> 
> So a better directory name than "nacl/nacl/" would be "nacl/loader/".
> ("nacl/nacl_loader" seems too redundant.)
> 
> However, "nacl_broker_listener.{cc,h}" doesn't belong in "nacl/loader/", so
> you could create "nacl/broker/" for that.
> 
> Mark

I like that :)
I split components/nacl /nacl to components/nacl/loader and
components/nacl/broker .

[jam, 2013-06-14 23:18:52]

https://codereview.chromium.org/16881004/diff/41001/components/nacl/common/na...
File components/nacl/common/nacl_delegate.h (right):

https://codereview.chromium.org/16881004/diff/41001/components/nacl/common/na...
components/nacl/common/nacl_delegate.h:35: };
this is very hacky, please find a clean way of doing this

for example, components/nacl will depend on content. so it can grab the content
flags directly. then, nacl specific flags like kNaClBrokerProcess etc can move
to src/components/nacl. for whatever remains after that, the components code
shouldn't have an enum of the flags in chrome. instead, it should just tell it
"hey, i'm about to launch a process, add any flags that you want". see
ContentBrowserClient::AppendExtraCommandLineSwitches as an example

[yael.aharon1, 2013-06-15 17:49:15]

On 2013/06/14 23:18:52, jam wrote:
>
https://codereview.chromium.org/16881004/diff/41001/components/nacl/common/na...
> File components/nacl/common/nacl_delegate.h (right):
> 
>
https://codereview.chromium.org/16881004/diff/41001/components/nacl/common/na...
> components/nacl/common/nacl_delegate.h:35: };
> this is very hacky, please find a clean way of doing this
> 
> for example, components/nacl will depend on content. so it can grab the
content
> flags directly. then, nacl specific flags like kNaClBrokerProcess etc can move
> to src/components/nacl. for whatever remains after that, the components code
> shouldn't have an enum of the flags in chrome. instead, it should just tell it
> "hey, i'm about to launch a process, add any flags that you want". see
> ContentBrowserClient::AppendExtraCommandLineSwitches as an example

Done.

[yael.aharon1, 2013-06-17 15:18:19]

Mark Seaborn, as am OWNER for many of these files, can you please review this
patch?

cpu@, Can you please review the files under chrome/app ?

jam@, if you have more comments or concerns, please let me know. And thanks for
your comments so far!

[yael.aharon1, 2013-06-17 17:08:38]

In order to fix the Windows build, I need to take ProcessNeedsProfileDir() out
of the common_constants project (chrome_paths_win.cc). Originally, this was
called from chrome_paths.cc, but that is not the case anymore. 

cpu@, jam@, you were the reviewers of the CLs that added this dependency in
https://chromiumcodereview.appspot.com/10390003 and
https://chromiumcodereview.appspot.com/10306009/ .
Do you have objection to taking this method out of chrome_paths_win.cc ?

[jam, 2013-06-17 20:10:42]

On 2013/06/17 17:08:38, yael.aharon1 wrote:
> In order to fix the Windows build, I need to take ProcessNeedsProfileDir() out
> of the common_constants project (chrome_paths_win.cc). Originally, this was
> called from chrome_paths.cc, but that is not the case anymore. 

I can't tell by inspection why this link error is now affecting
crash_service.exe and delegate_execute.exe?

https://codereview.chromium.org/16881004/diff/102001/chrome/app/chrome_main_d...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/16881004/diff/102001/chrome/app/chrome_main_d...
chrome/app/chrome_main_delegate.cc:66: #ifndef (DISABLE_NACL)
nit: here and below, "#if !defined(DISABLE_NACL)". that's preferred over ifndef

https://codereview.chromium.org/16881004/diff/102001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/16881004/diff/102001/chrome/chrome_common.gyp...
chrome/chrome_common.gypi:440: '../components/nacl/common/nacl_delegate.h',
you shouldn't be listing the files from components here. they should have their
own target and you should depend on it here

https://codereview.chromium.org/16881004/diff/102001/chrome/chrome_renderer.gypi
File chrome/chrome_renderer.gypi (right):

https://codereview.chromium.org/16881004/diff/102001/chrome/chrome_renderer.g...
chrome/chrome_renderer.gypi:335: 'common/nacl_delegate.cc',
this linker error is a symptom of the way you did the delegate, which is
incorrect

https://codereview.chromium.org/16881004/diff/102001/chrome/common/nacl_deleg...
File chrome/common/nacl_delegate.cc (right):

https://codereview.chromium.org/16881004/diff/102001/chrome/common/nacl_deleg...
chrome/common/nacl_delegate.cc:31: switches::kMemoryProfiling,
these two switches are the only ones that are in chrome layer, i.e. the only
ones that the chrome nacl delegate should add. the rest should be done in
components/nacl. that's what i tried to say in my last comment

https://codereview.chromium.org/16881004/diff/102001/components/nacl/broker/n...
File components/nacl/broker/nacl_exe_win_64.cc (right):

https://codereview.chromium.org/16881004/diff/102001/components/nacl/broker/n...
components/nacl/broker/nacl_exe_win_64.cc:25: extern void InitCrashReporter();
again, you can not forward declare code from chrome in nacl.

https://codereview.chromium.org/16881004/diff/102001/components/nacl/common/n...
File components/nacl/common/nacl_delegate.h (right):

https://codereview.chromium.org/16881004/diff/102001/components/nacl/common/n...
components/nacl/common/nacl_delegate.h:27: extern NaClDelegate*
GetNaClDelegate();
please see other delegates for other modules for examples on how we structure
this. but the current approach isn't something that we use (i.e. declaring a
method in one module, and expecting a module higher up to implement it).

can you plumb the delegate pointer to the code that eventually needs it?

another simpler option would be to forget about passing
switches::kSilentDumpOnDCHECK,
switches::kMemoryProfiling,

to the nacl broker. these are windows only, and i'm not sure if any of the nacl
devs care about them now. i..e i'm pretty sure the second one is not used by
anyone. as for the second one, it's just a debugging utility so we don't really
need it. i don't even see any DCHECKs in the nacl broker code. if we need them
in the future, we can figure out how to do it then.

https://codereview.chromium.org/16881004/diff/102001/components/nacl/common/n...
File components/nacl/common/nacl_switches.h (right):

https://codereview.chromium.org/16881004/diff/102001/components/nacl/common/n...
components/nacl/common/nacl_switches.h:22: // alphabetical order, or in one of
the ifdefs (also in order in each section).
this comment probably doesn't need to be copied. it makes more common in
chrome_switches.h which has a lot of switches, but not here

[yael.aharon1, 2013-06-17 21:05:01]

On 2013/06/17 20:10:42, jam wrote:
> On 2013/06/17 17:08:38, yael.aharon1 wrote:
> > In order to fix the Windows build, I need to take ProcessNeedsProfileDir()
out
> > of the common_constants project (chrome_paths_win.cc). Originally, this was
> > called from chrome_paths.cc, but that is not the case anymore. 
> 
> I can't tell by inspection why this link error is now affecting
> crash_service.exe and delegate_execute.exe?

Thank you for your review. I will address the rest of your comments shortly, but
wanted to explain the link error first.
chrome_paths_win.cc is using the switches kNaClBrokerProcess and
kNaClLoaderProcess . It is also included in common_constants.gyp. So now every
target that has 'common_constants' as a dependency, also has a dependency on
nacl_switches.cc, hence the link failures.
Instead of adding nacl_switches.cc to all these targets, I thought I could
separate ProcessNeedsProfileDir() into another file, so it is not included in
'common_constants' anymore. This method is only called from
ChromeMainDelegate::PreSandboxStartup() .
Does that make sense?

[jam, 2013-06-17 21:39:30]

On 2013/06/17 21:05:01, yael.aharon1 wrote:
> On 2013/06/17 20:10:42, jam wrote:
> > On 2013/06/17 17:08:38, yael.aharon1 wrote:
> > > In order to fix the Windows build, I need to take ProcessNeedsProfileDir()
> out
> > > of the common_constants project (chrome_paths_win.cc). Originally, this
was
> > > called from chrome_paths.cc, but that is not the case anymore. 
> > 
> > I can't tell by inspection why this link error is now affecting
> > crash_service.exe and delegate_execute.exe?
> 
> Thank you for your review. I will address the rest of your comments shortly,
but
> wanted to explain the link error first.
> chrome_paths_win.cc is using the switches kNaClBrokerProcess and
> kNaClLoaderProcess . It is also included in common_constants.gyp. So now every
> target that has 'common_constants' as a dependency, also has a dependency on
> nacl_switches.cc, hence the link failures.
> Instead of adding nacl_switches.cc to all these targets, I thought I could
> separate ProcessNeedsProfileDir() into another file, so it is not included in
> 'common_constants' anymore. This method is only called from
> ChromeMainDelegate::PreSandboxStartup() .
> Does that make sense?

why wouldn't common_constants just depend on the target which has the nacl
switches?

[yael.aharon1, 2013-06-18 17:16:29]

https://codereview.chromium.org/16881004/diff/102001/components/nacl/broker/n...
File components/nacl/broker/nacl_exe_win_64.cc (right):

https://codereview.chromium.org/16881004/diff/102001/components/nacl/broker/n...
components/nacl/broker/nacl_exe_win_64.cc:25: extern void InitCrashReporter();
On 2013/06/17 20:10:43, jam wrote:
> again, you can not forward declare code from chrome in nacl.

jochen is in the process of moving this code to components/breakpad. Is it ok to
keep it like this until he is done moving the code? This is called from wWinMain
so it will be very tricky to find a way to plumb a delegate to here.

https://codereview.chromium.org/16881004/diff/102001/components/nacl/common/n...
File components/nacl/common/nacl_delegate.h (right):

https://codereview.chromium.org/16881004/diff/102001/components/nacl/common/n...
components/nacl/common/nacl_delegate.h:27: extern NaClDelegate*
GetNaClDelegate();
On 2013/06/17 20:10:43, jam wrote:
> please see other delegates for other modules for examples on how we structure
> this. but the current approach isn't something that we use (i.e. declaring a
> method in one module, and expecting a module higher up to implement it).
> 
> can you plumb the delegate pointer to the code that eventually needs it?
> 
My problem here is that the only code from chrome that is used in the broker
process is breakpad_win, crash_keys and hard_error_handler_win. This makes it
very tricky for me to find a place from which I can plumb the delegate pointer. 
> another simpler option would be to forget about passing
> switches::kSilentDumpOnDCHECK,
> switches::kMemoryProfiling,
> 
> to the nacl broker. these are windows only, and i'm not sure if any of the
nacl
> devs care about them now. i..e i'm pretty sure the second one is not used by
> anyone. as for the second one, it's just a debugging utility so we don't
really
> need it. i don't even see any DCHECKs in the nacl broker code. if we need them
> in the future, we can figure out how to do it then.
Is it ok to just remove these 2 switches? If I can do that, I will not need a
delegate at all.

[yael.aharon1, 2013-06-18 19:08:20]

I don't understand why the trybots are updating to revision 206927 which is from
10 hours ago and 100 revisions behind :(

[jam, 2013-06-19 17:55:53]

On 2013/06/18 19:08:20, yael.aharon1 wrote:
> I don't understand why the trybots are updating to revision 206927 which is
from
> 10 hours ago and 100 revisions behind :(

you can go "gcl try foo -r ######" to specify a specific revision

https://codereview.chromium.org/16881004/diff/102001/components/nacl/broker/n...
File components/nacl/broker/nacl_exe_win_64.cc (right):

https://codereview.chromium.org/16881004/diff/102001/components/nacl/broker/n...
components/nacl/broker/nacl_exe_win_64.cc:25: extern void InitCrashReporter();
On 2013/06/18 17:16:30, yael.aharon1 wrote:
> On 2013/06/17 20:10:43, jam wrote:
> > again, you can not forward declare code from chrome in nacl.
> 
> jochen is in the process of moving this code to components/breakpad. Is it ok
to
> keep it like this until he is done moving the code? This is called from
wWinMain
> so it will be very tricky to find a way to plumb a delegate to here.

you should wait for that to happen first then. it's not acceptable to declare
methods in a low level module and define them higher up.

https://codereview.chromium.org/16881004/diff/102001/components/nacl/common/n...
File components/nacl/common/nacl_delegate.h (right):

https://codereview.chromium.org/16881004/diff/102001/components/nacl/common/n...
components/nacl/common/nacl_delegate.h:27: extern NaClDelegate*
GetNaClDelegate();
On 2013/06/18 17:16:30, yael.aharon1 wrote:
> On 2013/06/17 20:10:43, jam wrote:
> > please see other delegates for other modules for examples on how we
structure
> > this. but the current approach isn't something that we use (i.e. declaring a
> > method in one module, and expecting a module higher up to implement it).
> > 
> > can you plumb the delegate pointer to the code that eventually needs it?
> > 
> My problem here is that the only code from chrome that is used in the broker
> process is breakpad_win, crash_keys and hard_error_handler_win. This makes it
> very tricky for me to find a place from which I can plumb the delegate
pointer. 
> > another simpler option would be to forget about passing
> > switches::kSilentDumpOnDCHECK,
> > switches::kMemoryProfiling,
> > 
> > to the nacl broker. these are windows only, and i'm not sure if any of the
> nacl
> > devs care about them now. i..e i'm pretty sure the second one is not used by
> > anyone. as for the second one, it's just a debugging utility so we don't
> really
> > need it. i don't even see any DCHECKs in the nacl broker code. if we need
them
> > in the future, we can figure out how to do it then.
> Is it ok to just remove these 2 switches? If I can do that, I will not need a
> delegate at all.

yes, i'm totally fine with not passing these two switches. one is for sure not
used by anyone afaik (memory profiling), while the other is not that important

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

message: On 2013/06/19 17:55:53, jam wrote:
> On 2013/06/18 19:08:20, yael.aharon1 wrote:
> you can go "gcl try foo -r ######" to specify a specific revision
Thank you for the tip :)

>
https://codereview.chromium.org/16881004/diff/102001/components/nacl/broker/n...
> File components/nacl/broker/nacl_exe_win_64.cc (right):
> 
>
https://codereview.chromium.org/16881004/diff/102001/components/nacl/broker/n...
> components/nacl/broker/nacl_exe_win_64.cc:25: extern void InitCrashReporter();
> On 2013/06/18 17:16:30, yael.aharon1 wrote:
> > On 2013/06/17 20:10:43, jam wrote:
> > > again, you can not forward declare code from chrome in nacl.
> 
> you should wait for that to happen first then. it's not acceptable to declare
> methods in a low level module and define them higher up.
Another option is to move chrome/nacl/nacl_exe_win_64.cc to chrome/app until he
is done moving breakpad, or just keep it as the only file in chrome/nacl. Would
that be ok?

> yes, i'm totally fine with not passing these two switches. one is for sure not
> used by anyone afaik (memory profiling), while the other is not that important
Thanks!

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

https://codereview.chromium.org/16881004/diff/102001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/16881004/diff/102001/chrome/chrome_common.gyp...
chrome/chrome_common.gypi:440: '../components/nacl/common/nacl_delegate.h',
On 2013/06/17 20:10:43, jam wrote:
> you shouldn't be listing the files from components here. they should have
their
> own target and you should depend on it here

Done.

https://codereview.chromium.org/16881004/diff/102001/chrome/chrome_renderer.gypi
File chrome/chrome_renderer.gypi (right):

https://codereview.chromium.org/16881004/diff/102001/chrome/chrome_renderer.g...
chrome/chrome_renderer.gypi:335: 'common/nacl_delegate.cc',
On 2013/06/17 20:10:43, jam wrote:
> this linker error is a symptom of the way you did the delegate, which is
> incorrect
In the patch I just uploaded, there is no delegate anymore.

https://codereview.chromium.org/16881004/diff/102001/chrome/common/nacl_deleg...
File chrome/common/nacl_delegate.cc (right):

https://codereview.chromium.org/16881004/diff/102001/chrome/common/nacl_deleg...
chrome/common/nacl_delegate.cc:31: switches::kMemoryProfiling,
On 2013/06/17 20:10:43, jam wrote:
> these two switches are the only ones that are in chrome layer, i.e. the only
> ones that the chrome nacl delegate should add. the rest should be done in
> components/nacl. that's what i tried to say in my last comment
I removed the 2 switches.

https://codereview.chromium.org/16881004/diff/102001/components/nacl/broker/n...
File components/nacl/broker/nacl_exe_win_64.cc (right):

https://codereview.chromium.org/16881004/diff/102001/components/nacl/broker/n...
components/nacl/broker/nacl_exe_win_64.cc:25: extern void InitCrashReporter();
On 2013/06/17 20:10:43, jam wrote:
> again, you can not forward declare code from chrome in nacl.
Since I could not find an immediate solution, I moved nacl_exe_win_64.cc back to
its original location.

[cpu_(ooo_6.6-7.5), 2013-06-20 03:09:44]

apps folder lgtm

https://codereview.chromium.org/16881004/diff/100002/chrome/app/chrome_main_d...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/16881004/diff/100002/chrome/app/chrome_main_d...
chrome/app/chrome_main_delegate.cc:543: process_type == switches::kGpuProcess ||
indenting seems off

[yael.aharon1, 2013-06-20 14:05:48]

jam@, I think I addressed all your comments so far. Can you please review the
latest version of this CL?

[jam, 2013-06-20 23:20:33]

can you get a nacl owner to look at this first? then when they're done i'll give
it another pass.

[yael.aharon1, 2013-06-20 23:51:01]

On 2013/06/20 23:20:33, jam wrote:
> can you get a nacl owner to look at this first? then when they're done i'll
give
> it another pass.

Thanks!
Mark Seaborn, could you please review this?

[Mark Seaborn, 2013-06-21 00:00:46]

https://codereview.chromium.org/16881004/diff/170001/components/nacl/common/n...
File components/nacl/common/nacl_cmd_line.cc (left):

https://codereview.chromium.org/16881004/diff/170001/components/nacl/common/n...
components/nacl/common/nacl_cmd_line.cc:26: switches::kSilentDumpOnDCHECK,
Why are these two options being removed?  Do this as a separate change if you
want to remove them.

https://codereview.chromium.org/16881004/diff/170001/components/nacl/common/n...
File components/nacl/common/nacl_paths.h (right):

https://codereview.chromium.org/16881004/diff/170001/components/nacl/common/n...
components/nacl/common/nacl_paths.h:5: #ifndef
COMPONENTS_NACL_COMMON_NACL_PATHS_H__
Nit: Should use "_" not "__" (wunderbars are reserved)

https://codereview.chromium.org/16881004/diff/170001/components/nacl/common/n...
components/nacl/common/nacl_paths.h:16: PATH_START = 9000,
I don't know where this magic number comes from.  Can you do the addition of
this file (and nacl_paths.cc) as a separate change so that it can be reviewed on
its own?  It's not really manageable for me to review changes like this in the
middle of such a huge changeset.

https://codereview.chromium.org/16881004/diff/170001/components/nacl/common/n...
File components/nacl/common/nacl_switches.h (right):

https://codereview.chromium.org/16881004/diff/170001/components/nacl/common/n...
components/nacl/common/nacl_switches.h:14: extern const char
kNaClBrokerProcess[];
Same here: the addition of this file should be a separate change and not appear
as new in this change.

https://codereview.chromium.org/16881004/diff/170001/components/nacl/loader/n...
File components/nacl/loader/nacl_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/16881004/diff/170001/components/nacl/loader/n...
components/nacl/loader/nacl_main_platform_delegate_win.cc:39: base::FilePath
test_dll_name = command_line.GetSwitchValuePath(
Please undo this unnecessary formatting change.

[yael.aharon1, 2013-06-21 03:09:33]

I split some parts of this patch out, would appreciate your review.

https://codereview.chromium.org/17041003/ - removing the 2 switches
https://codereview.chromium.org/17379019/ - split nacl_switches
https://codereview.chromium.org/17198013/ - split nacl_paths

[yael.aharon1, 2013-06-25 12:10:57]

I rebased this CL after breaking off 

https://codereview.chromium.org/17041003/
https://codereview.chromium.org/17379019/
https://codereview.chromium.org/17198013/

What is left in this patch is moving the files, changing include locations, and
adjustment in gyp and DEP files.
Mark, could you please continue the review?

[yael.aharon1, 2013-06-27 13:55:38]

ping?

[Jói, 2013-06-27 14:18:38]

I reviewed the top-level //components stuff, not the stuff under
//components/nacl.

Please get folks in //components/nacl/OWNERS to review the stuff within
//components/nacl.  I would also suggest updating //components/OWNERS with
per-file OWNERS for the NaCl-related files in that directory.

Cheers,
Jói

https://codereview.chromium.org/16881004/diff/170001/components/nacl.gypi
File components/nacl.gypi (right):

https://codereview.chromium.org/16881004/diff/170001/components/nacl.gypi#new...
components/nacl.gypi:1: # Copyright (c) 2012 The Chromium Authors. All rights
reserved.
Shouldn't there be an entry for the two .gypi files in
//components/components.gyp?

https://codereview.chromium.org/16881004/diff/195001/components/nacl_common.gyp
File components/nacl_common.gyp (right):

https://codereview.chromium.org/16881004/diff/195001/components/nacl_common.g...
components/nacl_common.gyp:1: # Copyright (c) 2013 The Chromium Authors. All
rights reserved.
Not sure we still use the (c) but I don't think it matters either.

[yael.aharon1, 2013-06-27 15:03:39]

On 2013/06/27 14:18:38, Jói wrote:
> I reviewed the top-level //components stuff, not the stuff under
> //components/nacl.

Thank you for your review. 

> Please get folks in //components/nacl/OWNERS to review the stuff within
> //components/nacl.  I would also suggest updating //components/OWNERS with
> per-file OWNERS for the NaCl-related files in that directory.

Done

> https://codereview.chromium.org/16881004/diff/170001/components/nacl.gypi
> File components/nacl.gypi (right):
> 
>
https://codereview.chromium.org/16881004/diff/170001/components/nacl.gypi#new...
> components/nacl.gypi:1: # Copyright (c) 2012 The Chromium Authors. All rights
> reserved.
> Shouldn't there be an entry for the two .gypi files in
> //components/components.gyp?
I did not add nacl.gypi to components.gyp because when it is included in
chrome.gyp, it also inherits the variables that are defined in chrome.gyp, and I
am not sure yet how to move 'nacl_defines' from chrome.gyp to components.gyp.
I did not add nacl_common.gyp to components.gyp because part of it is included
in common_constants.gyp, and I did not want to introduce components.gyp into
common_constants.gyp.
Does that make sense?

>
https://codereview.chromium.org/16881004/diff/195001/components/nacl_common.gyp
> File components/nacl_common.gyp (right):
> 
>
https://codereview.chromium.org/16881004/diff/195001/components/nacl_common.g...
> components/nacl_common.gyp:1: # Copyright (c) 2013 The Chromium Authors. All
> rights reserved.
> Not sure we still use the (c) but I don't think it matters either.

Removed this change.

Can you please suggest who can review this from the NaCl team? I was told to ask
Mark Seaborn, but he seems to be unavailable.

[Mark Seaborn, 2013-06-27 15:30:16]

On 2013/06/27 15:03:39, yael.aharon1 wrote:
> Can you please suggest who can review this from the NaCl team? I was
> told to ask Mark Seaborn, but he seems to be unavailable.

I'm available, I've just been putting this off because of the size of the
change.

Can you put a summary into the commit message of the rationale behind the new
locations for the files?  i.e. Explain nacl/loader, nacl/broker etc.

Before you commit this, can you notify the build sheriffs and be available to
fix any fallout?  I don't really want to see a big change like this reverted,
because that would make it harder to follow the history of the files.

Please run non-default trybots such as cros_daisy (the ARM build; not tested by
trybots otherwise), the non-"*_rel" "linux", "mac" and "win", and the
"*_naclmore" trybots?

[yael.aharon1, 2013-06-27 15:41:28]

On 2013/06/27 15:30:16, Mark Seaborn wrote:
> On 2013/06/27 15:03:39, yael.aharon1 wrote:
> > Can you please suggest who can review this from the NaCl team? I was
> > told to ask Mark Seaborn, but he seems to be unavailable.
> 
> I'm available, I've just been putting this off because of the size of the
> change.
> 
Sorry, I assumed vacation, since it is summer :)

> Can you put a summary into the commit message of the rationale behind the new
> locations for the files?  i.e. Explain nacl/loader, nacl/broker etc.
> 
> Before you commit this, can you notify the build sheriffs and be available to
> fix any fallout?  I don't really want to see a big change like this reverted,
> because that would make it harder to follow the history of the files.
Sure, I will make sure to commit it in the morning (EST) and hopefully this can
go through during the day.

> Please run non-default trybots such as cros_daisy (the ARM build; not tested
by
> trybots otherwise), the non-"*_rel" "linux", "mac" and "win", and the
> "*_naclmore" trybots?
I think you already started these bots? In the future I will make sure to test
on them.

[Mark Seaborn, 2013-06-27 15:45:52]

https://codereview.chromium.org/16881004/diff/207001/components/nacl/common/n...
File components/nacl/common/nacl_fork_delegate_linux.h (right):

https://codereview.chromium.org/16881004/diff/207001/components/nacl/common/n...
components/nacl/common/nacl_fork_delegate_linux.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
Why is nacl_fork_delegate_linux.h in a different directory from
nacl_fork_delegate_linux.cc?

https://codereview.chromium.org/16881004/diff/207001/components/nacl/loader/n...
File components/nacl/loader/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/16881004/diff/207001/components/nacl/loader/n...
components/nacl/loader/nacl_fork_delegate_linux.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
This file was in chrome/app before.  It doesn't belong in nacl/loader because
it's not run inside the NaCl loader process.  It runs inside Chromium's zygote
process.

[yael.aharon1, 2013-06-27 15:57:25]

On 2013/06/27 15:45:52, Mark Seaborn wrote:
>
https://codereview.chromium.org/16881004/diff/207001/components/nacl/common/n...
> File components/nacl/common/nacl_fork_delegate_linux.h (right):
> 
>
https://codereview.chromium.org/16881004/diff/207001/components/nacl/common/n...
> components/nacl/common/nacl_fork_delegate_linux.h:1: // Copyright (c) 2012 The
> Chromium Authors. All rights reserved.
> Why is nacl_fork_delegate_linux.h in a different directory from
> nacl_fork_delegate_linux.cc?
nacl_fork_delegate_linux.h is included from chrome_main_delegate.cc. I assumed
that files which are included from other directories have to be in a directory
called "common". If that is not true, I should move nacl_fork_delegate_linux.h .

>
https://codereview.chromium.org/16881004/diff/207001/components/nacl/loader/n...
> File components/nacl/loader/nacl_fork_delegate_linux.cc (right):
> 
>
https://codereview.chromium.org/16881004/diff/207001/components/nacl/loader/n...
> components/nacl/loader/nacl_fork_delegate_linux.cc:1: // Copyright (c) 2012
The
> Chromium Authors. All rights reserved.
> This file was in chrome/app before.  It doesn't belong in nacl/loader because
> it's not run inside the NaCl loader process.  It runs inside Chromium's zygote
> process.
Sorry about this. I will create a folder called "zygote" for this file.

[yael.aharon1, 2013-06-27 16:39:52]

I moved nacl_fork_delegate_linux.cc to a folder called zygote.
Please let me know if you think nacl_fork_delegate_linux.h should move there too
(see previous comment).
BTW, I think mac_xcodebuild bot is broken.

[Jói, 2013-06-27 19:50:15]

LGTM for //components/*

>
https://codereview.chromium.org/16881004/diff/170001/components/nacl.gypi#new...
>>
>> components/nacl.gypi:1: # Copyright (c) 2012 The Chromium Authors. All
>> rights
>> reserved.
>> Shouldn't there be an entry for the two .gypi files in
>> //components/components.gyp?
>
> I did not add nacl.gypi to components.gyp because when it is included in
> chrome.gyp, it also inherits the variables that are defined in chrome.gyp,
> and I
> am not sure yet how to move 'nacl_defines' from chrome.gyp to
> components.gyp.
> I did not add nacl_common.gyp to components.gyp because part of it is
> included
> in common_constants.gyp, and I did not want to introduce components.gyp into
> common_constants.gyp.
> Does that make sense?

Yep, makes sense to me.

Cheers,
Jói


On Thu, Jun 27, 2013 at 3:03 PM,  <yael.aharon@chromium.org> wrote:
> On 2013/06/27 14:18:38, Jói wrote:
>>
>> I reviewed the top-level //components stuff, not the stuff under
>> //components/nacl.
>
>
> Thank you for your review.
>
>> Please get folks in //components/nacl/OWNERS to review the stuff within
>> //components/nacl.  I would also suggest updating //components/OWNERS with
>> per-file OWNERS for the NaCl-related files in that directory.
>
>
> Done
>
>
>> https://codereview.chromium.org/16881004/diff/170001/components/nacl.gypi
>> File components/nacl.gypi (right):
>
>
>
>
https://codereview.chromium.org/16881004/diff/170001/components/nacl.gypi#new...
>>
>> components/nacl.gypi:1: # Copyright (c) 2012 The Chromium Authors. All
>> rights
>> reserved.
>> Shouldn't there be an entry for the two .gypi files in
>> //components/components.gyp?
>
> I did not add nacl.gypi to components.gyp because when it is included in
> chrome.gyp, it also inherits the variables that are defined in chrome.gyp,
> and I
> am not sure yet how to move 'nacl_defines' from chrome.gyp to
> components.gyp.
> I did not add nacl_common.gyp to components.gyp because part of it is
> included
> in common_constants.gyp, and I did not want to introduce components.gyp into
> common_constants.gyp.
> Does that make sense?
>
>
>
>
https://codereview.chromium.org/16881004/diff/195001/components/nacl_common.gyp
>>
>> File components/nacl_common.gyp (right):
>
>
>
>
https://codereview.chromium.org/16881004/diff/195001/components/nacl_common.g...
>>
>> components/nacl_common.gyp:1: # Copyright (c) 2013 The Chromium Authors.
>> All
>> rights reserved.
>> Not sure we still use the (c) but I don't think it matters either.
>
>
> Removed this change.
>
> Can you please suggest who can review this from the NaCl team? I was told to
> ask
> Mark Seaborn, but he seems to be unavailable.
>
>
>
> https://codereview.chromium.org/16881004/

-- 
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.

[Jói, 2013-06-27 19:50:17]

LGTM for //components/*

>
https://codereview.chromium.org/16881004/diff/170001/components/nacl.gypi#new...
>>
>> components/nacl.gypi:1: # Copyright (c) 2012 The Chromium Authors. All
>> rights
>> reserved.
>> Shouldn't there be an entry for the two .gypi files in
>> //components/components.gyp?
>
> I did not add nacl.gypi to components.gyp because when it is included in
> chrome.gyp, it also inherits the variables that are defined in chrome.gyp,
> and I
> am not sure yet how to move 'nacl_defines' from chrome.gyp to
> components.gyp.
> I did not add nacl_common.gyp to components.gyp because part of it is
> included
> in common_constants.gyp, and I did not want to introduce components.gyp into
> common_constants.gyp.
> Does that make sense?

Yep, makes sense to me.

Cheers,
Jói


On Thu, Jun 27, 2013 at 3:03 PM,  <yael.aharon@chromium.org> wrote:
> On 2013/06/27 14:18:38, Jói wrote:
>>
>> I reviewed the top-level //components stuff, not the stuff under
>> //components/nacl.
>
>
> Thank you for your review.
>
>> Please get folks in //components/nacl/OWNERS to review the stuff within
>> //components/nacl.  I would also suggest updating //components/OWNERS with
>> per-file OWNERS for the NaCl-related files in that directory.
>
>
> Done
>
>
>> https://codereview.chromium.org/16881004/diff/170001/components/nacl.gypi
>> File components/nacl.gypi (right):
>
>
>
>
https://codereview.chromium.org/16881004/diff/170001/components/nacl.gypi#new...
>>
>> components/nacl.gypi:1: # Copyright (c) 2012 The Chromium Authors. All
>> rights
>> reserved.
>> Shouldn't there be an entry for the two .gypi files in
>> //components/components.gyp?
>
> I did not add nacl.gypi to components.gyp because when it is included in
> chrome.gyp, it also inherits the variables that are defined in chrome.gyp,
> and I
> am not sure yet how to move 'nacl_defines' from chrome.gyp to
> components.gyp.
> I did not add nacl_common.gyp to components.gyp because part of it is
> included
> in common_constants.gyp, and I did not want to introduce components.gyp into
> common_constants.gyp.
> Does that make sense?
>
>
>
>
https://codereview.chromium.org/16881004/diff/195001/components/nacl_common.gyp
>>
>> File components/nacl_common.gyp (right):
>
>
>
>
https://codereview.chromium.org/16881004/diff/195001/components/nacl_common.g...
>>
>> components/nacl_common.gyp:1: # Copyright (c) 2013 The Chromium Authors.
>> All
>> rights reserved.
>> Not sure we still use the (c) but I don't think it matters either.
>
>
> Removed this change.
>
> Can you please suggest who can review this from the NaCl team? I was told to
> ask
> Mark Seaborn, but he seems to be unavailable.
>
>
>
> https://codereview.chromium.org/16881004/

[yael.aharon1, 2013-06-27 20:41:46]

Mark, do you have any other comments?
Just FYI, "git blame" is not going to show my name on all these files. It is
very good in identifying files that were moved and keeping the history as
before.

[yael.aharon1, 2013-07-05 12:54:36]

ping?

[Mark Seaborn, 2013-10-09 19:19:51]

This change is superseded: Yael split it up into smaller changes, most of which
have been committed now.