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

blimp/engine/testing/app/blimp_browser_testing_main_parts.cc

Index: blimp/engine/testing/app/blimp_browser_testing_main_parts.cc
diff --git a/blimp/engine/app/blimp_browser_main_parts.cc b/blimp/engine/testing/app/blimp_browser_testing_main_parts.cc
similarity index 51%
copy from blimp/engine/app/blimp_browser_main_parts.cc
copy to blimp/engine/testing/app/blimp_browser_testing_main_parts.cc
index 32e405c65a1a240f452c0349c779a5dab668035a..d7ea0326986a88ae473a5c96d1a4873ff1247857 100644
--- a/blimp/engine/app/blimp_browser_main_parts.cc
+++ b/blimp/engine/testing/app/blimp_browser_testing_main_parts.cc
@@ -1,8 +1,8 @@
-// Copyright 2015 The Chromium Authors. All rights reserved.
+// Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "blimp/engine/app/blimp_browser_main_parts.h"
+#include "blimp/engine/testing/app/blimp_browser_testing_main_parts.h"
 
 #include <utility>
 
@@ -15,21 +15,26 @@
 #include "blimp/engine/common/blimp_browser_context.h"
 #include "blimp/engine/feature/geolocation/blimp_location_provider.h"
 #include "blimp/engine/session/blimp_engine_session.h"
+#include "blimp/engine/testing/app/blimp_url_rewriter.h"
+#include "blimp/engine/testing/session/blimp_engine_testing_session.h"
 #include "blimp/net/blimp_connection.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/browser_url_handler.h"
 #include "content/public/common/main_function_params.h"
 #include "net/base/net_module.h"
 #include "net/log/net_log.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
 
 namespace blimp {
 namespace engine {
 
-BlimpBrowserMainParts::BlimpBrowserMainParts(
-    const content::MainFunctionParams& parameters) {}
+const base::FilePath::CharType kBlimpTestRoot[] = FILE_PATH_LITERAL("");
 
-BlimpBrowserMainParts::~BlimpBrowserMainParts() {}
+BlimpBrowserTestingMainParts::BlimpBrowserTestingMainParts(
+    const content::MainFunctionParams& parameters)
+    : BlimpBrowserMainParts(parameters){};
 
-void BlimpBrowserMainParts::PreEarlyInitialization() {
+void BlimpBrowserTestingMainParts::PreEarlyInitialization() {
   // Fetch the engine config from the command line, and crash if invalid. Allow
   // IO operations even though this is not in the FILE thread as this is
   // necessary for Blimp startup and occurs before any user interaction.
@@ -41,39 +46,27 @@ void BlimpBrowserMainParts::PreEarlyInitialization() {
   }
 }
 
-void BlimpBrowserMainParts::PreMainMessageLoopRun() {
+void BlimpBrowserTestingMainParts::PreMainMessageLoopRun() {

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

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?

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

What about PostMainMessageLoopRun in this class?

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

Done.

+  blimp::engine::test::g_ets_instance.reset(new net::EmbeddedTestServer());
+  blimp::engine::test::g_ets_instance->ServeFilesFromSourceDirectory(
+      base::FilePath(kBlimpTestRoot));
+  DCHECK(blimp::engine::test::g_ets_instance->Start());
+
+  content::BrowserURLHandler::GetInstance()->AddHandlerPair(
+      &blimp::engine::test::HandleBlimpTestURL,
+      content::BrowserURLHandler::null_handler());
+
   net_log_.reset(new net::NetLog());
   settings_manager_.reset(new SettingsManager);
   std::unique_ptr<BlimpBrowserContext> browser_context(
       new BlimpBrowserContext(false, net_log_.get()));
-  engine_session_.reset(
-      new BlimpEngineSession(std::move(browser_context), net_log_.get(),
-                             engine_config_.get(), settings_manager_.get()));
+  engine_session_.reset(new BlimpEngineTestingSession(
+      std::move(browser_context), net_log_.get(), engine_config_.get(),

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

Should BlimpEngineTestingSession own the NetLog and the BlimpEngineConfig?

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

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...

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

Ah, I didn't look at BlimpEngineSession.

+      settings_manager_.get()));
   engine_session_->Initialize();
-}
-
-void BlimpBrowserMainParts::PostMainMessageLoopRun() {
-  engine_session_.reset();
-}
-
-BlimpBrowserContext* BlimpBrowserMainParts::GetBrowserContext() {
-  return engine_session_->browser_context();
-}
-
-SettingsManager* BlimpBrowserMainParts::GetSettingsManager() {
-  return settings_manager_.get();
-}
-
-BlobChannelSender* BlimpBrowserMainParts::GetBlobChannelSender() {
-  return engine_session_->blob_channel_sender();
-}
-
-BlobChannelService* BlimpBrowserMainParts::GetBlobChannelService() {
-  return engine_session_->GetBlobChannelService();
-}
-
-BlimpEngineSession* BlimpBrowserMainParts::GetBlimpEngineSession() {
-  return engine_session_.get();
+  BlimpBrowserMainParts::SetSettingsManagerForTesting(

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

seetings_manager_ and engine_session_ shouldn't be members of this class if
you're just passing them in to BlimpBrowserMainParts.

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

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_;

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

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

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

Make sense! Will refactor it.

+      std::move(settings_manager_));
+  BlimpBrowserMainParts::SetEngineSessionForTesting(std::move(engine_session_));
 }
 
 }  // namespace engine