[Blimp] Refactor Blimp test engine with embedded test server and URL rewriting

[Blimp] Refactor Blimp test engine with embedded test server and URL rewriting

Create test infrastructure to do end to end page loading tests in client
side.

The framework mainly builds in the blimp engine code, and deals with url
getting by the help of embedded test server. The main idea is to create
a main() function in blimp/engine/testing/app which should look similar
to the one in blimp/engine/app, using mostly same blimp engine methods,
and overrides/adding functions for testing needs.

Components in the test infrastructure should take care of:
  - Set up embedded test server
  - Create URL handler to take care of magic URLs
  - Deliver related URL to test server and get full URL in server
  - Rewrite the magic URLs and should pass it to LoadUrl

Need to add the test engine main function to be executable in BUILD.gn;
should mark it as 'testonly = true' to not interrupt with the normal
blimp function.

BUG=656122
***
This CL fixed bug 656122. It won't be committed due to bug is marked as WontFix.
***

[shenghuazhang, 2016-12-13 02:14:19]

https://codereview.chromium.org/2572563006/diff/1/blimp/engine/testing/app/co...
File blimp/engine/testing/app/content_testing_main.cc (right):

https://codereview.chromium.org/2572563006/diff/1/blimp/engine/testing/app/co...
blimp/engine/testing/app/content_testing_main.cc:34:
DCHECK(blimp::engine::test::g_ets_instance->Start());
main_runner->Run() complains that Zygote should be the single thread, while
embedded test server is considered as another thread, so RunZygote()() failed.

Error message: https://paste.googleplex.com/6451644472492032

I think we could do this by inserting the code of starting the ets instance
after the ZygoteMain() in the main_runner->Run() process, possibly place is:

https://codesearch.chromium.org/chromium/src/content/app/content_main_runner....

But I think that costs much to override the serial of content runner classes to
reach that.

[jbudorick, 2016-12-13 18:31:03]

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

[jbudorick, 2016-12-13 18:31:03]

https://codereview.chromium.org/2572563006/diff/1/blimp/engine/testing/app/bl...
File blimp/engine/testing/app/blimp_browser_testing_main_parts.h (right):

https://codereview.chromium.org/2572563006/diff/1/blimp/engine/testing/app/bl...
blimp/engine/testing/app/blimp_browser_testing_main_parts.h:33: void
PreEarlyInitialization() override;
Could we set up the ETS in one of these two functions?

https://codereview.chromium.org/2572563006/diff/1/blimp/engine/testing/app/co...
File blimp/engine/testing/app/content_testing_main.cc (right):

https://codereview.chromium.org/2572563006/diff/1/blimp/engine/testing/app/co...
blimp/engine/testing/app/content_testing_main.cc:34:
DCHECK(blimp::engine::test::g_ets_instance->Start());
On 2016/12/13 02:14:19, shenghuazhang wrote:
> main_runner->Run() complains that Zygote should be the single thread, while
> embedded test server is considered as another thread, so RunZygote()() failed.
> 
> Error message: https://paste.googleplex.com/6451644472492032
> 
> I think we could do this by inserting the code of starting the ets instance
> after the ZygoteMain() in the main_runner->Run() process, possibly place is:
> 
>
https://codesearch.chromium.org/chromium/src/content/app/content_main_runner....
> 
> But I think that costs much to override the serial of content runner classes
to
> reach that.

We would definitely need to start the ETS instance after we spawn the zygote.
What about doing something w/ BrowserMainParts?

[shenghuazhang, 2017-01-05 23:36:33]

The test infra is functionally working. Two things need some suggestions:
1. The location of embedded_test_server shutting down code.
2. Contentclient and browser setting up for blimp_url_rewriter_unittest.cc

Have also commented the questions in the CL. Thanks!

https://codereview.chromium.org/2572563006/diff/1/blimp/engine/testing/app/bl...
File blimp/engine/testing/app/blimp_browser_testing_main_parts.h (right):

https://codereview.chromium.org/2572563006/diff/1/blimp/engine/testing/app/bl...
blimp/engine/testing/app/blimp_browser_testing_main_parts.h:33: void
PreEarlyInitialization() override;
On 2016/12/13 18:31:03, jbudorick wrote:
> Could we set up the ETS in one of these two functions?

This should be the correct place to set up the ETS. In next patch I would set it
up in PreMainMessageLoopRun(), right before SetEngineSessionForTesting starts.

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_browser_testing_main_parts.cc (right):

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_browser_testing_main_parts.cc:49: void
BlimpBrowserTestingMainParts::PreMainMessageLoopRun() {
I think the ets instance needs to be shut down in the end, but not sure where is
the best place to locate the shut down code in blimp code.

The shut down logic is probably within or after main_runner->Shutdown(), and it
includes the delegate process exit logic:
https://cs.chromium.org/chromium/src/content/app/content_main_runner.cc?sq=pa...

So I could think about 2 places for it:

1. Extend content_testing_main.cc for blimp and add code after
main_runner->Shutdown(). Like line 47 at 
https://codereview.chromium.org/2572563006/diff/1/blimp/engine/testing/app/co...

2. Override ProcessExiting in blimp_content_main_delegate.cc.

Which do you think is better, or should be somewhere else?

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_url_rewriter_unittest.cc (right):

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_url_rewriter_unittest.cc:36:
browser_client_.reset(new BlimpContentBrowserClient);
Failed to set up contentclient and browser causes
BrowserURLHandler::GetInstance() failed below.

Know that blimp engine delegate is responsible to do that, but not sure how to
set up this in the test Setup function:
https://cs.chromium.org/chromium/src/blimp/engine/app/blimp_content_main_dele...

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_url_rewriter_unittest.cc:52:
content::BrowserURLHandler::GetInstance()->AddHandlerPair(
Failed at GetInstance(). Due to failed to set up ContentClient and the
ContentBrowserClient. 

https://codesearch.chromium.org/chromium/src/content/browser/browser_url_hand...

[jbudorick at gmail, 2017-01-06 16:09:36]

jbudorick@gmail.com changed reviewers:
+ jbudorick@gmail.com

[jbudorick at gmail, 2017-01-06 16:09:37]

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/DEPS
File blimp/engine/DEPS (right):

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/DEPS#newco...
blimp/engine/DEPS:12: "+content/browser",
 I think depending on the nonpublic part of content/ is frowned upon.

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_content_browser_client.h (right):

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_content_browser_client.h:46: //
DISALLOW_COPY_AND_ASSIGN(BlimpContentBrowserClient);
You shouldn't have to comment this out.

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_content_main_delegate.h (right):

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_content_main_delegate.h:43: //
DISALLOW_COPY_AND_ASSIGN(BlimpContentMainDelegate);
Same.

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_browser_testing_main_parts.cc (right):

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_browser_testing_main_parts.cc:49: void
BlimpBrowserTestingMainParts::PreMainMessageLoopRun() {
On 2017/01/05 23:36:33, shenghuazhang wrote:
> I think the ets instance needs to be shut down in the end, but not sure where
is
> the best place to locate the shut down code in blimp code.
> 
> The shut down logic is probably within or after main_runner->Shutdown(), and
it
> includes the delegate process exit logic:
>
https://cs.chromium.org/chromium/src/content/app/content_main_runner.cc?sq=pa...
> 
> So I could think about 2 places for it:
> 
> 1. Extend content_testing_main.cc for blimp and add code after
> main_runner->Shutdown(). Like line 47 at 
>
https://codereview.chromium.org/2572563006/diff/1/blimp/engine/testing/app/co...
> 
> 2. Override ProcessExiting in blimp_content_main_delegate.cc.
> 
> Which do you think is better, or should be somewhere else?

What about PostMainMessageLoopRun in this class?

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_browser_testing_main_parts.cc:64:
std::move(browser_context), net_log_.get(), engine_config_.get(),
Should BlimpEngineTestingSession own the NetLog and the BlimpEngineConfig?

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_browser_testing_main_parts.cc:67:
BlimpBrowserMainParts::SetSettingsManagerForTesting(
seetings_manager_ and engine_session_ shouldn't be members of this class if
you're just passing them in to BlimpBrowserMainParts.

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_url_rewriter_unittest.cc (right):

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_url_rewriter_unittest.cc:36:
browser_client_.reset(new BlimpContentBrowserClient);
On 2017/01/05 23:36:33, shenghuazhang wrote:
> Failed to set up contentclient and browser causes
> BrowserURLHandler::GetInstance() failed below.
> 
> Know that blimp engine delegate is responsible to do that, but not sure how to
> set up this in the test Setup function:
>
https://cs.chromium.org/chromium/src/blimp/engine/app/blimp_content_main_dele...
> 

Can you set content_client_->browser e.g.
https://codesearch.chromium.org/chromium/src/content/app/content_main_runner....
?

[shenghuazhang, 2017-01-07 01:21:24]

Thanks for the hints!
Refactored blimp_url_rewriter_unittest.cc which runs succesfully now. Still have
questions around blimp_browser_testing_main_parts.cc, replied to your comments.

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/DEPS
File blimp/engine/DEPS (right):

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/DEPS#newco...
blimp/engine/DEPS:12: "+content/browser",
On 2017/01/06 16:09:36, jbudorick at gmail wrote:
>  I think depending on the nonpublic part of content/ is frowned upon.

Replace the #include "content/browser/browser_url_handler_impl.h" by
"content/public/browser/browser_url_handler.h" in blimp_url_rewriter_unittest.cc
and it wouldn't complain excluding this rule. Will remove it in the next patch.

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_content_browser_client.h (right):

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_content_browser_client.h:46: //
DISALLOW_COPY_AND_ASSIGN(BlimpContentBrowserClient);
On 2017/01/06 16:09:36, jbudorick at gmail wrote:
> You shouldn't have to comment this out.

Done.

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_content_main_delegate.h (right):

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_content_main_delegate.h:43: //
DISALLOW_COPY_AND_ASSIGN(BlimpContentMainDelegate);
On 2017/01/06 16:09:36, jbudorick at gmail wrote:
> Same.

Forgot to uncomment it back after debugging :P
Done.

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_browser_testing_main_parts.cc (right):

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_browser_testing_main_parts.cc:49: void
BlimpBrowserTestingMainParts::PreMainMessageLoopRun() {
On 2017/01/06 16:09:37, jbudorick at gmail wrote:
> On 2017/01/05 23:36:33, shenghuazhang wrote:
> > I think the ets instance needs to be shut down in the end, but not sure
where
> is
> > the best place to locate the shut down code in blimp code.
> > 
> > The shut down logic is probably within or after main_runner->Shutdown(), and
> it
> > includes the delegate process exit logic:
> >
>
https://cs.chromium.org/chromium/src/content/app/content_main_runner.cc?sq=pa...
> > 
> > So I could think about 2 places for it:
> > 
> > 1. Extend content_testing_main.cc for blimp and add code after
> > main_runner->Shutdown(). Like line 47 at 
> >
>
https://codereview.chromium.org/2572563006/diff/1/blimp/engine/testing/app/co...
> > 
> > 2. Override ProcessExiting in blimp_content_main_delegate.cc.
> > 
> > Which do you think is better, or should be somewhere else?
> 
> What about PostMainMessageLoopRun in this class?

Done.

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_browser_testing_main_parts.cc:64:
std::move(browser_context), net_log_.get(), engine_config_.get(),
On 2017/01/06 16:09:37, jbudorick at gmail wrote:
> Should BlimpEngineTestingSession own the NetLog and the BlimpEngineConfig?

I think it should since it should pass the parameters to the parent class
BlimpEngineSession which own NetLog and the BlimpEngineConfig? 
https://cs.chromium.org/chromium/src/blimp/engine/session/blimp_engine_sessio...

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_browser_testing_main_parts.cc:67:
BlimpBrowserMainParts::SetSettingsManagerForTesting(
On 2017/01/06 16:09:37, jbudorick at gmail wrote:
> seetings_manager_ and engine_session_ shouldn't be members of this class if
> you're just passing them in to BlimpBrowserMainParts.

Hmm... Do you mean this class shouldn't set up the settings_manager_ and
engine_session_ at all? Or shouldn't be set as class members in header:
  std::unique_ptr<SettingsManager> settings_manager_;
  std::unique_ptr<BlimpEngineSession> engine_session_;

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_url_rewriter_unittest.cc (right):

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_url_rewriter_unittest.cc:36:
browser_client_.reset(new BlimpContentBrowserClient);
On 2017/01/06 16:09:37, jbudorick at gmail wrote:
> On 2017/01/05 23:36:33, shenghuazhang wrote:
> > Failed to set up contentclient and browser causes
> > BrowserURLHandler::GetInstance() failed below.
> > 
> > Know that blimp engine delegate is responsible to do that, but not sure how
to
> > set up this in the test Setup function:
> >
>
https://cs.chromium.org/chromium/src/blimp/engine/app/blimp_content_main_dele...
> > 
> 
> Can you set content_client_->browser e.g.
>
https://codesearch.chromium.org/chromium/src/content/app/content_main_runner....
> ?

I'm afraid it's not able to do in this way, since the browser is a private
variable for ContentClient.

Just found an example which worked in this case:
https://codesearch.chromium.org/chromium/src/content/public/test/test_content...

[jbudorick at gmail, 2017-01-07 02:34:51]

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_browser_testing_main_parts.cc (right):

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_browser_testing_main_parts.cc:64:
std::move(browser_context), net_log_.get(), engine_config_.get(),
On 2017/01/07 01:21:24, shenghuazhang wrote:
> On 2017/01/06 16:09:37, jbudorick at gmail wrote:
> > Should BlimpEngineTestingSession own the NetLog and the BlimpEngineConfig?
> 
> I think it should since it should pass the parameters to the parent class
> BlimpEngineSession which own NetLog and the BlimpEngineConfig? 
>
https://cs.chromium.org/chromium/src/blimp/engine/session/blimp_engine_sessio...

Ah, I didn't look at BlimpEngineSession.

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_browser_testing_main_parts.cc:67:
BlimpBrowserMainParts::SetSettingsManagerForTesting(
On 2017/01/07 01:21:24, shenghuazhang wrote:
> On 2017/01/06 16:09:37, jbudorick at gmail wrote:
> > seetings_manager_ and engine_session_ shouldn't be members of this class if
> > you're just passing them in to BlimpBrowserMainParts.
> 
> Hmm... Do you mean this class shouldn't set up the settings_manager_ and
> engine_session_ at all? Or shouldn't be set as class members in header:
>   std::unique_ptr<SettingsManager> settings_manager_;
>   std::unique_ptr<BlimpEngineSession> engine_session_;

Shouldn't be in the header. It's fine if this method sets them up.

[shenghuazhang, 2017-01-09 19:23:46]

shenghuazhang@chromium.org changed reviewers:
+ jochen@chromium.org

[shenghuazhang, 2017-01-09 19:23:47]

Refactored by John's comment.

+jochen

Hi Jochen, here is a file name added in src/PRESUBMIT.py to unlock the banned
cpp function 'ScopedAllowIO', which is needed in
src/blimp/engine/testing/app/blimp_browser_testing_main_parts.c. (Line 43 of
https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...)

Could you please have a review on the src/PRESUBMIT.py? Thanks!

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_browser_testing_main_parts.cc (right):

https://codereview.chromium.org/2572563006/diff/20001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_browser_testing_main_parts.cc:67:
BlimpBrowserMainParts::SetSettingsManagerForTesting(
On 2017/01/07 02:34:51, jbudorick at gmail wrote:
> On 2017/01/07 01:21:24, shenghuazhang wrote:
> > On 2017/01/06 16:09:37, jbudorick at gmail wrote:
> > > seetings_manager_ and engine_session_ shouldn't be members of this class
if
> > > you're just passing them in to BlimpBrowserMainParts.
> > 
> > Hmm... Do you mean this class shouldn't set up the settings_manager_ and
> > engine_session_ at all? Or shouldn't be set as class members in header:
> >   std::unique_ptr<SettingsManager> settings_manager_;
> >   std::unique_ptr<BlimpEngineSession> engine_session_;
> 
> Shouldn't be in the header. It's fine if this method sets them up.

Make sense! Will refactor it.

[shenghuazhang, 2017-01-09 19:44:20]

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

[shenghuazhang, 2017-01-09 19:44:20]

+kmarshall Thanks Kevin to be the project reviewer of this CL!

[jbudorick, 2017-01-09 19:44:46]

Description was changed from

==========
[Blimp] Refactor Blimp test engine with embedded test server and URL rewriting

Create test infrastructure to do end to end page loading tests in client
side.

The framework mainly builds in the blimp engine code, and deals with url
getting by the help of embedded test server. The main idea is to create
a main() function in blimp/engine/testing/app which should look similar
to the one in blimp/engine/app, using mostly same blimp engine methods,
and overrides/adding functions for testing needs.

Components in the test infrastructure should take care of:
  - Set up embedded test server
  - Create URL handler to take care of magic URLs
  - Deliver related URL to test server and get full URL in server
  - Rewrite the magic URLs and should pass it to LoadUrl

Need to add the test engine main function to be executable in BUILD.gn;
should mark it as 'testonly = true' to not interrupt with the normal
blimp function.

BUG=656122
==========

to

==========
[Blimp] Refactor Blimp test engine with embedded test server and URL rewriting

Create test infrastructure to do end to end page loading tests in client
side.

The framework mainly builds in the blimp engine code, and deals with url
getting by the help of embedded test server. The main idea is to create
a main() function in blimp/engine/testing/app which should look similar
to the one in blimp/engine/app, using mostly same blimp engine methods,
and overrides/adding functions for testing needs.

Components in the test infrastructure should take care of:
  - Set up embedded test server
  - Create URL handler to take care of magic URLs
  - Deliver related URL to test server and get full URL in server
  - Rewrite the magic URLs and should pass it to LoadUrl

Need to add the test engine main function to be executable in BUILD.gn;
should mark it as 'testonly = true' to not interrupt with the normal
blimp function.

BUG=656122
==========

[jbudorick, 2017-01-09 19:44:46]

jbudorick@chromium.org changed reviewers:
- jbudorick@gmail.com

[Kevin M, 2017-01-09 20:17:18]

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_browser_main_parts.h (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_browser_main_parts.h:39: // content::BrowserMainParts
implementation.
Move this below the non-overrides

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_content_browser_client.h (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_content_browser_client.h:37: BlimpBrowserContext*
GetBrowserContext();
Move non-overrides above overrides.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_content_browser_client.h:39: void
SetBrowserMainPartsForTesting(
Should be protected?

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_content_main_delegate.h (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_content_main_delegate.h:33: void
SetContentBrowserClientForTesting(
Move above overrides

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_browser_testing_main_parts.cc (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_browser_testing_main_parts.cc:41: {
Remove unnecessary curly braces?

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_browser_testing_main_parts.cc:50:
blimp::engine::test::g_ets_instance.reset(new net::EmbeddedTestServer());
Why is this a global singleton? Can we bind an instance here and pass pointers
to it into handlers etc.?

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_browser_testing_main_parts.cc:59:
net_log_.reset(new net::NetLog());
nit: use base::MakeUnique<X>() instead of "new X"

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_browser_testing_main_parts.h (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_browser_testing_main_parts.h:31: // Override
content::BrowserMainParts implementation
Use standard comment boilerplate, e.g.

"// BlimpBrowserMainParts implementation."

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_content_browser_client_for_test.cc (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_content_browser_client_for_test.cc:17:
BlimpContentBrowserClient::SetBrowserMainPartsForTesting(
Is it necessary to qualify this call with the base class name?

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_content_browser_client_for_test.h (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_content_browser_client_for_test.h:15: class
BlimpContentBrowserClientForTest : public BlimpContentBrowserClient {
XYZForTest is more of a function naming convention that is needed for logic in
presubmits etc. Can we use TestXYZ instead?

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_content_browser_client_for_test.h:20: // Override
content::ContentBrowserClient implementation
int: "BlimpContentBrowserClient implementation"

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_content_main_delegate_for_test.cc (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_content_main_delegate_for_test.cc:7: #include
"blimp/engine/testing/app/blimp_content_browser_client_for_test.h"
Missing include on base/ptr_util.h ?

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_content_main_delegate_for_test.cc:8: #include
"blimp/engine/testing/app/blimp_content_main_delegate_for_test.h"
The include that corresponds to this CC should be at the very top (here and
other files in this CL)

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_content_main_delegate_for_test.cc:9: #include
"blimp/engine/testing/app/blimp_url_rewriter.h"
Not used?

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_content_main_delegate_for_test.cc:21:
BlimpContentMainDelegate::SetContentBrowserClientForTesting(
Remove "BlimpContentMainDelegate"?

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_content_main_delegate_for_test.h (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_content_main_delegate_for_test.h:9: #include
<string>
Neither of these are used

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_content_main_delegate_for_test.h:16: class
BlimpContentMainDelegateForTest : public BlimpContentMainDelegate {
Add a small comment about what this class does? Here and for other new classes
that you define in this CL

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_content_main_delegate_for_test.h:21: // Override
content::ContentMainDelegate implementation
BlimpContentMainDelegate implementation.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_url_rewriter.cc (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_url_rewriter.cc:25: bool HandleBlimpTestURL(GURL*
url, content::BrowserContext* browser_context) {
browser_context is never referenced, so can we remove BrowserContext includes
too?

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_url_rewriter.cc:32: if (url->is_valid() &&
url->SchemeIsHTTPOrHTTPS())
What about non-HTTP urls? Can you fall back to a default handler for
non-"blimp-test" URLs instead of hardcoding a whitelist?

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_url_rewriter.cc:43: if (rewritten_url.is_valid())
{
Assuming our code isn't broken, should this ever fail? Can we just use
DCHECK(rewritten_url.is_valid())  instead?

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_url_rewriter.h (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_url_rewriter.h:8: #include <string>
Not used?

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_url_rewriter.h:25: bool HandleBlimpTestURL(GURL*
url, content::BrowserContext* browser_context);
Add function comment.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_url_rewriter.h:26: }  // namespace test
nit: add whitespace before closing curly brace

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/se...
File blimp/engine/testing/session/blimp_engine_testing_session.cc (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/se...
blimp/engine/testing/session/blimp_engine_testing_session.cc:27: #include
"ui/gfx/geometry/size.h"
That's a lot of includes. :) Can you trim them down to the bare essentials and
use prototypes whenever possible?

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/se...
File blimp/engine/testing/session/blimp_engine_testing_session.h (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/se...
blimp/engine/testing/session/blimp_engine_testing_session.h:13: #include
"content/public/browser/browser_url_handler.h"
Not used?

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/se...
blimp/engine/testing/session/blimp_engine_testing_session.h:20: class
BlimpEngineTestingSession : public BlimpEngineSession {
nit: This should be implemented as a BlimpMessage rewriting filter instead of as
BlimpEngineSession. No need to fix this, it's too major, but IMO this is change
is too invasive for the task this class accomplishes.

[jochen (gone - plz use gerrit), 2017-01-10 09:48:18]

the referenced bug is marked as WontFix. Should this CL also be closed?

[shenghuazhang, 2017-01-10 23:20:34]

On 2017/01/10 09:48:18, jochen wrote:
> the referenced bug is marked as WontFix. Should this CL also be closed?

Yes the project code is supposed to be deleted. While the test infrastructure by
this CL which is about to be finished overcomes many technical difficulties, and
it's a good practice to accomplish it. So we are trying to commit it, at least
finish it to be a reference if won't commit it. Anyway the project code is about
to be removed soon, and the PRESUBMIT.py change will be also revoked.
Based on our offline talk, I understand your concern about working on the
short-lived code. I agree to not commit that file if wouldn't get your lgtm and
close this CL asap :)

[shenghuazhang, 2017-01-10 23:20:50]

Hi Kevin, thank you for the review! Refactored most of them. Please look at the
new patch.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_browser_main_parts.h (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_browser_main_parts.h:39: // content::BrowserMainParts
implementation.
On 2017/01/09 20:17:16, Kevin M wrote:
> Move this below the non-overrides

Done.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_browser_main_parts.h:50: void
SetSettingsManagerForTesting(
Set to be protected too.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_content_browser_client.h (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_content_browser_client.h:37: BlimpBrowserContext*
GetBrowserContext();
On 2017/01/09 20:17:17, Kevin M wrote:
> Move non-overrides above overrides.

Done.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_content_browser_client.h:39: void
SetBrowserMainPartsForTesting(
On 2017/01/09 20:17:16, Kevin M wrote:
> Should be protected?

Done.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_content_main_delegate.h (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_content_main_delegate.h:33: void
SetContentBrowserClientForTesting(
On 2017/01/09 20:17:17, Kevin M wrote:
> Move above overrides

Changed the scope to be protected instead.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_browser_testing_main_parts.cc (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_browser_testing_main_parts.cc:41: {
On 2017/01/09 20:17:17, Kevin M wrote:
> Remove unnecessary curly braces?

Done.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_browser_testing_main_parts.cc:50:
blimp::engine::test::g_ets_instance.reset(new net::EmbeddedTestServer());
On 2017/01/09 20:17:17, Kevin M wrote:
> Why is this a global singleton? Can we bind an instance here and pass pointers
> to it into handlers etc.?

The embedded_test_server instance is called by the url rewriter handler method
(HandleBlimpTestURL() in
https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...),
and it needs to be started once when the engine session starts.

And the HandleBlimpTestURL which is passed into the handler below should be the
same type of URLHandler
(https://codesearch.chromium.org/chromium/src/content/public/browser/browser_u...).
So the additional g_ets_instance may not be able to be passed into handler
directly.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_browser_testing_main_parts.cc:59:
net_log_.reset(new net::NetLog());
On 2017/01/09 20:17:17, Kevin M wrote:
> nit: use base::MakeUnique<X>() instead of "new X"

Done.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_browser_testing_main_parts.h (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_browser_testing_main_parts.h:31: // Override
content::BrowserMainParts implementation
On 2017/01/09 20:17:17, Kevin M wrote:
> Use standard comment boilerplate, e.g.
> 
> "// BlimpBrowserMainParts implementation."

Done.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_content_browser_client_for_test.cc (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_content_browser_client_for_test.cc:17:
BlimpContentBrowserClient::SetBrowserMainPartsForTesting(
On 2017/01/09 20:17:17, Kevin M wrote:
> Is it necessary to qualify this call with the base class name?
No need to specify it with base class name. Will remove it.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_content_browser_client_for_test.h (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_content_browser_client_for_test.h:15: class
BlimpContentBrowserClientForTest : public BlimpContentBrowserClient {
On 2017/01/09 20:17:17, Kevin M wrote:
> XYZForTest is more of a function naming convention that is needed for logic in
> presubmits etc. Can we use TestXYZ instead?

Done.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_content_browser_client_for_test.h:20: // Override
content::ContentBrowserClient implementation
On 2017/01/09 20:17:17, Kevin M wrote:
> int: "BlimpContentBrowserClient implementation"

Done.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_content_main_delegate_for_test.cc (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_content_main_delegate_for_test.cc:7: #include
"blimp/engine/testing/app/blimp_content_browser_client_for_test.h"
On 2017/01/09 20:17:17, Kevin M wrote:
> Missing include on base/ptr_util.h ?

Done.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_content_main_delegate_for_test.cc:8: #include
"blimp/engine/testing/app/blimp_content_main_delegate_for_test.h"
On 2017/01/09 20:17:17, Kevin M wrote:
> The include that corresponds to this CC should be at the very top (here and
> other files in this CL)

Done.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_content_main_delegate_for_test.cc:9: #include
"blimp/engine/testing/app/blimp_url_rewriter.h"
On 2017/01/09 20:17:17, Kevin M wrote:
> Not used?

Done.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_content_main_delegate_for_test.cc:21:
BlimpContentMainDelegate::SetContentBrowserClientForTesting(
On 2017/01/09 20:17:17, Kevin M wrote:
> Remove "BlimpContentMainDelegate"?

Done.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_content_main_delegate_for_test.h (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_content_main_delegate_for_test.h:9: #include
<string>
On 2017/01/09 20:17:18, Kevin M wrote:
> Neither of these are used

Done.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_content_main_delegate_for_test.h:16: class
BlimpContentMainDelegateForTest : public BlimpContentMainDelegate {
On 2017/01/09 20:17:18, Kevin M wrote:
> Add a small comment about what this class does? Here and for other new classes
> that you define in this CL

Done.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_content_main_delegate_for_test.h:21: // Override
content::ContentMainDelegate implementation
On 2017/01/09 20:17:17, Kevin M wrote:
> BlimpContentMainDelegate implementation.

Done.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_url_rewriter.cc (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_url_rewriter.cc:25: bool HandleBlimpTestURL(GURL*
url, content::BrowserContext* browser_context) {
On 2017/01/09 20:17:18, Kevin M wrote:
> browser_context is never referenced, so can we remove BrowserContext includes
> too?

I think it needs the content::BrowserContext included?

This would be called by BrowserURLHandler->AddHandlerPair (line 55 in
https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...).
Here is an example in content/browser:
https://codesearch.chromium.org/chromium/src/content/browser/browser_url_hand...

So the method HandleBlimpTestURL should have the same type of function with
URLHandler:
https://codesearch.chromium.org/chromium/src/content/public/browser/browser_u...

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_url_rewriter.cc:32: if (url->is_valid() &&
url->SchemeIsHTTPOrHTTPS())
On 2017/01/09 20:17:18, Kevin M wrote:
> What about non-HTTP urls? Can you fall back to a default handler for
> non-"blimp-test" URLs instead of hardcoding a whitelist?

Got it.
Return false for all conditions except for 'blimp-test' urls, BrowserUrlHandler
would find a handler that can run it by default.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_url_rewriter.cc:43: if (rewritten_url.is_valid())
{
On 2017/01/09 20:17:18, Kevin M wrote:
> Assuming our code isn't broken, should this ever fail? Can we just use
> DCHECK(rewritten_url.is_valid())  instead?

That makes sense. Would use DCHECK thing instead.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_url_rewriter.h (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_url_rewriter.h:8: #include <string>
On 2017/01/09 20:17:18, Kevin M wrote:
> Not used?

Done.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_url_rewriter.h:25: bool HandleBlimpTestURL(GURL*
url, content::BrowserContext* browser_context);
On 2017/01/09 20:17:18, Kevin M wrote:
> Add function comment.

Done.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_url_rewriter.h:26: }  // namespace test
On 2017/01/09 20:17:18, Kevin M wrote:
> nit: add whitespace before closing curly brace

Done.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/se...
File blimp/engine/testing/session/blimp_engine_testing_session.cc (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/se...
blimp/engine/testing/session/blimp_engine_testing_session.cc:27: #include
"ui/gfx/geometry/size.h"
On 2017/01/09 20:17:18, Kevin M wrote:
> That's a lot of includes. :) Can you trim them down to the bare essentials and
> use prototypes whenever possible?

Done.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/se...
File blimp/engine/testing/session/blimp_engine_testing_session.h (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/se...
blimp/engine/testing/session/blimp_engine_testing_session.h:13: #include
"content/public/browser/browser_url_handler.h"
On 2017/01/09 20:17:18, Kevin M wrote:
> Not used?

Done.

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/se...
blimp/engine/testing/session/blimp_engine_testing_session.h:20: class
BlimpEngineTestingSession : public BlimpEngineSession {
On 2017/01/09 20:17:18, Kevin M wrote:
> nit: This should be implemented as a BlimpMessage rewriting filter instead of
as
> BlimpEngineSession. No need to fix this, it's too major, but IMO this is
change
> is too invasive for the task this class accomplishes.

Acknowledged.

[Kevin M, 2017-01-11 00:28:14]

lgtm

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
File blimp/engine/testing/app/blimp_browser_testing_main_parts.cc (right):

https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...
blimp/engine/testing/app/blimp_browser_testing_main_parts.cc:50:
blimp::engine::test::g_ets_instance.reset(new net::EmbeddedTestServer());
On 2017/01/10 23:20:49, shenghuazhang wrote:
> On 2017/01/09 20:17:17, Kevin M wrote:
> > Why is this a global singleton? Can we bind an instance here and pass
pointers
> > to it into handlers etc.?
> 
> The embedded_test_server instance is called by the url rewriter handler method
> (HandleBlimpTestURL() in
>
https://codereview.chromium.org/2572563006/diff/60001/blimp/engine/testing/ap...),
> and it needs to be started once when the engine session starts.
> 
> And the HandleBlimpTestURL which is passed into the handler below should be
the
> same type of URLHandler
>
(https://codesearch.chromium.org/chromium/src/content/public/browser/browser_u...).
> So the additional g_ets_instance may not be able to be passed into handler
> directly.

Got it. I wonder why this a free function, and not an object, or a Callback?
Weird.

[shenghuazhang, 2017-01-11 19:43:19]

Description was changed from

==========
[Blimp] Refactor Blimp test engine with embedded test server and URL rewriting

Create test infrastructure to do end to end page loading tests in client
side.

The framework mainly builds in the blimp engine code, and deals with url
getting by the help of embedded test server. The main idea is to create
a main() function in blimp/engine/testing/app which should look similar
to the one in blimp/engine/app, using mostly same blimp engine methods,
and overrides/adding functions for testing needs.

Components in the test infrastructure should take care of:
  - Set up embedded test server
  - Create URL handler to take care of magic URLs
  - Deliver related URL to test server and get full URL in server
  - Rewrite the magic URLs and should pass it to LoadUrl

Need to add the test engine main function to be executable in BUILD.gn;
should mark it as 'testonly = true' to not interrupt with the normal
blimp function.

BUG=656122
==========

to

==========
[Blimp] Refactor Blimp test engine with embedded test server and URL rewriting

Create test infrastructure to do end to end page loading tests in client
side.

The framework mainly builds in the blimp engine code, and deals with url
getting by the help of embedded test server. The main idea is to create
a main() function in blimp/engine/testing/app which should look similar
to the one in blimp/engine/app, using mostly same blimp engine methods,
and overrides/adding functions for testing needs.

Components in the test infrastructure should take care of:
  - Set up embedded test server
  - Create URL handler to take care of magic URLs
  - Deliver related URL to test server and get full URL in server
  - Rewrite the magic URLs and should pass it to LoadUrl

Need to add the test engine main function to be executable in BUILD.gn;
should mark it as 'testonly = true' to not interrupt with the normal
blimp function.

BUG=656122


***
Implementation is done and function works.
Bug 656122 is obsoleted, so this CL won't be landed.
***
==========

[shenghuazhang, 2017-01-11 19:48:24]

Description was changed from

==========
[Blimp] Refactor Blimp test engine with embedded test server and URL rewriting

Create test infrastructure to do end to end page loading tests in client
side.

The framework mainly builds in the blimp engine code, and deals with url
getting by the help of embedded test server. The main idea is to create
a main() function in blimp/engine/testing/app which should look similar
to the one in blimp/engine/app, using mostly same blimp engine methods,
and overrides/adding functions for testing needs.

Components in the test infrastructure should take care of:
  - Set up embedded test server
  - Create URL handler to take care of magic URLs
  - Deliver related URL to test server and get full URL in server
  - Rewrite the magic URLs and should pass it to LoadUrl

Need to add the test engine main function to be executable in BUILD.gn;
should mark it as 'testonly = true' to not interrupt with the normal
blimp function.

BUG=656122


***
Implementation is done and function works.
Bug 656122 is obsoleted, so this CL won't be landed.
***
==========

to

==========
[Blimp] Refactor Blimp test engine with embedded test server and URL rewriting

Create test infrastructure to do end to end page loading tests in client
side.

The framework mainly builds in the blimp engine code, and deals with url
getting by the help of embedded test server. The main idea is to create
a main() function in blimp/engine/testing/app which should look similar
to the one in blimp/engine/app, using mostly same blimp engine methods,
and overrides/adding functions for testing needs.

Components in the test infrastructure should take care of:
  - Set up embedded test server
  - Create URL handler to take care of magic URLs
  - Deliver related URL to test server and get full URL in server
  - Rewrite the magic URLs and should pass it to LoadUrl

Need to add the test engine main function to be executable in BUILD.gn;
should mark it as 'testonly = true' to not interrupt with the normal
blimp function.

BUG=656122


***
This CL fixed bug 656122. It won't be committed due to bug is marked as WontFix.
***
==========

[shenghuazhang, 2017-01-11 19:48:45]

This CL fixed bug 656122. It won't be committed due to bug is marked as WontFix.
Will close the CL.