[Blimp] Adds blimp engine browser test framework and LoadUrl test.

[Blimp] Adds blimp engine browser test framework and LoadUrl test.

It uses a headless Client (client_session with all features and mock feature delegates) to work with a full Engine listening on a local tcp port.

It expects certain messages received via these mock delegates when it sends messages to Engine.

other changes:
1. consolidate client's mock feature delegates with existing unit tests.
2. pull common engine command line configs into blimp::engine::SetUpCommandLine.

BUG=599184
Committed: https://crrev.com/d4f2c05f5d5d332f440d4ba46156a35228f2ed0c
Cr-Commit-Position: refs/heads/master@{#391974}

[haibinlu, 2016-04-27 22:36:02]

haibinlu@chromium.org changed reviewers:
+ wez@chromium.org

[haibinlu, 2016-04-27 22:40:08]

haibinlu@chromium.org changed reviewers:
+ bgoldman@chromium.org, nyquist@chromium.org

[nyquist, 2016-04-29 21:30:54]

https://codereview.chromium.org/1929723002/diff/20001/blimp/client/BUILD.gn
File blimp/client/BUILD.gn (right):

https://codereview.chromium.org/1929723002/diff/20001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:45: source_set("blimp_client") {
Just "client" nowadays :-)
Also, with that I think it should be the first entry.

https://codereview.chromium.org/1929723002/diff/20001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:159: source_set("compositor") {
should this be feature_compositor?

https://codereview.chromium.org/1929723002/diff/20001/blimp/client/feature/im...
File blimp/client/feature/ime_feature.cc (right):

https://codereview.chromium.org/1929723002/diff/20001/blimp/client/feature/im...
blimp/client/feature/ime_feature.cc:6: #include <string>
Nit: missing empty line here?

https://codereview.chromium.org/1929723002/diff/20001/blimp/test/blimp_browse...
File blimp/test/blimp_browser_test.cc (right):

https://codereview.chromium.org/1929723002/diff/20001/blimp/test/blimp_browse...
blimp/test/blimp_browser_test.cc:52:
command_line->AppendSwitch(::switches::kUseRemoteCompositing);
Could you file a bug that we should move these to the engine startup instead of
having them both here and in the shell script that starts the engine?

[haibinlu, 2016-04-29 22:43:13]

PTAL

https://codereview.chromium.org/1929723002/diff/20001/blimp/client/BUILD.gn
File blimp/client/BUILD.gn (right):

https://codereview.chromium.org/1929723002/diff/20001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:45: source_set("blimp_client") {
On 2016/04/29 21:30:54, nyquist wrote:
> Just "client" nowadays :-)
> Also, with that I think it should be the first entry.

Done.

https://codereview.chromium.org/1929723002/diff/20001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:159: source_set("compositor") {
On 2016/04/29 21:30:53, nyquist wrote:
> should this be feature_compositor?

Done.

https://codereview.chromium.org/1929723002/diff/20001/blimp/client/feature/im...
File blimp/client/feature/ime_feature.cc (right):

https://codereview.chromium.org/1929723002/diff/20001/blimp/client/feature/im...
blimp/client/feature/ime_feature.cc:6: #include <string>
On 2016/04/29 21:30:54, nyquist wrote:
> Nit: missing empty line here?

Done.

https://codereview.chromium.org/1929723002/diff/20001/blimp/test/blimp_browse...
File blimp/test/blimp_browser_test.cc (right):

https://codereview.chromium.org/1929723002/diff/20001/blimp/test/blimp_browse...
blimp/test/blimp_browser_test.cc:52:
command_line->AppendSwitch(::switches::kUseRemoteCompositing);
On 2016/04/29 21:30:54, nyquist wrote:
> Could you file a bug that we should move these to the engine startup instead
of
> having them both here and in the shell script that starts the engine?

pull engine command line config out of blimp/engine/app/blimp_main.cc and use it
here.

[Brian Goldman, 2016-05-02 16:48:58]

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/BUILD.gn
File blimp/test/BUILD.gn (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/BUILD.gn#new...
blimp/test/BUILD.gn:20: defines = [ "HAS_OUT_OF_PROC_TEST_RUNNER" ]
For my edification: what does this do?

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
File blimp/test/blimp_browser_test.cc (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
blimp/test/blimp_browser_test.cc:47:
command_line->AppendSwitchASCII(blimp::switches::kEnginePort, "25567");
Is this port guaranteed available on the environments where this test runs? I'm
imagining cases where, like, a test segfaults or something and socket cleanup
doesn't happen before you re-run it. Dynamic port assignment seems safer, but I
don't know if Blimp is currently set up for it.

[haibinlu, 2016-05-02 18:36:27]

PTAL

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/BUILD.gn
File blimp/test/BUILD.gn (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/BUILD.gn#new...
blimp/test/BUILD.gn:20: defines = [ "HAS_OUT_OF_PROC_TEST_RUNNER" ]
On 2016/05/02 16:48:58, Brian Goldman wrote:
> For my edification: what does this do?

It is used in content/public/test/browser_test.h so that we have
IN_PROC_BROWSER_TEST_F.

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
File blimp/test/blimp_browser_test.cc (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
blimp/test/blimp_browser_test.cc:47:
command_line->AppendSwitchASCII(blimp::switches::kEnginePort, "25567");
On 2016/05/02 16:48:58, Brian Goldman wrote:
> Is this port guaranteed available on the environments where this test runs?
I'm
> imagining cases where, like, a test segfaults or something and socket cleanup
> doesn't happen before you re-run it. Dynamic port assignment seems safer, but
I
> don't know if Blimp is currently set up for it.

It should be. we have been using static port test in
blimp/net/tcp_transport_unittest.cc (similar to
net/socket/tcp_server_socket_unittest.cc), no issue.

[nyquist, 2016-05-02 19:00:41]

lgtm

[Brian Goldman, 2016-05-02 19:02:20]

lgtm

[haibinlu, 2016-05-02 19:03:55]

haibinlu@chromium.org changed reviewers:
+ danakj@chromium.org, pkasting@chromium.org, sievers@chromium.org

[haibinlu, 2016-05-02 19:03:56]

danakj@, pkasting@, sievers@, Could I have your owners' review of
blimp/test/DEPS? Thx!

[Kevin M, 2016-05-02 19:05:42]

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

[Kevin M, 2016-05-02 19:05:42]

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

[Kevin M, 2016-05-02 19:05:42]

Adding myself to reviewers list, since I'm back :)

[danakj, 2016-05-02 19:58:26]

deps lgtm

[Kevin M, 2016-05-02 21:32:40]

The scope of this CL is really broad - it touches a lot of files. Can you break
this into multiple CLs with lists of bullet-point work items for clarity?

https://codereview.chromium.org/1929723002/diff/40001/blimp/client/BUILD.gn
File blimp/client/BUILD.gn (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:26: ":feature",
Why is this needed?

https://codereview.chromium.org/1929723002/diff/40001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:53: ":feature",
Not needed

https://codereview.chromium.org/1929723002/diff/40001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:159: source_set("feature_compositor") {
Why this name? Is this a capital-F Feature in the Blimp sense?

https://codereview.chromium.org/1929723002/diff/40001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:218: ":feature",
Already pulled in indirectly by feature_compositor?

https://codereview.chromium.org/1929723002/diff/40001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:428: ":feature",
Not needed

https://codereview.chromium.org/1929723002/diff/40001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_engine_config.cc (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_engine_config.cc:30: if
(cmd_line.HasSwitch(kClientToken)) {
As this is just a test hook, I don't think it's appropriate to add it as a flag.
We chose not to spin up processes running with sensitive data in argv, and it
seems like we shouldn't add logic to enable that. 

Can we plumb it into the client session object, instead?

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/BUILD.gn
File blimp/test/BUILD.gn (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/BUILD.gn#new...
blimp/test/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All rights
reserved.
We should probably put engine-specific tests under engine/test.

This is a common location that will get super cluttered in no time with client
and engine stuff.

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/BUILD.gn#new...
blimp/test/BUILD.gn:5: import("//build/config/features.gni")
Needed?

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/BUILD.gn#new...
blimp/test/BUILD.gn:6: import("//build/config/sanitizers/sanitizers.gni")
?

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/BUILD.gn#new...
blimp/test/BUILD.gn:7: import("//build/config/ui.gni")
?

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
File blimp/test/blimp_browser_test.cc (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
blimp/test/blimp_browser_test.cc:25: base::FilePath
test_data(FILE_PATH_LITERAL("blimp/engine/test/data"));
Path should be a const char* kConstant

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
blimp/test/blimp_browser_test.cc:47:
command_line->AppendSwitchASCII(blimp::switches::kEnginePort, "25567");
On 2016/05/02 18:36:27, haibinlu wrote:
> On 2016/05/02 16:48:58, Brian Goldman wrote:
> > Is this port guaranteed available on the environments where this test runs?
> I'm
> > imagining cases where, like, a test segfaults or something and socket
cleanup
> > doesn't happen before you re-run it. Dynamic port assignment seems safer,
but
> I
> > don't know if Blimp is currently set up for it.
> 
> It should be. we have been using static port test in
> blimp/net/tcp_transport_unittest.cc (similar to
> net/socket/tcp_server_socket_unittest.cc), no issue.

I think this introduces a flakiness risk for the waterfall test runners. We
won't be able to run the same test suite on the same runner simultaneously if
they are contending for the same port no.

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
blimp/test/blimp_browser_test.cc:49: // Engine switches
I don't think  the engine and client should be sharing the same set of command
line arguments like this. It'll fall apart if the client and engine need to set
different values on a given argument.

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
File blimp/test/blimp_browser_test.h (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
blimp/test/blimp_browser_test.h:21: // Base class for tests which require a full
instance of the blimp engine.
Capitalize Blimp

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
blimp/test/blimp_browser_test.h:26: void FinishAsynchronousTest();
Make this protected?

Call it "QuitRunLoop"? This isn't finishing the test, either - the testing::Test
does that.

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
blimp/test/blimp_browser_test.h:41: // Run an asynchronous test in a nested run
loop. The caller should call
Put this before the overrides

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
blimp/test/blimp_browser_test.h:43: void RunAsynchronousTest();
This isn't running the test - it's running the product code :)

RunUntilQuit?

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_test_l...
File blimp/test/blimp_test_launcher.cc (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_test_l...
blimp/test/blimp_test_launcher.cc:18: class BlimpTestLauncherDelegate : public
content::TestLauncherDelegate {
If this is engine-specific, we should have an engine-specific name and parent
dir path

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/engine_brows...
File blimp/test/engine_browsertest.cc (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/engine_brows...
blimp/test/engine_browsertest.cc:59: client_session_->Connect(std::string());
Add a new constructor for test connection parameters here, rather than using
flags?

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/test_client.cc
File blimp/test/test_client.cc (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/test_client....
blimp/test/test_client.cc:5: #include "blimp/test/test_client.h"
test_client_session.h ? Put this in client/test?

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/test_client.h
File blimp/test/test_client.h (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/test_client....
blimp/test/test_client.h:31: class MockNavigationFeatureDelegate
Put mock class definitions in .h files side-by-side with the .h that they mock

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/test_client....
blimp/test/test_client.h:37: MOCK_METHOD2(OnUrlChanged, void(int tab_id, const
GURL& url));
nit: Mocked parameter names aren't needed

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/test_client....
blimp/test/test_client.h:53: MOCK_METHOD2(OnCompositorMessageReceivedPtr,
nit: MockableOnCompositorMessageReceived for consistency with other Blimp mocks?

[Peter Kasting, 2016-05-03 00:51:07]

On 2016/05/02 19:03:56, haibinlu wrote:
> danakj@, pkasting@, sievers@, Could I have your owners' review of
> blimp/test/DEPS? Thx!

I don't think you need my signoff?  Nothing in your DEPS file is something I'm
an OWNER of AFAICT.

[haibinlu, 2016-05-03 19:32:18]

Description was changed from

==========
[Blimp] Adds blimp engine browser test framework and LoadUrl test.

It uses a headless Client (client_session with all features and mock feature
delegates) to work with a full Engine listening on a local tcp port.

It expects certain messages received via these mock delegates when it sends
messages to Engine.

BUG=599184
==========

to

==========
[Blimp] Adds blimp engine browser test framework and LoadUrl test.

It uses a headless Client (client_session with all features and mock feature
delegates) to work with a full Engine listening on a local tcp port.

It expects certain messages received via these mock delegates when it sends
messages to Engine.

other changes:
1. consolidate client's mock feature delegates with existing unit tests.
2. pull common engine command line configs into blimp::engine::SetUpCommandLine.


BUG=599184
==========

[haibinlu, 2016-05-03 19:32:44]

Kevin, PTAL

https://codereview.chromium.org/1929723002/diff/40001/blimp/client/BUILD.gn
File blimp/client/BUILD.gn (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:26: ":feature",
On 2016/05/02 21:32:39, Kevin M wrote:
> Why is this needed?

blimp_client_session.cc has
   #include "blimp/client/feature/ime_feature.h"

"gn check out/NoX11 //blimp/*" requires it.

https://codereview.chromium.org/1929723002/diff/40001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:53: ":feature",
On 2016/05/02 21:32:39, Kevin M wrote:
> Not needed

Done.

https://codereview.chromium.org/1929723002/diff/40001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:159: source_set("feature_compositor") {
On 2016/05/02 21:32:39, Kevin M wrote:
> Why this name? Is this a capital-F Feature in the Blimp sense?

Tommy suggested the name. 

What is the gn naming convention?

https://codereview.chromium.org/1929723002/diff/40001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:218: ":feature",
On 2016/05/02 21:32:39, Kevin M wrote:
> Already pulled in indirectly by feature_compositor?

Done.

https://codereview.chromium.org/1929723002/diff/40001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:428: ":feature",
On 2016/05/02 21:32:39, Kevin M wrote:
> Not needed

Done.

https://codereview.chromium.org/1929723002/diff/40001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_engine_config.cc (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_engine_config.cc:30: if
(cmd_line.HasSwitch(kClientToken)) {
On 2016/05/02 21:32:39, Kevin M wrote:
> As this is just a test hook, I don't think it's appropriate to add it as a
flag.
> We chose not to spin up processes running with sensitive data in argv, and it
> seems like we shouldn't add logic to enable that. 
> 
> Can we plumb it into the client session object, instead?

reverted. per offline discussion, added a data file for client token.

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/BUILD.gn
File blimp/test/BUILD.gn (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/BUILD.gn#new...
blimp/test/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All rights
reserved.
On 2016/05/02 21:32:39, Kevin M wrote:
> We should probably put engine-specific tests under engine/test.
> 
> This is a common location that will get super cluttered in no time with client
> and engine stuff.

this browser test uses both engine and client module. We do not want engine to
depend on client.

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/BUILD.gn#new...
blimp/test/BUILD.gn:5: import("//build/config/features.gni")
On 2016/05/02 21:32:39, Kevin M wrote:
> Needed?

removed

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/BUILD.gn#new...
blimp/test/BUILD.gn:6: import("//build/config/sanitizers/sanitizers.gni")
On 2016/05/02 21:32:39, Kevin M wrote:
> ?


removed

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/BUILD.gn#new...
blimp/test/BUILD.gn:7: import("//build/config/ui.gni")
On 2016/05/02 21:32:39, Kevin M wrote:
> ?


removed

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
File blimp/test/blimp_browser_test.cc (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
blimp/test/blimp_browser_test.cc:25: base::FilePath
test_data(FILE_PATH_LITERAL("blimp/engine/test/data"));
On 2016/05/02 21:32:39, Kevin M wrote:
> Path should be a const char* kConstant

Done.

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
blimp/test/blimp_browser_test.cc:47:
command_line->AppendSwitchASCII(blimp::switches::kEnginePort, "25567");
On 2016/05/02 21:32:39, Kevin M wrote:
> On 2016/05/02 18:36:27, haibinlu wrote:
> > On 2016/05/02 16:48:58, Brian Goldman wrote:
> > > Is this port guaranteed available on the environments where this test
runs?
> > I'm
> > > imagining cases where, like, a test segfaults or something and socket
> cleanup
> > > doesn't happen before you re-run it. Dynamic port assignment seems safer,
> but
> > I
> > > don't know if Blimp is currently set up for it.
> > 
> > It should be. we have been using static port test in
> > blimp/net/tcp_transport_unittest.cc (similar to
> > net/socket/tcp_server_socket_unittest.cc), no issue.
> 
> I think this introduces a flakiness risk for the waterfall test runners. We
> won't be able to run the same test suite on the same runner simultaneously if
> they are contending for the same port no.

ah, my bad. both tcp_transport_unittest and tcp_server_socket_unittest use
dynamically allocated port. changed to get available local port.

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
blimp/test/blimp_browser_test.cc:49: // Engine switches
On 2016/05/02 21:32:39, Kevin M wrote:
> I don't think  the engine and client should be sharing the same set of command
> line arguments like this. It'll fall apart if the client and engine need to
set
> different values on a given argument.

what is the alternative way to provide these args?

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
File blimp/test/blimp_browser_test.h (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
blimp/test/blimp_browser_test.h:21: // Base class for tests which require a full
instance of the blimp engine.
On 2016/05/02 21:32:40, Kevin M wrote:
> Capitalize Blimp

Done.

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
blimp/test/blimp_browser_test.h:26: void FinishAsynchronousTest();
On 2016/05/02 21:32:40, Kevin M wrote:
> Make this protected?
> 
> Call it "QuitRunLoop"? This isn't finishing the test, either - the
testing::Test
> does that.

Done.

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
blimp/test/blimp_browser_test.h:41: // Run an asynchronous test in a nested run
loop. The caller should call
On 2016/05/02 21:32:40, Kevin M wrote:
> Put this before the overrides

Done.

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_browse...
blimp/test/blimp_browser_test.h:43: void RunAsynchronousTest();
On 2016/05/02 21:32:40, Kevin M wrote:
> This isn't running the test - it's running the product code :)
> 
> RunUntilQuit?

Done.

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_test_l...
File blimp/test/blimp_test_launcher.cc (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/blimp_test_l...
blimp/test/blimp_test_launcher.cc:18: class BlimpTestLauncherDelegate : public
content::TestLauncherDelegate {
On 2016/05/02 21:32:40, Kevin M wrote:
> If this is engine-specific, we should have an engine-specific name and parent
> dir path

standard test launcher. It just happens to launch a full engine. per offline
discussion, move these files to browser_tests/ subfolder.

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/engine_brows...
File blimp/test/engine_browsertest.cc (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/engine_brows...
blimp/test/engine_browsertest.cc:59: client_session_->Connect(std::string());
On 2016/05/02 21:32:40, Kevin M wrote:
> Add a new constructor for test connection parameters here, rather than using
> flags?

do you mean command line flags?

getting values from command line is spreading out in different parts of code.
how can one achieve that via test-specific constructors?

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/test_client.cc
File blimp/test/test_client.cc (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/test_client....
blimp/test/test_client.cc:5: #include "blimp/test/test_client.h"
On 2016/05/02 21:32:40, Kevin M wrote:
> test_client_session.h ? Put this in client/test?

Done. put it under client/session

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/test_client.h
File blimp/test/test_client.h (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/test_client....
blimp/test/test_client.h:31: class MockNavigationFeatureDelegate
On 2016/05/02 21:32:40, Kevin M wrote:
> Put mock class definitions in .h files side-by-side with the .h that they mock

Done.

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/test_client....
blimp/test/test_client.h:37: MOCK_METHOD2(OnUrlChanged, void(int tab_id, const
GURL& url));
On 2016/05/02 21:32:40, Kevin M wrote:
> nit: Mocked parameter names aren't needed

much easier to read with the parameter names.

https://codereview.chromium.org/1929723002/diff/40001/blimp/test/test_client....
blimp/test/test_client.h:53: MOCK_METHOD2(OnCompositorMessageReceivedPtr,
On 2016/05/02 21:32:40, Kevin M wrote:
> nit: MockableOnCompositorMessageReceived for consistency with other Blimp
mocks?

Done.

[haibinlu, 2016-05-03 20:23:01]

pkasting@, you are the owner of ui/native_theme. sorry, I forgot to mention
change to blimp/engine/DEPS. We are using a switch (kEnableOverlayScrollbar)
defined in "ui/native_theme/native_theme_switches.h"

[Kevin M, 2016-05-03 20:29:34]

https://codereview.chromium.org/1929723002/diff/40001/blimp/client/BUILD.gn
File blimp/client/BUILD.gn (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:26: ":feature",
On 2016/05/03 19:32:42, haibinlu wrote:
> On 2016/05/02 21:32:39, Kevin M wrote:
> > Why is this needed?
> 
> blimp_client_session.cc has
>    #include "blimp/client/feature/ime_feature.h"
> 
> "gn check out/NoX11 //blimp/*" requires it.

Acknowledged.

https://codereview.chromium.org/1929723002/diff/40001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:159: source_set("feature_compositor") {
On 2016/05/03 19:32:42, haibinlu wrote:
> On 2016/05/02 21:32:39, Kevin M wrote:
> > Why this name? Is this a capital-F Feature in the Blimp sense?
> 
> Tommy suggested the name. 
> 
> What is the gn naming convention?

It's not that it's a bad name for GN, I just think it's a misleading prefix
because it doesn't reference any source files that fit the Blimp definition of a
feature.

I really think we should choose a different name, e.g. simply "compositor". I
don't think that the fact that they're nested under "feature" requires that we
expose that detail in the GN rule name.

https://codereview.chromium.org/1929723002/diff/60001/blimp/test/browser_test...
File blimp/test/browser_tests/blimp_browser_test.cc (right):

https://codereview.chromium.org/1929723002/diff/60001/blimp/test/browser_test...
blimp/test/browser_tests/blimp_browser_test.cc:30: const char* kTestDataFilePath
= "blimp/engine/test/data";
const char kTestDataFilePath[]

https://codereview.chromium.org/1929723002/diff/60001/blimp/test/browser_test...
blimp/test/browser_tests/blimp_browser_test.cc:31: const char*
kClientTokenFilePath = "blimp/test/data/test_client_token";
ditto

https://codereview.chromium.org/1929723002/diff/60001/blimp/test/browser_test...
File blimp/test/browser_tests/engine_browsertest.cc (right):

https://codereview.chromium.org/1929723002/diff/60001/blimp/test/browser_test...
blimp/test/browser_tests/engine_browsertest.cc:71: 
Attach a mocked request handler to the embedded test server to ensure that the
engine is retrieving the page?

[Peter Kasting, 2016-05-03 20:41:32]

DEPS change LGTM

[haibinlu, 2016-05-03 20:51:57]

haibinlu@chromium.org changed reviewers:
+ mmenke@chromium.org

[haibinlu, 2016-05-03 20:51:59]

mmenke@, could I have your owner's review of blimp/test/DEPS? Thanks!

https://codereview.chromium.org/1929723002/diff/40001/blimp/client/BUILD.gn
File blimp/client/BUILD.gn (right):

https://codereview.chromium.org/1929723002/diff/40001/blimp/client/BUILD.gn#n...
blimp/client/BUILD.gn:159: source_set("feature_compositor") {
On 2016/05/03 20:29:34, Kevin M wrote:
> On 2016/05/03 19:32:42, haibinlu wrote:
> > On 2016/05/02 21:32:39, Kevin M wrote:
> > > Why this name? Is this a capital-F Feature in the Blimp sense?
> > 
> > Tommy suggested the name. 
> > 
> > What is the gn naming convention?
> 
> It's not that it's a bad name for GN, I just think it's a misleading prefix
> because it doesn't reference any source files that fit the Blimp definition of
a
> feature.
> 
> I really think we should choose a different name, e.g. simply "compositor". I
> don't think that the fact that they're nested under "feature" requires that we
> expose that detail in the GN rule name.

Done.

https://codereview.chromium.org/1929723002/diff/60001/blimp/test/browser_test...
File blimp/test/browser_tests/blimp_browser_test.cc (right):

https://codereview.chromium.org/1929723002/diff/60001/blimp/test/browser_test...
blimp/test/browser_tests/blimp_browser_test.cc:30: const char* kTestDataFilePath
= "blimp/engine/test/data";
On 2016/05/03 20:29:34, Kevin M wrote:
> const char kTestDataFilePath[]

Done.

https://codereview.chromium.org/1929723002/diff/60001/blimp/test/browser_test...
blimp/test/browser_tests/blimp_browser_test.cc:31: const char*
kClientTokenFilePath = "blimp/test/data/test_client_token";
On 2016/05/03 20:29:34, Kevin M wrote:
> ditto

Done.

https://codereview.chromium.org/1929723002/diff/60001/blimp/test/browser_test...
File blimp/test/browser_tests/engine_browsertest.cc (right):

https://codereview.chromium.org/1929723002/diff/60001/blimp/test/browser_test...
blimp/test/browser_tests/engine_browsertest.cc:71: 
On 2016/05/03 20:29:34, Kevin M wrote:
> Attach a mocked request handler to the embedded test server to ensure that the
> engine is retrieving the page?

Done.

[Kevin M, 2016-05-03 21:11:59]

lgtm

[mmenke, 2016-05-03 21:21:53]

https://codereview.chromium.org/1929723002/diff/80001/blimp/test/browser_test...
File blimp/test/browser_tests/blimp_browser_test.cc (right):

https://codereview.chromium.org/1929723002/diff/80001/blimp/test/browser_test...
blimp/test/browser_tests/blimp_browser_test.cc:35: net::TCPServerSocket
sock(NULL, source);
nit:  nullptr.  Can also just inline net::NetLog::Source(), if you want.

https://codereview.chromium.org/1929723002/diff/80001/blimp/test/browser_test...
blimp/test/browser_tests/blimp_browser_test.cc:37: net::OK);
This is not a robust way to find a local port.  Something could have started
using it by the time you tear down the socket and start trying to use it.

Not sure how the code is wired up, but you're better off passing the socket
directly, or using an ephemeral port directly.

https://codereview.chromium.org/1929723002/diff/80001/blimp/test/browser_test...
File blimp/test/browser_tests/blimp_browser_test.h (right):

https://codereview.chromium.org/1929723002/diff/80001/blimp/test/browser_test...
blimp/test/browser_tests/blimp_browser_test.h:9: #include
"content/public/test/browser_test_base.h"
nit:  Blank line between standard and chrome headers.

[haibinlu, 2016-05-04 21:55:23]

mmenke@, kmarshall@, PTAL.

sievers@, could I get your owners' approval for the DEPS file?

https://codereview.chromium.org/1929723002/diff/80001/blimp/test/browser_test...
File blimp/test/browser_tests/blimp_browser_test.cc (right):

https://codereview.chromium.org/1929723002/diff/80001/blimp/test/browser_test...
blimp/test/browser_tests/blimp_browser_test.cc:35: net::TCPServerSocket
sock(NULL, source);
On 2016/05/03 21:21:53, mmenke wrote:
> nit:  nullptr.  Can also just inline net::NetLog::Source(), if you want.

removed

https://codereview.chromium.org/1929723002/diff/80001/blimp/test/browser_test...
blimp/test/browser_tests/blimp_browser_test.cc:37: net::OK);
On 2016/05/03 21:21:53, mmenke wrote:
> This is not a robust way to find a local port.  Something could have started
> using it by the time you tear down the socket and start trying to use it.
> 
> Not sure how the code is wired up, but you're better off passing the socket
> directly, or using an ephemeral port directly.

Changed to let Engine (similar to content shell) allocate dynamic port and hold
on to it. Client then uses that port to connect.

https://codereview.chromium.org/1929723002/diff/80001/blimp/test/browser_test...
File blimp/test/browser_tests/blimp_browser_test.h (right):

https://codereview.chromium.org/1929723002/diff/80001/blimp/test/browser_test...
blimp/test/browser_tests/blimp_browser_test.h:9: #include
"content/public/test/browser_test_base.h"
On 2016/05/03 21:21:53, mmenke wrote:
> nit:  Blank line between standard and chrome headers.

Done.

[mmenke, 2016-05-05 14:45:01]

LGTM, from a network standpoint, but not sure the new code is threadsafe.

https://codereview.chromium.org/1929723002/diff/100001/blimp/test/DEPS
File blimp/test/DEPS (right):

https://codereview.chromium.org/1929723002/diff/100001/blimp/test/DEPS#newcode6
blimp/test/DEPS:6: "+net",
Do you still need this?  I'm fine with it, just not sure it's still needed.

https://codereview.chromium.org/1929723002/diff/100001/blimp/test/browser_tes...
File blimp/test/browser_tests/blimp_browser_test.cc (right):

https://codereview.chromium.org/1929723002/diff/100001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:33: }  // namespace
nit:  should be consistent here - either a blank line both after the start and
before the end of the anonymous namespace, or in neither spot.

https://codereview.chromium.org/1929723002/diff/100001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:59: blimp::switches::kEnginePort,
std::to_string(port));
Modifying the command line after we've started other threads doesn't look
threadsafe to me. 
https://code.google.com/p/chromium/codesearch#chromium/src/base/command_line....

[haibinlu, 2016-05-05 17:30:17]

haibinlu@chromium.org changed reviewers:
+ piman@chromium.org

[haibinlu, 2016-05-05 17:30:18]

piman@, could I get your owners approval of the DEPS files updated here? Thanks!

https://codereview.chromium.org/1929723002/diff/100001/blimp/test/DEPS
File blimp/test/DEPS (right):

https://codereview.chromium.org/1929723002/diff/100001/blimp/test/DEPS#newcode6
blimp/test/DEPS:6: "+net",
On 2016/05/05 14:45:01, mmenke wrote:
> Do you still need this?  I'm fine with it, just not sure it's still needed.

I'd like to keep it here, because actual blimp browser tests will likely some
net classes.

[haibinlu, 2016-05-05 18:06:30]

https://codereview.chromium.org/1929723002/diff/100001/blimp/test/DEPS
File blimp/test/DEPS (right):

https://codereview.chromium.org/1929723002/diff/100001/blimp/test/DEPS#newcode6
blimp/test/DEPS:6: "+net",
On 2016/05/05 17:30:18, haibinlu wrote:
> On 2016/05/05 14:45:01, mmenke wrote:
> > Do you still need this?  I'm fine with it, just not sure it's still needed.
> 
> I'd like to keep it here, because actual blimp browser tests will likely some
> net classes.

back to using "net" in the test.

https://codereview.chromium.org/1929723002/diff/100001/blimp/test/browser_tes...
File blimp/test/browser_tests/blimp_browser_test.cc (right):

https://codereview.chromium.org/1929723002/diff/100001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:33: }  // namespace
On 2016/05/05 14:45:01, mmenke wrote:
> nit:  should be consistent here - either a blank line both after the start and
> before the end of the anonymous namespace, or in neither spot.

Done.

https://codereview.chromium.org/1929723002/diff/100001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:59: blimp::switches::kEnginePort,
std::to_string(port));
On 2016/05/05 14:45:01, mmenke wrote:
> Modifying the command line after we've started other threads doesn't look
> threadsafe to me. 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/command_line....

Thanks for catching another bug!  

Changed to not updating command line here. Client, which starts after
SetUpOnMainThread, will use assignment directly without consulting command line.

[haibinlu, 2016-05-05 18:09:09]

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

[commit-bot: I haz the power, 2016-05-05 18:09:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1929723002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1929723002/120001

[commit-bot: I haz the power, 2016-05-05 18:19:07]

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

[commit-bot: I haz the power, 2016-05-05 18:19:08]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[haibinlu, 2016-05-05 19:40:21]

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

[commit-bot: I haz the power, 2016-05-05 19:40:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1929723002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1929723002/140001

[commit-bot: I haz the power, 2016-05-05 19:47:27]

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

[commit-bot: I haz the power, 2016-05-05 19:47:29]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[haibinlu, 2016-05-05 19:57:57]

haibinlu@chromium.org changed reviewers:
- piman@chromium.org, sievers@chromium.org

[haibinlu, 2016-05-05 19:58:57]

No longer need ui/gl in DPES. Committing now. Thanks for the reviews!

[haibinlu, 2016-05-05 19:59:05]

The CQ bit was checked by haibinlu@chromium.org

[haibinlu, 2016-05-05 19:59:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
bgoldman@chromium.org, danakj@chromium.org, nyquist@chromium.org,
pkasting@chromium.org, kmarshall@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1929723002/#ps160001
(title: "update gn and deps")

[commit-bot: I haz the power, 2016-05-05 19:59:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1929723002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1929723002/160001

[Kevin M, 2016-05-05 20:01:02]

The CQ bit was unchecked by kmarshall@chromium.org

[Kevin M, 2016-05-05 20:28:52]

https://codereview.chromium.org/1929723002/diff/120001/blimp/client/BUILD.gn
File blimp/client/BUILD.gn (right):

https://codereview.chromium.org/1929723002/diff/120001/blimp/client/BUILD.gn#...
blimp/client/BUILD.gn:158: source_set("feature_test_support") {
You can make a "test_support" target that builds everything - little overhead
and cuts down on build target clutter.

https://codereview.chromium.org/1929723002/diff/120001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_content_main_delegate.h (right):

https://codereview.chromium.org/1929723002/diff/120001/blimp/engine/app/blimp...
blimp/engine/app/blimp_content_main_delegate.h:25: BlimpContentBrowserClient*
GetBlimpContentBrowserClient();
This is a simple getter, so how about "browser_client()"?

https://codereview.chromium.org/1929723002/diff/160001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_content_main_delegate.cc (right):

https://codereview.chromium.org/1929723002/diff/160001/blimp/engine/app/blimp...
blimp/engine/app/blimp_content_main_delegate.cc:35: BlimpContentMainDelegate*
g_content_main_delegate = nullptr;
I don't think we should be adding test-hook singletons into production code -
see my comment about making a separate test-only BlimpContentMainDelegate
subclass (in its own file) which will track its own global pointer stored in a
static variable.

The getter might look like this: TestBlimpContentMainDelegate::GetInstance();

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
File blimp/test/browser_tests/blimp_browser_test.cc (right):

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:35: CreateTestServer(test_data);
Just put FILE_PATH_LITERAL inline, no need to make it a variable. The name of
kTestDataFilePath is obvious.

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:43: BrowserTestBase::SetUp();
Does this need to come after the BlimpBrowserTest impl, or can it precede it?

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:55: // A test Client is started
after BlimpBrowserTest::SetUpOnMainThread()
lowercase C, trailing period, newline before comment

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:60: client::Assignment
BlimpBrowserTest::GetClientAssignment() {
We just call them "assignments", so how about GetAssignment?

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:82:
command_line->AppendSwitchASCII(::switches::kVModule, "blimp*=2");
This will make the test output very noisy. We shouldn't have verbosity turned up
by default (but we should allow the person running the tests to set vmodule
themselves)

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:84: // Engine switches
trailing period

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:86: // Use dynamically allocated
port for browser test.
Newline before comment

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:118: void
BlimpBrowserTest::QuitRunLoop() {
This function is too trivial, we should just make it inline.

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
File blimp/test/browser_tests/blimp_test_launcher.cc (right):

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_test_launcher.cc:17: 
Create a TestBlimpContentMainDelegate, which subclasses BlimpContentMainDelegate
and exposes a static instance getter?

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_test_launcher.cc:35:
content::ContentMainDelegate* CreateContentMainDelegate() override {
Why is this protected and other overrides public?

[piman, 2016-05-05 20:56:22]

piman@chromium.org changed reviewers:
+ piman@chromium.org

[piman, 2016-05-05 20:56:23]

lgtm for DEPS

[haibinlu, 2016-05-05 22:41:36]

Kevin, PTAL

https://codereview.chromium.org/1929723002/diff/120001/blimp/client/BUILD.gn
File blimp/client/BUILD.gn (right):

https://codereview.chromium.org/1929723002/diff/120001/blimp/client/BUILD.gn#...
blimp/client/BUILD.gn:158: source_set("feature_test_support") {
On 2016/05/05 20:28:51, Kevin M wrote:
> You can make a "test_support" target that builds everything - little overhead
> and cuts down on build target clutter.

Done.

https://codereview.chromium.org/1929723002/diff/120001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_content_main_delegate.h (right):

https://codereview.chromium.org/1929723002/diff/120001/blimp/engine/app/blimp...
blimp/engine/app/blimp_content_main_delegate.h:25: BlimpContentBrowserClient*
GetBlimpContentBrowserClient();
On 2016/05/05 20:28:51, Kevin M wrote:
> This is a simple getter, so how about "browser_client()"?

Done.

https://codereview.chromium.org/1929723002/diff/160001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_content_main_delegate.cc (right):

https://codereview.chromium.org/1929723002/diff/160001/blimp/engine/app/blimp...
blimp/engine/app/blimp_content_main_delegate.cc:35: BlimpContentMainDelegate*
g_content_main_delegate = nullptr;
On 2016/05/05 20:28:51, Kevin M wrote:
> I don't think we should be adding test-hook singletons into production code -
> see my comment about making a separate test-only BlimpContentMainDelegate
> subclass (in its own file) which will track its own global pointer stored in a
> static variable.
> 
> The getter might look like this: TestBlimpContentMainDelegate::GetInstance();

Done.

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
File blimp/test/browser_tests/blimp_browser_test.cc (right):

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:35: CreateTestServer(test_data);
On 2016/05/05 20:28:51, Kevin M wrote:
> Just put FILE_PATH_LITERAL inline, no need to make it a variable. The name of
> kTestDataFilePath is obvious.

Done.

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:43: BrowserTestBase::SetUp();
On 2016/05/05 20:28:51, Kevin M wrote:
> Does this need to come after the BlimpBrowserTest impl, or can it precede it?

No, it should be called after our command line is setup. BrowserTestBase::SetUp
processes command line.

All overrides of Setup method invokes baseclass SetUp after impl.

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:55: // A test Client is started
after BlimpBrowserTest::SetUpOnMainThread()
On 2016/05/05 20:28:51, Kevin M wrote:
> lowercase C, trailing period, newline before comment

Done.

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:60: client::Assignment
BlimpBrowserTest::GetClientAssignment() {
On 2016/05/05 20:28:51, Kevin M wrote:
> We just call them "assignments", so how about GetAssignment?

Done.

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:82:
command_line->AppendSwitchASCII(::switches::kVModule, "blimp*=2");
On 2016/05/05 20:28:51, Kevin M wrote:
> This will make the test output very noisy. We shouldn't have verbosity turned
up
> by default (but we should allow the person running the tests to set vmodule
> themselves)

removed

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:86: // Use dynamically allocated
port for browser test.
On 2016/05/05 20:28:51, Kevin M wrote:
> Newline before comment

Done.

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:118: void
BlimpBrowserTest::QuitRunLoop() {
On 2016/05/05 20:28:51, Kevin M wrote:
> This function is too trivial, we should just make it inline.

IN_PROC_BROWSER_TEST_F(EngineBrowserTest, LoadUrl) also uses it, and run_loop_
is private.

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
File blimp/test/browser_tests/blimp_test_launcher.cc (right):

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_test_launcher.cc:17: 
On 2016/05/05 20:28:51, Kevin M wrote:
> Create a TestBlimpContentMainDelegate, which subclasses
BlimpContentMainDelegate
> and exposes a static instance getter?

Done.

https://codereview.chromium.org/1929723002/diff/160001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_test_launcher.cc:35:
content::ContentMainDelegate* CreateContentMainDelegate() override {
On 2016/05/05 20:28:52, Kevin M wrote:
> Why is this protected and other overrides public?

moved it to public.

[haibinlu, 2016-05-05 22:41:47]

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

[commit-bot: I haz the power, 2016-05-05 22:42:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1929723002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1929723002/180001

[Kevin M, 2016-05-05 22:52:54]

lgtm % nits

https://codereview.chromium.org/1929723002/diff/180001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_engine_config.cc (right):

https://codereview.chromium.org/1929723002/diff/180001/blimp/engine/app/blimp...
blimp/engine/app/blimp_engine_config.cc:39: void
SetUpCommandLine(base::CommandLine* command_line) {
SetCommandLineDefaults?

https://codereview.chromium.org/1929723002/diff/180001/blimp/engine/session/b...
File blimp/engine/session/blimp_engine_session.h (right):

https://codereview.chromium.org/1929723002/diff/180001/blimp/engine/session/b...
blimp/engine/session/blimp_engine_session.h:70: using GetPortCallback =
base::Callback<void(uint16_t)>;
nit: make this a public member of BlimpEngineSession

https://codereview.chromium.org/1929723002/diff/180001/blimp/test/browser_tes...
File blimp/test/browser_tests/blimp_browser_test.cc (right):

https://codereview.chromium.org/1929723002/diff/180001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:55: // a test Client is started
after BlimpBrowserTest::SetUpOnMainThread().
Sorry, I was unclear. Capitalize "A" and lowercase "C"

[haibinlu, 2016-05-05 23:39:07]

The CQ bit was checked by haibinlu@chromium.org

[haibinlu, 2016-05-05 23:39:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, bgoldman@chromium.org, piman@chromium.org,
danakj@chromium.org, nyquist@chromium.org, mmenke@chromium.org,
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1929723002/#ps200001
(title: " ")

[haibinlu, 2016-05-05 23:39:25]

https://codereview.chromium.org/1929723002/diff/180001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_engine_config.cc (right):

https://codereview.chromium.org/1929723002/diff/180001/blimp/engine/app/blimp...
blimp/engine/app/blimp_engine_config.cc:39: void
SetUpCommandLine(base::CommandLine* command_line) {
On 2016/05/05 22:52:54, Kevin M wrote:
> SetCommandLineDefaults?

Done.

https://codereview.chromium.org/1929723002/diff/180001/blimp/engine/session/b...
File blimp/engine/session/blimp_engine_session.h (right):

https://codereview.chromium.org/1929723002/diff/180001/blimp/engine/session/b...
blimp/engine/session/blimp_engine_session.h:70: using GetPortCallback =
base::Callback<void(uint16_t)>;
On 2016/05/05 22:52:54, Kevin M wrote:
> nit: make this a public member of BlimpEngineSession

Done.

https://codereview.chromium.org/1929723002/diff/180001/blimp/test/browser_tes...
File blimp/test/browser_tests/blimp_browser_test.cc (right):

https://codereview.chromium.org/1929723002/diff/180001/blimp/test/browser_tes...
blimp/test/browser_tests/blimp_browser_test.cc:55: // a test Client is started
after BlimpBrowserTest::SetUpOnMainThread().
On 2016/05/05 22:52:54, Kevin M wrote:
> Sorry, I was unclear. Capitalize "A" and lowercase "C"

Done.

[commit-bot: I haz the power, 2016-05-05 23:39:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1929723002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1929723002/200001

[commit-bot: I haz the power, 2016-05-05 23:42:20]

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

[commit-bot: I haz the power, 2016-05-05 23:42:22]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[haibinlu, 2016-05-05 23:59:48]

The CQ bit was checked by haibinlu@chromium.org

[haibinlu, 2016-05-05 23:59:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, bgoldman@chromium.org, piman@chromium.org,
danakj@chromium.org, nyquist@chromium.org, kmarshall@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1929723002/#ps220001
(title: " ")

[commit-bot: I haz the power, 2016-05-06 00:00:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1929723002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1929723002/220001

[commit-bot: I haz the power, 2016-05-06 01:03:48]

Description was changed from

==========
[Blimp] Adds blimp engine browser test framework and LoadUrl test.

It uses a headless Client (client_session with all features and mock feature
delegates) to work with a full Engine listening on a local tcp port.

It expects certain messages received via these mock delegates when it sends
messages to Engine.

other changes:
1. consolidate client's mock feature delegates with existing unit tests.
2. pull common engine command line configs into blimp::engine::SetUpCommandLine.


BUG=599184
==========

to

==========
[Blimp] Adds blimp engine browser test framework and LoadUrl test.

It uses a headless Client (client_session with all features and mock feature
delegates) to work with a full Engine listening on a local tcp port.

It expects certain messages received via these mock delegates when it sends
messages to Engine.

other changes:
1. consolidate client's mock feature delegates with existing unit tests.
2. pull common engine command line configs into blimp::engine::SetUpCommandLine.


BUG=599184
==========

[commit-bot: I haz the power, 2016-05-06 01:03:51]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-05-06 01:05:49]

Description was changed from

==========
[Blimp] Adds blimp engine browser test framework and LoadUrl test.

It uses a headless Client (client_session with all features and mock feature
delegates) to work with a full Engine listening on a local tcp port.

It expects certain messages received via these mock delegates when it sends
messages to Engine.

other changes:
1. consolidate client's mock feature delegates with existing unit tests.
2. pull common engine command line configs into blimp::engine::SetUpCommandLine.


BUG=599184
==========

to

==========
[Blimp] Adds blimp engine browser test framework and LoadUrl test.

It uses a headless Client (client_session with all features and mock feature
delegates) to work with a full Engine listening on a local tcp port.

It expects certain messages received via these mock delegates when it sends
messages to Engine.

other changes:
1. consolidate client's mock feature delegates with existing unit tests.
2. pull common engine command line configs into blimp::engine::SetUpCommandLine.

BUG=599184

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

[commit-bot: I haz the power, 2016-05-06 01:05:51]

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