Move more files from chrome/browser/nacl_host/ to components/nacl/browser/

Move more files from chrome/browser/nacl_host/ to components/nacl/browser/

These files have no dependencies on chrome/ so they can be safely moved
to the components/ directory.

This is part of an effort to componentize NaCl code.

BUG=244791
R=brettw@chromium.org, cpu@chromium.org, jochen@chromium.org, mseaborn@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236821

[darktears, 2013-11-18 21:49:22]

Mark : if you could review it would be nice, thanks. There is still the link
error on Windows but I will look at it tomorrow morning (shouldn't be hard to
fix). I had to run before the try jobs finished. All these files were including
each others so you have to move them all at once.

[darktears, 2013-11-19 21:15:21]

On 2013/11/18 21:49:22, darktears wrote:
> Mark : if you could review it would be nice, thanks. There is still the link
> error on Windows but I will look at it tomorrow morning (shouldn't be hard to
> fix). I had to run before the try jobs finished. All these files were
including
> each others so you have to move them all at once.

I still can't figure out why it doesn't link on Windows. Still looking....

[jochen (gone - plz use gerrit), 2013-11-19 21:21:59]

https://codereview.chromium.org/75463005/diff/460001/components/nacl_common.gyp
File components/nacl_common.gyp (right):

https://codereview.chromium.org/75463005/diff/460001/components/nacl_common.g...
components/nacl_common.gyp:24: 'nacl/common/nacl_host_messages.h',
if you move this file here, you also need to have something like
chrome/common/common_message_generator.cc here

The reason is that windows will drop symbols from a static library that aren't
used in it.

[cpu_(ooo_6.6-7.5), 2013-11-20 02:05:06]

lgtm on component_updater/pnacl/pnacl_component_installer.cc

[darktears, 2013-11-21 15:00:41]

On 2013/11/20 02:05:06, cpu wrote:
> lgtm on component_updater/pnacl/pnacl_component_installer.cc

@Mark : I fixed Windows thanks to @jochen, so if you can review it would be
awesome. Thanks.

[Mark Seaborn, 2013-11-21 23:47:34]

LGTM with changes below.

https://codereview.chromium.org/75463005/diff/530001/components/components_te...
File components/components_tests.gyp (right):

https://codereview.chromium.org/75463005/diff/530001/components/components_te...
components/components_tests.gyp:143: 'nacl/common/nacl_host_messages.h',
This header doesn't belong in this list.  (Headers listed in Gyp don't affect
the build -- they only affect what is displayed in an IDE, for those still using
MSVS or XCode to build Chromium.)

https://codereview.chromium.org/75463005/diff/530001/components/nacl.gyp
File components/nacl.gyp (right):

https://codereview.chromium.org/75463005/diff/530001/components/nacl.gyp#newc...
components/nacl.gyp:133: '../skia/skia.gyp:skia',
This shouldn't be necessary.  Nothing in NaCl depends on Skia.

https://codereview.chromium.org/75463005/diff/530001/components/nacl/browser/...
File components/nacl/browser/nacl_broker_service_win.cc (right):

https://codereview.chromium.org/75463005/diff/530001/components/nacl/browser/...
components/nacl/browser/nacl_broker_service_win.cc:24: nacl::NaClBrokerHost*
broker_host = new nacl::NaClBrokerHost;
"nacl::" prefix shouldn't be necessary.  Same below.

https://codereview.chromium.org/75463005/diff/530001/components/nacl/browser/...
File components/nacl/browser/nacl_broker_service_win.h (right):

https://codereview.chromium.org/75463005/diff/530001/components/nacl/browser/...
components/nacl/browser/nacl_broker_service_win.h:59: nacl::NaClBrokerHost*
GetBrokerHost();
"nacl::" prefix shouldn't be necessary here since this is inside "namespace
nacl".

https://codereview.chromium.org/75463005/diff/530001/components/nacl/browser/...
File components/nacl/browser/nacl_host_message_filter.h (right):

https://codereview.chromium.org/75463005/diff/530001/components/nacl/browser/...
components/nacl/browser/nacl_host_message_filter.h:25: namespace nacl {
Add empty line after this to match closing '}'

https://codereview.chromium.org/75463005/diff/530001/components/nacl/browser/...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/75463005/diff/530001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:301:
nacl::NaClBrokerService::GetInstance()->OnLoaderDied();
Same here, and below...

https://codereview.chromium.org/75463005/diff/530001/components/nacl/browser/...
File components/nacl/browser/nacl_process_host.h (right):

https://codereview.chromium.org/75463005/diff/530001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.h:35: namespace nacl {
Add empty line after this line

https://codereview.chromium.org/75463005/diff/530001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.h:80: void
Launch(nacl::NaClHostMessageFilter* nacl_host_message_filter,
"nacl::" prefix not needed.  Same below.

https://codereview.chromium.org/75463005/diff/530001/components/nacl/browser/...
File components/nacl/browser/pnacl_host.h (right):

https://codereview.chromium.org/75463005/diff/530001/components/nacl/browser/...
components/nacl/browser/pnacl_host.h:22: namespace pnacl {
Add empty line after this

[jochen (gone - plz use gerrit), 2013-11-22 12:09:56]

lgtm

[darktears, 2013-11-22 14:03:47]

On 2013/11/22 12:09:56, jochen wrote:
> lgtm

brettw : mind LGTM the dependencies added in components/nacl/browser/DEPS ?
Thanks.

[brettw, 2013-11-22 19:26:10]

lgtm

[commit-bot: I haz the power, 2013-11-22 19:29:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexis.menard@intel.com/75463005/830001

[commit-bot: I haz the power, 2013-11-22 20:22:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexis.menard@intel.com/75463005/970001

[Mark Seaborn, 2013-11-22 20:41:47]

https://codereview.chromium.org/75463005/diff/970001/components/nacl/browser/...
File components/nacl/browser/nacl_broker_service_win.cc (right):

https://codereview.chromium.org/75463005/diff/970001/components/nacl/browser/...
components/nacl/browser/nacl_broker_service_win.cc:99: iter.GetDelegate());
Nit: unnecessary whitespace change

https://codereview.chromium.org/75463005/diff/970001/components/nacl/browser/...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/75463005/diff/970001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:275: VLOG(1) << message;
Why are you changing this?  Please undo this change.

https://codereview.chromium.org/75463005/diff/970001/components/nacl/common/n...
File components/nacl/common/nacl_switches.cc (right):

https://codereview.chromium.org/75463005/diff/970001/components/nacl/common/n...
components/nacl/common/nacl_switches.cc:40: const char
kDisablePnaclCrashThrottling[]   = "disable-pnacl-crash-throttling";
Please put these in the same order as in the header (see other comment)

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

https://codereview.chromium.org/75463005/diff/970001/components/nacl/common/n...
components/nacl/common/nacl_switches.h:21: extern const char
kDisablePnaclCrashThrottling[];
Nit: the comment above says "All switches in alphabetical order"

[darktears, 2013-11-22 20:44:19]

Committed patchset #5 manually as r236821.

[darktears, 2013-11-22 20:46:54]

On 2013/11/22 20:41:47, Mark Seaborn wrote:
>
https://codereview.chromium.org/75463005/diff/970001/components/nacl/browser/...
> File components/nacl/browser/nacl_broker_service_win.cc (right):
> 
>
https://codereview.chromium.org/75463005/diff/970001/components/nacl/browser/...
> components/nacl/browser/nacl_broker_service_win.cc:99: iter.GetDelegate());
> Nit: unnecessary whitespace change
> 
>
https://codereview.chromium.org/75463005/diff/970001/components/nacl/browser/...
> File components/nacl/browser/nacl_process_host.cc (right):
> 
>
https://codereview.chromium.org/75463005/diff/970001/components/nacl/browser/...
> components/nacl/browser/nacl_process_host.cc:275: VLOG(1) << message;
> Why are you changing this?  Please undo this change.

The presubmit is now complaining about it. According to the log people are
changing it that way.

> 
>
https://codereview.chromium.org/75463005/diff/970001/components/nacl/common/n...
> File components/nacl/common/nacl_switches.cc (right):
> 
>
https://codereview.chromium.org/75463005/diff/970001/components/nacl/common/n...
> components/nacl/common/nacl_switches.cc:40: const char
> kDisablePnaclCrashThrottling[]   = "disable-pnacl-crash-throttling";
> Please put these in the same order as in the header (see other comment)
> 
>
https://codereview.chromium.org/75463005/diff/970001/components/nacl/common/n...
> File components/nacl/common/nacl_switches.h (right):
> 
>
https://codereview.chromium.org/75463005/diff/970001/components/nacl/common/n...
> components/nacl/common/nacl_switches.h:21: extern const char
> kDisablePnaclCrashThrottling[];
> Nit: the comment above says "All switches in alphabetical order"

Oops I commited. I'll do a following CL.

[Mark Seaborn, 2013-11-22 20:54:02]

On 22 November 2013 12:46, <alexis.menard@intel.com> wrote:

> https://codereview.chromium.org/75463005/diff/970001/
> components/nacl/browser/nacl_process_host.cc#newcode275
>
>> components/nacl/browser/nacl_process_host.cc:275: VLOG(1) << message;
>> Why are you changing this?  Please undo this change.
>>
>
> The presubmit is now complaining about it. According to the log people are
> changing it that way.
>

This log message tends to be quite important for debugging whether a NaCl
process has exited or is hanging, especially on the buildbots.  The
quantity of log output from this message is very minimal compared with
other log spam from Chromium and NaCl.  In any case, this is a
functionality change that doesn't belong in a change that is just moving
files around, and even if a presubmit check asks for a change I would
prefer to review that change.  Can you change it back in a follow-on
changeset, please?

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-11-22 20:54:03]

On 22 November 2013 12:46, <alexis.menard@intel.com> wrote:

> https://codereview.chromium.org/75463005/diff/970001/
> components/nacl/browser/nacl_process_host.cc#newcode275
>
>> components/nacl/browser/nacl_process_host.cc:275: VLOG(1) << message;
>> Why are you changing this?  Please undo this change.
>>
>
> The presubmit is now complaining about it. According to the log people are
> changing it that way.
>

This log message tends to be quite important for debugging whether a NaCl
process has exited or is hanging, especially on the buildbots.  The
quantity of log output from this message is very minimal compared with
other log spam from Chromium and NaCl.  In any case, this is a
functionality change that doesn't belong in a change that is just moving
files around, and even if a presubmit check asks for a change I would
prefer to review that change.  Can you change it back in a follow-on
changeset, please?

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[darktears, 2013-11-22 21:06:03]

On 2013/11/22 20:54:03, Mark Seaborn wrote:
> On 22 November 2013 12:46, <mailto:alexis.menard@intel.com> wrote:
> 
> > https://codereview.chromium.org/75463005/diff/970001/
> > components/nacl/browser/nacl_process_host.cc#newcode275
> >
> >> components/nacl/browser/nacl_process_host.cc:275: VLOG(1) << message;
> >> Why are you changing this?  Please undo this change.
> >>
> >
> > The presubmit is now complaining about it. According to the log people are
> > changing it that way.
> >
> 
> This log message tends to be quite important for debugging whether a NaCl
> process has exited or is hanging, especially on the buildbots.  The
> quantity of log output from this message is very minimal compared with
> other log spam from Chromium and NaCl.  In any case, this is a
> functionality change that doesn't belong in a change that is just moving
> files around, and even if a presubmit check asks for a change I would
> prefer to review that change.  Can you change it back in a follow-on
> changeset, please?

Sorry about that :

https://codereview.chromium.org/83553003/

to fix it.

> 
> Cheers,
> Mark
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.