Clean up thread handling in Blimp browser tests.

Clean up thread handling in Blimp browser tests.

* Make BlobChannelService IO-thread clean and add ThreadChecker to
  enforce it.
* Add thread-hopping to cache state pulling logic.
* Replace RunLoops with test utility functions.
* Add helper function "RunUntilCompletion()" which relieves test code
  of the need to explicitly schedule RunLoops on threads.
* Misc. cleanup (e.g. moving some fields from unique_ptrs to regular fields, renaming methods, etc.)

R=wez@chromium.org,lethalantidote@chromium.org
BUG=

Committed: https://crrev.com/4e3446b74d446d5e2132fb3640feaabc933bdca0
Cr-Commit-Position: refs/heads/master@{#413523}

[Kevin M, 2016-08-19 01:28:33]

Description was changed from

==========
Clean up thread handling in Blimp browser tests.

* Make BlobChannelService IO-thread clean and add ThreadChecker to
  enforce it.
* Add thread-hopping to cache state pulling logic.
* Replace RunLoops with test utility functions.
* Add helper function "RunUntilCompletion()" which relieves test code
  of the need to explicitly schedule RunLoops on threads.
* Misc. cleanup (e.g. moving some fields from unique_ptrs to regular fields,
renaming methods, etc.)

R=wez@chromium.org,lethalantidote@chromium.org
BUG=
==========

to

==========
Clean up thread handling in Blimp browser tests.

* Make BlobChannelService IO-thread clean and add ThreadChecker to
  enforce it.
* Add thread-hopping to cache state pulling logic.
* Replace RunLoops with test utility functions.
* Add helper function "RunUntilCompletion()" which relieves test code
  of the need to explicitly schedule RunLoops on threads.
* Misc. cleanup (e.g. moving some fields from unique_ptrs to regular fields,
renaming methods, etc.)

R=wez@chromium.org,lethalantidote@chromium.org
BUG=
==========

[Wez, 2016-08-19 19:15:50]

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/browser_tests/...
File blimp/engine/browser_tests/blimp_browser_test.cc (right):

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/browser_tests/...
blimp/engine/browser_tests/blimp_browser_test.cc:101:
content::RunAllPendingInMessageLoop(content::BrowserThread::UI);
There is an assumption here that GetEnginePortForTesting() effectively completes
synchronously on the IO thread and posts a result to the UI thread - is that
valid, or might there be an async socket operation (e.g. listen or bind), so
that just running pending tasks isn't sufficient? Should we add an explicit
init-completion event instead?

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/browser_tests/...
File blimp/engine/browser_tests/blimp_browser_test.h (right):

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/browser_tests/...
blimp/engine/browser_tests/blimp_browser_test.h:65: void
set_engine_port(uint16_t port) { engine_port_ = port; }
nit: This comment doesn't make clear why we need this, versus just setting the
port member directly.

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
File blimp/engine/mojo/blob_channel_service.cc (right):

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_service.cc:42: base::Unretained(this)),
What happens if you just Bind(BlobChannelSender::GetCachedBlobIds,
blob_channel_sender_) here?  Then the task-runner should check the bound WeakPtr
and skip the task if it is null, but I'm not sure what that means for the
return-value...?

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_service.cc:45: response_callback));
What prevents BlobChannelService being torn-down before ReceivedBlobIds is
processed?

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_service.cc:49:
BlobChannelService::GetCachedBlobIdsOnUiThread() {
If we must have this method (i.e if Bind()ing directly won't work) then let's
make it a static function so that the BlobChannelService is only ever "touched"
on the IO thread.

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
File blimp/engine/mojo/blob_channel_service.h (right):

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_service.h:21: // Runs on the IO thread of the
browser process.
nit: Is the caller responsible for calling it on the IO thread, or does it punt
work to the IO thread internally? Not clear from the context and less obvious
given the UI thread comments below.

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_service.h:22: // Calls to the UI thread are
executed using the UI thread's TaskRunner.
nit: This comment seems tautologous; how else would you call to the UI thread
but via a TaskRunner? :P

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_service.h:27: // |task_runner|: The UI thread
task runner.
Can we instead clarify _why_ the caller needs to provide the UI task runner?
What will it be used for?

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_service.h:30:
scoped_refptr<base::SingleThreadTaskRunner> task_runner);
ui_task_runner?

Here and above, though, the desired property is that this is the task-runner
that the blob-channel-sender must be called on, so I'd suggest describing it in
those terms and naming it accordingly, rather than assuming the UI thread.

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_service.h:42: // UI thread.
IIUC they "arrive" only because we request them, so I think it'd be clearer to
say "Used to receive the results of BlobChannelSender::GetCachedBlobs()
asynchronously from the BlobChannelSender's thread" or similar.

Suggest renaming to OnGetCachedBlobsCompleted().

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/session/blimp_...
File blimp/engine/session/blimp_engine_session.cc (right):

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/session/blimp_...
blimp/engine/session/blimp_engine_session.cc:139: void
Initialize(scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner,
nit: Consider replacing the quit_closure ctor parameter with a ui_task_runner
parameter, since EngineNetworkComponents can then post the
MessageLoop:QuitClosure() directly to the UI thread itself.

https://codereview.chromium.org/2256363003/diff/1/blimp/net/blob_channel/blob...
File blimp/net/blob_channel/blob_channel_sender_impl.h (right):

https://codereview.chromium.org/2256363003/diff/1/blimp/net/blob_channel/blob...
blimp/net/blob_channel/blob_channel_sender_impl.h:41:
base::WeakPtr<BlobChannelSenderImpl> GetWeakPtr();
Rather than pollute BlobChannelSenderImpl with WeakPtriness, can't we just
create a WeakPtrFactory<BlobChannelSender> member in BlimpEngineSession to wrap
it into a WeakPtr?

https://codereview.chromium.org/2256363003/diff/1/blimp/net/blob_channel/blob...
blimp/net/blob_channel/blob_channel_sender_impl.h:58: base::ThreadChecker
thread_checker_;
Doesn't look like you're actually using the thread-checker anywhere?

[Kevin M, 2016-08-19 21:43:34]

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/browser_tests/...
File blimp/engine/browser_tests/blimp_browser_test.cc (right):

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/browser_tests/...
blimp/engine/browser_tests/blimp_browser_test.cc:101:
content::RunAllPendingInMessageLoop(content::BrowserThread::UI);
On 2016/08/19 19:15:50, Wez wrote:
> There is an assumption here that GetEnginePortForTesting() effectively
completes
> synchronously on the IO thread and posts a result to the UI thread - is that
> valid, or might there be an async socket operation (e.g. listen or bind), so
> that just running pending tasks isn't sufficient? Should we add an explicit
> init-completion event instead?

Done.

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/browser_tests/...
File blimp/engine/browser_tests/blimp_browser_test.h (right):

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/browser_tests/...
blimp/engine/browser_tests/blimp_browser_test.h:65: void
set_engine_port(uint16_t port) { engine_port_ = port; }
On 2016/08/19 19:15:50, Wez wrote:
> nit: This comment doesn't make clear why we need this, versus just setting the
> port member directly.

Done.

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
File blimp/engine/mojo/blob_channel_service.cc (right):

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_service.cc:42: base::Unretained(this)),
On 2016/08/19 19:15:50, Wez wrote:
> What happens if you just Bind(BlobChannelSender::GetCachedBlobIds,
> blob_channel_sender_) here?  Then the task-runner should check the bound
WeakPtr
> and skip the task if it is null, but I'm not sure what that means for the
> return-value...?

There's a static assert that prevents one from Binding() non-void methods of a
WeakPtr'd object.

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_service.cc:45: response_callback));
On 2016/08/19 19:15:50, Wez wrote:
> What prevents BlobChannelService being torn-down before ReceivedBlobIds is
> processed?

Nothing... switched to WeakPtr and moved the UI thread getter outside the class
to an anonymous namespace function.

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_service.cc:49:
BlobChannelService::GetCachedBlobIdsOnUiThread() {
On 2016/08/19 19:15:50, Wez wrote:
> If we must have this method (i.e if Bind()ing directly won't work) then let's
> make it a static function so that the BlobChannelService is only ever
"touched"
> on the IO thread.

Done.

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
File blimp/engine/mojo/blob_channel_service.h (right):

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_service.h:21: // Runs on the IO thread of the
browser process.
On 2016/08/19 19:15:50, Wez wrote:
> nit: Is the caller responsible for calling it on the IO thread, or does it
punt
> work to the IO thread internally? Not clear from the context and less obvious
> given the UI thread comments below.

Done.

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_service.h:22: // Calls to the UI thread are
executed using the UI thread's TaskRunner.
On 2016/08/19 19:15:50, Wez wrote:
> nit: This comment seems tautologous; how else would you call to the UI thread
> but via a TaskRunner? :P

Acknowledged.

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_service.h:27: // |task_runner|: The UI thread
task runner.
On 2016/08/19 19:15:50, Wez wrote:
> Can we instead clarify _why_ the caller needs to provide the UI task runner?
> What will it be used for?

Done.

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_service.h:30:
scoped_refptr<base::SingleThreadTaskRunner> task_runner);
On 2016/08/19 19:15:50, Wez wrote:
> ui_task_runner?
> 
> Here and above, though, the desired property is that this is the task-runner
> that the blob-channel-sender must be called on, so I'd suggest describing it
in
> those terms and naming it accordingly, rather than assuming the UI thread.

Done.

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_service.h:42: // UI thread.
On 2016/08/19 19:15:50, Wez wrote:
> IIUC they "arrive" only because we request them, so I think it'd be clearer to
> say "Used to receive the results of BlobChannelSender::GetCachedBlobs()
> asynchronously from the BlobChannelSender's thread" or similar.
> 
> Suggest renaming to OnGetCachedBlobsCompleted().

Done.

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/session/blimp_...
File blimp/engine/session/blimp_engine_session.cc (right):

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/session/blimp_...
blimp/engine/session/blimp_engine_session.cc:139: void
Initialize(scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner,
On 2016/08/19 19:15:50, Wez wrote:
> nit: Consider replacing the quit_closure ctor parameter with a ui_task_runner
> parameter, since EngineNetworkComponents can then post the
> MessageLoop:QuitClosure() directly to the UI thread itself.

I just made NetworkComponents compute the quit closure for itself.

https://codereview.chromium.org/2256363003/diff/1/blimp/net/blob_channel/blob...
File blimp/net/blob_channel/blob_channel_sender_impl.h (right):

https://codereview.chromium.org/2256363003/diff/1/blimp/net/blob_channel/blob...
blimp/net/blob_channel/blob_channel_sender_impl.h:41:
base::WeakPtr<BlobChannelSenderImpl> GetWeakPtr();
On 2016/08/19 19:15:50, Wez wrote:
> Rather than pollute BlobChannelSenderImpl with WeakPtriness, can't we just
> create a WeakPtrFactory<BlobChannelSender> member in BlimpEngineSession to
wrap
> it into a WeakPtr?

Done.

https://codereview.chromium.org/2256363003/diff/1/blimp/net/blob_channel/blob...
blimp/net/blob_channel/blob_channel_sender_impl.h:58: base::ThreadChecker
thread_checker_;
On 2016/08/19 19:15:50, Wez wrote:
> Doesn't look like you're actually using the thread-checker anywhere?

Done.

[Kevin M, 2016-08-19 21:43:37]

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

[commit-bot: I haz the power, 2016-08-19 21:44:32]

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-08-19 22:56:34]

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

[commit-bot: I haz the power, 2016-08-19 22:56:35]

Dry run: This issue passed the CQ dry run.

[CJ, 2016-08-19 23:22:50]

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
File blimp/engine/mojo/blob_channel_service.cc (right):

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_service.cc:63: for (const auto& next_entry : ids)
{
I'd suggest renaming |next_entry| to just entry, since in a context of a loop it
can be a little confusing.

[Wez, 2016-08-20 00:40:24]

LGTM!

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
File blimp/engine/mojo/blob_channel_service.cc (right):

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_service.cc:42: base::Unretained(this)),
On 2016/08/19 21:43:33, Kevin M wrote:
> On 2016/08/19 19:15:50, Wez wrote:
> > What happens if you just Bind(BlobChannelSender::GetCachedBlobIds,
> > blob_channel_sender_) here?  Then the task-runner should check the bound
> WeakPtr
> > and skip the task if it is null, but I'm not sure what that means for the
> > return-value...?
> 
> There's a static assert that prevents one from Binding() non-void methods of a
> WeakPtr'd object.

Awww, sad panda. But also yay for static asserts preventing heinous choices. :P

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
File blimp/engine/mojo/blob_channel_service.h (right):

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_service.h:30:
scoped_refptr<base::SingleThreadTaskRunner> task_runner);
On 2016/08/19 21:43:33, Kevin M wrote:
> On 2016/08/19 19:15:50, Wez wrote:
> > ui_task_runner?
> > 
> > Here and above, though, the desired property is that this is the task-runner
> > that the blob-channel-sender must be called on, so I'd suggest describing it
> in
> > those terms and naming it accordingly, rather than assuming the UI thread.
> 
> Done.

Is it necessary that this is all on the UI thread? Or should we call it
blob_channel_sender_task_runner or equivalent?

https://codereview.chromium.org/2256363003/diff/20001/blimp/engine/browser_te...
File blimp/engine/browser_tests/blimp_browser_test.h (right):

https://codereview.chromium.org/2256363003/diff/20001/blimp/engine/browser_te...
blimp/engine/browser_tests/blimp_browser_test.h:64: // Calls SignalCompletion()
when set.
nit: I suggest being more explicit - this is used during synchronous test setup
to receive the port number from the async socket API, and signal that we're
ready to continue.

Would it help to call this OnGetEnginePortCompletion() to emphasize that it's
not a general-purpose setter?

[Kevin M, 2016-08-22 18:01:13]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2016-08-22 18:01:13]

The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org
Link to the patchset: https://codereview.chromium.org/2256363003/#ps40001
(title: "wez feedback")

[commit-bot: I haz the power, 2016-08-22 18:02:29]

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

[Kevin M, 2016-08-22 19:46:56]

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
File blimp/engine/mojo/blob_channel_service.h (right):

https://codereview.chromium.org/2256363003/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_service.h:30:
scoped_refptr<base::SingleThreadTaskRunner> task_runner);
On 2016/08/20 00:40:24, Wez wrote:
> On 2016/08/19 21:43:33, Kevin M wrote:
> > On 2016/08/19 19:15:50, Wez wrote:
> > > ui_task_runner?
> > > 
> > > Here and above, though, the desired property is that this is the
task-runner
> > > that the blob-channel-sender must be called on, so I'd suggest describing
it
> > in
> > > those terms and naming it accordingly, rather than assuming the UI thread.
> > 
> > Done.
> 
> Is it necessary that this is all on the UI thread? Or should we call it
> blob_channel_sender_task_runner or equivalent?

Done.

https://codereview.chromium.org/2256363003/diff/20001/blimp/engine/browser_te...
File blimp/engine/browser_tests/blimp_browser_test.h (right):

https://codereview.chromium.org/2256363003/diff/20001/blimp/engine/browser_te...
blimp/engine/browser_tests/blimp_browser_test.h:64: // Calls SignalCompletion()
when set.
On 2016/08/20 00:40:24, Wez wrote:
> nit: I suggest being more explicit - this is used during synchronous test
setup
> to receive the port number from the async socket API, and signal that we're
> ready to continue.
> 
> Would it help to call this OnGetEnginePortCompletion() to emphasize that it's
> not a general-purpose setter?

Done.

[commit-bot: I haz the power, 2016-08-22 20:34:26]

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

[commit-bot: I haz the power, 2016-08-22 20:34:28]

Failed to apply the patch.
On branch working_branch
Your branch is up-to-date with 'origin/refs/pending/heads/master'.
nothing to commit, working tree clean

[commit-bot: I haz the power, 2016-08-22 20:34:53]

Description was changed from

==========
Clean up thread handling in Blimp browser tests.

* Make BlobChannelService IO-thread clean and add ThreadChecker to
  enforce it.
* Add thread-hopping to cache state pulling logic.
* Replace RunLoops with test utility functions.
* Add helper function "RunUntilCompletion()" which relieves test code
  of the need to explicitly schedule RunLoops on threads.
* Misc. cleanup (e.g. moving some fields from unique_ptrs to regular fields,
renaming methods, etc.)

R=wez@chromium.org,lethalantidote@chromium.org
BUG=
==========

to

==========
Clean up thread handling in Blimp browser tests.

* Make BlobChannelService IO-thread clean and add ThreadChecker to
  enforce it.
* Add thread-hopping to cache state pulling logic.
* Replace RunLoops with test utility functions.
* Add helper function "RunUntilCompletion()" which relieves test code
  of the need to explicitly schedule RunLoops on threads.
* Misc. cleanup (e.g. moving some fields from unique_ptrs to regular fields,
renaming methods, etc.)

R=wez@chromium.org,lethalantidote@chromium.org
BUG=

Committed: https://crrev.com/4e3446b74d446d5e2132fb3640feaabc933bdca0
Cr-Commit-Position: refs/heads/master@{#413523}
==========

[commit-bot: I haz the power, 2016-08-22 20:34:55]

Patchset 3 (id:??) landed as
https://crrev.com/4e3446b74d446d5e2132fb3640feaabc933bdca0
Cr-Commit-Position: refs/heads/master@{#413523}