Add network setup code to BlimpClientContextImpl

Add network setup code to BlimpClientContextImpl

The BlimpClientSession is only used for the shell, and it sets up
network related code for that. This CL takes the network setup code and
moves it to BlimpClientContextImpl.

The CL also moves some exposed headers related to protocol buffers
to the public_deps of those targets. This is because of the
file blimp/net/browser_connection_handler.h which exposes the use
of BlimpMessage::FeatureCase, which can not be forward declared.

BUG=611097
Committed: https://crrev.com/d4f943953bbfaa0f4f5c0c448a2caa19df678321
Cr-Commit-Position: refs/heads/master@{#409370}

[nyquist, 2016-07-27 21:57:10]

Description was changed from

==========
Add network setup code to BlimpClientContextImpl

The BlimpClientSession is only used for the shell, and it sets up
network related code for that. This CL takes the network setup code and
moves it to BlimpClientContextImpl.

This includes creating the IO thread for blimp.

BUG=611097
==========

to

==========
Add network setup code to BlimpClientContextImpl

The BlimpClientSession is only used for the shell, and it sets up
network related code for that. This CL takes the network setup code and
moves it to BlimpClientContextImpl. This includes creating the IO
thread for blimp.

The CL also moves some exposed headers related to protocol buffers
to the public_deps of those targets. This is because of the
file blimp/net/browser_connection_handler.h which exposes the use
of BlimpMessage::FeatureCase, which can not be forward declared.

BUG=611097
==========

[nyquist, 2016-07-27 21:57:37]

nyquist@chromium.org changed reviewers:
+ kmarshall@chromium.org

[nyquist, 2016-07-27 21:57:38]

kmarshall: PTAL

[nyquist, 2016-07-27 22:48:46]

https://codereview.chromium.org/2190753002/diff/1/blimp/client/core/blimp_cli...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2190753002/diff/1/blimp/client/core/blimp_cli...
blimp/client/core/blimp_client_context_impl.h:63: base::Thread io_thread_;
Should the embedder provide a SequencedTaskRunner instead?

[Kevin M, 2016-07-28 19:18:56]

One of the things that I think is missing from the current impl is that we have
a lot of code duplication across the Client and Engine classes. This CL improves
the structure of the code, but IMO we might want to consider how we could turn
these big containers into common code?

https://codereview.chromium.org/2190753002/diff/1/blimp/client/core/blimp_cli...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2190753002/diff/1/blimp/client/core/blimp_cli...
blimp/client/core/blimp_client_context_impl.h:63: base::Thread io_thread_;
On 2016/07/27 22:48:46, nyquist wrote:
> Should the embedder provide a SequencedTaskRunner instead?

Or a SingleThreadTaskRunner?

[nyquist, 2016-07-28 22:08:31]

PTAL

https://codereview.chromium.org/2190753002/diff/1/blimp/client/core/blimp_cli...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2190753002/diff/1/blimp/client/core/blimp_cli...
blimp/client/core/blimp_client_context_impl.h:63: base::Thread io_thread_;
On 2016/07/28 19:18:56, Kevin M wrote:
> On 2016/07/27 22:48:46, nyquist wrote:
> > Should the embedder provide a SequencedTaskRunner instead?
> 
> Or a SingleThreadTaskRunner?

Done.

[Kevin M, 2016-07-29 19:23:17]

General comment: for these refactoring CLs, let's keep a critical eye on the
structure and naming of the code we are relocating. Some of the assumptions
they're built on may be out of date.

https://codereview.chromium.org/2190753002/diff/20001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2190753002/diff/20001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.cc:52: RegisterFeatures();
* This seems like a misnomer, since this is really just setting up the
ThreadPipeManager?
* IMO this should just be inlined.

https://codereview.chromium.org/2190753002/diff/20001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl_unittest.cc (right):

https://codereview.chromium.org/2190753002/diff/20001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl_unittest.cc:35:
delegate.GetBlimpContentsWithLastAttachedHelpers());
Rather than using a getter and a field for saving this value, just use
MOCK_METHOD for AttachBlimpContentsHelpers w/SaveArg(), then inspect the saved
arg.

[nyquist, 2016-07-29 22:24:25]

Description was changed from

==========
Add network setup code to BlimpClientContextImpl

The BlimpClientSession is only used for the shell, and it sets up
network related code for that. This CL takes the network setup code and
moves it to BlimpClientContextImpl. This includes creating the IO
thread for blimp.

The CL also moves some exposed headers related to protocol buffers
to the public_deps of those targets. This is because of the
file blimp/net/browser_connection_handler.h which exposes the use
of BlimpMessage::FeatureCase, which can not be forward declared.

BUG=611097
==========

to

==========
Add network setup code to BlimpClientContextImpl

The BlimpClientSession is only used for the shell, and it sets up
network related code for that. This CL takes the network setup code and
moves it to BlimpClientContextImpl.

The CL also moves some exposed headers related to protocol buffers
to the public_deps of those targets. This is because of the
file blimp/net/browser_connection_handler.h which exposes the use
of BlimpMessage::FeatureCase, which can not be forward declared.

BUG=611097
==========

[nyquist, 2016-07-29 22:26:57]

PTAL

https://codereview.chromium.org/2190753002/diff/20001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2190753002/diff/20001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.cc:52: RegisterFeatures();
On 2016/07/29 19:23:17, Kevin M wrote:
> * This seems like a misnomer, since this is really just setting up the
> ThreadPipeManager?
> * IMO this should just be inlined.

True. How about this; I'll move the threadpipemanager out to the constructor,
and then when the features come in later, we can add RegisterFeatures if it
fits?

https://codereview.chromium.org/2190753002/diff/20001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl_unittest.cc (right):

https://codereview.chromium.org/2190753002/diff/20001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl_unittest.cc:35:
delegate.GetBlimpContentsWithLastAttachedHelpers());
On 2016/07/29 19:23:17, Kevin M wrote:
> Rather than using a getter and a field for saving this value, just use
> MOCK_METHOD for AttachBlimpContentsHelpers w/SaveArg(), then inspect the saved
> arg.

Done.

[nyquist, 2016-07-29 23:37:08]

The CQ bit was checked by nyquist@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 23:37:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-30 00:42:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-30 00:42:36]

Dry run: This issue passed the CQ dry run.

[Kevin M, 2016-08-02 19:55:41]

lgtm

https://codereview.chromium.org/2190753002/diff/60001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2190753002/diff/60001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.cc:57: // Initialize must only be
posted after the RegisterFeature calls have
RegisterFeature() isn't a thing anymore?

https://codereview.chromium.org/2190753002/diff/60001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2190753002/diff/60001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.h:70: BlimpConnectionStatistics*
blimp_connection_statistics_;
This will be going away soon (we're going to be using singletons), but I'm
commenting on this just as a FYI: we should probably specify which object owns
blimp_connection_statistics_, not which thread. Otherwise I might assume that
|this| owns the |blimp_connection_statistics_| ptr, but needs to delete it on
the IOthread.

https://codereview.chromium.org/2190753002/diff/60001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl_unittest.cc (right):

https://codereview.chromium.org/2190753002/diff/60001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl_unittest.cc:19:
TEST(BlimpClientContextImpl, CreatedBlimpContentsGetsHelpersAttached) {
Strongly prefer using TEST_F, even if the class body is empty.

[nyquist, 2016-08-02 21:47:04]

https://codereview.chromium.org/2190753002/diff/60001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2190753002/diff/60001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.cc:57: // Initialize must only be
posted after the RegisterFeature calls have
On 2016/08/02 19:55:40, Kevin M wrote:
> RegisterFeature() isn't a thing anymore?

Done.

https://codereview.chromium.org/2190753002/diff/60001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2190753002/diff/60001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.h:70: BlimpConnectionStatistics*
blimp_connection_statistics_;
On 2016/08/02 19:55:41, Kevin M wrote:
> This will be going away soon (we're going to be using singletons), but I'm
> commenting on this just as a FYI: we should probably specify which object owns
> blimp_connection_statistics_, not which thread. Otherwise I might assume that
> |this| owns the |blimp_connection_statistics_| ptr, but needs to delete it on
> the IOthread.

Totally agree! Done.

https://codereview.chromium.org/2190753002/diff/60001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl_unittest.cc (right):

https://codereview.chromium.org/2190753002/diff/60001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl_unittest.cc:19:
TEST(BlimpClientContextImpl, CreatedBlimpContentsGetsHelpersAttached) {
On 2016/08/02 19:55:41, Kevin M wrote:
> Strongly prefer using TEST_F, even if the class body is empty.

In this CL https://codereview.chromium.org/1912153004/ between patch set 1 and 2
and the file 
blimp/common/blob_cache/id_util_unittest.cc, the opposite advice was given,
which is why I did it that way. I don't feel strongly myself though.

Anyway, for this CL, I moved the thread setup and shutdown to a test-class, so
now it should all be fine.

[nyquist, 2016-08-02 21:50:15]

The CQ bit was checked by nyquist@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 21:51:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nyquist, 2016-08-02 21:59:05]

The CQ bit was unchecked by nyquist@chromium.org

[nyquist, 2016-08-02 21:59:11]

The CQ bit was checked by nyquist@chromium.org

[nyquist, 2016-08-02 21:59:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/2190753002/#ps80001
(title: "Updated comments and changed test to use a testing::Test class")

[commit-bot: I haz the power, 2016-08-02 21:59:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 23:08:25]

Description was changed from

==========
Add network setup code to BlimpClientContextImpl

The BlimpClientSession is only used for the shell, and it sets up
network related code for that. This CL takes the network setup code and
moves it to BlimpClientContextImpl.

The CL also moves some exposed headers related to protocol buffers
to the public_deps of those targets. This is because of the
file blimp/net/browser_connection_handler.h which exposes the use
of BlimpMessage::FeatureCase, which can not be forward declared.

BUG=611097
==========

to

==========
Add network setup code to BlimpClientContextImpl

The BlimpClientSession is only used for the shell, and it sets up
network related code for that. This CL takes the network setup code and
moves it to BlimpClientContextImpl.

The CL also moves some exposed headers related to protocol buffers
to the public_deps of those targets. This is because of the
file blimp/net/browser_connection_handler.h which exposes the use
of BlimpMessage::FeatureCase, which can not be forward declared.

BUG=611097
==========

[commit-bot: I haz the power, 2016-08-02 23:08:26]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-02 23:10:21]

Description was changed from

==========
Add network setup code to BlimpClientContextImpl

The BlimpClientSession is only used for the shell, and it sets up
network related code for that. This CL takes the network setup code and
moves it to BlimpClientContextImpl.

The CL also moves some exposed headers related to protocol buffers
to the public_deps of those targets. This is because of the
file blimp/net/browser_connection_handler.h which exposes the use
of BlimpMessage::FeatureCase, which can not be forward declared.

BUG=611097
==========

to

==========
Add network setup code to BlimpClientContextImpl

The BlimpClientSession is only used for the shell, and it sets up
network related code for that. This CL takes the network setup code and
moves it to BlimpClientContextImpl.

The CL also moves some exposed headers related to protocol buffers
to the public_deps of those targets. This is because of the
file blimp/net/browser_connection_handler.h which exposes the use
of BlimpMessage::FeatureCase, which can not be forward declared.

BUG=611097

Committed: https://crrev.com/d4f943953bbfaa0f4f5c0c448a2caa19df678321
Cr-Commit-Position: refs/heads/master@{#409370}
==========

[commit-bot: I haz the power, 2016-08-02 23:10:22]

Patchset 5 (id:??) landed as
https://crrev.com/d4f943953bbfaa0f4f5c0c448a2caa19df678321
Cr-Commit-Position: refs/heads/master@{#409370}