Refactoring: Split NaClListener into two delegated classes.

Refactoring: Split NaClListener into two delegated classes.

Currently, NaClListener has supports two modes, SFI and non-SFI.
As a preparation of newlib switching of non-SFI mode, this CL splits into
each delegate class, so that, we can easily build non-SFI related code
only by PNaCl toolchain.

BUG=358465
TEST=Ran trybots.
CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_rel_precise32

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

[hidehiko, 2014-08-04 17:46:58]

Hi Mark, Justin,

could you take a look at this CL?
The goal of this CL is splitting start-up code in SFI-mode and one for non-SFI
mode. IMHO, there would be some more rooms for refactoring, but it is now out of
scope.

Also, Justin, I know this CL (slightly) conflicts with your crrev.com/424893003,
but we can easily merge/rebase on top of the other CL, I think. So please let me
send this to review now.

Thank you for review in advance,
- hidehiko

[teravest, 2014-08-04 21:15:28]

lgtm

This seems reasonable to me.

https://codereview.chromium.org/439713002/diff/20001/components/nacl/loader/l...
File components/nacl/loader/listener_delegate.h (right):

https://codereview.chromium.org/439713002/diff/20001/components/nacl/loader/l...
components/nacl/loader/listener_delegate.h:27: // Called when the plugin start
message is alived from the browser process.
Do you mean received instead of alived?

[hidehiko, 2014-08-05 04:07:26]

Thank you for review, Justin.
Mark, could you take a look, too?

https://codereview.chromium.org/439713002/diff/20001/components/nacl/loader/l...
File components/nacl/loader/listener_delegate.h (right):

https://codereview.chromium.org/439713002/diff/20001/components/nacl/loader/l...
components/nacl/loader/listener_delegate.h:27: // Called when the plugin start
message is alived from the browser process.
On 2014/08/04 21:15:27, teravest wrote:
> Do you mean received instead of alived?

You're right. Fixed.

[hidehiko, 2014-08-06 16:56:53]

Mark, friendly ping?
- hidehiko


On 2014/08/05 04:07:26, hidehiko wrote:
> Thank you for review, Justin.
> Mark, could you take a look, too?
> 
>
https://codereview.chromium.org/439713002/diff/20001/components/nacl/loader/l...
> File components/nacl/loader/listener_delegate.h (right):
> 
>
https://codereview.chromium.org/439713002/diff/20001/components/nacl/loader/l...
> components/nacl/loader/listener_delegate.h:27: // Called when the plugin start
> message is alived from the browser process.
> On 2014/08/04 21:15:27, teravest wrote:
> > Do you mean received instead of alived?
> 
> You're right. Fixed.

[Mark Seaborn, 2014-08-07 00:50:20]

LGTM with some naming convention nits, but:

It seems like this leaves nacl_listener.cc doing very little:

 * Creating an IO thread (just one call).
 * A destructor (probably unnecessary).
 * Complexity in Send() and Listen() that is for using sync channels, which SFI
NaCl needs but Non-SFI NaCl doesn't need.
 * Trivial OnMessageReceived (OK to duplicate).
 * CreateTrustedListener(), which could be moved to nacl_trusted_listener.cc
(which doesn't do anything yet anyway).

Adding another level of indirection via a delegate adds more complexity.  It
might make sense to duplicate the NaClListener code into the files for the SFI
and Non-SFI cases.  This would allow removing the sync-channel handling in the
Non-SFI copy.

What do you think?  I won't insist -- if you want to keep the approach you've
got, that's OK.

https://codereview.chromium.org/439713002/diff/40001/components/nacl.gyp
File components/nacl.gyp (right):

https://codereview.chromium.org/439713002/diff/40001/components/nacl.gyp#newc...
components/nacl.gyp:30: 'nacl/loader/listener_delegate.h',
Nit: doesn't follow the "nacl_*" naming scheme.  The existing naming scheme is a
bit redundant, but... maybe you could just declare the delegate base class in
nacl_listener.h?

https://codereview.chromium.org/439713002/diff/40001/components/nacl.gyp#newc...
components/nacl.gyp:45: 'nacl/loader/sfi_listener_delegate.cc',
Hmm, using "sfi_" would be a new naming scheme.  Other stuff is SFI-only too. 
It might be confusing to change the naming scheme piecemeal.

Can we just call this "loader/nacl_listener_delegate.cc" for consistency with
existing code?

https://codereview.chromium.org/439713002/diff/40001/components/nacl.gyp#newc...
components/nacl.gyp:244: 'nacl/loader/nonsfi_listener_delegate.cc',
Shouldn't this go in the loader/nonsfi/ subdir for consistency with the existing
nonsfi code?  e.g. as "loader/nonsfi/nonsfi_listener_delegate.cc"

https://codereview.chromium.org/439713002/diff/40001/components/nacl/loader/s...
File components/nacl/loader/sfi_listener_delegate.cc (left):

https://codereview.chromium.org/439713002/diff/40001/components/nacl/loader/s...
components/nacl/loader/sfi_listener_delegate.cc:203: // The listener never dies,
otherwise this might be a dangling reference.
Can you keep this line, in addition to the comment you added?

[hidehiko, 2014-08-07 13:32:46]

So, I redesigned the CL.

Justin, Mark, sorry, but could you take another look at PS3?
I think I addressed Mark's suggestion.

If PS2 looks better, please let me know. I'll just revert and submit PS2.

Thanks,
- hidehiko


On 2014/08/07 00:50:20, Mark Seaborn wrote:
> LGTM with some naming convention nits, but:
> 
> It seems like this leaves nacl_listener.cc doing very little:
> 
>  * Creating an IO thread (just one call).
>  * A destructor (probably unnecessary).
>  * Complexity in Send() and Listen() that is for using sync channels, which
SFI
> NaCl needs but Non-SFI NaCl doesn't need.
>  * Trivial OnMessageReceived (OK to duplicate).
>  * CreateTrustedListener(), which could be moved to nacl_trusted_listener.cc
> (which doesn't do anything yet anyway).
> 
> Adding another level of indirection via a delegate adds more complexity.  It
> might make sense to duplicate the NaClListener code into the files for the SFI
> and Non-SFI cases.  This would allow removing the sync-channel handling in the
> Non-SFI copy.
> 
> What do you think?  I won't insist -- if you want to keep the approach you've
> got, that's OK.
> 
> https://codereview.chromium.org/439713002/diff/40001/components/nacl.gyp
> File components/nacl.gyp (right):
> 
>
https://codereview.chromium.org/439713002/diff/40001/components/nacl.gyp#newc...
> components/nacl.gyp:30: 'nacl/loader/listener_delegate.h',
> Nit: doesn't follow the "nacl_*" naming scheme.  The existing naming scheme is
a
> bit redundant, but... maybe you could just declare the delegate base class in
> nacl_listener.h?
> 
>
https://codereview.chromium.org/439713002/diff/40001/components/nacl.gyp#newc...
> components/nacl.gyp:45: 'nacl/loader/sfi_listener_delegate.cc',
> Hmm, using "sfi_" would be a new naming scheme.  Other stuff is SFI-only too. 
> It might be confusing to change the naming scheme piecemeal.
> 
> Can we just call this "loader/nacl_listener_delegate.cc" for consistency with
> existing code?
> 
>
https://codereview.chromium.org/439713002/diff/40001/components/nacl.gyp#newc...
> components/nacl.gyp:244: 'nacl/loader/nonsfi_listener_delegate.cc',
> Shouldn't this go in the loader/nonsfi/ subdir for consistency with the
existing
> nonsfi code?  e.g. as "loader/nonsfi/nonsfi_listener_delegate.cc"
> 
>
https://codereview.chromium.org/439713002/diff/40001/components/nacl/loader/s...
> File components/nacl/loader/sfi_listener_delegate.cc (left):
> 
>
https://codereview.chromium.org/439713002/diff/40001/components/nacl/loader/s...
> components/nacl/loader/sfi_listener_delegate.cc:203: // The listener never
dies,
> otherwise this might be a dangling reference.
> Can you keep this line, in addition to the comment you added?

[Mark Seaborn, 2014-08-07 16:37:52]

LGTM, thanks.  Much simpler!

[teravest, 2014-08-07 16:42:12]

lgtm

[hidehiko, 2014-08-07 22:08:27]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-08-07 23:00:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hidehiko@chromium.org/439713002/60001

[commit-bot: I haz the power, 2014-08-08 00:27:11]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[commit-bot: I haz the power, 2014-08-08 08:54:53]

Change committed as 288271