Revert 67153 - Ajusted the BrokerRpcClient, BHO and mediumtests to correctly EXPECT on Rpc calls. Also ajusted some invalid comments in the RpcServer and fixed a typo in the BHO mediumtest name.

Reason for revert: broke the windows build.

BUG=None
TEST=mediumtest_ie.exe

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

TBR=hansl@google.com

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

8 files changed, 43 insertions, 87 deletions

diff --git a/ceee/ie/broker/broker_rpc_client.h b/ceee/ie/broker/broker_rpc_client.h
index fc38c0d..d204e62 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();
-  virtual ~BrokerRpcClient();
+  ~BrokerRpcClient();
 
   // Initialize connection with server.
-  virtual HRESULT Connect();
+  HRESULT Connect();
 
   // Relese connection with server
-  virtual void Disconnect();
+  void Disconnect();
 
   // Returns true if object ready for remote calls.
-  virtual bool is_connected() const {
+  bool is_connected() const {
     return context_ != NULL && binding_handle_ != NULL;
   }
 
   // @name Remote calls.
   // @{
   // Calls FireEvent on server side.
-  virtual HRESULT FireEvent(const char* event_name, const char* event_args);
+  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 6600efd..0e899be 100644
--- a/ceee/ie/broker/broker_rpc_server.cc
+++ b/ceee/ie/broker/broker_rpc_server.cc
@@ -138,7 +138,8 @@ 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.
+  // static variables to save time. FactoryTimeGet is not so expensive though
+  // and this call is non-blocking.
   AutoLock lock(g_metrics_lock);
   std::string name(CW2A(event_name).m_psz);
   scoped_refptr<base::Histogram> counter =
@@ -157,7 +158,8 @@ 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.
+  // static variables to save time. FactoryTimeGet is not so expensive though
+  // and this call is non-blocking.
   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 082204c..2acc41c 100644
--- a/ceee/ie/common/metrics_util_unittest.cc
+++ b/ceee/ie/common/metrics_util_unittest.cc
@@ -36,6 +36,12 @@ 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) {
@@ -58,7 +64,7 @@ TEST_F(CeeeMetricsUtilTest, ScopedTimerSuccess) {
 
   // Test that timing is right. This should ultimately succeed.
   // We expect less than a couple milliseconds on this call.
-  testing::MockBrokerRpcClient broker_rpc;
+  BrokerRpcClientMock 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 2b2478a..d0f5cdc 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,9 +107,7 @@ BrowserHelperObject::BrowserHelperObject()
 }
 
 BrowserHelperObject::~BrowserHelperObject() {
-  if (broker_rpc_ != NULL) {
-    broker_rpc_->Disconnect();
-  }
+  broker_rpc_.Disconnect();
 
   TRACE_EVENT_END("ceee.bho", this, "");
 }
@@ -135,7 +133,7 @@ STDMETHODIMP BrowserHelperObject::SetSite(IUnknown* site) {
   }
 
   if (NULL == site) {
-    mu::ScopedTimer metrics_timer("ceee/BHO.TearDown", broker_rpc_.get());
+    metrics_util::ScopedTimer metrics_timer("ceee/BHO.TearDown", &broker_rpc_);
 
     // We're being torn down.
     TearDown();
@@ -145,26 +143,12 @@ 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);
 
@@ -291,6 +275,9 @@ 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=" <<
@@ -781,7 +768,8 @@ STDMETHODIMP_(void) BrowserHelperObject::OnBeforeNavigate2(
 
 void BrowserHelperObject::OnBeforeNavigate2Impl(
     const ScopedDispatchPtr& webbrowser_disp, const CComBSTR& url) {
-  mu::ScopedTimer metrics_timer("ceee/BHO.BeforeNavigate", broker_rpc_.get());
+  metrics_util::ScopedTimer metrics_timer("ceee/BHO.BeforeNavigate",
+                                          &broker_rpc_);
 
   base::win::ScopedComPtr<IWebBrowser2> webbrowser;
   HRESULT hr = webbrowser.QueryFrom(webbrowser_disp);
@@ -846,7 +834,8 @@ STDMETHODIMP_(void) BrowserHelperObject::OnDocumentComplete(
 
 void BrowserHelperObject::OnDocumentCompleteImpl(
     const ScopedWebBrowser2Ptr& webbrowser, const CComBSTR& url) {
-  mu::ScopedTimer metrics_timer("ceee/BHO.DocumentComplete", broker_rpc_.get());
+  metrics_util::ScopedTimer metrics_timer("ceee/BHO.DocumentComplete",
+                                          &broker_rpc_);
   for (std::vector<Sink*>::iterator iter = sinks_.begin();
        iter != sinks_.end(); ++iter) {
     (*iter)->OnDocumentComplete(webbrowser, url);
@@ -885,7 +874,8 @@ STDMETHODIMP_(void) BrowserHelperObject::OnNavigateComplete2(
 
 void BrowserHelperObject::OnNavigateComplete2Impl(
     const ScopedWebBrowser2Ptr& webbrowser, const CComBSTR& url) {
-  mu::ScopedTimer metrics_timer("ceee/BHO.NavigateComplete", broker_rpc_.get());
+  metrics_util::ScopedTimer metrics_timer("ceee/BHO.NavigateComplete",
+                                          &broker_rpc_);
 
   HandleNavigateComplete(webbrowser, url);
 
@@ -936,7 +926,8 @@ STDMETHODIMP_(void) BrowserHelperObject::OnNavigateError(
 void BrowserHelperObject::OnNavigateErrorImpl(
     const ScopedWebBrowser2Ptr& webbrowser, const CComBSTR& url,
     LONG status_code) {
-  mu::ScopedTimer metrics_timer("ceee/BHO.NavigateError", broker_rpc_.get());
+  metrics_util::ScopedTimer metrics_timer("ceee/BHO.NavigateError",
+                                          &broker_rpc_);
 
   for (std::vector<Sink*>::iterator iter = sinks_.begin();
        iter != sinks_.end(); ++iter) {
@@ -1115,11 +1106,6 @@ 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 293858f..8f79453 100644
--- a/ceee/ie/plugin/bho/browser_helper_object.h
+++ b/ceee/ie/plugin/bho/browser_helper_object.h
@@ -205,25 +205,22 @@ class ATL_NO_VTABLE BrowserHelperObject
   virtual HRESULT HandleReadyStateChanged(READYSTATE old_state,
                                           READYSTATE new_state);
 
-  // Unit testing seam to create the frame event handler.
+  // Unit testing seems to create the frame event handler.
   virtual HRESULT CreateFrameEventHandler(IWebBrowser2* browser,
                                           IWebBrowser2* parent_browser,
                                           IFrameEventHandler** handler);
 
-  // Unit testing seam to connect to Broker RPC server.
-  virtual HRESULT ConnectRpcBrokerClient();
-
-  // Unit testing seam to get the parent of a browser.
+  // Unit testing seems to get the parent of a browser.
   virtual HRESULT GetParentBrowser(IWebBrowser2* browser,
                                    IWebBrowser2** parent_browser);
 
-  // Unit testing seam to create the broker registrar.
+  // Unit testing seems to create the broker registrar.
   virtual HRESULT GetBrokerRegistrar(ICeeeBrokerRegistrar** broker);
 
-  // Unit testing seam to create an executor.
+  // Unit testing seems to create an executor.
   virtual HRESULT CreateExecutor(IUnknown** executor);
 
-  // Unit testing seam to create a WebProgressNotifier instance.
+  // Unit testing seems to create a WebProgressNotifier instance.
   virtual WebProgressNotifier* CreateWebProgressNotifier();
 
   // Initializes the BHO to the given site.
@@ -378,7 +375,7 @@ class ATL_NO_VTABLE BrowserHelperObject
   bool full_tab_chrome_frame_;
 
   // The RPC client used to communicate with the broker.
-  scoped_ptr<BrokerRpcClient> broker_rpc_;
+  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 9f4f7d8..be970e8 100644
--- a/ceee/ie/plugin/bho/browser_helper_object_unittest.cc
+++ b/ceee/ie/plugin/bho/browser_helper_object_unittest.cc
@@ -29,7 +29,6 @@ using testing::CopyInterfaceToArgument;
 using testing::DoAll;
 using testing::GetConnectionCount;
 using testing::InstanceCountMixin;
-using testing::MockBrokerRpcClient;
 using testing::MockChromeFrameHost;
 using testing::MockDispatchEx;
 using testing::MockIOleWindow;
@@ -107,16 +106,6 @@ 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 88b8385..102f9c0 100644
--- a/ceee/ie/plugin/bho/mediumtest_browser_helper_object.cc
+++ b/ceee/ie/plugin/bho/mediumtest_browser_helper_object.cc
@@ -41,7 +41,6 @@ using testing::kLevelTwoFrame;
 using testing::kOrphansPage;
 using testing::kSimplePage;
 using testing::kTwoFramesPage;
-using testing::MockBrokerRpcClient;
 using testing::MockChromeFrameHost;
 using testing::NotNull;
 using testing::Return;
@@ -107,7 +106,7 @@ class TestingBrowserHelperObject
     return S_OK;
   }
 
-  virtual HRESULT CreateChromeFrameHost() {
+  HRESULT CreateChromeFrameHost() {
     HRESULT hr = MockChromeFrameHost::CreateInitializedIID(
         &mock_chrome_frame_host_, IID_IChromeFrameHost, &chrome_frame_host_);
 
@@ -138,16 +137,6 @@ 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() {}
 
@@ -292,11 +281,11 @@ class BrowserEventSink
   }
 };
 
-class BrowserHelperObjectTest: public ShellBrowserTestImpl<BrowserEventSink> {
+class BrowerHelperObjectTest: public ShellBrowserTestImpl<BrowserEventSink> {
  public:
   typedef ShellBrowserTestImpl<BrowserEventSink> Super;
 
-  BrowserHelperObjectTest() : bho_(NULL), site_(NULL) {
+  BrowerHelperObjectTest() : bho_(NULL), site_(NULL) {
   }
 
   virtual void SetUp() {
@@ -372,8 +361,7 @@ class BrowserHelperObjectTest: 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(BrowserHelperObjectTest,
-       FrameHandlerCreationAndDestructionOnNavigation) {
+TEST_F(BrowerHelperObjectTest, FrameHandlerCreationAndDestructionOnNavigation) {
   const std::wstring two_frames_resources[] = {
       GetTestUrl(kTwoFramesPage),
       GetTestUrl(kFrameOne),
@@ -426,7 +414,7 @@ TEST_F(BrowserHelperObjectTest,
 // 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(BrowserHelperObjectTest, DeepFramesAreCorrectlyHandled) {
+TEST_F(BrowerHelperObjectTest, DeepFramesAreCorrectlyHandled) {
   const std::wstring simple_page_resources[] = { GetTestUrl(kSimplePage) };
   const std::wstring deep_frames_resources[] = {
       GetTestUrl(kDeepFramesPage),
@@ -510,7 +498,7 @@ TEST_F(BrowserHelperObjectTest, 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(BrowserHelperObjectTest, OrphanFrame) {
+TEST_F(BrowerHelperObjectTest, 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 29ab078..7c02845 100644
--- a/ceee/ie/testing/mock_broker_and_friends.h
+++ b/ceee/ie/testing/mock_broker_and_friends.h
@@ -9,7 +9,6 @@
 #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"
@@ -355,17 +354,6 @@ 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_