diff options
author | hansl@google.com <hansl@google.com@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-11-29 16:24:30 +0000 |
---|---|---|
committer | hansl@google.com <hansl@google.com@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-11-29 16:24:30 +0000 |
commit | a11c5baa79e94a982987ba6e672a3d6ef0eeac2a (patch) | |
tree | a4955c62d68c0d619fdbda4250ce94aabb86d34e /ceee | |
parent | 32cde1c38fd8d939f34cdf6d37e181b468adc35a (diff) | |
download | chromium_src-a11c5baa79e94a982987ba6e672a3d6ef0eeac2a.zip chromium_src-a11c5baa79e94a982987ba6e672a3d6ef0eeac2a.tar.gz chromium_src-a11c5baa79e94a982987ba6e672a3d6ef0eeac2a.tar.bz2 |
Switch from having decoupled funnels and notifiers to a single RpcClient which is held by the BHO. This ensures that the order of events is serialized through a single queue (when related to a tab).
With that change, we can effectively remove the deferral of events from IE, and only defer the queue itself until the tab_id is available. This means the events will keep their orders and that we can attach everything we need (ie FrameEventsHandler) and send events in the order we want them to be, while the actual Broker will receive them only when we have a tab_id. Sending events out of order is practically impossible now (at least in tab-related events through the BHO).
From this point on we should see less out-of-order attachments and be as stable as before the TabID first change.
Also, don't log metrics if the Rpc Client isn't connected correctly.
BUG=None
TEST=smoke_tested.
Review URL: http://codereview.chromium.org/5376003
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@67537 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'ceee')
30 files changed, 306 insertions, 268 deletions
diff --git a/ceee/ie/broker/broker_rpc_client.h b/ceee/ie/broker/broker_rpc_client.h index 0468276..38b84cc 100644 --- a/ceee/ie/broker/broker_rpc_client.h +++ b/ceee/ie/broker/broker_rpc_client.h @@ -10,8 +10,17 @@ #include <wtypes.h> #include "base/basictypes.h" -// Class provides comunication with BrokerRpcServer. -class BrokerRpcClient { +// Interface for sending events. +class IEventSender { + public: + virtual ~IEventSender() {} + virtual HRESULT FireEvent(const char* event_name, + const char* event_args) = 0; +}; + + +// Class provides communication with BrokerRpcServer. +class BrokerRpcClient : public IEventSender { public: BrokerRpcClient(); virtual ~BrokerRpcClient(); diff --git a/ceee/ie/common/metrics_util.h b/ceee/ie/common/metrics_util.h index 8af9034..c38fba5 100644 --- a/ceee/ie/common/metrics_util.h +++ b/ceee/ie/common/metrics_util.h @@ -33,6 +33,7 @@ class ScopedTimer { broker_rpc_ = NULL; // Ensure we don't call the broker_rpc. } } + ~ScopedTimer() { if (broker_rpc_) { base::TimeDelta delta = base::TimeTicks::Now() - start_; @@ -43,6 +44,11 @@ class ScopedTimer { } } + void Drop() { + // Something happened and we should not log. + broker_rpc_ = NULL; + } + protected: base::TimeTicks start_; diff --git a/ceee/ie/plugin/bho/browser_helper_object.cc b/ceee/ie/plugin/bho/browser_helper_object.cc index 54c24df..e62cd80 100644 --- a/ceee/ie/plugin/bho/browser_helper_object.cc +++ b/ceee/ie/plugin/bho/browser_helper_object.cc @@ -100,14 +100,13 @@ BrowserHelperObject::BrowserHelperObject() lower_bound_ready_state_(READYSTATE_UNINITIALIZED), ie7_or_later_(false), thread_id_(::GetCurrentThreadId()), - full_tab_chrome_frame_(false) { + full_tab_chrome_frame_(false), + broker_client_queue_(this), + tab_events_funnel_(broker_client()) { TRACE_EVENT_BEGIN("ceee.bho", this, ""); } BrowserHelperObject::~BrowserHelperObject() { - if (broker_rpc_ != NULL) - broker_rpc_->Disconnect(); - TRACE_EVENT_END("ceee.bho", this, ""); } @@ -125,6 +124,9 @@ HRESULT BrowserHelperObject::FinalConstruct() { } void BrowserHelperObject::FinalRelease() { + // Need to disconnect outside of destructor, because we use a virtual method + // for unit testing. + broker_rpc().Disconnect(); web_browser_.Release(); } @@ -149,8 +151,7 @@ void BrowserHelperObject::ReportAddonLoadTime(const char* addon_name, counter_name += 'x'; break; } - if (broker_rpc_.get()) - broker_rpc_->SendUmaHistogramTimes(counter_name.c_str(), time); + broker_rpc().SendUmaHistogramTimes(counter_name.c_str(), time); } STDMETHODIMP BrowserHelperObject::SetSite(IUnknown* site) { @@ -166,7 +167,7 @@ STDMETHODIMP BrowserHelperObject::SetSite(IUnknown* site) { } if (NULL == site) { - mu::ScopedTimer metrics_timer("ceee/BHO.TearDown", broker_rpc_.get()); + mu::ScopedTimer metrics_timer("ceee/BHO.TearDown", &broker_rpc()); // TODO(vitalybuka@chromium.org): switch to sampling when we have enough // users. @@ -288,7 +289,9 @@ HRESULT BrowserHelperObject::CreateExecutor(IUnknown** executor) { WebProgressNotifier* BrowserHelperObject::CreateWebProgressNotifier() { scoped_ptr<WebProgressNotifier> web_progress_notifier( new WebProgressNotifier()); - HRESULT hr = web_progress_notifier->Initialize(this, tab_window_, + HRESULT hr = web_progress_notifier->Initialize(this, + broker_client(), + tab_window_, web_browser_); return SUCCEEDED(hr) ? web_progress_notifier.release() : NULL; @@ -333,19 +336,15 @@ HRESULT BrowserHelperObject::SetTabIdForHandle(int tool_band_id, HRESULT BrowserHelperObject::Initialize(IUnknown* site) { TRACE_EVENT_INSTANT("ceee.bho.initialize", this, ""); + mu::ScopedTimer metrics_timer("ceee/BHO.Initialize", &broker_rpc()); - // 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. - DCHECK(broker_rpc_ == NULL); HRESULT hr = ConnectRpcBrokerClient(); - if (FAILED(hr) || !broker_rpc_->is_connected()) { + if (FAILED(hr) || !broker_rpc().is_connected()) { + // Cancel the logging. The BrokerRpcClient isn't ready. + metrics_timer.Drop(); NOTREACHED() << "Couldn't connect to the RPC server."; return com::AlwaysError(hr); } - mu::ScopedTimer metrics_timer("ceee/BHO.Initialize", broker_rpc_.get()); ie7_or_later_ = ie_util::GetIeVersion() > ie_util::IEVERSION_IE6; @@ -646,12 +645,12 @@ bool BrowserHelperObject::EnsureTabId() { } VLOG(2) << "TabId(" << tab_id_ << ") set for Handle(" << handle << ")"; - // Call back all the methods we deferred. In order, please. - while (!deferred_tab_id_call_.empty()) { + // Call back all the events we deferred. In order, please. + while (!deferred_events_call_.empty()) { // We pop here so that if an error happens in the call we don't recall the // faulty method. This has the side-effect of losing events. - DeferredCallListType::value_type call = deferred_tab_id_call_.front(); - deferred_tab_id_call_.pop_front(); + DeferredCallListType::value_type call = deferred_events_call_.front(); + deferred_events_call_.pop_front(); call->Run(); delete call; } @@ -784,22 +783,25 @@ void BrowserHelperObject::CloseAll(IContentScriptNativeApi* instance) { HRESULT BrowserHelperObject::OpenChannelToExtension( IContentScriptNativeApi* instance, const std::string& extension, const std::string& channel_name, int cookie) { - ScopedContentScriptNativeApiPtr scoped_instance(instance); + ScopedContentScriptNativeApiPtr instance_ptr(instance); + // Here we cheat a bit. Since we want to make sure port connection are done + // in order of regular events, we queue the task with a call to + // OpenChannelToExtension, as if it was a regular deferred event. If the + // tab_id is available, we of course call the method directly. if (EnsureTabId() == false) { - deferred_tab_id_call_.push_back(NewRunnableMethod( + deferred_events_call_.push_back(NewRunnableMethod( this, &BrowserHelperObject::OpenChannelToExtensionImpl, - scoped_instance, extension, channel_name, cookie)); - return S_OK; + instance_ptr, extension, channel_name, cookie)); + VLOG(2) << "Deferred OpenChannelToExtension"; + return S_FALSE; } else { - return OpenChannelToExtensionImpl(scoped_instance, - extension, - channel_name, + return OpenChannelToExtensionImpl(instance_ptr, extension, channel_name, cookie); } } HRESULT BrowserHelperObject::OpenChannelToExtensionImpl( - const ScopedContentScriptNativeApiPtr& instance, + ScopedContentScriptNativeApiPtr instance, const std::string& extension, const std::string& channel_name, int cookie) { @@ -822,30 +824,30 @@ HRESULT BrowserHelperObject::OpenChannelToExtensionImpl( HRESULT BrowserHelperObject::PostMessage(int port_id, const std::string& message) { - return extension_port_manager_.PostMessage(port_id, message); -} - -HRESULT BrowserHelperObject::OnCfPrivateMessage(BSTR msg, - BSTR origin, - BSTR target) { - // OnPortMessage uses tab_id_, so we need to check here. + // As with OpenChannelToExtension, we cheat by deferring actual calls to + // PostMessage, in order for those calls to be synchronized with queued + // events. if (EnsureTabId() == false) { - deferred_tab_id_call_.push_back(NewRunnableMethod( - this, &BrowserHelperObject::OnCfPrivateMessageImpl, - CComBSTR(msg), CComBSTR(origin), CComBSTR(target))); - return S_OK; + deferred_events_call_.push_back(NewRunnableMethod( + this, &BrowserHelperObject::PostMessageImpl, + port_id, message)); + VLOG(2) << "Deferred PostMessage(" << port_id << ", \"" << message << "\")"; + return S_FALSE; } else { - OnCfPrivateMessageImpl(msg, origin, target); + return PostMessageImpl(port_id, message); } +} - return S_OK; +HRESULT BrowserHelperObject::PostMessageImpl(int port_id, + const std::string& message) { + return extension_port_manager_.PostMessage(port_id, message); } -void BrowserHelperObject::OnCfPrivateMessageImpl(const CComBSTR& msg, - const CComBSTR& origin, - const CComBSTR& target) { +HRESULT BrowserHelperObject::OnCfPrivateMessage(BSTR msg, + BSTR origin, + BSTR target) { const wchar_t* start = com::ToString(target); - const wchar_t* end = start + target.Length(); + const wchar_t* end = start + SysStringLen(target); if (LowerCaseEqualsASCII(start, end, ext::kAutomationPortRequestTarget) || LowerCaseEqualsASCII(start, end, ext::kAutomationPortResponseTarget)) { extension_port_manager_.OnPortMessage(msg); @@ -853,6 +855,7 @@ void BrowserHelperObject::OnCfPrivateMessageImpl(const CComBSTR& msg, LOG(ERROR) << "Unexpected message: '" << msg << "' to invalid target: " << target; } + return S_OK; } STDMETHODIMP_(void) BrowserHelperObject::OnBeforeNavigate2( @@ -869,21 +872,7 @@ STDMETHODIMP_(void) BrowserHelperObject::OnBeforeNavigate2( return; } - DCHECK(V_VT(url) == VT_BSTR); - CComBSTR url_bstr(url->bstrVal); - if (EnsureTabId() == false) { - VARIANT* null_param = NULL; - deferred_tab_id_call_.push_back(NewRunnableMethod( - this, &BrowserHelperObject::OnBeforeNavigate2Impl, - ScopedDispatchPtr(webbrowser_disp), url_bstr)); - } else { - OnBeforeNavigate2Impl(ScopedDispatchPtr(webbrowser_disp), url_bstr); - } -} - -void BrowserHelperObject::OnBeforeNavigate2Impl( - const ScopedDispatchPtr& webbrowser_disp, const CComBSTR& url) { - mu::ScopedTimer metrics_timer("ceee/BHO.BeforeNavigate", broker_rpc_.get()); + mu::ScopedTimer metrics_timer("ceee/BHO.BeforeNavigate", &broker_rpc()); ScopedWebBrowser2Ptr webbrowser; HRESULT hr = webbrowser.QueryFrom(webbrowser_disp); @@ -892,9 +881,10 @@ void BrowserHelperObject::OnBeforeNavigate2Impl( return; } + base::win::ScopedBstr bstr_url(url->bstrVal); for (std::vector<Sink*>::iterator iter = sinks_.begin(); iter != sinks_.end(); ++iter) { - (*iter)->OnBeforeNavigate(webbrowser, url); + (*iter)->OnBeforeNavigate(webbrowser, bstr_url); } // Notify the infobar executor on the event but only for the main browser. @@ -908,7 +898,7 @@ void BrowserHelperObject::OnBeforeNavigate2Impl( DCHECK(SUCCEEDED(hr)) << "Failed to get ICeeeInfobarExecutor interface " << com::LogHr(hr); if (SUCCEEDED(hr)) { - infobar_executor->OnTopFrameBeforeNavigate(url); + infobar_executor->OnTopFrameBeforeNavigate(bstr_url); } } } @@ -932,24 +922,11 @@ STDMETHODIMP_(void) BrowserHelperObject::OnDocumentComplete( return; } - DCHECK(V_VT(url) == VT_BSTR); CComBSTR url_bstr(url->bstrVal); - - if (EnsureTabId() == false) { - deferred_tab_id_call_.push_back(NewRunnableMethod( - this, &BrowserHelperObject::OnDocumentCompleteImpl, - webbrowser, url_bstr)); - } else { - OnDocumentCompleteImpl(webbrowser, url_bstr); - } -} - -void BrowserHelperObject::OnDocumentCompleteImpl( - const ScopedWebBrowser2Ptr& webbrowser, const CComBSTR& url) { - mu::ScopedTimer metrics_timer("ceee/BHO.DocumentComplete", broker_rpc_.get()); + mu::ScopedTimer metrics_timer("ceee/BHO.DocumentComplete", &broker_rpc()); for (std::vector<Sink*>::iterator iter = sinks_.begin(); iter != sinks_.end(); ++iter) { - (*iter)->OnDocumentComplete(webbrowser, url); + (*iter)->OnDocumentComplete(webbrowser, url_bstr); } } @@ -973,26 +950,13 @@ STDMETHODIMP_(void) BrowserHelperObject::OnNavigateComplete2( return; } - DCHECK(V_VT(url) == VT_BSTR); - CComBSTR url_bstr(url->bstrVal); - if (EnsureTabId() == false) { - deferred_tab_id_call_.push_back(NewRunnableMethod( - this, &BrowserHelperObject::OnNavigateComplete2Impl, - webbrowser, url_bstr)); - } else { - BrowserHelperObject::OnNavigateComplete2Impl(webbrowser, url_bstr); - } -} - -void BrowserHelperObject::OnNavigateComplete2Impl( - const ScopedWebBrowser2Ptr& webbrowser, const CComBSTR& url) { - mu::ScopedTimer metrics_timer("ceee/BHO.NavigateComplete", broker_rpc_.get()); - - HandleNavigateComplete(webbrowser, url); + mu::ScopedTimer metrics_timer("ceee/BHO.NavigateComplete", &broker_rpc()); + base::win::ScopedBstr url_bstr(url->bstrVal); + HandleNavigateComplete(webbrowser, url_bstr); for (std::vector<Sink*>::iterator iter = sinks_.begin(); - iter != sinks_.end(); ++iter) { - (*iter)->OnNavigateComplete(webbrowser, url); + iter != sinks_.end(); ++iter) { + (*iter)->OnNavigateComplete(webbrowser, url_bstr); } } @@ -1021,27 +985,11 @@ STDMETHODIMP_(void) BrowserHelperObject::OnNavigateError( return; } - DCHECK(V_VT(url) == VT_BSTR); - CComBSTR url_bstr(url->bstrVal); - DCHECK(V_VT(status_code) == VT_I4); - LONG status = status_code->lVal; - if (EnsureTabId() == false) { - deferred_tab_id_call_.push_back(NewRunnableMethod( - this, &BrowserHelperObject::OnNavigateErrorImpl, - webbrowser, url_bstr, status)); - } else { - OnNavigateErrorImpl(webbrowser, url_bstr, status); - } -} - -void BrowserHelperObject::OnNavigateErrorImpl( - const ScopedWebBrowser2Ptr& webbrowser, const CComBSTR& url, - LONG status_code) { - mu::ScopedTimer metrics_timer("ceee/BHO.NavigateError", broker_rpc_.get()); - + mu::ScopedTimer metrics_timer("ceee/BHO.NavigateError", &broker_rpc()); + base::win::ScopedBstr url_bstr(url->bstrVal); for (std::vector<Sink*>::iterator iter = sinks_.begin(); iter != sinks_.end(); ++iter) { - (*iter)->OnNavigateError(webbrowser, url, status_code); + (*iter)->OnNavigateError(webbrowser, url_bstr, status_code->lVal); } } @@ -1261,8 +1209,7 @@ HRESULT BrowserHelperObject::CreateFrameEventHandler( } HRESULT BrowserHelperObject::ConnectRpcBrokerClient() { - broker_rpc_.reset(new BrokerRpcClient); - return broker_rpc_->Connect(true); + return broker_rpc().Connect(true); } HRESULT BrowserHelperObject::AttachBrowser(IWebBrowser2* browser, @@ -1429,20 +1376,10 @@ HRESULT BrowserHelperObject::OnReadyStateChanged(READYSTATE ready_state) { } } - if (EnsureTabId() == false) { - deferred_tab_id_call_.push_back(NewRunnableMethod( - this, &BrowserHelperObject::OnReadyStateChangedImpl, - lower_bound_ready_state_, new_lowest_ready_state)); - } else { - OnReadyStateChangedImpl(lower_bound_ready_state_, new_lowest_ready_state); - } - return S_OK; -} - -void BrowserHelperObject::OnReadyStateChangedImpl(READYSTATE old_state, - READYSTATE new_state) { + READYSTATE old_state = lower_bound_ready_state_; + READYSTATE new_state = new_lowest_ready_state; if (old_state == new_state) - return; + return S_OK; // Remember the new lowest ready state as our current one. lower_bound_ready_state_ = new_state; @@ -1450,6 +1387,7 @@ void BrowserHelperObject::OnReadyStateChangedImpl(READYSTATE old_state, // Fire the event if the new ready state got us to or away from complete. if (old_state == READYSTATE_COMPLETE || new_state == READYSTATE_COMPLETE) FireOnUpdatedEvent(NULL, new_state); + return S_OK; } HRESULT BrowserHelperObject::GetReadyState(READYSTATE* ready_state) { @@ -1481,20 +1419,6 @@ HRESULT BrowserHelperObject::GetExtensionPortMessagingProvider( HRESULT BrowserHelperObject::InsertCode(BSTR code, BSTR file, BOOL all_frames, CeeeTabCodeType type) { - if (EnsureTabId() == false) { - deferred_tab_id_call_.push_back(NewRunnableMethod( - this, &BrowserHelperObject::InsertCodeImpl, - CComBSTR(code), CComBSTR(file), all_frames == TRUE, type)); - } else { - InsertCodeImpl(code, file, all_frames == TRUE, type); - } - - return S_OK; -} - -void BrowserHelperObject::InsertCodeImpl( - const CComBSTR& code, const CComBSTR& file, bool all_frames, - CeeeTabCodeType type) { // If all_frames is false, we execute only in the top level frame. Otherwise // we do the top level frame as well as all the inner frames. if (all_frames) { @@ -1522,9 +1446,9 @@ void BrowserHelperObject::InsertCodeImpl( // extension. Clean this up once we support multiple extensions. } } + return S_OK; } - HRESULT BrowserHelperObject::GetMatchingUserScriptsCssContent( const GURL& url, bool require_all_frames, std::string* css_content) { return librarian_.GetMatchingUserScriptsCssContent(url, require_all_frames, @@ -1651,3 +1575,22 @@ STDMETHODIMP BrowserHelperObject::SetToolBandSessionId(long session_id) { ")"; return hr; } + +HRESULT BrowserHelperObject::SendEventToBroker(const char* event_name, + const char* event_args) { + if (EnsureTabId() == false) { + deferred_events_call_.push_back(NewRunnableMethod( + this, &BrowserHelperObject::SendEventToBrokerImpl, + std::string(event_name), std::string(event_args))); + VLOG(2) << "Deferred SendEventToBroker. Name: \"" << event_name << + "\", args: \"" << event_args << "\"."; + return S_FALSE; + } else { + return SendEventToBrokerImpl(event_name, event_args); + } +} + +HRESULT BrowserHelperObject::SendEventToBrokerImpl( + const std::string& event_name, const std::string& event_args) { + return broker_rpc().FireEvent(event_name.c_str(), event_args.c_str()); +} diff --git a/ceee/ie/plugin/bho/browser_helper_object.h b/ceee/ie/plugin/bho/browser_helper_object.h index 273b8e6..726f8ee 100644 --- a/ceee/ie/plugin/bho/browser_helper_object.h +++ b/ceee/ie/plugin/bho/browser_helper_object.h @@ -193,29 +193,12 @@ class ATL_NO_VTABLE BrowserHelperObject // Unregister proxy/stubs for executor interfaces. virtual void UnregisterProxies(); - HRESULT OpenChannelToExtensionImpl( - const ScopedContentScriptNativeApiPtr& instance, - const std::string& extension, const std::string& channel_name, - int cookie); - void OnBeforeNavigate2Impl(const ScopedDispatchPtr& webbrowser_disp, - const CComBSTR& url); - - void OnCfPrivateMessageImpl(const CComBSTR& msg, - const CComBSTR& origin, - const CComBSTR& target); - void OnDocumentCompleteImpl(const ScopedWebBrowser2Ptr& webbrowser, - const CComBSTR& url); - void OnNavigateComplete2Impl(const ScopedWebBrowser2Ptr& webbrowser, - const CComBSTR& url); - void OnNavigateErrorImpl(const ScopedWebBrowser2Ptr& webbrowser, - const CComBSTR& url, - LONG status_code); - void OnReadyStateChangedImpl(READYSTATE old_state, READYSTATE new_state); - void InsertCodeImpl(const CComBSTR& code, - const CComBSTR& file, - bool all_frames, - CeeeTabCodeType type); - + // Implement the real call for OpenChannelToExtension and PostMessage. + HRESULT OpenChannelToExtensionImpl(ScopedContentScriptNativeApiPtr instance, + const std::string& extension, + const std::string& channel_name, + int cookie); + HRESULT PostMessageImpl(int port_id, const std::string& message); // Finds the handler attached to webbrowser. // @returns S_OK if handler is found. @@ -273,6 +256,12 @@ class ATL_NO_VTABLE BrowserHelperObject // Accessor so that we can mock it in unit tests. virtual TabEventsFunnel& tab_events_funnel() { return tab_events_funnel_; } + // Accessor so that we can mock it in unit tests. + virtual IEventSender* broker_client() { return &broker_client_queue_; } + + // Accessor so that we can mock it in unit tests. + virtual BrokerRpcClient& broker_rpc() { return broker_rpc_; } + // Fires the tab.onCreated event via the tab event funnel. virtual void FireOnCreatedEvent(BSTR url); @@ -366,9 +355,6 @@ class ATL_NO_VTABLE BrowserHelperObject // Extension port messaging and management is delegated to this. ExtensionPortManager extension_port_manager_; - // Used to dispatch tab events back to Chrome. - TabEventsFunnel tab_events_funnel_; - // Remember the tab window handle so that we can use it. HWND tab_window_; @@ -403,7 +389,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. @@ -419,10 +405,51 @@ class ATL_NO_VTABLE BrowserHelperObject HRESULT VerifyBrowserInHierarchy(IWebBrowser2* webbrowser, IWebBrowser2* root_browser); + // Send metrics histograms about the average load time of IE addons. void ReportAddonLoadTime(const char* addon_name, const CLSID& addon_id); + // This class is used as a replacement for the BrokerRpcClient for the + // event funnels contained inside a BHO. When sending an event, this will + // forward it to the BHO which will queue it until the tab_id is available, + // then simply forward all events directly to the Broker in the order they + // were sent to this queue. We don't do the whole queueing here for + // simplicity. + // TODO(hansl@chromium.org): This should be a generic class in base. Really + // useful elsewhere in CEEE and maybe elsewhere. + class BrokerEventQueue : public IEventSender { + public: + explicit BrokerEventQueue(BrowserHelperObject* bho) : bho_(bho) {} + + // If the BHO outlived the queue, we send it the events. + HRESULT FireEvent(const char* event_name, const char* event_args) { + if (bho_) { + return bho_->SendEventToBroker(event_name, event_args); + } + return S_OK; + } + + private: + BrowserHelperObject* bho_; + DISALLOW_COPY_AND_ASSIGN(BrokerEventQueue); + }; + + // This either calls its Impl or queues the call for later, when we have a + // tab_id. + HRESULT SendEventToBroker(const char* event_name, const char* event_args); + + // Sends the event to the broker, for real, without delay, right now. + HRESULT SendEventToBrokerImpl(const std::string& event_name, + const std::string& event_args); + + // List of calls to send to the broker. typedef std::deque<Task*> DeferredCallListType; - DeferredCallListType deferred_tab_id_call_; + DeferredCallListType deferred_events_call_; + + // This is passed to every funnel so they use the queue for sending events. + BrokerEventQueue broker_client_queue_; + + // Used to dispatch tab events back to Chrome. + TabEventsFunnel tab_events_funnel_; }; #endif // CEEE_IE_PLUGIN_BHO_BROWSER_HELPER_OBJECT_H_ diff --git a/ceee/ie/plugin/bho/browser_helper_object_unittest.cc b/ceee/ie/plugin/bho/browser_helper_object_unittest.cc index ce7c5a5..f37ac02 100644 --- a/ceee/ie/plugin/bho/browser_helper_object_unittest.cc +++ b/ceee/ie/plugin/bho/browser_helper_object_unittest.cc @@ -109,13 +109,15 @@ class TestingBrowserHelperObject return S_OK; } + virtual BrokerRpcClient& broker_rpc() { + return mock_broker_rpc_client_; + } + virtual HRESULT ConnectRpcBrokerClient() { - MockBrokerRpcClient* rpc_client = new MockBrokerRpcClient(); - EXPECT_CALL(*rpc_client, is_connected()).WillOnce(Return(true)); - EXPECT_CALL(*rpc_client, SendUmaHistogramTimes(_, _)) + EXPECT_CALL(mock_broker_rpc_client_, is_connected()).WillOnce(Return(true)); + EXPECT_CALL(mock_broker_rpc_client_, SendUmaHistogramTimes(_, _)) .WillRepeatedly(Return(true)); - EXPECT_CALL(*rpc_client, Disconnect()).Times(1); - broker_rpc_.reset(rpc_client); + EXPECT_CALL(mock_broker_rpc_client_, Disconnect()).Times(1); return S_OK; } @@ -159,6 +161,7 @@ class TestingBrowserHelperObject StrictMock<testing::MockTabEventsFunnel> mock_tab_events_funnel_; MockChromeFrameHost* mock_chrome_frame_host_; CComPtr<IChromeFrameHost> mock_chrome_frame_host_keeper_; + MockBrokerRpcClient mock_broker_rpc_client_; testing::MockExecutorIUnknown* executor_; CComPtr<IUnknown> executor_keeper_; @@ -194,6 +197,9 @@ class BrowserHelperObjectTest: public testing::Test { ASSERT_HRESULT_SUCCEEDED(testing::MockBroker::CreateInitialized( &bho_->broker_, &bho_->broker_keeper_)); + // At the start of a test we shouldn't have a tab_id. + bho_->SetTabId(kInvalidChromeSessionId); + // We always go beyond Chrome Frame start, broker reg and event funnel init. // TODO(mad@chromium.org): Also cover failure cases from those. ExpectBrokerRegistration(); @@ -264,13 +270,20 @@ class BrowserHelperObjectTest: public testing::Test { return false; } + void ChromeFrameStarted() { + // If we expect chrome to start, we have a tab_id. + bho_->SetTabId(kGoodTabId); + } + void ExpectChromeFrameStart() { EXPECT_CALL(*(bho_->mock_chrome_frame_host_), SetEventSink(_)). Times(1); EXPECT_CALL(*(bho_->mock_chrome_frame_host_), SetChromeProfileName(_)). Times(1); EXPECT_CALL(*(bho_->mock_chrome_frame_host_), StartChromeFrame()). - WillOnce(Return(S_OK)); + WillOnce(DoAll(InvokeWithoutArgs(this, + &BrowserHelperObjectTest::ChromeFrameStarted), + Return(S_OK))); } CComBSTR CreateTabInfo(int tab_id) { @@ -278,12 +291,6 @@ class BrowserHelperObjectTest: public testing::Test { iss << L"{\"id\":" << tab_id << L"}"; return CComBSTR(iss.str().c_str()); } - void ExpectChromeFrameGetSessionId() { - EXPECT_CALL(*(bho_->mock_chrome_frame_host_), GetSessionId(NotNull())). - WillOnce(DoAll(SetArgumentPointee<0>(kGoodTabId), Return(S_OK))); - EXPECT_CALL(*(bho_->broker_), SetTabIdForHandle(kGoodTabId, - kGoodTabHandle)).WillOnce(Return(S_OK)); - } void ExpectChromeFrameTearDown() { EXPECT_CALL(*(bho_->mock_chrome_frame_host_), SetEventSink(NULL)). @@ -434,10 +441,9 @@ TEST_F(BrowserHelperObjectTest, SetSiteWithBrowserSucceeds) { IID_IUnknown, reinterpret_cast<void**>(&set_site))); ASSERT_TRUE(set_site == site_keeper_); - // No teardown events are fired because neither the create event nor the tab - // ID mapping occurred. + // No OnRemoved isn't fired because the create event didn't occur. ExpectFireOnRemovedEvent(0); - ExpectFireOnUnmappedEvent(0); + ExpectFireOnUnmappedEvent(1); ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL)); // And check that the connection was severed. @@ -451,7 +457,6 @@ TEST_F(BrowserHelperObjectTest, OnNavigateCompleteHandled) { CreateSite(); CreateBrowser(); CreateHandler(); - ExpectChromeFrameGetSessionId(); // The site needs to return the top-level browser. site_->browser_ = browser_; @@ -476,7 +481,6 @@ TEST_F(BrowserHelperObjectTest, OnNavigateCompleteChromeFrameTab) { CreateSite(); CreateBrowser(); CreateHandler(); - ExpectChromeFrameGetSessionId(); // The site needs to return the top-level browser. site_->browser_ = browser_; @@ -511,7 +515,6 @@ TEST_F(BrowserHelperObjectTest, RenavigationNotifiesUrl) { CreateSite(); CreateBrowser(); CreateHandler(); - ExpectChromeFrameGetSessionId(); ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(site_keeper_)); @@ -561,10 +564,9 @@ TEST_F(BrowserHelperObjectTest, OnNavigateCompleteUnhandled) { // Non-BSTR url parameter. browser_->FireOnNavigateComplete(browser_, &CComVariant(non_browser)); - // No teardown events are fired because neither the create event nor the tab - // ID mapping occurred. + // No OnRemoved isn't fired because the create event didn't occur. ExpectFireOnRemovedEvent(0); - ExpectFireOnUnmappedEvent(0); + ExpectFireOnUnmappedEvent(1); ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL)); } @@ -623,7 +625,7 @@ TEST_F(BrowserHelperObjectTest, HandleNavigateComplete) { bho_->HandleNavigateComplete(browser_, CComBSTR(kUrl1)); ExpectFireOnRemovedEvent(1); - ExpectFireOnUnmappedEvent(0); + ExpectFireOnUnmappedEvent(1); ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL)); } @@ -651,7 +653,7 @@ TEST_F(BrowserHelperObjectTest, OnNavigationCompletedWithUnrelatedBrowser) { bho_->HandleNavigateComplete(browser2, CComBSTR(kUrl1)); ExpectFireOnRemovedEvent(0); - ExpectFireOnUnmappedEvent(0); + ExpectFireOnUnmappedEvent(1); ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL)); } @@ -706,10 +708,9 @@ TEST_F(BrowserHelperObjectTest, AttachOrphanedBrowser) { EXPECT_HRESULT_SUCCEEDED(bho_->AttachBrowser(browser3, browser3_parent, handler3)); - // No teardown events are fired because neither the create event nor the tab - // ID mapping occurred. + // No OnRemoved isn't fired because the create event didn't occur. ExpectFireOnRemovedEvent(0); - ExpectFireOnUnmappedEvent(0); + ExpectFireOnUnmappedEvent(1); ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL)); } @@ -733,8 +734,9 @@ TEST_F(BrowserHelperObjectTest, IFrameEventHandlerHost) { // But not twice. EXPECT_HRESULT_FAILED(bho_->DetachBrowser(browser_, NULL, handler_)); + // No OnRemoved isn't fired because the create event didn't occur. ExpectFireOnRemovedEvent(0); - ExpectFireOnUnmappedEvent(0); + ExpectFireOnUnmappedEvent(1); ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL)); // TODO(siggi@chromium.org): test hierarchial attach/detach/TearDown. @@ -744,7 +746,6 @@ TEST_F(BrowserHelperObjectTest, IFrameEventHandlerHostReadyStateChanged) { CreateSite(); CreateBrowser(); CreateHandler(); - ExpectChromeFrameGetSessionId(); ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(site_keeper_)); ASSERT_TRUE(BhoHasSite()); @@ -767,7 +768,6 @@ TEST_F(BrowserHelperObjectTest, InsertCode) { CreateSite(); CreateBrowser(); CreateHandler(); - ExpectChromeFrameGetSessionId(); ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(site_keeper_)); ASSERT_TRUE(BhoHasSite()); @@ -789,7 +789,6 @@ TEST_F(BrowserHelperObjectTest, InsertCodeAllFrames) { CreateSite(); CreateBrowser(); CreateHandler(); - ExpectChromeFrameGetSessionId(); ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(site_keeper_)); ASSERT_TRUE(BhoHasSite()); @@ -856,7 +855,7 @@ TEST_F(BrowserHelperObjectTest, IsHashChange) { EXPECT_FALSE(bho_->CallIsHashChange(url5, url6)); ExpectFireOnRemovedEvent(0); - ExpectFireOnUnmappedEvent(0); + ExpectFireOnUnmappedEvent(1); ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL)); } @@ -874,7 +873,7 @@ TEST_F(BrowserHelperObjectTest, SetToolBandSessionId) { EXPECT_EQ(E_FAIL, bho_->SetToolBandSessionId(kGoodTabId)); ExpectFireOnRemovedEvent(0); - ExpectFireOnUnmappedEvent(0); + ExpectFireOnUnmappedEvent(1); ASSERT_HRESULT_SUCCEEDED(bho_with_site_->SetSite(NULL)); } diff --git a/ceee/ie/plugin/bho/cookie_accountant.h b/ceee/ie/plugin/bho/cookie_accountant.h index f26b102..7b6c1b2 100644 --- a/ceee/ie/plugin/bho/cookie_accountant.h +++ b/ceee/ie/plugin/bho/cookie_accountant.h @@ -49,7 +49,10 @@ class CookieAccountant { protected: // Exposed to subclasses mainly for unit testing purposes; production code // should use the ProductionCookieAccountant class instead. - CookieAccountant() {} + // Since this class has one instance per window, not per tab, we cannot + // queue the events sent to the broker. They don't need to be sent to the BHO + // because they don't need tab_id anyway. + CookieAccountant() : cookie_events_funnel_(new BrokerRpcClient) {} virtual ~CookieAccountant(); // Records the modification or creation of a cookie. Fires off a diff --git a/ceee/ie/plugin/bho/cookie_events_funnel.h b/ceee/ie/plugin/bho/cookie_events_funnel.h index 27976da..271e8e4 100644 --- a/ceee/ie/plugin/bho/cookie_events_funnel.h +++ b/ceee/ie/plugin/bho/cookie_events_funnel.h @@ -14,6 +14,8 @@ class CookieEventsFunnel : public EventsFunnel { public: CookieEventsFunnel() {} + explicit CookieEventsFunnel(IEventSender* client) + : EventsFunnel(client) {} // Sends the cookies.onChanged event to the Broker. // @param removed True if the cookie was removed vs. set. diff --git a/ceee/ie/plugin/bho/events_funnel.cc b/ceee/ie/plugin/bho/events_funnel.cc index c73d1f2..e859cb3 100644 --- a/ceee/ie/plugin/bho/events_funnel.cc +++ b/ceee/ie/plugin/bho/events_funnel.cc @@ -15,12 +15,6 @@ #include "ceee/ie/common/ceee_module_util.h" -EventsFunnel::EventsFunnel() { -} - -EventsFunnel::~EventsFunnel() { -} - HRESULT EventsFunnel::SendEvent(const char* event_name, const Value& event_args) { // Event arguments for FireEventToBroker always need to be stored in a list. @@ -38,14 +32,13 @@ HRESULT EventsFunnel::SendEvent(const char* event_name, HRESULT EventsFunnel::SendEventToBroker(const char* event_name, const char* event_args) { - if (!broker_rpc_client_.get()) { - broker_rpc_client_.reset(new BrokerRpcClient()); - if (!broker_rpc_client_.get()) - return E_OUTOFMEMORY; - HRESULT hr = broker_rpc_client_->Connect(true); - // Don't reset broker_rpc_client_. See comment in *h file. - if (FAILED(hr)) - return hr; + DCHECK(broker_client_); + if (broker_client_) { + return broker_client_->FireEvent(event_name, event_args); + } else { + // Someone must be told... + LOG(WARNING) << "Fired event without receiver: name :\"" << event_name << + "\", args: \"" << event_args << "\"."; + return S_FALSE; } - return broker_rpc_client_->FireEvent(event_name, event_args); } diff --git a/ceee/ie/plugin/bho/events_funnel.h b/ceee/ie/plugin/bho/events_funnel.h index 2fcc19f..1d107e8 100644 --- a/ceee/ie/plugin/bho/events_funnel.h +++ b/ceee/ie/plugin/bho/events_funnel.h @@ -11,17 +11,19 @@ #include <windows.h> #include "base/basictypes.h" +#include "base/ref_counted.h" #include "base/scoped_ptr.h" +#include "ceee/ie/broker/broker_rpc_client.h" class Value; -class BrokerRpcClient; - // Defines a base class for sending events to the Broker. class EventsFunnel { protected: - EventsFunnel(); - virtual ~EventsFunnel(); + explicit EventsFunnel(IEventSender* client) : broker_client_(client) {} + EventsFunnel() : broker_client_(NULL) {} + + virtual ~EventsFunnel() {} // Send the given event to the Broker. // @param event_name The name of the event. @@ -29,14 +31,20 @@ class EventsFunnel { // protected virtual for testability... virtual HRESULT SendEvent(const char* event_name, const Value& event_args); + public: + virtual void AssignEventSender(IEventSender* client) { + broker_client_ = client; + } + protected: virtual HRESULT SendEventToBroker(const char* event_name, const char* event_args); private: - // Pointer to broker client. If is not NULL attempt to connect was performed - // and new attempts are not nessesary. - scoped_ptr<BrokerRpcClient> broker_rpc_client_; + // We're not responsible to send things to the broker directly. There is a + // client (which can be BrokerRpcClient, or not). The client should always + // outlive this class. + IEventSender* broker_client_; DISALLOW_COPY_AND_ASSIGN(EventsFunnel); }; diff --git a/ceee/ie/plugin/bho/executor.cc b/ceee/ie/plugin/bho/executor.cc index 929912f..cb6a553 100644 --- a/ceee/ie/plugin/bho/executor.cc +++ b/ceee/ie/plugin/bho/executor.cc @@ -422,8 +422,13 @@ HRESULT CeeeExecutor::Initialize(CeeeWindowHandle hwnd) { // If this is tab window then create the infobar manager. // TODO(mad@chromium.org): We are starting to need to have different classes // for the different executors. + // TODO(hansl@chromium.org): We might not need to have an Executor for + // Infobar. In any case, the construction below should have a reference to + // a BHO and its EventSender so we don't create Infobars before the tab_id + // is ready. if (window_utils::GetTopLevelParent(hwnd_) != hwnd_) - infobar_manager_.reset(new infobar_api::InfobarManager(hwnd_)); + infobar_manager_.reset( + new infobar_api::InfobarManager(hwnd_, new BrokerRpcClient)); return S_OK; } diff --git a/ceee/ie/plugin/bho/executor_unittest.cc b/ceee/ie/plugin/bho/executor_unittest.cc index 902c994..c367269 100644 --- a/ceee/ie/plugin/bho/executor_unittest.cc +++ b/ceee/ie/plugin/bho/executor_unittest.cc @@ -83,7 +83,7 @@ class TestingMockExecutorCreatorTeardown class MockInfobarManager : public infobar_api::InfobarManager { public: - MockInfobarManager() : InfobarManager(kGoodWindow) {} + MockInfobarManager() : InfobarManager(kGoodWindow, NULL) {} MOCK_METHOD4(Show, HRESULT(infobar_api::InfobarType, int, const std::wstring&, bool)); MOCK_METHOD1(Hide, HRESULT(infobar_api::InfobarType)); diff --git a/ceee/ie/plugin/bho/infobar_browser_window.cc b/ceee/ie/plugin/bho/infobar_browser_window.cc index c5de414..fa255b5c 100644 --- a/ceee/ie/plugin/bho/infobar_browser_window.cc +++ b/ceee/ie/plugin/bho/infobar_browser_window.cc @@ -27,7 +27,8 @@ _ATL_FUNC_INFO InfobarBrowserWindow::handler_type_bstr_i4_= _ATL_FUNC_INFO InfobarBrowserWindow::handler_type_void_= { CC_STDCALL, VT_EMPTY, 0, { } }; -InfobarBrowserWindow::InfobarBrowserWindow() : delegate_(NULL) { +InfobarBrowserWindow::InfobarBrowserWindow() + : delegate_(NULL), infobar_events_funnel_(new InfobarEventsFunnel) { } InfobarBrowserWindow::~InfobarBrowserWindow() { @@ -82,7 +83,9 @@ STDMETHODIMP_(void) InfobarBrowserWindow::OnCfClose() { delegate_->OnBrowserWindowClose(); } -HRESULT InfobarBrowserWindow::Initialize(BSTR url, Delegate* delegate) { +HRESULT InfobarBrowserWindow::Initialize(BSTR url, Delegate* delegate, + IEventSender* event_sender) { + infobar_events_funnel_.reset(new InfobarEventsFunnel(event_sender)); set_delegate(delegate); SetUrl(url); diff --git a/ceee/ie/plugin/bho/infobar_browser_window.h b/ceee/ie/plugin/bho/infobar_browser_window.h index fa4d8a7..568162c 100644 --- a/ceee/ie/plugin/bho/infobar_browser_window.h +++ b/ceee/ie/plugin/bho/infobar_browser_window.h @@ -123,11 +123,11 @@ class ATL_NO_VTABLE InfobarBrowserWindow // @} // Initializes the browser window to the given site. - HRESULT Initialize(BSTR url, Delegate* delegate); + HRESULT Initialize(BSTR url, Delegate* delegate, IEventSender* event_sender); // Unit test seam. virtual InfobarEventsFunnel& infobar_events_funnel() { - return infobar_events_funnel_; + return *infobar_events_funnel_.get(); } protected: @@ -141,7 +141,7 @@ class ATL_NO_VTABLE InfobarBrowserWindow private: // The funnel for sending infobar events to the broker. - InfobarEventsFunnel infobar_events_funnel_; + scoped_ptr<InfobarEventsFunnel> infobar_events_funnel_; // Our Chrome frame instance. CComPtr<IChromeFrame> chrome_frame_; diff --git a/ceee/ie/plugin/bho/infobar_events_funnel.h b/ceee/ie/plugin/bho/infobar_events_funnel.h index f4a7d58..a8dbd45 100644 --- a/ceee/ie/plugin/bho/infobar_events_funnel.h +++ b/ceee/ie/plugin/bho/infobar_events_funnel.h @@ -13,6 +13,7 @@ class InfobarEventsFunnel : public EventsFunnel { public: InfobarEventsFunnel() {} + explicit InfobarEventsFunnel(IEventSender* client) : EventsFunnel(client) {} // Sends the infobar.onDocumentComplete event to the Broker. virtual HRESULT OnDocumentComplete(); diff --git a/ceee/ie/plugin/bho/infobar_manager.cc b/ceee/ie/plugin/bho/infobar_manager.cc index 30ff9a9..b78cdfa 100644 --- a/ceee/ie/plugin/bho/infobar_manager.cc +++ b/ceee/ie/plugin/bho/infobar_manager.cc @@ -143,8 +143,8 @@ class InfobarManager::ContainerWindow DISALLOW_COPY_AND_ASSIGN(ContainerWindow); }; -InfobarManager::InfobarManager(HWND tab_window) - : tab_window_(tab_window) { +InfobarManager::InfobarManager(HWND tab_window, IEventSender* event_sender) + : tab_window_(tab_window), event_sender_(event_sender) { } HRESULT InfobarManager::Show(InfobarType type, int max_height, @@ -284,7 +284,8 @@ void InfobarManager::LazyInitialize(InfobarType type) { // Note that when InfobarManager is being initialized the IE has not created // the tab. Therefore we cannot find the container window here and have to // pass interface for a function that finds windows to be called later. - infobars_[type].reset(InfobarWindow::CreateInfobar(type, this)); + infobars_[type].reset(InfobarWindow::CreateInfobar(type, this, + event_sender_)); } InfobarManager::ContainerWindowInterface* InfobarManager::CreateContainerWindow( diff --git a/ceee/ie/plugin/bho/infobar_manager.h b/ceee/ie/plugin/bho/infobar_manager.h index d09e3d9..2ecf2f8 100644 --- a/ceee/ie/plugin/bho/infobar_manager.h +++ b/ceee/ie/plugin/bho/infobar_manager.h @@ -20,7 +20,7 @@ namespace infobar_api { class InfobarManager : public InfobarWindow::Delegate, public WebBrowserEventsSource::Sink { public: - explicit InfobarManager(HWND tab_window); + InfobarManager(HWND tab_window, IEventSender* event_sender); // Shows the infobar of the specified type and navigates it to the specified // URL. @@ -63,6 +63,9 @@ class InfobarManager : public InfobarWindow::Delegate, // Infobar windows. scoped_ptr<InfobarWindow> infobars_[END_OF_INFOBAR_TYPE]; + // Passed to the InfobarWindow during initialization. + IEventSender* event_sender_; + // ContainerWindow callbacks. // Callback for WM_NCCALCSIZE. void OnContainerWindowNcCalcSize(RECT* rect); diff --git a/ceee/ie/plugin/bho/infobar_manager_unittest.cc b/ceee/ie/plugin/bho/infobar_manager_unittest.cc index 610634e..391598e 100644 --- a/ceee/ie/plugin/bho/infobar_manager_unittest.cc +++ b/ceee/ie/plugin/bho/infobar_manager_unittest.cc @@ -41,8 +41,10 @@ class MockInfobarDelegate : public infobar_api::InfobarWindow::Delegate { class MockInfobarWindow : public infobar_api::InfobarWindow { public: + // Unfortunately, until we have a full link from BHO to Infobar, it's no use + // to use a mock RpcClient here. So we use NULL instead. MockInfobarWindow(infobar_api::InfobarType type, Delegate* delegate) - : infobar_api::InfobarWindow(type, delegate) {} + : infobar_api::InfobarWindow(type, delegate, NULL) {} MOCK_METHOD2(InternalCreate, HWND(HWND, DWORD)); MOCK_METHOD2(Show, HRESULT(int, bool)); MOCK_METHOD0(Hide, HRESULT()); @@ -67,7 +69,7 @@ class TestingInfobarManager : public infobar_api::InfobarManager { }; explicit TestingInfobarManager(HWND tab_window) - : infobar_api::InfobarManager(tab_window) { + : infobar_api::InfobarManager(tab_window, NULL) { } MockInfobarWindow* GetInfobarWindow(infobar_api::InfobarType type) { diff --git a/ceee/ie/plugin/bho/infobar_window.cc b/ceee/ie/plugin/bho/infobar_window.cc index d484670..9c3b3ad 100644 --- a/ceee/ie/plugin/bho/infobar_window.cc +++ b/ceee/ie/plugin/bho/infobar_window.cc @@ -30,19 +30,23 @@ const int kInfobarDefaultHeight = 39; namespace infobar_api { InfobarWindow* InfobarWindow::CreateInfobar(InfobarType type, - Delegate* delegate) { + Delegate* delegate, + IEventSender* event_sender) { DCHECK(delegate); - return NULL == delegate ? NULL : new InfobarWindow(type, delegate); + return NULL == delegate ? NULL : new InfobarWindow(type, delegate, + event_sender); } -InfobarWindow::InfobarWindow(InfobarType type, Delegate* delegate) +InfobarWindow::InfobarWindow(InfobarType type, Delegate* delegate, + IEventSender* event_sender) : type_(type), delegate_(delegate), show_(false), target_height_(1), current_height_(1), sliding_infobar_(false), - timer_id_(0) { + timer_id_(0), + event_sender_(event_sender) { DCHECK(delegate); } @@ -275,7 +279,7 @@ LRESULT InfobarWindow::OnTimer(UINT_PTR nIDEvent) { LRESULT InfobarWindow::OnCreate(LPCREATESTRUCT lpCreateStruct) { InitializingCoClass<InfobarBrowserWindow>::CreateInitialized( - CComBSTR(url_.c_str()), this, &chrome_frame_host_); + CComBSTR(url_.c_str()), this, event_sender_, &chrome_frame_host_); if (chrome_frame_host_) { chrome_frame_host_->CreateAndShowWindow(m_hWnd); diff --git a/ceee/ie/plugin/bho/infobar_window.h b/ceee/ie/plugin/bho/infobar_window.h index 9d8aa5c..d165b36 100644 --- a/ceee/ie/plugin/bho/infobar_window.h +++ b/ceee/ie/plugin/bho/infobar_window.h @@ -42,7 +42,8 @@ class InfobarWindow : public InfobarBrowserWindow::Delegate, virtual void OnWindowClose(InfobarType type) = 0; }; - static InfobarWindow* CreateInfobar(InfobarType type, Delegate* delegate); + static InfobarWindow* CreateInfobar(InfobarType type, Delegate* delegate, + IEventSender* event_sender); ~InfobarWindow(); // Implementation of InfobarBrowserWindow::Delegate. @@ -113,8 +114,13 @@ class InfobarWindow : public InfobarBrowserWindow::Delegate, // The Chrome Frame host handling a Chrome Frame instance for us. CComPtr<IInfobarBrowserWindow> chrome_frame_host_; + // The event sender object, where the InfobarEventsFunnel will send its + // events. + IEventSender* event_sender_; + // Constructor. - InfobarWindow(InfobarType type, Delegate* delegate); + InfobarWindow(InfobarType type, Delegate* delegate, + IEventSender* event_sender); // If show is true, shrinks IE content window and shows the infobar // either at the top or at the bottom. Otherwise, hides the infobar and diff --git a/ceee/ie/plugin/bho/infobar_window_unittest.cc b/ceee/ie/plugin/bho/infobar_window_unittest.cc index 378b09f..45af6bb 100644 --- a/ceee/ie/plugin/bho/infobar_window_unittest.cc +++ b/ceee/ie/plugin/bho/infobar_window_unittest.cc @@ -55,10 +55,12 @@ class MockInfobarWindowDelegate : public infobar_api::InfobarWindow::Delegate { class TestingInfobarWindow : public infobar_api::InfobarWindow { public: + // Until we have a real use of BrokerRpc, we just pass a NULL pointer to + // InfobarWindow. TestingInfobarWindow(infobar_api::InfobarType type, InfobarWindow::Delegate* delegate, MockInfobarBrowserWindow* browser_window) - : infobar_api::InfobarWindow(type, delegate) { + : infobar_api::InfobarWindow(type, delegate, NULL) { chrome_frame_host_ = browser_window; } }; diff --git a/ceee/ie/plugin/bho/mediumtest_browser_helper_object.cc b/ceee/ie/plugin/bho/mediumtest_browser_helper_object.cc index 217e04d..8f8b8a9 100644 --- a/ceee/ie/plugin/bho/mediumtest_browser_helper_object.cc +++ b/ceee/ie/plugin/bho/mediumtest_browser_helper_object.cc @@ -27,6 +27,7 @@ namespace { using testing::_; using testing::AnyNumber; +using testing::AtMost; using testing::BrowserEventSinkBase; using testing::GetTestUrl; using testing::DoAll; @@ -88,7 +89,7 @@ class TestingBrowserHelperObject WebProgressNotifier* CreateWebProgressNotifier() { scoped_ptr<WebProgressNotifier> web_progress_notifier( new TestingWebProgressNotifier()); - HRESULT hr = web_progress_notifier->Initialize(this, tab_window_, + HRESULT hr = web_progress_notifier->Initialize(this, NULL, tab_window_, web_browser_); return SUCCEEDED(hr) ? web_progress_notifier.release() : NULL; } @@ -132,21 +133,27 @@ class TestingBrowserHelperObject .WillOnce(Return(S_OK)); EXPECT_CALL(*mock_chrome_frame_host_, GetSessionId(NotNull())) + .Times(AtMost(1)) .WillOnce(DoAll(SetArgumentPointee<0>(44), Return(S_OK))); + // We expect 0 or 1 calls, but not more. Some tests don't generate calls + // to events; those won't call SetTabIdForHandle. EXPECT_CALL(*broker_, SetTabIdForHandle(44, _)) + .Times(AtMost(1)) .WillOnce(Return(S_OK)); } return hr; } + virtual BrokerRpcClient& broker_rpc() { + return mock_broker_rpc_client_; + } + virtual HRESULT ConnectRpcBrokerClient() { - MockBrokerRpcClient* rpc_client = new MockBrokerRpcClient(); - EXPECT_CALL(*rpc_client, is_connected()).WillOnce(Return(true)); - EXPECT_CALL(*rpc_client, SendUmaHistogramTimes(_, _)) + EXPECT_CALL(mock_broker_rpc_client_, is_connected()).WillOnce(Return(true)); + EXPECT_CALL(mock_broker_rpc_client_, SendUmaHistogramTimes(_, _)) .WillRepeatedly(Return(true)); - EXPECT_CALL(*rpc_client, Disconnect()).Times(1); - broker_rpc_.reset(rpc_client); + EXPECT_CALL(mock_broker_rpc_client_, Disconnect()).Times(1); return S_OK; } @@ -235,10 +242,12 @@ class TestingBrowserHelperObject testing::MockBroker* broker_; CComPtr<ICeeeBrokerRegistrar> broker_keeper_; + MockBrokerRpcClient mock_broker_rpc_client_; + private: class TestingWebProgressNotifier : public WebProgressNotifier { public: - class TesingNavigationEventsFunne : public WebNavigationEventsFunnel { + class TestingNavigationEventsFunnel : public WebNavigationEventsFunnel { public: HRESULT SendEventToBroker(const char*, const char*) { return S_OK; @@ -246,11 +255,10 @@ class TestingBrowserHelperObject }; virtual WebNavigationEventsFunnel* webnavigation_events_funnel() { - if (!webnavigation_events_funnel_.get()) - webnavigation_events_funnel_.reset(new TesingNavigationEventsFunne()); - - return webnavigation_events_funnel_.get(); + return &webnavigation_events_funnel_; } + + TestingNavigationEventsFunnel webnavigation_events_funnel_; }; MockChromeFrameHost* mock_chrome_frame_host_; @@ -340,7 +348,8 @@ class BrowserHelperObjectTest: public ShellBrowserTestImpl<BrowserEventSink> { virtual void TearDown() { if (bho_ != NULL) { // To match SetUp failure modes. EXPECT_CALL(*bho_->mock_tab_events_funnel(), OnRemoved(_)); - EXPECT_CALL(*bho_->mock_tab_events_funnel(), OnTabUnmapped(_, _)); + EXPECT_CALL(*bho_->mock_tab_events_funnel(), OnTabUnmapped(_, _)) + .Times(AtMost(1)); ASSERT_HRESULT_SUCCEEDED(bho_keeper_->SetSite(NULL)); bho_->executor_ = NULL; diff --git a/ceee/ie/plugin/bho/tab_events_funnel.h b/ceee/ie/plugin/bho/tab_events_funnel.h index 6c21fb9..650457c 100644 --- a/ceee/ie/plugin/bho/tab_events_funnel.h +++ b/ceee/ie/plugin/bho/tab_events_funnel.h @@ -9,6 +9,7 @@ #include <ocidl.h> // for READYSTATE +#include "base/ref_counted.h" #include "ceee/ie/plugin/bho/events_funnel.h" // Implements a set of methods to send tab related events to the Broker. @@ -16,6 +17,8 @@ class TabEventsFunnel : public EventsFunnel { public: TabEventsFunnel() {} + explicit TabEventsFunnel(IEventSender* client) : EventsFunnel(client) {} + // Sends the tabs.onMoved event to the Broker. // @param tab_handle The HWND of the tab that moved. // @param window_id The identifier of the window containing the moving tab. diff --git a/ceee/ie/plugin/bho/web_progress_notifier.cc b/ceee/ie/plugin/bho/web_progress_notifier.cc index dc2e3912..567c543 100644 --- a/ceee/ie/plugin/bho/web_progress_notifier.cc +++ b/ceee/ie/plugin/bho/web_progress_notifier.cc @@ -54,6 +54,7 @@ WebProgressNotifier::~WebProgressNotifier() { HRESULT WebProgressNotifier::Initialize( WebBrowserEventsSource* web_browser_events_source, + IEventSender* client, HWND tab_window, IWebBrowser2* main_browser) { if (web_browser_events_source == NULL || tab_window == NULL || @@ -76,6 +77,7 @@ HRESULT WebProgressNotifier::Initialize( web_browser_events_source_ = web_browser_events_source; web_browser_events_source_->RegisterSink(this); + webnavigation_events_funnel_.reset(new WebNavigationEventsFunnel(client)); window_message_source_.reset(CreateWindowMessageSource()); if (window_message_source_ == NULL) { @@ -332,8 +334,6 @@ void WebProgressNotifier::OnHandleMessage( } WebNavigationEventsFunnel* WebProgressNotifier::webnavigation_events_funnel() { - if (!webnavigation_events_funnel_.get()) - webnavigation_events_funnel_.reset(new WebNavigationEventsFunnel()); return webnavigation_events_funnel_.get(); } diff --git a/ceee/ie/plugin/bho/web_progress_notifier.h b/ceee/ie/plugin/bho/web_progress_notifier.h index d92e0c6..1b2c191 100644 --- a/ceee/ie/plugin/bho/web_progress_notifier.h +++ b/ceee/ie/plugin/bho/web_progress_notifier.h @@ -30,6 +30,7 @@ class WebProgressNotifier : public WebBrowserEventsSource::Sink, HRESULT Initialize( WebBrowserEventsSource* web_browser_events_source, + IEventSender* client, HWND tab_window, IWebBrowser2* main_browser); void TearDown(); diff --git a/ceee/ie/plugin/bho/web_progress_notifier_unittest.cc b/ceee/ie/plugin/bho/web_progress_notifier_unittest.cc index d68ec78..662254a 100644 --- a/ceee/ie/plugin/bho/web_progress_notifier_unittest.cc +++ b/ceee/ie/plugin/bho/web_progress_notifier_unittest.cc @@ -180,7 +180,7 @@ class WebProgressNotifierTestFixture : public testing::Test { web_progress_notifier_.reset(new TestWebProgressNotifier()); ASSERT_HRESULT_SUCCEEDED(web_progress_notifier_->Initialize( - web_browser_events_source_.get(), reinterpret_cast<HWND>(1024), + web_browser_events_source_.get(), NULL, reinterpret_cast<HWND>(1024), web_browser_)); } diff --git a/ceee/ie/plugin/bho/webnavigation_events_funnel.h b/ceee/ie/plugin/bho/webnavigation_events_funnel.h index 6138faf..744a8ea 100644 --- a/ceee/ie/plugin/bho/webnavigation_events_funnel.h +++ b/ceee/ie/plugin/bho/webnavigation_events_funnel.h @@ -18,6 +18,8 @@ class WebNavigationEventsFunnel : public EventsFunnel { public: WebNavigationEventsFunnel() {} + explicit WebNavigationEventsFunnel(IEventSender* client) + : EventsFunnel(client) {} // Sends the webNavigation.onBeforeNavigate event to the Broker. // @param tab_handle The window handle of the tab in which the navigation is diff --git a/ceee/ie/plugin/bho/webnavigation_events_funnel_unittest.cc b/ceee/ie/plugin/bho/webnavigation_events_funnel_unittest.cc index bc7d7f9b..87d307a 100644 --- a/ceee/ie/plugin/bho/webnavigation_events_funnel_unittest.cc +++ b/ceee/ie/plugin/bho/webnavigation_events_funnel_unittest.cc @@ -27,6 +27,7 @@ MATCHER_P(ValuesEqual, value, "") { class TestWebNavigationEventsFunnel : public WebNavigationEventsFunnel { public: + TestWebNavigationEventsFunnel() {} MOCK_METHOD2(SendEvent, HRESULT(const char*, const Value&)); }; diff --git a/ceee/ie/plugin/bho/webrequest_events_funnel.h b/ceee/ie/plugin/bho/webrequest_events_funnel.h index f59d162..d5c81f0 100644 --- a/ceee/ie/plugin/bho/webrequest_events_funnel.h +++ b/ceee/ie/plugin/bho/webrequest_events_funnel.h @@ -19,6 +19,8 @@ class WebRequestEventsFunnel : public EventsFunnel { public: WebRequestEventsFunnel() {} + explicit WebRequestEventsFunnel(IEventSender* client) + : EventsFunnel(client) {} // Sends the webRequest.onBeforeRedirect event to the broker. // @param request_id The ID of the request. diff --git a/ceee/ie/plugin/bho/webrequest_notifier.cc b/ceee/ie/plugin/bho/webrequest_notifier.cc index e0449e4..4661616 100644 --- a/ceee/ie/plugin/bho/webrequest_notifier.cc +++ b/ceee/ie/plugin/bho/webrequest_notifier.cc @@ -29,9 +29,10 @@ const char kInternetReadFileFunctionName[] = "InternetReadFile"; } // namespace WebRequestNotifier::WebRequestNotifier() - : internet_status_callback_stub_(NULL), - start_count_(0), - initialize_state_(NOT_INITIALIZED) { + : internet_status_callback_stub_(NULL), + start_count_(0), + initialize_state_(NOT_INITIALIZED), + webrequest_events_funnel_(new BrokerRpcClient) { } WebRequestNotifier::~WebRequestNotifier() { diff --git a/ceee/ie/testing/mock_broker_and_friends.h b/ceee/ie/testing/mock_broker_and_friends.h index 5658a48..e05344f 100644 --- a/ceee/ie/testing/mock_broker_and_friends.h +++ b/ceee/ie/testing/mock_broker_and_friends.h @@ -174,6 +174,8 @@ class MockTabEventsFunnel : public TabEventsFunnel { MOCK_METHOD3(OnUpdated, HRESULT(HWND tab, BSTR url, READYSTATE ready_state)); MOCK_METHOD2(OnTabUnmapped, HRESULT(HWND tab, int tab_id)); + + MOCK_METHOD2(SendEventToBroker, HRESULT(const char*, const char*)); }; class MockWindowEventsFunnel : public WindowEventsFunnel { |