Add a test for the mouseleave event on window.

chrome/browser/mouseleave_browsertest.cc

Index: chrome/browser/mouseleave_browsertest.cc
diff --git a/chrome/browser/mouseleave_browsertest.cc b/chrome/browser/mouseleave_browsertest.cc
index 416b081f112b51b7f0acaec195cefb78fef5393a..a2b20557362b5878e1189e61c74a7f7c6e4ce9fc 100644
--- a/chrome/browser/mouseleave_browsertest.cc
+++ b/chrome/browser/mouseleave_browsertest.cc
@@ -4,13 +4,23 @@
 
 #include "base/files/file_path.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/app/chrome_command_ids.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/renderer_context_menu/render_view_context_menu.h"
+#include "chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_commands.h"
+#include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "components/app_modal/app_modal_dialog.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test_utils.h"
+#include "content/public/test/test_navigation_observer.h"
 #include "ui/base/test/ui_controls.h"
 
 namespace {
@@ -19,46 +29,46 @@ class MouseLeaveTest : public InProcessBrowserTest {
  public:
   MouseLeaveTest() {}
 
-  void MouseLeaveTestCommon() {
-    GURL test_url = ui_test_utils::GetTestUrl(
-        base::FilePath(), base::FilePath(FILE_PATH_LITERAL("mouseleave.html")));
-
-    content::WebContents* tab =
-        browser()->tab_strip_model()->GetActiveWebContents();
+  void NavigateAndLoadContent(content::WebContents* tab, GURL& url) {

[pkotwicz, 2015/02/10 22:50:15]

Would LoadTestPageAndWaitForMouseOver() be a more appropriate name?

NavigateAndLoadContent() would likely not work with a page other than
"mouseleave.html". I don't think that you gain anything from passing the URL as
parameter to NavigateAndLoadContent().

[Miyoung Shin, 2015/02/12 14:23:27]

Done.

     gfx::Rect tab_view_bounds = tab->GetContainerBounds();
 
-    gfx::Point in_content_point(
-        tab_view_bounds.x() + tab_view_bounds.width() / 2,
-        tab_view_bounds.y() + 10);
-    gfx::Point above_content_point(
-        tab_view_bounds.x() + tab_view_bounds.width() / 2,
-        tab_view_bounds.y() - 2);
+    in_content_ = gfx::Point(tab_view_bounds.x() + tab_view_bounds.width() / 2,
+                             tab_view_bounds.y() + 10);
+    above_content_ =
+        gfx::Point(tab_view_bounds.x() + tab_view_bounds.width() / 2,
+                   tab_view_bounds.y() - 2);
 
-    // Start by moving the point just above the content.

[pkotwicz, 2015/02/10 22:50:15]

Isn't this comment still accurate?

[Miyoung Shin, 2015/02/12 14:23:27]

Done.

-    ui_controls::SendMouseMove(above_content_point.x(),
-                               above_content_point.y());
+    ui_controls::SendMouseMove(above_content_.x(), above_content_.y());
 
     // Navigate to the test html page.
     base::string16 load_expected_title(base::ASCIIToUTF16("onload"));
     content::TitleWatcher load_title_watcher(tab, load_expected_title);
-    ui_test_utils::NavigateToURL(browser(), test_url);
+    ui_test_utils::NavigateToURL(browser(), url);
     // Wait for the onload() handler to complete so we can do the
     // next part of the test.
     EXPECT_EQ(load_expected_title, load_title_watcher.WaitAndGetTitle());
 
-    // Move the cursor to the top-center of the content, which will trigger
-    // a javascript onMouseOver event.

[pkotwicz, 2015/02/10 22:50:15]

Isn't this comment still accurate?

[Miyoung Shin, 2015/02/12 14:23:27]

Done.

-    ui_controls::SendMouseMove(in_content_point.x(), in_content_point.y());
+    // Move the cursor to the top-center of the content which will trigger
+    // a javascript event.
+    ui_controls::SendMouseMove(in_content_.x(), in_content_.y());
 
     // Wait on the correct intermediate title.
     base::string16 entered_expected_title(base::ASCIIToUTF16("entered"));
     content::TitleWatcher entered_title_watcher(tab, entered_expected_title);
     EXPECT_EQ(entered_expected_title, entered_title_watcher.WaitAndGetTitle());
+  }
+
+  void MouseLeaveTestCommon() {
+    content::WebContents* tab =
+        browser()->tab_strip_model()->GetActiveWebContents();
+    GURL test_url = ui_test_utils::GetTestUrl(
+        base::FilePath(), base::FilePath(FILE_PATH_LITERAL("mouseleave.html")));
+
+    NavigateAndLoadContent(tab, test_url);
 
     // Move the cursor above the content again, which should trigger
     // a javascript onMouseOut event.
-    ui_controls::SendMouseMove(above_content_point.x(),
-                               above_content_point.y());
+    ui_controls::SendMouseMove(above_content().x(), above_content().y());
 
     // Wait on the correct final value of the cookie.
     base::string16 left_expected_title(base::ASCIIToUTF16("left"));
@@ -66,6 +76,14 @@ class MouseLeaveTest : public InProcessBrowserTest {
     EXPECT_EQ(left_expected_title, left_title_watcher.WaitAndGetTitle());
   }
 
+  gfx::Point in_content() { return in_content_; }
+
+  gfx::Point above_content() { return above_content_; }

[pkotwicz, 2015/02/10 22:50:15]

Can these two methods be removed? in_content() does not seem to be used
anywhere.

[Miyoung Shin, 2015/02/12 14:23:27]

I removed in_content() but I keep to use above_content() because it is used in
MouseLeaveTestCommon().

+
+ private:
+  gfx::Point in_content_;
+  gfx::Point above_content_;
+
   DISALLOW_COPY_AND_ASSIGN(MouseLeaveTest);
 };
 
@@ -95,4 +113,96 @@ IN_PROC_BROWSER_TEST_F(MouseLeaveTest, MouseDownOnBrowserCaption) {
 }
 #endif
 
+#if defined(OS_WIN)
+// This test is for preventing a regression that the mouseleave is triggered
+// on Window when opening the new window.
+#define MAYBE_NewWindow NewWindow
+#else
+// Others except OS_WIN do not send the mouseleave when opening the new window

[pkotwicz, 2015/02/10 22:50:14]

I think it is definitely worth enabling the test on other platforms. Other OSes
could start sending a mouse leave when opening a new window. (The behavior of
ChromeOS changes every 6 weeks) In fact, this test fails on Desktop Linux if I
change MakeWebMouseEventFromAuraEvent() to convert events of type
ui::ET_MOUSE_EXITED to blink::WebInputEvent::MouseLeave

[Miyoung Shin, 2015/02/12 14:23:27]

Um.. I can see the different result with yours.
As I change MakeWebMouseEventFromAuraEvent() to ui::ET_MOUSE_EXITED to
blink::WebInputEvent::MouseLeave on Desktop Linux(X86_64), this test passes. 

[pkotwicz, 2015/02/12 16:42:03]

Sorry. Yes, you are right. (And without the change to
MakeWebMouseEventFromAuraEvent() the test fails).

It sounds like the current Linux/ChromeOS behavior is a bug. (Or maybe the
behavior does not matter either way in which case we should not have the
MouseLeaveTest.NewWindow test)

If the Linux/ChromeOS behavior is in fact buggy, can you file a bug and mention
the bug number in a comment similar to what you have in
MouseLeaveTest.ContextMenu?

[Miyoung Shin, 2015/02/13 07:48:40]

I think so it looks like a bug but I'd like to check if someone intend this
behavior. I'm asking it to Rick. if any updating, I would update the comment.

[Rick Byers, 2015/02/13 08:59:43]

All platforms should be sending a mouseleave when a different window comes up
under the mouse cursor.  I just did a quick check on MacOS using Chrome canary
and I see the mouseleave event (using http://rbyers.net/eventTest.html).

So yes, it sounds like there's an X11-specific bug here too.  Feel free to keep
the test disabled on Linux/ChromeOS and file another bug as Peter suggests.  I
can't say offhand why the behavior is the way it is - you could look at the git
history, or just find out what happens when we 'fix' it <grin>.  Either way,
best to separate that into another bug (incase we have to revert etc.).

[Miyoung Shin, 2015/02/13 12:16:57]

Thanks rick, I will feel free to report the bug if I meet a doubtful thing. :)
I added the comment after creating the new bug at https://crrev.com/458492. (I
borrowed the test case and the reproduction step from https://crrev.com450631)

+#define MAYBE_NewWindow DISABLED_NewWindow
+#endif
+
+IN_PROC_BROWSER_TEST_F(MouseLeaveTest, MAYBE_NewWindow) {
+  content::WebContents* tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  GURL test_url = ui_test_utils::GetTestUrl(
+      base::FilePath(), base::FilePath(FILE_PATH_LITERAL("mouseleave.html")));
+
+  NavigateAndLoadContent(tab, test_url);
+
+  // Create an new window.
+  ui_test_utils::BrowserAddedObserver browser_added_observer;
+  chrome::NewWindow(browser());
+  browser_added_observer.WaitForSingleNewBrowser();
+
+  ASSERT_EQ(2u, chrome::GetTotalBrowserCount());
+
+  content::RunAllPendingInMessageLoop();
+
+  tab->GetMainFrame()->ExecuteJavaScript(base::ASCIIToUTF16("done()"));
+  base::string16 done_expected_title(base::ASCIIToUTF16("with mouseleave"));
+  content::TitleWatcher done_title_watcher(tab, done_expected_title);
+  EXPECT_EQ(done_expected_title, done_title_watcher.WaitAndGetTitle());
+}
+
+
+#if !defined(OS_WIN) && !defined(OS_MACOSX)
+// This test is for preventing a regression that a mouseleave is triggered
+// when showing the context menu.
+// TODO:Make test pass on OS_WIN and OS_MACOSX
+// OS_WIN : http://crbug.com/450138
+// OS_MACOSX: Missing automation provider support: http://crbug.com/45892.

[pkotwicz, 2015/02/10 22:50:15]

Nits:
- Rename the test to MAYBE_ContextMenu and define macros to rename it to
ContextMenu / DISABLED_ContextMenu as appropriate similar to MAYBE_NewWindow
- Be consistent. ": " Colon, then space

[Miyoung Shin, 2015/02/12 14:23:27]

Done.

+IN_PROC_BROWSER_TEST_F(MouseLeaveTest, ContextMenu) {
+  content::WebContents* tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  GURL test_url = ui_test_utils::GetTestUrl(
+      base::FilePath(), base::FilePath(FILE_PATH_LITERAL("mouseleave.html")));
+
+  NavigateAndLoadContent(tab, test_url);
+
+  SaveLinkAsContextMenuObserver menu_observer(
+      content::NotificationService::AllSources());

[pkotwicz, 2015/02/10 22:50:14]

I think that SaveLinkAsContextMenuObserver is a bad name. (The class name does
not reflect what it actually does)

Can you please rename it to something more relevant?
My suggestion:
class ContextMenuWaiter {
 public:
  content::ContextMenuParams params();  // previously GetSuggestedFilename()
  void WaitForMenuOpenAndClose();  // previously WaitForMenu()
};

Can you also make ContextMenuWaiter extend content::NotificationObserver instead
of ContextMenuNotificationObserver?

I am fine if you wait for OWNERs review prior to doing the renaming. (Everyone
has different suggestions when it comes to naming)

[Miyoung Shin, 2015/02/12 14:23:27]

Done. Thank you for your kindly comments.

[pkotwicz, 2015/02/12 16:42:03]

Sorry I should have been clearer. I meant renaming the methods of
SaveLinkAsContextMenuObserver in render_view_context_menu_browsertest_util.cc
(Not adding a clone of SaveLinkAsContextMenuObserver)

[Miyoung Shin, 2015/02/13 07:48:40]

Ah, I got it. I had not catched your point. I renamed it at the next patch

+  ui_controls::SendMouseClick(ui_controls::RIGHT);
+  // Wait for context menu to be visible.
+  menu_observer.WaitForMenu();
+
+  base::string16 show_expected_title(base::ASCIIToUTF16("showed"));
+  content::TitleWatcher show_title_watcher(tab, show_expected_title);
+  EXPECT_EQ(show_expected_title, show_title_watcher.WaitAndGetTitle());

[pkotwicz, 2015/02/10 22:50:14]

show_title_watcher.WaitAndGetTitle() seems to return immediately. Is it
necessary?

[Miyoung Shin, 2015/02/12 14:23:27]

Done. I think so it's not neccessary. I removed it.

+
+  content::RunAllPendingInMessageLoop();
+
+  tab->GetMainFrame()->ExecuteJavaScript(base::ASCIIToUTF16("done()"));
+  base::string16 done_expected_title(base::ASCIIToUTF16("without mouseleave"));
+  content::TitleWatcher done_title_watcher(tab, done_expected_title);
+  EXPECT_EQ(done_expected_title, done_title_watcher.WaitAndGetTitle());
+}
+
+// This test is for preventing a regression that a mouseleave is triggered
+// when showing the modal dialog.
+// TODO:Make test pass on OS_WIN and OS_MACOSX
+// OS_WIN : http://crbug.com/450138
+// OS_MACOSX: Missing automation provider support: http://crbug.com/45892.
+IN_PROC_BROWSER_TEST_F(MouseLeaveTest, ModalDialog) {
+  content::WebContents* tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  GURL test_url = ui_test_utils::GetTestUrl(
+      base::FilePath(), base::FilePath(FILE_PATH_LITERAL("mouseleave.html")));
+
+  NavigateAndLoadContent(tab, test_url);
+
+  tab->GetMainFrame()->ExecuteJavaScript(base::UTF8ToUTF16("alert()"));
+  // Cancel the dialog.
+  app_modal::AppModalDialog* alert = ui_test_utils::WaitForAppModalDialog();
+  alert->CloseModalDialog();
+
+  content::RunAllPendingInMessageLoop();
+
+  tab->GetMainFrame()->ExecuteJavaScript(base::ASCIIToUTF16("done()"));
+  base::string16 done_expected_title(base::ASCIIToUTF16("without mouseleave"));
+  content::TitleWatcher done_title_watcher(tab, done_expected_title);
+  EXPECT_EQ(done_expected_title, done_title_watcher.WaitAndGetTitle());
+}
+#endif
+
 }  // namespace