Attempt to recommit r67153 with successful tries.

Review here: http://codereview.chromium.org/5257003
BUG=None
TEST=None

Review URL: http://codereview.chromium.org/5356002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@67167 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 87 insertions, 43 deletions

diff --git a/ceee/ie/broker/broker_rpc_client.h b/ceee/ie/broker/broker_rpc_client.h
index d204e62..fc38c0d 100644
--- a/ceee/ie/broker/broker_rpc_client.h
+++ b/ceee/ie/broker/broker_rpc_client.h
@@ -14,23 +14,23 @@
 class BrokerRpcClient {
  public:
   BrokerRpcClient();
-  ~BrokerRpcClient();
+  virtual ~BrokerRpcClient();
 
   // Initialize connection with server.
-  HRESULT Connect();
+  virtual HRESULT Connect();
 
   // Relese connection with server
-  void Disconnect();
+  virtual void Disconnect();
 
   // Returns true if object ready for remote calls.
-  bool is_connected() const {
+  virtual bool is_connected() const {
     return context_ != NULL && binding_handle_ != NULL;
   }
 
   // @name Remote calls.
   // @{
   // Calls FireEvent on server side.
-  HRESULT FireEvent(const char* event_name, const char* event_args);
+  virtual HRESULT FireEvent(const char* event_name, const char* event_args);
   // Adds uma to histograms on the server side. Either performance timings or
   // generic histogram.
   virtual bool SendUmaHistogramTimes(BSTR event_name,
diff --git a/ceee/ie/broker/broker_rpc_server.cc b/ceee/ie/broker/broker_rpc_server.cc
index 0e899be..6600efd 100644
--- a/ceee/ie/broker/broker_rpc_server.cc
+++ b/ceee/ie/broker/broker_rpc_server.cc
@@ -138,8 +138,7 @@ void BrokerRpcServer_SendUmaHistogramTimes(handle_t binding_handle,
                                            BSTR event_name,
                                            int sample) {
   // We can't unfortunately use the HISTOGRAM_*_TIMES here because they use
-  // static variables to save time. FactoryTimeGet is not so expensive though
-  // and this call is non-blocking.
+  // static variables to save time.
   AutoLock lock(g_metrics_lock);
   std::string name(CW2A(event_name).m_psz);
   scoped_refptr<base::Histogram> counter =
@@ -158,8 +157,7 @@ void BrokerRpcServer_SendUmaHistogramData(handle_t binding_handle,
                                           int min, int max,
                                           int bucket_count) {
   // We can't unfortunately use the HISTOGRAM_*_COUNT here because they use
-  // static variables to save time. FactoryTimeGet is not so expensive though
-  // and this call is non-blocking.
+  // static variables to save time.
   AutoLock lock(g_metrics_lock);
   std::string name(CW2A(event_name).m_psz);
   scoped_refptr<base::Histogram> counter =
diff --git a/ceee/ie/common/metrics_util_unittest.cc b/ceee/ie/common/metrics_util_unittest.cc
index 2acc41c..082204c 100644
--- a/ceee/ie/common/metrics_util_unittest.cc
+++ b/ceee/ie/common/metrics_util_unittest.cc
@@ -36,12 +36,6 @@ using testing::SetArgumentPointee;
 using testing::StrictMock;
 using testing::Return;
 
-class BrokerRpcClientMock : public BrokerRpcClient {
-public:
-  MOCK_METHOD2(SendUmaHistogramTimes, bool(BSTR, int));
-  MOCK_METHOD5(SendUmaHistogramData, bool(BSTR, int, int, int, int));
-};
-
 class CeeeMetricsUtilTest : public testing::Test {
  protected:
   virtual void CreateAndDestroyTimer(BrokerRpcClient* rpc_client) {
@@ -64,7 +58,7 @@ TEST_F(CeeeMetricsUtilTest, ScopedTimerSuccess) {
 
   // Test that timing is right. This should ultimately succeed.
   // We expect less than a couple milliseconds on this call.
-  BrokerRpcClientMock broker_rpc;
+  testing::MockBrokerRpcClient broker_rpc;
   EXPECT_CALL(broker_rpc, SendUmaHistogramTimes(_, testing::Lt(10)));
   CreateAndDestroyTimer(&broker_rpc);
 }
diff --git a/ceee/ie/plugin/bho/browser_helper_object.cc b/ceee/ie/plugin/bho/browser_helper_object.cc
index d0f5cdc..2b2478a 100644
--- a/ceee/ie/plugin/bho/browser_helper_object.cc
+++ b/ceee/ie/plugin/bho/browser_helper_object.cc
@@ -38,7 +38,7 @@
 
 namespace keys = extension_tabs_module_constants;
 namespace ext = extension_automation_constants;
-
+namespace mu = metrics_util;
 
 _ATL_FUNC_INFO
     BrowserHelperObject::handler_type_idispatch_5variantptr_boolptr_ = {
@@ -107,7 +107,9 @@ BrowserHelperObject::BrowserHelperObject()
 }
 
 BrowserHelperObject::~BrowserHelperObject() {
-  broker_rpc_.Disconnect();
+  if (broker_rpc_ != NULL) {
+    broker_rpc_->Disconnect();
+  }
 
   TRACE_EVENT_END("ceee.bho", this, "");
 }
@@ -133,7 +135,7 @@ STDMETHODIMP BrowserHelperObject::SetSite(IUnknown* site) {
   }
 
   if (NULL == site) {
-    metrics_util::ScopedTimer metrics_timer("ceee/BHO.TearDown", &broker_rpc_);
+    mu::ScopedTimer metrics_timer("ceee/BHO.TearDown", broker_rpc_.get());
 
     // We're being torn down.
     TearDown();
@@ -143,12 +145,26 @@ STDMETHODIMP BrowserHelperObject::SetSite(IUnknown* site) {
     FireOnUnmappedEvent();
   }
 
-  metrics_util::ScopedTimer metrics_timer("ceee/BHO.Initialize", &broker_rpc_);
   HRESULT hr = SuperSite::SetSite(site);
   if (FAILED(hr))
     return hr;
 
   if (NULL != site) {
+    if (broker_rpc_ == NULL) {
+      // Unfortunately, we need to connect before taking performance. Including
+      // this call in our Timing would be useful. Unit tests make it
+      // complicated.
+      // TODO(hansl@chromium.org): Change initialization to be able to time
+      // this call too.
+      hr = ConnectRpcBrokerClient();
+      if (FAILED(hr) || !broker_rpc_->is_connected()) {
+        NOTREACHED() << "Couldn't connect to the RPC server.";
+        TearDown();
+        SuperSite::SetSite(NULL);
+      }
+    }
+
+    mu::ScopedTimer metrics_timer("ceee/BHO.Initialize", broker_rpc_.get());
     // We're being initialized.
     hr = Initialize(site);
 
@@ -275,9 +291,6 @@ HRESULT BrowserHelperObject::Initialize(IUnknown* site) {
       com::LogHr(hr);
   DCHECK(SUCCEEDED(hr)) << "CoCreating Broker. " << com::LogHr(hr);
   if (SUCCEEDED(hr)) {
-    broker_rpc_.Connect();
-    DCHECK(broker_rpc_.is_connected());
-
     DCHECK(executor_ == NULL);
     hr = CreateExecutor(&executor_);
     LOG_IF(ERROR, FAILED(hr)) << "Failed to create Executor, hr=" <<
@@ -768,8 +781,7 @@ STDMETHODIMP_(void) BrowserHelperObject::OnBeforeNavigate2(
 
 void BrowserHelperObject::OnBeforeNavigate2Impl(
     const ScopedDispatchPtr& webbrowser_disp, const CComBSTR& url) {
-  metrics_util::ScopedTimer metrics_timer("ceee/BHO.BeforeNavigate",
-                                          &broker_rpc_);
+  mu::ScopedTimer metrics_timer("ceee/BHO.BeforeNavigate", broker_rpc_.get());
 
   base::win::ScopedComPtr<IWebBrowser2> webbrowser;
   HRESULT hr = webbrowser.QueryFrom(webbrowser_disp);
@@ -834,8 +846,7 @@ STDMETHODIMP_(void) BrowserHelperObject::OnDocumentComplete(
 
 void BrowserHelperObject::OnDocumentCompleteImpl(
     const ScopedWebBrowser2Ptr& webbrowser, const CComBSTR& url) {
-  metrics_util::ScopedTimer metrics_timer("ceee/BHO.DocumentComplete",
-                                          &broker_rpc_);
+  mu::ScopedTimer metrics_timer("ceee/BHO.DocumentComplete", broker_rpc_.get());
   for (std::vector<Sink*>::iterator iter = sinks_.begin();
        iter != sinks_.end(); ++iter) {
     (*iter)->OnDocumentComplete(webbrowser, url);
@@ -874,8 +885,7 @@ STDMETHODIMP_(void) BrowserHelperObject::OnNavigateComplete2(
 
 void BrowserHelperObject::OnNavigateComplete2Impl(
     const ScopedWebBrowser2Ptr& webbrowser, const CComBSTR& url) {
-  metrics_util::ScopedTimer metrics_timer("ceee/BHO.NavigateComplete",
-                                          &broker_rpc_);
+  mu::ScopedTimer metrics_timer("ceee/BHO.NavigateComplete", broker_rpc_.get());
 
   HandleNavigateComplete(webbrowser, url);
 
@@ -926,8 +936,7 @@ STDMETHODIMP_(void) BrowserHelperObject::OnNavigateError(
 void BrowserHelperObject::OnNavigateErrorImpl(
     const ScopedWebBrowser2Ptr& webbrowser, const CComBSTR& url,
     LONG status_code) {
-  metrics_util::ScopedTimer metrics_timer("ceee/BHO.NavigateError",
-                                          &broker_rpc_);
+  mu::ScopedTimer metrics_timer("ceee/BHO.NavigateError", broker_rpc_.get());
 
   for (std::vector<Sink*>::iterator iter = sinks_.begin();
        iter != sinks_.end(); ++iter) {
@@ -1106,6 +1115,11 @@ HRESULT BrowserHelperObject::CreateFrameEventHandler(
       browser, parent_browser, this, IID_IFrameEventHandler, handler);
 }
 
+HRESULT BrowserHelperObject::ConnectRpcBrokerClient() {
+  broker_rpc_.reset(new BrokerRpcClient);
+  return broker_rpc_->Connect();
+}
+
 HRESULT BrowserHelperObject::AttachBrowser(IWebBrowser2* browser,
                                            IWebBrowser2* parent_browser,
                                            IFrameEventHandler* handler) {
diff --git a/ceee/ie/plugin/bho/browser_helper_object.h b/ceee/ie/plugin/bho/browser_helper_object.h
index 8f79453..293858f 100644
--- a/ceee/ie/plugin/bho/browser_helper_object.h
+++ b/ceee/ie/plugin/bho/browser_helper_object.h
@@ -205,22 +205,25 @@ class ATL_NO_VTABLE BrowserHelperObject
   virtual HRESULT HandleReadyStateChanged(READYSTATE old_state,
                                           READYSTATE new_state);
 
-  // Unit testing seems to create the frame event handler.
+  // Unit testing seam to create the frame event handler.
   virtual HRESULT CreateFrameEventHandler(IWebBrowser2* browser,
                                           IWebBrowser2* parent_browser,
                                           IFrameEventHandler** handler);
 
-  // Unit testing seems to get the parent of a browser.
+  // Unit testing seam to connect to Broker RPC server.
+  virtual HRESULT ConnectRpcBrokerClient();
+
+  // Unit testing seam to get the parent of a browser.
   virtual HRESULT GetParentBrowser(IWebBrowser2* browser,
                                    IWebBrowser2** parent_browser);
 
-  // Unit testing seems to create the broker registrar.
+  // Unit testing seam to create the broker registrar.
   virtual HRESULT GetBrokerRegistrar(ICeeeBrokerRegistrar** broker);
 
-  // Unit testing seems to create an executor.
+  // Unit testing seam to create an executor.
   virtual HRESULT CreateExecutor(IUnknown** executor);
 
-  // Unit testing seems to create a WebProgressNotifier instance.
+  // Unit testing seam to create a WebProgressNotifier instance.
   virtual WebProgressNotifier* CreateWebProgressNotifier();
 
   // Initializes the BHO to the given site.
@@ -375,7 +378,7 @@ class ATL_NO_VTABLE BrowserHelperObject
   bool full_tab_chrome_frame_;
 
   // The RPC client used to communicate with the broker.
-  BrokerRpcClient broker_rpc_;
+  scoped_ptr<BrokerRpcClient> broker_rpc_;
 
  private:
   // The BHO registers proxy/stubs for the CEEE executor on initialization.
diff --git a/ceee/ie/plugin/bho/browser_helper_object_unittest.cc b/ceee/ie/plugin/bho/browser_helper_object_unittest.cc
index be970e8..9f4f7d8 100644
--- a/ceee/ie/plugin/bho/browser_helper_object_unittest.cc
+++ b/ceee/ie/plugin/bho/browser_helper_object_unittest.cc
@@ -29,6 +29,7 @@ using testing::CopyInterfaceToArgument;
 using testing::DoAll;
 using testing::GetConnectionCount;
 using testing::InstanceCountMixin;
+using testing::MockBrokerRpcClient;
 using testing::MockChromeFrameHost;
 using testing::MockDispatchEx;
 using testing::MockIOleWindow;
@@ -106,6 +107,16 @@ class TestingBrowserHelperObject
     return S_OK;
   }
 
+  virtual HRESULT ConnectRpcBrokerClient() {
+    MockBrokerRpcClient* rpc_client = new MockBrokerRpcClient();
+    EXPECT_CALL(*rpc_client, is_connected()).WillOnce(Return(true));
+    EXPECT_CALL(*rpc_client, SendUmaHistogramTimes(_, _))
+        .WillRepeatedly(Return(true));
+    EXPECT_CALL(*rpc_client, Disconnect()).Times(1);
+    broker_rpc_.reset(rpc_client);
+    return S_OK;
+  }
+
   virtual HRESULT GetTabWindow(IServiceProvider* service_provider) {
     tab_window_ = reinterpret_cast<HWND>(kGoodTab);
     return S_OK;
diff --git a/ceee/ie/plugin/bho/mediumtest_browser_helper_object.cc b/ceee/ie/plugin/bho/mediumtest_browser_helper_object.cc
index 102f9c0..88b8385 100644
--- a/ceee/ie/plugin/bho/mediumtest_browser_helper_object.cc
+++ b/ceee/ie/plugin/bho/mediumtest_browser_helper_object.cc
@@ -41,6 +41,7 @@ using testing::kLevelTwoFrame;
 using testing::kOrphansPage;
 using testing::kSimplePage;
 using testing::kTwoFramesPage;
+using testing::MockBrokerRpcClient;
 using testing::MockChromeFrameHost;
 using testing::NotNull;
 using testing::Return;
@@ -106,7 +107,7 @@ class TestingBrowserHelperObject
     return S_OK;
   }
 
-  HRESULT CreateChromeFrameHost() {
+  virtual HRESULT CreateChromeFrameHost() {
     HRESULT hr = MockChromeFrameHost::CreateInitializedIID(
         &mock_chrome_frame_host_, IID_IChromeFrameHost, &chrome_frame_host_);
 
@@ -137,6 +138,16 @@ class TestingBrowserHelperObject
     return hr;
   }
 
+  virtual HRESULT ConnectRpcBrokerClient() {
+    MockBrokerRpcClient* rpc_client = new MockBrokerRpcClient();
+    EXPECT_CALL(*rpc_client, is_connected()).WillOnce(Return(true));
+    EXPECT_CALL(*rpc_client, SendUmaHistogramTimes(_, _))
+        .WillRepeatedly(Return(true));
+    EXPECT_CALL(*rpc_client, Disconnect()).Times(1);
+    broker_rpc_.reset(rpc_client);
+    return S_OK;
+  }
+
   // Stub content script manifest loading.
   void LoadManifestFile() {}
 
@@ -281,11 +292,11 @@ class BrowserEventSink
   }
 };
 
-class BrowerHelperObjectTest: public ShellBrowserTestImpl<BrowserEventSink> {
+class BrowserHelperObjectTest: public ShellBrowserTestImpl<BrowserEventSink> {
  public:
   typedef ShellBrowserTestImpl<BrowserEventSink> Super;
 
-  BrowerHelperObjectTest() : bho_(NULL), site_(NULL) {
+  BrowserHelperObjectTest() : bho_(NULL), site_(NULL) {
   }
 
   virtual void SetUp() {
@@ -361,7 +372,8 @@ class BrowerHelperObjectTest: public ShellBrowserTestImpl<BrowserEventSink> {
 //    document's URL changes.
 // 3. That we don't leak discarded frame event handlers.
 // 4. That we don't crash during any of this.
-TEST_F(BrowerHelperObjectTest, FrameHandlerCreationAndDestructionOnNavigation) {
+TEST_F(BrowserHelperObjectTest,
+       FrameHandlerCreationAndDestructionOnNavigation) {
   const std::wstring two_frames_resources[] = {
       GetTestUrl(kTwoFramesPage),
       GetTestUrl(kFrameOne),
@@ -414,7 +426,7 @@ TEST_F(BrowerHelperObjectTest, FrameHandlerCreationAndDestructionOnNavigation) {
 // I hope to find a viable repro for this later, and because this
 // code exercises some modes of navigation that the above test does
 // not.
-TEST_F(BrowerHelperObjectTest, DeepFramesAreCorrectlyHandled) {
+TEST_F(BrowserHelperObjectTest, DeepFramesAreCorrectlyHandled) {
   const std::wstring simple_page_resources[] = { GetTestUrl(kSimplePage) };
   const std::wstring deep_frames_resources[] = {
       GetTestUrl(kDeepFramesPage),
@@ -498,7 +510,7 @@ TEST_F(BrowerHelperObjectTest, DeepFramesAreCorrectlyHandled) {
 // attach the new frame to its parent that we had seen before. So we needed to
 // add code to create a handler for the ancestors of such orphans.
 //
-TEST_F(BrowerHelperObjectTest, OrphanFrame) {
+TEST_F(BrowserHelperObjectTest, OrphanFrame) {
   const std::wstring simple_page_resources[] = { GetTestUrl(kSimplePage) };
   const std::wstring orphan_page_resources[] = {
       GetTestUrl(kOrphansPage),
diff --git a/ceee/ie/testing/mock_broker_and_friends.h b/ceee/ie/testing/mock_broker_and_friends.h
index 7c02845..29ab078 100644
--- a/ceee/ie/testing/mock_broker_and_friends.h
+++ b/ceee/ie/testing/mock_broker_and_friends.h
@@ -9,6 +9,7 @@
 #include <string>
 
 #include "ceee/ie/broker/api_dispatcher.h"
+#include "ceee/ie/broker/broker_rpc_client.h"
 #include "ceee/ie/plugin/bho/cookie_events_funnel.h"
 #include "ceee/ie/plugin/bho/tab_events_funnel.h"
 #include "ceee/ie/plugin/bho/webnavigation_events_funnel.h"
@@ -354,6 +355,17 @@ class MockWebRequestEventsFunnel : public WebRequestEventsFunnel {
                                       const base::Time& time_stamp));
 };
 
+class MockBrokerRpcClient : public BrokerRpcClient {
+ public:
+  MOCK_METHOD0(Connect, HRESULT());
+  MOCK_METHOD0(Disconnect, void());
+  MOCK_CONST_METHOD0(is_connected, bool());
+  MOCK_METHOD2(FireEvent, HRESULT(const char*, const char*));
+  MOCK_METHOD2(SendUmaHistogramTimes, bool(BSTR, int));
+  MOCK_METHOD5(SendUmaHistogramData, bool(BSTR, int, int, int, int));
+};
+
+
 }  // namespace testing
 
 #endif  // CEEE_IE_TESTING_MOCK_BROKER_AND_FRIENDS_H_