From 130efb094a3c96bda00da28615b5c1e0481b7218 Mon Sep 17 00:00:00 2001
From: "jcampan@chromium.org"
<jcampan@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Fri, 18 Sep 2009 18:54:35 +0000
Subject: [PATCH] This CL makes the browser focus tests faster by replacing
some time-outs with notifications.
BUG=22065
TEST=Run the interactive tests, especially BrowserFocusTest*FocusTraversal*
These tests should run in few seconds.
Review URL: http://codereview.chromium.org/210013
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@26597 0039d316-1c4b-4281-b951-d872f2087c98
---
chrome/browser/browser.cc | 8 ++
chrome/browser/browser.h | 1 +
chrome/browser/browser_focus_uitest.cc | 106 +++++++++++-------
.../browser/renderer_host/render_view_host.cc | 4 +
.../browser/renderer_host/render_view_host.h | 1 +
.../renderer_host/render_view_host_delegate.h | 3 +
.../renderer_host/render_widget_host.cc | 4 +
.../renderer_host/render_widget_host.h | 2 +
chrome/browser/tab_contents/tab_contents.cc | 7 ++
chrome/browser/tab_contents/tab_contents.h | 1 +
.../tab_contents/tab_contents_view_gtk.cc | 6 +-
chrome/common/notification_type.h | 9 +-
chrome/common/render_messages_internal.h | 1 +
chrome/renderer/render_widget.cc | 6 +
chrome/test/ui_test_utils.cc | 11 ++
chrome/test/ui_test_utils.h | 7 ++
16 files changed, 135 insertions(+), 42 deletions(-)
diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index 88cf354ff1dfc..a8e546930c018 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -1999,6 +1999,14 @@ void Browser::TabContentsFocused(TabContents* tab_content) {
window_->TabContentsFocused(tab_content);
}
+bool Browser::TakeFocus(bool reverse) {
+ NotificationService::current()->Notify(
+ NotificationType::FOCUS_RETURNED_TO_BROWSER,
+ Source<Browser>(this),
+ NotificationService::NoDetails());
+ return false;
+}
+
bool Browser::IsApplication() const {
return (type_ & TYPE_APP) != 0;
}
diff --git a/chrome/browser/browser.h b/chrome/browser/browser.h
index 5635c23e8d5de..f5c1bd6b0bd1f 100644
--- a/chrome/browser/browser.h
+++ b/chrome/browser/browser.h
@@ -515,6 +515,7 @@ class Browser : public TabStripModelDelegate,
virtual void ContentsMouseEvent(TabContents* source, bool motion);
virtual void ContentsZoomChange(bool zoom_in);
virtual void TabContentsFocused(TabContents* tab_content);
+ virtual bool TakeFocus(bool reverse);
virtual bool IsApplication() const;
virtual void ConvertContentsToApplication(TabContents* source);
virtual bool ShouldDisplayURLField();
diff --git a/chrome/browser/browser_focus_uitest.cc b/chrome/browser/browser_focus_uitest.cc
index d2cab010b15ae..89abf91806f7b 100644
--- a/chrome/browser/browser_focus_uitest.cc
+++ b/chrome/browser/browser_focus_uitest.cc
@@ -147,7 +147,8 @@ class TestInterstitialPage : public InterstitialPage {
public:
TestInterstitialPage(TabContents* tab, bool new_navigation, const GURL& url)
: InterstitialPage(tab, new_navigation, url),
- waiting_for_dom_response_(false) {
+ waiting_for_dom_response_(false),
+ waiting_for_focus_change_(false) {
FilePath file_path;
bool r = PathService::Get(chrome::DIR_TEST_DATA, &file_path);
EXPECT_TRUE(r);
@@ -192,10 +193,25 @@ class TestInterstitialPage : public InterstitialPage {
return render_view_host()->view()->HasFocus();
}
+ void WaitForFocusChange() {
+ waiting_for_focus_change_ = true;
+ ui_test_utils::RunMessageLoop();
+ }
+
+ protected:
+ virtual void FocusedNodeChanged() {
+ if (!waiting_for_focus_change_)
+ return;
+
+ waiting_for_focus_change_= false;
+ MessageLoop::current()->PostTask(FROM_HERE, new MessageLoop::QuitTask());
+ }
+
private:
std::string html_contents_;
bool waiting_for_dom_response_;
+ bool waiting_for_focus_change_;
std::string dom_response_;
};
@@ -396,6 +412,10 @@ IN_PROC_BROWSER_TEST_F(BrowserFocusTest, LocationBarLockFocus) {
}
// Focus traversal on a regular page.
+// Note that this test relies on a notification from the renderer that the
+// focus has changed in the page. The notification in the renderer may change
+// at which point this test would fail (see comment in
+// RenderWidget::didFocus()).
IN_PROC_BROWSER_TEST_F(BrowserFocusTest, FocusTraversal) {
HTTPTestServer* server = StartHTTPServer();
@@ -420,7 +440,7 @@ IN_PROC_BROWSER_TEST_F(BrowserFocusTest, FocusTraversal) {
CheckViewHasFocus(VIEW_ID_LOCATION_BAR);
// Now let's press tab to move the focus.
- for (int j = 0; j < 7; ++j) {
+ for (size_t j = 0; j < arraysize(kExpElementIDs); ++j) {
// Let's make sure the focus is on the expected element in the page.
std::string actual;
ASSERT_TRUE(ui_test_utils::ExecuteJavaScriptAndExtractString(
@@ -430,21 +450,22 @@ IN_PROC_BROWSER_TEST_F(BrowserFocusTest, FocusTraversal) {
&actual));
ASSERT_STREQ(kExpElementIDs[j], actual.c_str());
- ui_controls::SendKeyPressNotifyWhenDone(window, base::VKEY_TAB, false,
- false, false,
- new MessageLoop::QuitTask());
- ui_test_utils::RunMessageLoop();
- // Ideally, we wouldn't sleep here and instead would use the event
- // processed ack notification from the renderer. I am reluctant to create
- // a new notification/callback for that purpose just for this test.
- PlatformThread::Sleep(kActionDelayMs);
+ ASSERT_TRUE(ui_controls::SendKeyPress(window, base::VKEY_TAB,
+ false, false, false));
+
+ if (j < arraysize(kExpElementIDs) - 1) {
+ ui_test_utils::WaitForFocusChange(browser()->GetSelectedTabContents()->
+ render_view_host());
+ } else {
+ // On the last tab key press, the focus returns to the browser.
+ ui_test_utils::WaitForFocusInBrowser(browser());
+ }
}
// At this point the renderer has sent us a message asking to advance the
// focus (as the end of the focus loop was reached in the renderer).
// We need to run the message loop to process it.
- MessageLoop::current()->PostTask(FROM_HERE, new MessageLoop::QuitTask());
- ui_test_utils::RunMessageLoop();
+ MessageLoop::current()->RunAllPending();
}
// Now let's try reverse focus traversal.
@@ -453,12 +474,17 @@ IN_PROC_BROWSER_TEST_F(BrowserFocusTest, FocusTraversal) {
CheckViewHasFocus(VIEW_ID_LOCATION_BAR);
// Now let's press shift-tab to move the focus in reverse.
- for (int j = 0; j < 7; ++j) {
- ui_controls::SendKeyPressNotifyWhenDone(window, base::VKEY_TAB, false,
- true, false,
- new MessageLoop::QuitTask());
- ui_test_utils::RunMessageLoop();
- PlatformThread::Sleep(kActionDelayMs);
+ for (size_t j = 0; j < 7; ++j) {
+ ASSERT_TRUE(ui_controls::SendKeyPress(window, base::VKEY_TAB,
+ false, true, false));
+
+ if (j < arraysize(kExpElementIDs) - 1) {
+ ui_test_utils::WaitForFocusChange(browser()->GetSelectedTabContents()->
+ render_view_host());
+ } else {
+ // On the last tab key press, the focus returns to the browser.
+ ui_test_utils::WaitForFocusInBrowser(browser());
+ }
// Let's make sure the focus is on the expected element in the page.
std::string actual;
@@ -473,8 +499,7 @@ IN_PROC_BROWSER_TEST_F(BrowserFocusTest, FocusTraversal) {
// At this point the renderer has sent us a message asking to advance the
// focus (as the end of the focus loop was reached in the renderer).
// We need to run the message loop to process it.
- MessageLoop::current()->PostTask(FROM_HERE, new MessageLoop::QuitTask());
- ui_test_utils::RunMessageLoop();
+ MessageLoop::current()->RunAllPending();
}
}
@@ -517,26 +542,26 @@ IN_PROC_BROWSER_TEST_F(BrowserFocusTest, MAYBE_FocusTraversalOnInterstitial) {
CheckViewHasFocus(VIEW_ID_LOCATION_BAR);
// Now let's press tab to move the focus.
- for (int j = 0; j < 7; ++j) {
+ for (size_t j = 0; j < 7; ++j) {
// Let's make sure the focus is on the expected element in the page.
std::string actual = interstitial_page->GetFocusedElement();
ASSERT_STREQ(kExpElementIDs[j], actual.c_str());
- ui_controls::SendKeyPressNotifyWhenDone(window, base::VKEY_TAB, false,
- false, false,
- new MessageLoop::QuitTask());
- ui_test_utils::RunMessageLoop();
- // Ideally, we wouldn't sleep here and instead would use the event
- // processed ack notification from the renderer. I am reluctant to create
- // a new notification/callback for that purpose just for this test.
- PlatformThread::Sleep(kActionDelayMs);
+ ASSERT_TRUE(ui_controls::SendKeyPress(window, base::VKEY_TAB,
+ false, false, false));
+
+ if (j < arraysize(kExpElementIDs) - 1) {
+ interstitial_page->WaitForFocusChange();
+ } else {
+ // On the last tab key press, the focus returns to the browser.
+ ui_test_utils::WaitForFocusInBrowser(browser());
+ }
}
// At this point the renderer has sent us a message asking to advance the
// focus (as the end of the focus loop was reached in the renderer).
// We need to run the message loop to process it.
- MessageLoop::current()->PostTask(FROM_HERE, new MessageLoop::QuitTask());
- ui_test_utils::RunMessageLoop();
+ MessageLoop::current()->RunAllPending();
}
// Now let's try reverse focus traversal.
@@ -545,12 +570,16 @@ IN_PROC_BROWSER_TEST_F(BrowserFocusTest, MAYBE_FocusTraversalOnInterstitial) {
CheckViewHasFocus(VIEW_ID_LOCATION_BAR);
// Now let's press shift-tab to move the focus in reverse.
- for (int j = 0; j < 7; ++j) {
- ui_controls::SendKeyPressNotifyWhenDone(window, base::VKEY_TAB, false,
- true, false,
- new MessageLoop::QuitTask());
- ui_test_utils::RunMessageLoop();
- PlatformThread::Sleep(kActionDelayMs);
+ for (size_t j = 0; j < 7; ++j) {
+ ASSERT_TRUE(ui_controls::SendKeyPress(window, base::VKEY_TAB,
+ false, true, false));
+
+ if (j < arraysize(kExpElementIDs) - 1) {
+ interstitial_page->WaitForFocusChange();
+ } else {
+ // On the last tab key press, the focus returns to the browser.
+ ui_test_utils::WaitForFocusInBrowser(browser());
+ }
// Let's make sure the focus is on the expected element in the page.
std::string actual = interstitial_page->GetFocusedElement();
@@ -560,8 +589,7 @@ IN_PROC_BROWSER_TEST_F(BrowserFocusTest, MAYBE_FocusTraversalOnInterstitial) {
// At this point the renderer has sent us a message asking to advance the
// focus (as the end of the focus loop was reached in the renderer).
// We need to run the message loop to process it.
- MessageLoop::current()->PostTask(FROM_HERE, new MessageLoop::QuitTask());
- ui_test_utils::RunMessageLoop();
+ MessageLoop::current()->RunAllPending();
}
}
diff --git a/chrome/browser/renderer_host/render_view_host.cc b/chrome/browser/renderer_host/render_view_host.cc
index 932bf89d7d8b6..63cc15e1f9480 100644
--- a/chrome/browser/renderer_host/render_view_host.cc
+++ b/chrome/browser/renderer_host/render_view_host.cc
@@ -1572,6 +1572,10 @@ void RenderViewHost::NotifyRendererResponsive() {
delegate_->RendererResponsive(this);
}
+void RenderViewHost::OnMsgFocusedNodeChanged() {
+ delegate_->FocusedNodeChanged();
+}
+
gfx::Rect RenderViewHost::GetRootWindowResizerRect() const {
return delegate_->GetRootWindowResizerRect();
}
diff --git a/chrome/browser/renderer_host/render_view_host.h b/chrome/browser/renderer_host/render_view_host.h
index 52a0c101776dc..b7f2d86da6787 100644
--- a/chrome/browser/renderer_host/render_view_host.h
+++ b/chrome/browser/renderer_host/render_view_host.h
@@ -452,6 +452,7 @@ class RenderViewHost : public RenderWidgetHost,
virtual void OnUserGesture();
virtual void NotifyRendererUnresponsive();
virtual void NotifyRendererResponsive();
+ virtual void OnMsgFocusedNodeChanged();
// IPC message handlers.
void OnMsgShowView(int route_id,
diff --git a/chrome/browser/renderer_host/render_view_host_delegate.h b/chrome/browser/renderer_host/render_view_host_delegate.h
index c1e301cf2fe85..74dc44ef40940 100644
--- a/chrome/browser/renderer_host/render_view_host_delegate.h
+++ b/chrome/browser/renderer_host/render_view_host_delegate.h
@@ -544,6 +544,9 @@ class RenderViewHostDelegate {
// The RenderView has inserted one css file into page.
virtual void DidInsertCSS() {}
+
+ // A different node in the page got focused.
+ virtual void FocusedNodeChanged() {}
};
#endif // CHROME_BROWSER_RENDERER_HOST_RENDER_VIEW_HOST_DELEGATE_H_
diff --git a/chrome/browser/renderer_host/render_widget_host.cc b/chrome/browser/renderer_host/render_widget_host.cc
index 05b90c98a73e7..2bdf11cf86c1e 100644
--- a/chrome/browser/renderer_host/render_widget_host.cc
+++ b/chrome/browser/renderer_host/render_widget_host.cc
@@ -132,6 +132,7 @@ void RenderWidgetHost::OnMessageReceived(const IPC::Message &msg) {
IPC_MESSAGE_HANDLER(ViewHostMsg_HandleInputEvent_ACK, OnMsgInputEventAck)
IPC_MESSAGE_HANDLER(ViewHostMsg_Focus, OnMsgFocus)
IPC_MESSAGE_HANDLER(ViewHostMsg_Blur, OnMsgBlur)
+ IPC_MESSAGE_HANDLER(ViewHostMsg_FocusedNodeChanged, OnMsgFocusedNodeChanged)
IPC_MESSAGE_HANDLER(ViewHostMsg_SetCursor, OnMsgSetCursor)
IPC_MESSAGE_HANDLER(ViewHostMsg_ImeUpdateStatus, OnMsgImeUpdateStatus)
#if defined(OS_LINUX)
@@ -778,6 +779,9 @@ void RenderWidgetHost::OnMsgBlur() {
}
}
+void RenderWidgetHost::OnMsgFocusedNodeChanged() {
+}
+
void RenderWidgetHost::OnMsgSetCursor(const WebCursor& cursor) {
if (!view_) {
return;
diff --git a/chrome/browser/renderer_host/render_widget_host.h b/chrome/browser/renderer_host/render_widget_host.h
index b76d4c7b6dc53..a80d1fde3794b 100644
--- a/chrome/browser/renderer_host/render_widget_host.h
+++ b/chrome/browser/renderer_host/render_widget_host.h
@@ -406,6 +406,8 @@ class RenderWidgetHost : public IPC::Channel::Listener,
void OnMsgInputEventAck(const IPC::Message& message);
void OnMsgFocus();
void OnMsgBlur();
+ virtual void OnMsgFocusedNodeChanged();
+
void OnMsgSetCursor(const WebCursor& cursor);
// Using int instead of ViewHostMsg_ImeControl for control's type to avoid
// having to bring in render_messages.h in a header file.
diff --git a/chrome/browser/tab_contents/tab_contents.cc b/chrome/browser/tab_contents/tab_contents.cc
index c7c1c37bf9c09..1568790eebd51 100644
--- a/chrome/browser/tab_contents/tab_contents.cc
+++ b/chrome/browser/tab_contents/tab_contents.cc
@@ -2410,6 +2410,13 @@ void TabContents::DidInsertCSS() {
// This RVHDelegate function is used for extensions and not us.
}
+void TabContents::FocusedNodeChanged() {
+ NotificationService::current()->Notify(
+ NotificationType::FOCUS_CHANGED_IN_PAGE,
+ Source<RenderViewHost>(render_view_host()),
+ NotificationService::NoDetails());
+}
+
void TabContents::FileSelected(const FilePath& path,
int index, void* params) {
render_view_host()->FileSelected(path);
diff --git a/chrome/browser/tab_contents/tab_contents.h b/chrome/browser/tab_contents/tab_contents.h
index 61404f3098d3c..cd8ec58af7f4e 100644
--- a/chrome/browser/tab_contents/tab_contents.h
+++ b/chrome/browser/tab_contents/tab_contents.h
@@ -897,6 +897,7 @@ class TabContents : public PageNavigator,
virtual void LoadStateChanged(const GURL& url, net::LoadState load_state);
virtual bool IsExternalTabContainer() const;
virtual void DidInsertCSS();
+ virtual void FocusedNodeChanged();
// SelectFileDialog::Listener ------------------------------------------------
diff --git a/chrome/browser/tab_contents/tab_contents_view_gtk.cc b/chrome/browser/tab_contents/tab_contents_view_gtk.cc
index 6326ae19dc1fb..32ac7cf0ac63c 100644
--- a/chrome/browser/tab_contents/tab_contents_view_gtk.cc
+++ b/chrome/browser/tab_contents/tab_contents_view_gtk.cc
@@ -549,8 +549,10 @@ void TabContentsViewGtk::GotFocus() {
// This is called when we the renderer asks us to take focus back (i.e., it has
// iterated past the last focusable element on the page).
void TabContentsViewGtk::TakeFocus(bool reverse) {
- gtk_widget_child_focus(GTK_WIDGET(GetTopLevelNativeWindow()),
- reverse ? GTK_DIR_TAB_BACKWARD : GTK_DIR_TAB_FORWARD);
+ if (!tab_contents()->delegate()->TakeFocus(reverse)) {
+ gtk_widget_child_focus(GTK_WIDGET(GetTopLevelNativeWindow()),
+ reverse ? GTK_DIR_TAB_BACKWARD : GTK_DIR_TAB_FORWARD);
+ }
}
void TabContentsViewGtk::HandleKeyboardEvent(
diff --git a/chrome/common/notification_type.h b/chrome/common/notification_type.h
index dad6dafb2de8a..72e0d131406f5 100644
--- a/chrome/common/notification_type.h
+++ b/chrome/common/notification_type.h
@@ -222,6 +222,10 @@ class NotificationType {
// Linux.
ACTIVE_WINDOW_CHANGED,
+ // Sent when the renderer returns focus to the browser, as part of focus
+ // traversal. The source is the browser, there are no details.
+ FOCUS_RETURNED_TO_BROWSER,
+
// Application-modal dialogs -----------------------------------------------
// Sent after an application-modal dialog has been shown. The source
@@ -324,7 +328,6 @@ class NotificationType {
// pointer.
RENDER_VIEW_HOST_CREATED_FOR_TAB,
-
// Stuff inside the tabs ---------------------------------------------------
// This message is sent after a constrained window has been closed. The
@@ -405,6 +408,10 @@ class NotificationType {
// guaranteed to be valid after the notification.
WEB_CACHE_STATS_OBSERVED,
+ // The focused element inside a page has changed. The source is the render
+ // view host for the page, there are no details.
+ FOCUS_CHANGED_IN_PAGE,
+
// Child Processes ---------------------------------------------------------
// This notification is sent when a child process host has connected to a
diff --git a/chrome/common/render_messages_internal.h b/chrome/common/render_messages_internal.h
index da53fef60ce1a..700f3a1bc9639 100644
--- a/chrome/common/render_messages_internal.h
+++ b/chrome/common/render_messages_internal.h
@@ -898,6 +898,7 @@ IPC_BEGIN_MESSAGES(ViewHost)
IPC_MESSAGE_ROUTED0(ViewHostMsg_Focus)
IPC_MESSAGE_ROUTED0(ViewHostMsg_Blur)
+ IPC_MESSAGE_ROUTED0(ViewHostMsg_FocusedNodeChanged)
// Returns the window location of the given window.
// TODO(shess): Provide a mapping from reply_msg->routing_id() to
diff --git a/chrome/renderer/render_widget.cc b/chrome/renderer/render_widget.cc
index 3aa58e3f2ae5f..0d7fadffc2938 100644
--- a/chrome/renderer/render_widget.cc
+++ b/chrome/renderer/render_widget.cc
@@ -650,6 +650,12 @@ void RenderWidget::show(WebNavigationPolicy) {
}
void RenderWidget::didFocus() {
+ // Note that didFocus() is invoked everytime a new node is focused in the
+ // page. It could be expected that it would be called only when the widget
+ // gets the focus. If the current behavior was to change in WebKit for the
+ // expected one, the following notification would not work anymore.
+ Send(new ViewHostMsg_FocusedNodeChanged(routing_id_));
+
// Prevent the widget from stealing the focus if it does not have focus
// already. We do this by explicitely setting the focus to false again.
// We only let the browser focus the renderer.
diff --git a/chrome/test/ui_test_utils.cc b/chrome/test/ui_test_utils.cc
index e92f26099f2ab..a793cca69355f 100644
--- a/chrome/test/ui_test_utils.cc
+++ b/chrome/test/ui_test_utils.cc
@@ -419,4 +419,15 @@ void CrashTab(TabContents* tab) {
crash_observer(NotificationType::RENDERER_PROCESS_CLOSED, rph);
}
+void WaitForFocusChange(RenderViewHost* rvh) {
+ SimpleNotificationObserver<RenderViewHost>
+ focus_observer(NotificationType::FOCUS_CHANGED_IN_PAGE, rvh);
+}
+
+void WaitForFocusInBrowser(Browser* browser) {
+ SimpleNotificationObserver<Browser>
+ focus_observer(NotificationType::FOCUS_RETURNED_TO_BROWSER,
+ browser);
+}
+
} // namespace ui_test_utils
diff --git a/chrome/test/ui_test_utils.h b/chrome/test/ui_test_utils.h
index b623027f3a57d..ec7d6a5f48407 100644
--- a/chrome/test/ui_test_utils.h
+++ b/chrome/test/ui_test_utils.h
@@ -104,6 +104,13 @@ AppModalDialog* WaitForAppModalDialog();
// Causes the specified tab to crash. Blocks until it is crashed.
void CrashTab(TabContents* tab);
+
+// Waits for the focus to change in the specified RenderViewHost.
+void WaitForFocusChange(RenderViewHost* rvh);
+
+// Waits for the renderer to return focus to the browser (happens through tab
+// traversal).
+void WaitForFocusInBrowser(Browser* browser);
}
#endif // CHROME_TEST_UI_TEST_UTILS_H_
--
GitLab