Separate NaCl messages from the rest of chrome messages and create a new message filter. This is pa…

Separate NaCl messages from the rest of chrome messages and create a new message filter. This is part of an effort to componentize NaCl code.

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

[yael.aharon1, 2013-06-11 00:47:31]

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#%2521topic/pepper-...
.

Of course any comment would be appreciated, but could each owner review at least
his area? thanks

dmichael@chromium.org
chrome/renderer/pepper/DEPS
chrome/renderer/pepper/ppb_nacl_private_impl.cc
ipc/ipc_message_start.h

mseaborn@chromium.org
chrome/browser/chrome_content_browser_client.cc
chrome/browser/nacl_host/nacl_file_host.h
chrome/browser/nacl_host/nacl_file_host.cc
chrome/browser/nacl_host/nacl_host_message_filter.h
chrome/browser/nacl_host/nacl_host_message_filter.cc
chrome/browser/nacl_host/nacl_host_messages.h
chrome/browser/nacl_host/nacl_process_host.h
chrome/browser/nacl_host/nacl_process_host.cc
chrome/browser/renderer_host/chrome_render_message_filter.h
chrome/browser/renderer_host/chrome_render_message_filter.cc
chrome/chrome_browser.gypi
chrome/common/DEPS
chrome/common/common_message_generator.h

jschuh@chromium.org
chrome/common/render_messages.h

[jam, 2013-06-11 00:53:59]

https://codereview.chromium.org/15906013/diff/21001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_host_messages.h (right):

https://codereview.chromium.org/15906013/diff/21001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_host_messages.h:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
nit: this seems like it should just be called "nacl_messages.h"

https://codereview.chromium.org/15906013/diff/21001/chrome/renderer/pepper/DEPS
File chrome/renderer/pepper/DEPS (right):

https://codereview.chromium.org/15906013/diff/21001/chrome/renderer/pepper/DE...
chrome/renderer/pepper/DEPS:2: "+chrome/browser/nacl_host", # This is temporary.
See crbug.com/244791
why put this file in chrome/browser instead of chrome/common? even if it's
moving out to src/nacl

[yael.aharon1, 2013-06-11 00:56:38]

On 2013/06/11 00:53:59, jam wrote:
>
https://codereview.chromium.org/15906013/diff/21001/chrome/renderer/pepper/DE...
> chrome/renderer/pepper/DEPS:2: "+chrome/browser/nacl_host", # This is
temporary.
> See crbug.com/244791
> why put this file in chrome/browser instead of chrome/common? even if it's
> moving out to src/nacl

It is moving to components/nacl, not to chrome/nacl. I can't finalize the move
before breaking all the dependencies first.

[yael.aharon1, 2013-06-11 01:25:12]

On 2013/06/11 00:53:59, jam wrote:
>
https://codereview.chromium.org/15906013/diff/21001/chrome/browser/nacl_host/...
> File chrome/browser/nacl_host/nacl_host_messages.h (right):
> 
>
https://codereview.chromium.org/15906013/diff/21001/chrome/browser/nacl_host/...
> chrome/browser/nacl_host/nacl_host_messages.h:1: // Copyright (c) 2013 The
> Chromium Authors. All rights reserved.
> nit: this seems like it should just be called "nacl_messages.h"

There is a file called chrome/common/nacl_messages.h. I thought the name
collision could cause problems?

[jam, 2013-06-11 22:50:44]

On 2013/06/11 01:25:12, yael.aharon1 wrote:
> On 2013/06/11 00:53:59, jam wrote:
> >
>
https://codereview.chromium.org/15906013/diff/21001/chrome/browser/nacl_host/...
> > File chrome/browser/nacl_host/nacl_host_messages.h (right):
> > 
> >
>
https://codereview.chromium.org/15906013/diff/21001/chrome/browser/nacl_host/...
> > chrome/browser/nacl_host/nacl_host_messages.h:1: // Copyright (c) 2013 The
> > Chromium Authors. All rights reserved.
> > nit: this seems like it should just be called "nacl_messages.h"
> 
> There is a file called chrome/common/nacl_messages.h. I thought the name
> collision could cause problems?

oh, if there's nacl_messages.h already, why are you creating a new file instead
of moving the messages to the existing one? i.e. if the nacl code is all moving
out, it follows that the existing nacl messages file will move as well.

[jam, 2013-06-11 22:50:44]

On 2013/06/11 01:25:12, yael.aharon1 wrote:
> On 2013/06/11 00:53:59, jam wrote:
> >
>
https://codereview.chromium.org/15906013/diff/21001/chrome/browser/nacl_host/...
> > File chrome/browser/nacl_host/nacl_host_messages.h (right):
> > 
> >
>
https://codereview.chromium.org/15906013/diff/21001/chrome/browser/nacl_host/...
> > chrome/browser/nacl_host/nacl_host_messages.h:1: // Copyright (c) 2013 The
> > Chromium Authors. All rights reserved.
> > nit: this seems like it should just be called "nacl_messages.h"
> 
> There is a file called chrome/common/nacl_messages.h. I thought the name
> collision could cause problems?

oh, if there's nacl_messages.h already, why are you creating a new file instead
of moving the messages to the existing one? i.e. if the nacl code is all moving
out, it follows that the existing nacl messages file will move as well.

[yael.aharon1, 2013-06-12 12:15:37]

On 2013/06/11 22:50:44, jam wrote:
> On 2013/06/11 01:25:12, yael.aharon1 wrote:
> > On 2013/06/11 00:53:59, jam wrote:
> > >
> >
> 
> oh, if there's nacl_messages.h already, why are you creating a new file
instead
> of moving the messages to the existing one? i.e. if the nacl code is all
moving
> out, it follows that the existing nacl messages file will move as well.

The messages are separated in different files because they are serving 2
different processes - the browser (chrome) process and the nacl process.

[jam, 2013-06-12 15:20:25]

On 2013/06/12 12:15:37, yael.aharon1 wrote:
> On 2013/06/11 22:50:44, jam wrote:
> > On 2013/06/11 01:25:12, yael.aharon1 wrote:
> > > On 2013/06/11 00:53:59, jam wrote:
> > > >
> > >
> > 
> > oh, if there's nacl_messages.h already, why are you creating a new file
> instead
> > of moving the messages to the existing one? i.e. if the nacl code is all
> moving
> > out, it follows that the existing nacl messages file will move as well.
> 
> The messages are separated in different files because they are serving 2
> different processes - the browser (chrome) process and the nacl process.

they're still all nacl related, i don't see why they should be in a different
file

[jam, 2013-06-12 15:20:40]

On 2013/06/12 15:20:25, jam wrote:
> On 2013/06/12 12:15:37, yael.aharon1 wrote:
> > On 2013/06/11 22:50:44, jam wrote:
> > > On 2013/06/11 01:25:12, yael.aharon1 wrote:
> > > > On 2013/06/11 00:53:59, jam wrote:
> > > > >
> > > >
> > > 
> > > oh, if there's nacl_messages.h already, why are you creating a new file
> > instead
> > > of moving the messages to the existing one? i.e. if the nacl code is all
> > moving
> > > out, it follows that the existing nacl messages file will move as well.
> > 
> > The messages are separated in different files because they are serving 2
> > different processes - the browser (chrome) process and the nacl process.
> 
> they're still all nacl related, i don't see why they should be in a different
> file

to be clearer: please put them in the same file

[yael.aharon1, 2013-06-12 17:36:46]

On 2013/06/12 15:20:40, jam wrote:
> On 2013/06/12 15:20:25, jam wrote:
> > On 2013/06/12 12:15:37, yael.aharon1 wrote:
> > > On 2013/06/11 22:50:44, jam wrote:
> > > > On 2013/06/11 01:25:12, yael.aharon1 wrote:
> > > > > On 2013/06/11 00:53:59, jam wrote:
> > > > > >
> > > > >
> > > > 
> > > > oh, if there's nacl_messages.h already, why are you creating a new file
> > > instead
> > > > of moving the messages to the existing one? i.e. if the nacl code is all
> > > moving
> > > > out, it follows that the existing nacl messages file will move as well.
> > > 
> > > The messages are separated in different files because they are serving 2
> > > different processes - the browser (chrome) process and the nacl process.
> > 
> > they're still all nacl related, i don't see why they should be in a
different
> > file
> 
> to be clearer: please put them in the same file

ok, I need a little advice :)
I tried to combine the messages into nacl_messages.h but then nacl_helper does
not link due to the dependency on ParamTraits<GURL>::Read,
ParamTraits<GURL>::Write and ParamTraits<GURL>::Log. which are in
content/common/common_param_traits.cc. Should nacl_helper depend on content.gyp
? thanks.

[jam, 2013-06-12 20:09:53]

On 2013/06/12 17:36:46, yael.aharon1 wrote:
> On 2013/06/12 15:20:40, jam wrote:
> > On 2013/06/12 15:20:25, jam wrote:
> > > On 2013/06/12 12:15:37, yael.aharon1 wrote:
> > > > On 2013/06/11 22:50:44, jam wrote:
> > > > > On 2013/06/11 01:25:12, yael.aharon1 wrote:
> > > > > > On 2013/06/11 00:53:59, jam wrote:
> > > > > > >
> > > > > >
> > > > > 
> > > > > oh, if there's nacl_messages.h already, why are you creating a new
file
> > > > instead
> > > > > of moving the messages to the existing one? i.e. if the nacl code is
all
> > > > moving
> > > > > out, it follows that the existing nacl messages file will move as
well.
> > > > 
> > > > The messages are separated in different files because they are serving 2
> > > > different processes - the browser (chrome) process and the nacl process.
> > > 
> > > they're still all nacl related, i don't see why they should be in a
> different
> > > file
> > 
> > to be clearer: please put them in the same file
> 
> ok, I need a little advice :)
> I tried to combine the messages into nacl_messages.h but then nacl_helper does
> not link due to the dependency on ParamTraits<GURL>::Read,
> ParamTraits<GURL>::Write and ParamTraits<GURL>::Log. which are in
> content/common/common_param_traits.cc. Should nacl_helper depend on
content.gyp
> ? thanks.

ah. in that case, they need to remain separate. sorry for the noise.

please move the new file then to chrome\common

[yael.aharon1, 2013-06-12 21:12:34]

On 2013/06/12 20:09:53, jam wrote:
> On 2013/06/12 17:36:46, yael.aharon1 wrote:
> > On 2013/06/12 15:20:40, jam wrote:
> > > On 2013/06/12 15:20:25, jam wrote:
> > > > On 2013/06/12 12:15:37, yael.aharon1 wrote:
> > > > > On 2013/06/11 22:50:44, jam wrote:
> > > > > > On 2013/06/11 01:25:12, yael.aharon1 wrote:
> > > > > > > On 2013/06/11 00:53:59, jam wrote:
> > > > > > > >
> > > > > > >
> > > > > > 
> > > > > > oh, if there's nacl_messages.h already, why are you creating a new
> file
> > > > > instead
> > > > > > of moving the messages to the existing one? i.e. if the nacl code is
> all
> > > > > moving
> > > > > > out, it follows that the existing nacl messages file will move as
> well.
> > > > > 
> > > > > The messages are separated in different files because they are serving
2
> > > > > different processes - the browser (chrome) process and the nacl
process.
> > > > 
> > > > they're still all nacl related, i don't see why they should be in a
> > different
> > > > file
> > > 
> > > to be clearer: please put them in the same file
> > 
> > ok, I need a little advice :)
> > I tried to combine the messages into nacl_messages.h but then nacl_helper
does
> > not link due to the dependency on ParamTraits<GURL>::Read,
> > ParamTraits<GURL>::Write and ParamTraits<GURL>::Log. which are in
> > content/common/common_param_traits.cc. Should nacl_helper depend on
> content.gyp
> > ? thanks.
> 
> ah. in that case, they need to remain separate. sorry for the noise.
> 
> please move the new file then to chrome\common

done.

[yael.aharon1, 2013-06-12 22:07:33]

I don't know how this happened but the changes to render_messages.h do not match
the changes that I have locally. No wonder the patch fails to apply.

[jschuh, 2013-06-13 06:59:15]

appears to be refactoring without changing functionality or attack surface.
rubberstamp ipc security lgtm.

[jam, 2013-06-13 15:55:26]

lgtm

https://codereview.chromium.org/15906013/diff/34002/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_host_message_filter.h (right):

https://codereview.chromium.org/15906013/diff/34002/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_host_message_filter.h:9: #include
"chrome/browser/profiles/profile.h"
nit: don't include, forward declare

https://codereview.chromium.org/15906013/diff/34002/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_host_message_filter.h:29: Profile* profile,
nit: indentation

https://codereview.chromium.org/15906013/diff/34002/chrome/common/DEPS
File chrome/common/DEPS (right):

https://codereview.chromium.org/15906013/diff/34002/chrome/common/DEPS#newcode4
chrome/common/DEPS:4: "+chrome/browser/nacl_host", # This is temporary. See
crbug.com/244791
remove this now

[yael.aharon, 2013-06-13 16:14:25]

On 2013/06/13 15:55:26, jam wrote:
>
https://codereview.chromium.org/15906013/diff/34002/chrome/common/DEPS#newcode4
> chrome/common/DEPS:4: "+chrome/browser/nacl_host", # This is temporary. See
> crbug.com/244791
> remove this now
This is still needed by nacl_host_message_filter.h/cc. So far I moved
nacl_host_messages.h only. Would you prefer that the other files will be moved
to chrome/common too?

[yael.aharon1, 2013-06-13 18:07:58]

I fixed the nits, but did not remove the change in the DPES file yet. I wasn't
sure if nacl_host_message_filter.h needs to move or not.

[jam, 2013-06-13 20:33:53]

On 2013/06/13 16:14:25, yael.aharon wrote:
> On 2013/06/13 15:55:26, jam wrote:
> >
>
https://codereview.chromium.org/15906013/diff/34002/chrome/common/DEPS#newcode4
> > chrome/common/DEPS:4: "+chrome/browser/nacl_host", # This is temporary. See
> > crbug.com/244791
> > remove this now
> This is still needed by nacl_host_message_filter.h/cc. So far I moved
> nacl_host_messages.h only. Would you prefer that the other files will be moved
> to chrome/common too?

I think you're looking at the DEPS check backwards. chrome/common/DEPS would
only need to be updated to allow chrome/browser if code in chrome/common
included chrome/browser.

[yael.aharon1, 2013-06-13 21:00:32]

On 2013/06/13 20:33:53, jam wrote:
> On 2013/06/13 16:14:25, yael.aharon wrote:
> > On 2013/06/13 15:55:26, jam wrote:
> > >
> >
>
https://codereview.chromium.org/15906013/diff/34002/chrome/common/DEPS#newcode4
> > > chrome/common/DEPS:4: "+chrome/browser/nacl_host", # This is temporary.
See
> > > crbug.com/244791
> > > remove this now
> > This is still needed by nacl_host_message_filter.h/cc. So far I moved
> > nacl_host_messages.h only. Would you prefer that the other files will be
moved
> > to chrome/common too?
> 
> I think you're looking at the DEPS check backwards. chrome/common/DEPS would
> only need to be updated to allow chrome/browser if code in chrome/common
> included chrome/browser.

Got it, I removed the DPES .
thanks!

[yael.aharon1, 2013-06-13 21:11:39]

mseaborn, dmichael, can you please review this CL? I listed suggestion for
dividing the files in comment #1, but any feedback is welcome.

[commit-bot: I haz the power, 2013-06-17 18:14:33]

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

[commit-bot: I haz the power, 2013-06-17 18:40:48]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-17 19:23:16]

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

[dmichael (off chromium), 2013-06-17 22:31:09]

On 2013/06/13 21:11:39, yael.aharon1 wrote:
> mseaborn, dmichael, can you please review this CL? I listed suggestion for
> dividing the files in comment #1, but any feedback is welcome.
Sorry for not responding sooner. I was basically ignoring this CL, since I'm not
an owner anywhere you're touching. I should have been explicit earlier. 

lgtm FWIW

[commit-bot: I haz the power, 2013-06-18 00:17:36]

Change committed as 206855