diff options
author | mad@google.com <mad@google.com@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-11-09 20:41:54 +0000 |
---|---|---|
committer | mad@google.com <mad@google.com@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-11-09 20:41:54 +0000 |
commit | c4556418578b2c31744e82f78d3b12901bb48176 (patch) | |
tree | 5c44bdc1e3d5bbd6ccef9f772e8b043a80a4af05 | |
parent | aa613d6a7ac972a04dd8cdacf2f859f94a8323f1 (diff) | |
download | chromium_src-c4556418578b2c31744e82f78d3b12901bb48176.zip chromium_src-c4556418578b2c31744e82f78d3b12901bb48176.tar.gz chromium_src-c4556418578b2c31744e82f78d3b12901bb48176.tar.bz2 |
Commit for vadimb to improved the way InfobarBrowserWindow is instantiated.
Taken from:
http://codereview.chromium.org/4648004/show
Improved the way InfobarBrowserWindow is instantiated.
2. Fixed AddRef/Release correspondence.
3. Fixed an assertion if infobar is closed before it was opened.
4. Renamed InfobarBrowserWindow::Delegate::OnWindowClose to OnBrowserWindowClose to reduce confusion with InfobarWindow::Delegate::OnWindowClose.
5. Created unit tests for infobar_api_module.
BUG=none
TEST=none
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@65567 0039d316-1c4b-4281-b951-d872f2087c98
-rw-r--r-- | ceee/ie/broker/infobar_api_module.cc | 19 | ||||
-rw-r--r-- | ceee/ie/broker/infobar_api_module_unittest.cc | 100 | ||||
-rw-r--r-- | ceee/ie/plugin/bho/executor.cc | 2 | ||||
-rw-r--r-- | ceee/ie/plugin/bho/infobar_browser_window.cc | 36 | ||||
-rw-r--r-- | ceee/ie/plugin/bho/infobar_browser_window.h | 43 | ||||
-rw-r--r-- | ceee/ie/plugin/bho/infobar_window.cc | 32 | ||||
-rw-r--r-- | ceee/ie/plugin/bho/infobar_window.h | 4 |
7 files changed, 180 insertions, 56 deletions
diff --git a/ceee/ie/broker/infobar_api_module.cc b/ceee/ie/broker/infobar_api_module.cc index f857b4a..ca7a46f 100644 --- a/ceee/ie/broker/infobar_api_module.cc +++ b/ceee/ie/broker/infobar_api_module.cc @@ -74,13 +74,18 @@ void ShowInfoBar::Execute(const ListValue& args, int request_id) { return; } - // Store the window information returned by ShowInfobar(). - result->CreateWindowValue(dispatcher->GetWindowHandleFromId(window_handle), - false); - - // Now wait until the document is complete before responding. - dispatcher->RegisterEphemeralEventHandler(kOnDocumentCompleteEventName, - ShowInfoBar::ContinueExecution, result.release()); + // If we created a window then we have to wait until the document is complete + // before responding. If window is not created then we can post result now. + if (0 == window_handle) { + result->PostResult(); + } else { + // Store the window information returned by ShowInfobar(). + result->CreateWindowValue(dispatcher->GetWindowHandleFromId(window_handle), + false); + + dispatcher->RegisterEphemeralEventHandler(kOnDocumentCompleteEventName, + ShowInfoBar::ContinueExecution, result.release()); + } } HRESULT ShowInfoBar::ContinueExecution( diff --git a/ceee/ie/broker/infobar_api_module_unittest.cc b/ceee/ie/broker/infobar_api_module_unittest.cc index d9bce10..7eee1ac 100644 --- a/ceee/ie/broker/infobar_api_module_unittest.cc +++ b/ceee/ie/broker/infobar_api_module_unittest.cc @@ -8,6 +8,7 @@ // incompatibility with atlwin.h. #include "ceee/testing/utils/mock_win32.h" // NOLINT +#include "ceee/ie/broker/api_module_constants.h" #include "ceee/ie/broker/infobar_api_module.h" #include "ceee/ie/testing/mock_broker_and_friends.h" #include "ceee/testing/utils/mock_window_utils.h" @@ -40,8 +41,9 @@ const int kGoodFrameWindowId = 88; const int kRequestId = 43; const char kHtmlPath[] = "/infobar/test.html"; -const HWND kGoodTabWindow = (HWND)(kGoodTabWindowId + 1); -const HWND kGoodFrameWindow = (HWND)(kGoodFrameWindowId + 1); +const HWND kGoodTabWindow = reinterpret_cast<HWND>(kGoodTabWindowId + 1); +const HWND kGoodFrameWindow = reinterpret_cast<HWND>(kGoodFrameWindowId + 1); +const HWND kNoWindow = reinterpret_cast<HWND>(NULL); TEST(InfobarApi, RegisterInvocations) { StrictMock<MockApiDispatcher> disp; @@ -53,6 +55,7 @@ class MockInfobarApiResult : public InfobarApiResult { public: explicit MockInfobarApiResult(int request_id) : InfobarApiResult(request_id) {} + MOCK_METHOD2(CreateWindowValue, bool(HWND, bool)); MOCK_METHOD0(PostResult, void()); MOCK_METHOD1(PostError, void(const std::string&)); @@ -96,6 +99,14 @@ class InfobarApiTests: public testing::Test { AddRef(mock_infobar_executor_keeper_.p))); } + void SetGoodArgs(ListValue* good_args) { + ASSERT_TRUE(good_args != NULL); + DictionaryValue dict; + dict.Set(ext::kTabId, Value::CreateIntegerValue(kGoodTabWindowId)); + dict.Set(ext::kHtmlPath, Value::CreateStringValue(std::string(kHtmlPath))); + ASSERT_TRUE(good_args->Set(0, dict.DeepCopy())); + } + StrictMock<testing::MockUser32> user32_; // The executor classes are already strict from their base class impl. testing::MockInfobarExecutor* mock_infobar_executor_; @@ -106,17 +117,12 @@ class InfobarApiTests: public testing::Test { }; TEST_F(InfobarApiTests, ShowInfoBarNoErrors) { - // TODO(vadimb@google.com): Make the implementation work. -#if 0 testing::LogDisabler no_dchecks; + StrictMock<testing::MockWindowUtils> window_utils; - DictionaryValue dict; - dict.Set(ext::kTabId, Value::CreateIntegerValue(kGoodTabWindowId)); - dict.Set(ext::kHtmlPath, Value::CreateStringValue(std::string(kHtmlPath))); ListValue good_args; - ASSERT_TRUE(good_args.Set(0, dict.DeepCopy())); + SetGoodArgs(&good_args); - StrictMock<testing::MockWindowUtils> window_utils; EXPECT_CALL(window_utils, IsWindowClass(kGoodTabWindow, _)). WillRepeatedly(Return(true)); @@ -131,14 +137,86 @@ TEST_F(InfobarApiTests, ShowInfoBarNoErrors) { MockGetInfobarExecutorOnce(&invocation.mock_api_dispatcher_, kGoodTabWindow); CComBSTR html_path(kHtmlPath); CeeeWindowHandle window_handle = - reinterpret_cast<CeeeWindowHandle>(kGoodFrameWindow); + static_cast<CeeeWindowHandle>(kGoodFrameWindowId); EXPECT_CALL(*mock_infobar_executor_, ShowInfobar(StrEq(html_path.m_str), _)). WillOnce(DoAll(SetArgumentPointee<1>(window_handle), Return(S_OK))); + EXPECT_CALL(invocation.mock_api_dispatcher_, RegisterEphemeralEventHandler( + _, infobar_api::ShowInfoBar::ContinueExecution, _)).Times(1); + + invocation.AllocateNewResult(kRequestId); + EXPECT_CALL(*invocation.invocation_result_, + CreateWindowValue(_, false)). + WillOnce(Return(true)); + EXPECT_CALL(*invocation.invocation_result_, PostResult()).Times(1); + + ApiDispatcher::InvocationResult* result = invocation.invocation_result_.get(); + invocation.Execute(good_args, kRequestId); + + std::ostringstream args; + EXPECT_HRESULT_SUCCEEDED(invocation.ContinueExecution( + args.str(), result, invocation.GetDispatcher())); +} + +TEST_F(InfobarApiTests, ShowInfoBarTestErrors) { + testing::LogDisabler no_dchecks; + StrictMock<testing::MockWindowUtils> window_utils; + + StrictMock<MockApiInvocation<InfobarApiResult, MockInfobarApiResult, + ShowInfoBar> > invocation; + invocation.AllocateNewResult(kRequestId); + + // Test no parameters at all. + EXPECT_CALL(*invocation.invocation_result_, + PostError(api_module_constants::kInvalidArgumentsError)).Times(1); + ListValue bad_args; + invocation.Execute(bad_args, kRequestId); + // Test no tabId parameter. + DictionaryValue dict; + dict.Set(ext::kTabId, Value::CreateIntegerValue(kGoodTabWindowId)); + ASSERT_TRUE(bad_args.Set(0, dict.DeepCopy())); + invocation.AllocateNewResult(kRequestId); + EXPECT_CALL(*invocation.invocation_result_, + PostError(api_module_constants::kInvalidArgumentsError)).Times(1); + invocation.Execute(bad_args, kRequestId); + + // Good parameters for the next tests. + ListValue good_args; + SetGoodArgs(&good_args); + + // Test wrong tab id. + invocation.AllocateNewResult(kRequestId); + EXPECT_CALL(invocation.mock_api_dispatcher_, + GetTabHandleFromId(kGoodTabWindowId)).WillOnce(Return(kNoWindow)); + EXPECT_CALL(window_utils, IsWindowClass(kNoWindow, _)). + WillOnce(Return(false)); + EXPECT_CALL(*invocation.invocation_result_, PostError(_)).Times(1); + invocation.Execute(good_args, kRequestId); + + // Test failed executor access. invocation.AllocateNewResult(kRequestId); + EXPECT_CALL(invocation.mock_api_dispatcher_, + GetTabHandleFromId(kGoodTabWindowId)).WillOnce(Return(kGoodTabWindow)); + EXPECT_CALL(window_utils, IsWindowClass(kGoodTabWindow, _)). + WillOnce(Return(true)); + EXPECT_CALL(invocation.mock_api_dispatcher_, + GetExecutor(kGoodTabWindow, _, NotNull())). + WillOnce(SetArgumentPointee<2>(static_cast<void*>(NULL))); + EXPECT_CALL(*invocation.invocation_result_, PostError(_)).Times(1); + invocation.Execute(good_args, kRequestId); + // Test failed ShowInfobar. + invocation.AllocateNewResult(kRequestId); + EXPECT_CALL(invocation.mock_api_dispatcher_, + GetTabHandleFromId(kGoodTabWindowId)).WillOnce(Return(kGoodTabWindow)); + EXPECT_CALL(window_utils, IsWindowClass(kGoodTabWindow, _)). + WillOnce(Return(true)); + MockGetInfobarExecutorOnce(&invocation.mock_api_dispatcher_, kGoodTabWindow); + CComBSTR html_path(kHtmlPath); + EXPECT_CALL(*mock_infobar_executor_, ShowInfobar(StrEq(html_path.m_str), _)). + WillOnce(Return(E_FAIL)); + EXPECT_CALL(*invocation.invocation_result_, PostError(_)).Times(1); invocation.Execute(good_args, kRequestId); -#endif } } // namespace diff --git a/ceee/ie/plugin/bho/executor.cc b/ceee/ie/plugin/bho/executor.cc index d7719f1..d32410f 100644 --- a/ceee/ie/plugin/bho/executor.cc +++ b/ceee/ie/plugin/bho/executor.cc @@ -732,6 +732,8 @@ STDMETHODIMP CeeeExecutor::ShowInfobar(BSTR url, // how to treat this. size_t url_string_length = SysStringLen(url); if (0 == url_string_length) { + if (window_handle != NULL) + *window_handle = 0; infobar_manager_->HideAll(); return S_OK; } diff --git a/ceee/ie/plugin/bho/infobar_browser_window.cc b/ceee/ie/plugin/bho/infobar_browser_window.cc index fecf5e6..49fbd85 100644 --- a/ceee/ie/plugin/bho/infobar_browser_window.cc +++ b/ceee/ie/plugin/bho/infobar_browser_window.cc @@ -87,10 +87,17 @@ STDMETHODIMP_(void) InfobarBrowserWindow::OnCfExtensionReady(BSTR path, STDMETHODIMP_(void) InfobarBrowserWindow::OnCfClose() { if (delegate_ != NULL) - delegate_->OnWindowClose(); + delegate_->OnBrowserWindowClose(); } - HRESULT InfobarBrowserWindow::Initialize(HWND parent) { +HRESULT InfobarBrowserWindow::Initialize(BSTR url, Delegate* delegate) { + set_delegate(delegate); + SetUrl(url); + + return S_OK; +} + +HRESULT InfobarBrowserWindow::CreateAndShowWindow(HWND parent) { HRESULT hr = InitializeAndShowWindow(parent); if (FAILED(hr)) { LOG(ERROR) << "Infobar browser failed to initialize its site window: " << @@ -112,6 +119,8 @@ HRESULT InfobarBrowserWindow::InitializeAndShowWindow(HWND parent) { } HRESULT InfobarBrowserWindow::Teardown() { + set_delegate(NULL); + if (IsWindow()) { // Teardown the ActiveX host window. CAxWindow host(m_hWnd); @@ -125,20 +134,27 @@ HRESULT InfobarBrowserWindow::Teardown() { if (chrome_frame_) { ChromeFrameEvents::DispEventUnadvise(chrome_frame_); + chrome_frame_.Release(); } return S_OK; } -void InfobarBrowserWindow::SetUrl(const std::wstring& url) { +STDMETHODIMP InfobarBrowserWindow::SetUrl(BSTR url) { // Navigate to the URL if the browser exists, otherwise just store the URL. url_ = url; Navigate(); + return S_OK; } +STDMETHODIMP InfobarBrowserWindow::SetWindowSize(int width, int height) { + if (IsWindow()) + SetWindowPos(NULL, 0, 0, width, height, SWP_NOACTIVATE | SWP_NOZORDER); + return S_OK; +} LRESULT InfobarBrowserWindow::OnCreate(LPCREATESTRUCT lpCreateStruct) { - // Grab a self-reference. + // Grab a self-reference. Released in OnFinalMessage. GetUnknown()->AddRef(); // Create a host window instance. @@ -147,7 +163,7 @@ LRESULT InfobarBrowserWindow::OnCreate(LPCREATESTRUCT lpCreateStruct) { if (FAILED(hr)) { LOG(ERROR) << "Infobar failed to create ActiveX host window. " << com::LogHr(hr); - return 1; + return -1; } // We're the site for the host window, this needs to be in place @@ -161,7 +177,7 @@ LRESULT InfobarBrowserWindow::OnCreate(LPCREATESTRUCT lpCreateStruct) { if (FAILED(hr)) { LOG(ERROR) << "Failed to create the Chrome Frame instance. " << com::LogHr(hr); - return 1; + return -1; } // And attach it to our window. @@ -169,7 +185,7 @@ LRESULT InfobarBrowserWindow::OnCreate(LPCREATESTRUCT lpCreateStruct) { if (FAILED(hr)) { LOG(ERROR) << "Failed to attach Chrome Frame to the host. " << com::LogHr(hr); - return 1; + return -1; } // Hook up the chrome frame event listener. @@ -178,7 +194,7 @@ LRESULT InfobarBrowserWindow::OnCreate(LPCREATESTRUCT lpCreateStruct) { LOG(ERROR) << "Failed to hook up event sink. " << com::LogHr(hr); } - return S_OK; + return 0; } void InfobarBrowserWindow::OnDestroy() { @@ -188,6 +204,10 @@ void InfobarBrowserWindow::OnDestroy() { } } +void InfobarBrowserWindow::OnFinalMessage(HWND window) { + GetUnknown()->Release(); +} + void InfobarBrowserWindow::Navigate() { // If the browser has not been created then just return. if (!chrome_frame_) diff --git a/ceee/ie/plugin/bho/infobar_browser_window.h b/ceee/ie/plugin/bho/infobar_browser_window.h index fd6c975..7e6dbcb 100644 --- a/ceee/ie/plugin/bho/infobar_browser_window.h +++ b/ceee/ie/plugin/bho/infobar_browser_window.h @@ -30,16 +30,32 @@ class InfobarBrowserWindow; typedef IDispEventSimpleImpl<0, InfobarBrowserWindow, &DIID_DIChromeFrameEvents> ChromeFrameEvents; +class __declspec(uuid("1AE72DA8-1E4E-4101-8D5D-D7859A366D60")) +IInfobarBrowserWindow : public IUnknown { + public: + // Creates and shows window. + STDMETHOD(CreateAndShowWindow)(HWND parent) = 0; + // Navigates the browser to the given URL if the browser has already been + // created, otherwise stores the URL to navigate later on. + STDMETHOD(SetUrl)(BSTR url) = 0; + // Sets the window size for the browser window. + STDMETHOD(SetWindowSize)(int width, int height) = 0; + // Tear the window down. + STDMETHOD(Teardown)() = 0; +}; + // The window that hosts CF where infobar URL is loaded. It implements a limited // site functionality needed for CF as well as handles sink events from CF. // TODO(vadimb@google.com): Refactor this class, ChromeFrameHost and ToolBand // to have a common functionality supported in a common base class. class ATL_NO_VTABLE InfobarBrowserWindow : public CComObjectRootEx<CComSingleThreadModel>, + public InitializingCoClass<InfobarBrowserWindow>, public IObjectWithSiteImpl<InfobarBrowserWindow>, public IServiceProviderImpl<InfobarBrowserWindow>, public IChromeFramePrivileged, public ChromeFrameEvents, + public IInfobarBrowserWindow, public CWindowImpl<InfobarBrowserWindow> { public: // Class to connect this an instance of InfobarBrowserWindow with a hosting @@ -49,7 +65,7 @@ class ATL_NO_VTABLE InfobarBrowserWindow public: virtual ~Delegate() {} // Informs about window.close() event. - virtual void OnWindowClose() = 0; + virtual void OnBrowserWindowClose() = 0; }; InfobarBrowserWindow(); @@ -58,6 +74,7 @@ class ATL_NO_VTABLE InfobarBrowserWindow BEGIN_COM_MAP(InfobarBrowserWindow) COM_INTERFACE_ENTRY(IServiceProvider) COM_INTERFACE_ENTRY(IChromeFramePrivileged) + COM_INTERFACE_ENTRY(IInfobarBrowserWindow) END_COM_MAP() BEGIN_SERVICE_MAP(InfobarBrowserWindow) @@ -97,17 +114,16 @@ class ATL_NO_VTABLE InfobarBrowserWindow STDMETHOD_(void, OnCfClose)(); // @} - // Initializes the browser window to the given site. - HRESULT Initialize(HWND parent); - // Tears down an initialized browser window. - HRESULT Teardown(); - - // Navigates the browser to the given URL if the browser has already been - // created, otherwise stores the URL to navigate later on. - void SetUrl(const std::wstring& url); + // @name IInfobarBrowserWindow implementation. + // @{ + STDMETHOD(CreateAndShowWindow)(HWND parent); + STDMETHOD(SetUrl)(BSTR url); + STDMETHOD(SetWindowSize)(int width, int height); + STDMETHOD(Teardown)(); + // @} - // Set the delegate to be informed about window.close() events. - void set_delegate(Delegate* delegate) { delegate_ = delegate; } + // Initializes the browser window to the given site. + HRESULT Initialize(BSTR url, Delegate* delegate); // Unit test seam. virtual InfobarEventsFunnel& infobar_events_funnel() { @@ -121,6 +137,8 @@ class ATL_NO_VTABLE InfobarBrowserWindow void OnDestroy(); // @} + virtual void OnFinalMessage(HWND window); + private: // The funnel for sending infobar events to the broker. InfobarEventsFunnel infobar_events_funnel_; @@ -145,6 +163,9 @@ class ATL_NO_VTABLE InfobarBrowserWindow // Subroutine of general initialization. Extracted to make testable. virtual HRESULT InitializeAndShowWindow(HWND parent); + // Set the delegate to be informed about window.close() events. + void set_delegate(Delegate* delegate) { delegate_ = delegate; } + // Navigate the browser to url_ if the browser has been created. void Navigate(); diff --git a/ceee/ie/plugin/bho/infobar_window.cc b/ceee/ie/plugin/bho/infobar_window.cc index 62e9301..f7c06c1 100644 --- a/ceee/ie/plugin/bho/infobar_window.cc +++ b/ceee/ie/plugin/bho/infobar_window.cc @@ -55,7 +55,7 @@ InfobarWindow::~InfobarWindow() { } } -void InfobarWindow::OnWindowClose() { +void InfobarWindow::OnBrowserWindowClose() { // Propagate the event to the manager. if (delegate_ != NULL) delegate_->OnWindowClose(type_); @@ -70,7 +70,12 @@ HRESULT InfobarWindow::Show(int max_height, bool slide) { } HRESULT InfobarWindow::Hide() { - StartUpdatingLayout(false, 0, false); + // This could be called on the infobar that was not yet shown if show infobar + // function is called with an empty URL (see CeeeExecutor::ShowInfobar + // implementation). Therefore we should check if window exists and do not + // treat this as an error if not. + if (IsWindow()) + StartUpdatingLayout(false, 0, false); return S_OK; } @@ -81,7 +86,7 @@ HRESULT InfobarWindow::Navigate(const std::wstring& url) { // it will be created. url_ = url; if (chrome_frame_host_) - chrome_frame_host_->SetUrl(url_); + chrome_frame_host_->SetUrl(CComBSTR(url_.c_str())); return S_OK; } @@ -122,7 +127,7 @@ void InfobarWindow::Reset() { Hide(); if (chrome_frame_host_) - chrome_frame_host_->set_delegate(NULL); + chrome_frame_host_->Teardown(); DCHECK(!show_ && !sliding_infobar_); if (m_hWnd != NULL) { @@ -266,17 +271,11 @@ LRESULT InfobarWindow::OnTimer(UINT_PTR nIDEvent) { } LRESULT InfobarWindow::OnCreate(LPCREATESTRUCT lpCreateStruct) { - // TODO(vadimb@google.com): Better way to do this is to derive - // InfobarBrowserWindow from InitializingCoClass and give it an - // InitializeMethod, then create it with - // InfobarBrowserWindow::CreateInitialized(...). - CComObject<InfobarBrowserWindow>* chrome_frame_host = NULL; - CComObject<InfobarBrowserWindow>::CreateInstance(&chrome_frame_host); - if (chrome_frame_host) { - chrome_frame_host_.Attach(chrome_frame_host); - chrome_frame_host_->SetUrl(url_); - chrome_frame_host_->Initialize(m_hWnd); - chrome_frame_host_->set_delegate(this); + InitializingCoClass<InfobarBrowserWindow>::CreateInitialized( + CComBSTR(url_.c_str()), this, &chrome_frame_host_); + + if (chrome_frame_host_) { + chrome_frame_host_->CreateAndShowWindow(m_hWnd); AdjustSize(); } return S_OK; @@ -306,8 +305,7 @@ void InfobarWindow::AdjustSize() { if (NULL != chrome_frame_host_) { CRect rect; GetClientRect(&rect); - chrome_frame_host_->SetWindowPos(NULL, 0, 0, rect.Width(), rect.Height(), - SWP_NOACTIVATE | SWP_NOZORDER); + chrome_frame_host_->SetWindowSize(rect.Width(), rect.Height()); } } diff --git a/ceee/ie/plugin/bho/infobar_window.h b/ceee/ie/plugin/bho/infobar_window.h index e745cef..6b373b06 100644 --- a/ceee/ie/plugin/bho/infobar_window.h +++ b/ceee/ie/plugin/bho/infobar_window.h @@ -47,7 +47,7 @@ class InfobarWindow : public InfobarBrowserWindow::Delegate, // Implementation of InfobarBrowserWindow::Delegate. // Informs about window.close() event. - virtual void OnWindowClose(); + virtual void OnBrowserWindowClose(); // Shows the infobar. // NOTE: Navigate should be called before Show. @@ -104,7 +104,7 @@ class InfobarWindow : public InfobarBrowserWindow::Delegate, bool sliding_infobar_; // The Chrome Frame host handling a Chrome Frame instance for us. - CComPtr<InfobarBrowserWindow> chrome_frame_host_; + CComPtr<IInfobarBrowserWindow> chrome_frame_host_; // Constructor. InfobarWindow(InfobarType type, Delegate* delegate); |