Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
891213002:
Add a test for the mouseleave event on window. (Closed)
|
Created:
5 years, 10 months ago by Miyoung Shin Modified:
5 years, 9 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a test for the mouseleave event on window.
When opening the new window, it is opened on the current
window. Even though the mouse position is not updated, the
mouse leave event should be triggered on the previous
window. However, when showing the context menu or the popup,
the mouse leave event shouldn't be triggered.
The test checks if the mouseleave event is triggered when
showing the UI.
BUG=450631
R=pkotwicz@chromium.org
Committed: https://crrev.com/3e66e1af60b2f5b2f488ab8f4c3ca3532e57f049
Cr-Commit-Position: refs/heads/master@{#321280}
Patch Set 1 #Patch Set 2 : #
Total comments: 18
Patch Set 3 : #
Total comments: 22
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #Patch Set 6 : #
Total comments: 7
Patch Set 7 : #
Total comments: 4
Patch Set 8 : #Patch Set 9 : #
Total comments: 5
Patch Set 10 : #
Total comments: 4
Patch Set 11 : #
Total comments: 24
Patch Set 12 : #
Messages
Total messages: 62 (6 generated)
rbyers@, I was aware of the issue that we couldn't show the context menu and JS alert popup in content_shell. So I added the test in interactive_ui_test of chrome. PTAL.
On 2015/02/01 14:11:39, Miyoung Shin wrote: > rbyers@, > I was aware of the issue that we couldn't show the context menu and JS alert > popup in content_shell. So I added the test in interactive_ui_test of chrome. > PTAL. sorry. I need to check the result on Mac. Please postpone the review.
On 2015/02/01 15:46:52, Miyoung Shin wrote: > On 2015/02/01 14:11:39, Miyoung Shin wrote: > > rbyers@, > > I was aware of the issue that we couldn't show the context menu and JS alert > > popup in content_shell. So I added the test in interactive_ui_test of chrome. > > PTAL. > > sorry. I need to check the result on Mac. Please postpone the review. rbyers@, Trybot on mac showed the failure, but it seems that it is unrelated to this patch. Could you review this patch?
On 2015/02/03 03:42:11, Miyoung Shin wrote: > On 2015/02/01 15:46:52, Miyoung Shin wrote: > > On 2015/02/01 14:11:39, Miyoung Shin wrote: > > > rbyers@, > > > I was aware of the issue that we couldn't show the context menu and JS alert > > > popup in content_shell. So I added the test in interactive_ui_test of > chrome. > > > PTAL. > > > > sorry. I need to check the result on Mac. Please postpone the review. > > rbyers@, Trybot on mac showed the failure, but it seems that it is unrelated to > this patch. > Could you review this patch? Is there a reason why you decided this was best tested by a browser test? Since the fix is in blink, I thought the simplest / most reliable test would be a layout test (eg. see here for details: https://code.google.com/p/chromium/issues/detail?id=450631#c2) although it looks like event_sender doesn't currently have a way of sending mouseleave events (it really should!). We don't run browsertests on every blink change, so it won't be as easy to catch regressions for this as a browser test. If we had a layout test for directly testing the 'leave within window boundaries' case, do you think there's additional value in also having this browsertest? There might be...
On 2015/02/03 05:33:36, Rick Byers wrote: > On 2015/02/03 03:42:11, Miyoung Shin wrote: > > On 2015/02/01 15:46:52, Miyoung Shin wrote: > > > On 2015/02/01 14:11:39, Miyoung Shin wrote: > > > > rbyers@, > > > > I was aware of the issue that we couldn't show the context menu and JS > alert > > > > popup in content_shell. So I added the test in interactive_ui_test of > > chrome. > > > > PTAL. > > > > > > sorry. I need to check the result on Mac. Please postpone the review. > > > > rbyers@, Trybot on mac showed the failure, but it seems that it is unrelated > to > > this patch. > > Could you review this patch? > > Is there a reason why you decided this was best tested by a browser test? Since > the fix is in blink, I thought the simplest / most reliable test would be a > layout test (eg. see here for details: > https://code.google.com/p/chromium/issues/detail?id=450631#c2) although it looks > like event_sender doesn't currently have a way of sending mouseleave events (it > really should!). We don't run browsertests on every blink change, so it won't > be as easy to catch regressions for this as a browser test. > > If we had a layout test for directly testing the 'leave within window > boundaries' case, do you think there's additional value in also having this > browsertest? There might be... Originally you asked me to add the test for triggering the mouseleave event when covering the window by something[1] (perhape new window) Addtionally I was asked to add the test for the contextmenu and the alert popup at https://code.google.com/p/chromium/issues/detail?id=450138 [2]. IIUC, we don't support the contextmenu and the alert popup in content_shell. So, I tried to add it in browser test in a lump. How about adding this CL in browser_test and I will add the additional test in layout test for only [1]
rbyers@chromium.org changed reviewers: + pkotwicz@chromium.org - rbyers@chromium.org
On 2015/02/03 12:03:03, Miyoung Shin wrote: > On 2015/02/03 05:33:36, Rick Byers wrote: > > On 2015/02/03 03:42:11, Miyoung Shin wrote: > > > On 2015/02/01 15:46:52, Miyoung Shin wrote: > > > > On 2015/02/01 14:11:39, Miyoung Shin wrote: > > > > > rbyers@, > > > > > I was aware of the issue that we couldn't show the context menu and JS > > alert > > > > > popup in content_shell. So I added the test in interactive_ui_test of > > > chrome. > > > > > PTAL. > > > > > > > > sorry. I need to check the result on Mac. Please postpone the review. > > > > > > rbyers@, Trybot on mac showed the failure, but it seems that it is unrelated > > to > > > this patch. > > > Could you review this patch? > > > > Is there a reason why you decided this was best tested by a browser test? > Since > > the fix is in blink, I thought the simplest / most reliable test would be a > > layout test (eg. see here for details: > > https://code.google.com/p/chromium/issues/detail?id=450631#c2) although it > looks > > like event_sender doesn't currently have a way of sending mouseleave events > (it > > really should!). We don't run browsertests on every blink change, so it won't > > be as easy to catch regressions for this as a browser test. > > > > If we had a layout test for directly testing the 'leave within window > > boundaries' case, do you think there's additional value in also having this > > browsertest? There might be... > > Originally you asked me to add the test for triggering the mouseleave event when > covering the window by something[1] (perhape new window) > Addtionally I was asked to add the test for the contextmenu and the alert popup > at https://code.google.com/p/chromium/issues/detail?id=450138 [2]. > IIUC, we don't support the contextmenu and the alert popup in content_shell. > So, I tried to add it in browser test in a lump. > How about adding this CL in browser_test and I will add the additional test in > layout test for only [1] Ah, I see - thanks. Sure having a browsertest for the UI side and a layouttest for the blink issue I was concerned with makes sense. pkotwicz is probably a better reviewer for this CL then since he had a specific concern (alert()) that you're addressing.
pkotwicz@, PTAL.
pkotwicz@chromium.org changed reviewers: + rbyers@chromium.org
+rbyers@ I am trying out the MouseLeaveTest.ContextMenu in case where a mouse leave event is in fact sent and the test seems to pass. It seems that WebContentsViewAura::ShowContextMenu() is sometimes called prior to |show_title_watcher| ending its base::RunLoop. rbyers@ do you have any suggestions how to improve the MouseLeaveTest.ContextMenu test? I am going to keep on looking to see what suggestions I come up with.
I have added a few comments. Overall looks pretty good. Can you check that in addition to the test passing on the platforms where it is enabled that it fails on the platforms where it is supposed to fail? https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:120: #else Can you add comments as to why the test is disabled on Mac, Desktop Linux and ChromeOS? https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:139: // Close the second window. Is there a reason that you are closing the browser window? https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:156: // when showing the context menu. Can you please link to crbug.com/450138 for an example of such a regression? https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:174: IDC_CONTENT_CONTEXT_COPYLINKLOCATION); I think that if you use SaveLinkAsContextMenuObserver, SaveLinkAsContextMenuObserver::WaitForMenu() will block till the menu is opened. I think that it will wait long enough that any ET_MOUSE_EXITED events sent as a result of the menu showing will have been dispatched to Blink. I think that SaveLinkAsContextMenuObserver::WaitForMenu() automatically closes the menu when the menu is opened so you don't need to worry about sending a mouse click to hide the menu. SaveLinkAsContextMenuObserver::WaitForMenu() automatically closing the menu when the menu is opened is a good thing. (If it didn't SaveLinkAsContextMenuObserver::WaitForMenu() would block forever) It is probably worth creating a new base class for SaveLinkAsContextMenuObserver and using it here. https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:184: content::TitleWatcher done_title_watcher(tab, done_expected_title); Will the test timeout if a ET_MOUSE_EXITED event is sent? https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:188: #if defined(OS_WIN) || defined(OS_MACOSX) Can you reference crbug.com/394672 for an example of such a regression? https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:209: alert->CloseModalDialog(); Can you call content::RunAllPendingInMessageLoop() here for sanity. This way if there is a task which dispatches an ET_MOUSE_EXITED event it will get run. For instance, WindowEventDispatcher::OnOtherRootGotCapture() sends a ET_MOUSE_EXITED event in a task (I would do this for the other two tests too)
> Can you check that in addition to the test passing on the platforms where it is > enabled that it fails on the platforms where it is supposed to fail? NewWindow test works well only on Window. ContextMenu and ModalDialog tests work well on Linux, ChromeOS. I've uploaded the new patch. PTAL. https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:120: #else On 2015/02/04 19:43:31, pkotwicz wrote: > Can you add comments as to why the test is disabled on Mac, Desktop Linux and > ChromeOS? I've added the comment at patchset3 https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:139: // Close the second window. On 2015/02/04 19:43:31, pkotwicz wrote: > Is there a reason that you are closing the browser window? I removed it. I thought that I should close the browser window to finish completely the testbut it's not necessary. https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:156: // when showing the context menu. On 2015/02/04 19:43:31, pkotwicz wrote: > Can you please link to crbug.com/450138 for an example of such a regression? Done https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:174: IDC_CONTENT_CONTEXT_COPYLINKLOCATION); On 2015/02/04 19:43:31, pkotwicz wrote: > I think that if you use SaveLinkAsContextMenuObserver, > SaveLinkAsContextMenuObserver::WaitForMenu() will block till the menu is opened. > I think that it will wait long enough that any ET_MOUSE_EXITED events sent as a > result of the menu showing will have been dispatched to Blink. > > I think that SaveLinkAsContextMenuObserver::WaitForMenu() automatically closes > the menu when the menu is opened so you don't need to worry about sending a > mouse click to hide the menu. SaveLinkAsContextMenuObserver::WaitForMenu() > automatically closing the menu when the menu is opened is a good thing. (If it > didn't SaveLinkAsContextMenuObserver::WaitForMenu() would block forever) > > It is probably worth creating a new base class for SaveLinkAsContextMenuObserver > and using it here. thank you for your suggestion. I changed to use SaveLinkAsContextMenuObserver and it works well. https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:184: content::TitleWatcher done_title_watcher(tab, done_expected_title); On 2015/02/04 19:43:31, pkotwicz wrote: > Will the test timeout if a ET_MOUSE_EXITED event is sent? Yes, the test is timeout. https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:188: #if defined(OS_WIN) || defined(OS_MACOSX) On 2015/02/04 19:43:31, pkotwicz wrote: > Can you reference crbug.com/394672 for an example of such a regression? I changed it. If I didn't catch your point from the reference, please let me know. https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:209: alert->CloseModalDialog(); On 2015/02/04 19:43:31, pkotwicz wrote: > Can you call content::RunAllPendingInMessageLoop() here for sanity. This way if > there is a task which dispatches an ET_MOUSE_EXITED event it will get run. For > instance, WindowEventDispatcher::OnOtherRootGotCapture() sends a ET_MOUSE_EXITED > event in a task > > (I would do this for the other two tests too) Done
Sorry for the long delay. Overall, looks pretty good https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:184: content::TitleWatcher done_title_watcher(tab, done_expected_title); It is hard to tell for a sheriff whether a test times out due to a regression or due to flakiness. I think that you can use TitleWatcher::AlsoWaitForTitle() to make TitleWatcher::WaitAndGetTitle() stop waiting if the title is set to "with mouseleave" (the failure case) https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:188: #if defined(OS_WIN) || defined(OS_MACOSX) I was suggesting changing the comment to something like this: "Test that a mouseleave is not triggered when showing a modal dialog. Sample regression: crbug.com/394672" The reason that I want to include a sample regression is because: - The reason why the test exists is clear - Testing for something not occurring is weird You don't need to do something similar for MouseLeaveTest.MAYBE_ContextMenu because the regression has not yet been fixed https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... File chrome/browser/mouseleave_browsertest.cc (left): https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:37: // Start by moving the point just above the content. Isn't this comment still accurate? https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:50: // a javascript onMouseOver event. Isn't this comment still accurate? https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:32: void NavigateAndLoadContent(content::WebContents* tab, GURL& url) { 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(). https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:81: gfx::Point above_content() { return above_content_; } Can these two methods be removed? in_content() does not seem to be used anywhere. https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:121: // Others except OS_WIN do not send the mouseleave when opening the new window 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 https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:154: // OS_MACOSX: Missing automation provider support: http://crbug.com/45892. 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 https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:164: content::NotificationService::AllSources()); 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) https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:171: EXPECT_EQ(show_expected_title, show_title_watcher.WaitAndGetTitle()); show_title_watcher.WaitAndGetTitle() seems to return immediately. Is it necessary?
https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:184: content::TitleWatcher done_title_watcher(tab, done_expected_title); On 2015/02/10 22:50:14, pkotwicz wrote: > It is hard to tell for a sheriff whether a test times out due to a regression or > due to flakiness. I think that you can use TitleWatcher::AlsoWaitForTitle() to > make TitleWatcher::WaitAndGetTitle() stop waiting if the title is set to "with > mouseleave" (the failure case) Thank your for your good information. I added AlsoWaitForTitle for the failure case https://codereview.chromium.org/891213002/diff/20001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:188: #if defined(OS_WIN) || defined(OS_MACOSX) On 2015/02/10 22:50:14, pkotwicz wrote: > I was suggesting changing the comment to something like this: > "Test that a mouseleave is not triggered when showing a modal dialog. Sample > regression: crbug.com/394672" > > The reason that I want to include a sample regression is because: > - The reason why the test exists is clear > - Testing for something not occurring is weird > > You don't need to do something similar for MouseLeaveTest.MAYBE_ContextMenu > because the regression has not yet been fixed Done https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... File chrome/browser/mouseleave_browsertest.cc (left): https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:37: // Start by moving the point just above the content. On 2015/02/10 22:50:15, pkotwicz wrote: > Isn't this comment still accurate? Done. https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:50: // a javascript onMouseOver event. On 2015/02/10 22:50:15, pkotwicz wrote: > Isn't this comment still accurate? Done. https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:32: void NavigateAndLoadContent(content::WebContents* tab, GURL& url) { On 2015/02/10 22:50:15, pkotwicz wrote: > 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(). Done. https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:81: gfx::Point above_content() { return above_content_; } On 2015/02/10 22:50:15, pkotwicz wrote: > Can these two methods be removed? in_content() does not seem to be used > anywhere. I removed in_content() but I keep to use above_content() because it is used in MouseLeaveTestCommon(). https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:121: // Others except OS_WIN do not send the mouseleave when opening the new window On 2015/02/10 22:50:14, pkotwicz wrote: > 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 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. https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:154: // OS_MACOSX: Missing automation provider support: http://crbug.com/45892. On 2015/02/10 22:50:15, pkotwicz wrote: > 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 Done. https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:164: content::NotificationService::AllSources()); On 2015/02/10 22:50:14, pkotwicz wrote: > 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) Done. Thank you for your kindly comments. https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:171: EXPECT_EQ(show_expected_title, show_title_watcher.WaitAndGetTitle()); On 2015/02/10 22:50:14, pkotwicz wrote: > show_title_watcher.WaitAndGetTitle() seems to return immediately. Is it > necessary? Done. I think so it's not neccessary. I removed it.
https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:121: // Others except OS_WIN do not send the mouseleave when opening the new window 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? https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:164: content::NotificationService::AllSources()); 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) https://codereview.chromium.org/891213002/diff/60001/chrome/browser/mouseleav... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/60001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:74: ui_controls::SendMouseMove(above_content().x(), above_content().y()); Nit: Can you access |above_content_| directly? (And remove the above_content() method) https://codereview.chromium.org/891213002/diff/60001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:237: // TODO:Make test pass on OS_WIN and OS_MACOSX Nit: "space" after "TODO:"
https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:121: // Others except OS_WIN do not send the mouseleave when opening the new window On 2015/02/12 16:42:03, pkotwicz wrote: > 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? > 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. https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:164: content::NotificationService::AllSources()); On 2015/02/12 16:42:03, pkotwicz wrote: > 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) Ah, I got it. I had not catched your point. I renamed it at the next patch https://codereview.chromium.org/891213002/diff/60001/chrome/browser/mouseleav... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/60001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:74: ui_controls::SendMouseMove(above_content().x(), above_content().y()); On 2015/02/12 16:42:03, pkotwicz wrote: > Nit: Can you access |above_content_| directly? > (And remove the above_content() method) Done. https://codereview.chromium.org/891213002/diff/60001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:237: // TODO:Make test pass on OS_WIN and OS_MACOSX On 2015/02/12 16:42:03, pkotwicz wrote: > Nit: "space" after "TODO:" Done.
https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:121: // Others except OS_WIN do not send the mouseleave when opening the new window On 2015/02/13 07:48:40, Miyoung Shin wrote: > On 2015/02/12 16:42:03, pkotwicz wrote: > > 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? > > > > 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. 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.).
https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/40001/chrome/browser/mouseleav... chrome/browser/mouseleave_browsertest.cc:121: // Others except OS_WIN do not send the mouseleave when opening the new window On 2015/02/13 08:59:43, Rick Byers wrote: > On 2015/02/13 07:48:40, Miyoung Shin wrote: > > On 2015/02/12 16:42:03, pkotwicz wrote: > > > 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? > > > > > > > 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. > > 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.). 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)
Once we figure out whether the window position affects whether a ET_MOUSE_EXITED event is sent L-G-T-M https://codereview.chromium.org/891213002/diff/100001/chrome/browser/mouselea... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/100001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:115: // on Window when opening the new window. How about: "Test that a mouse leave event is triggered when opening a new window." In your comment it is unclear whether "the mouseleave is triggered on Window when opening the new window" refers to (A) correct behavior (B) a regression which would make this test fail https://codereview.chromium.org/891213002/diff/100001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:123: I suspect that a mouse leave is sent only when the new window overlaps the old window. Can you check? Sorry I did not bring this up in an earlier review. https://codereview.chromium.org/891213002/diff/100001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.h (right): https://codereview.chromium.org/891213002/diff/100001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.h:34: Sorry for the confusion. Can you rename this class to ContextMenuWaiter and make it inherit content::NotificationObserver?
*window position affects whether a ET_MOUSE_EXITED event is sent when a new window is opened
> Once we figure out *window position affects whether a ET_MOUSE_EXITED >event is sent when a new window is opened Yes. ET_MOUSE_EXITED is sent when a new window overlaps the old window's area where the mouse position is located. https://codereview.chromium.org/891213002/diff/100001/chrome/browser/mouselea... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/100001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:115: // on Window when opening the new window. On 2015/02/13 20:52:13, pkotwicz wrote: > How about: "Test that a mouse leave event is triggered when opening a new > window." > > In your comment it is unclear whether "the mouseleave is triggered on Window > when opening the new window" refers to (A) correct behavior (B) a regression > which would make this test fail Done. Thank you for correcting it. https://codereview.chromium.org/891213002/diff/100001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:123: On 2015/02/13 20:52:13, pkotwicz wrote: > I suspect that a mouse leave is sent only when the new window overlaps the old > window. Can you check? > > Sorry I did not bring this up in an earlier review. The mouse leave is sent when the mouse position is out of the old window by the new window overlaps the old window without moving the mouse position. On the contrary, the mouse leave is not sent if the new window doesn't overlaps the area located the mouse position. I hope this comment answers your question. If it's not your point. please let me know
https://codereview.chromium.org/891213002/diff/100001/chrome/browser/mouselea... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/100001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:123: It sounds like this test will pass / fail based on the new browser's bounds? If that's correct, it is worth ensuring that the new browser has "correct bounds" It looks like Browser::CreateParams allows you to specify the browser window's initial bounds https://codereview.chromium.org/891213002/diff/120001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc (right): https://codereview.chromium.org/891213002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc:86: menu_observer.Wait(); Nit: It's probably worth calling content::RunAllPendingInMessageLoop() to ensure that ContextMenuWaiter::Cancel() has been called by the time that we return in the case that |menu_visible_| is false on line 85. https://codereview.chromium.org/891213002/diff/120001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.h (right): https://codereview.chromium.org/891213002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.h:46: // Wait until opening the context menu and closing. Nit: "Wait until the context menu is opened and closed."
it's minor but I couldn't find your comment in render_view_context_menu_browsertest_util.cc. I don't know why I can't see it, so I replied the comment manually. >chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc:86: >menu_observer.Wait(); >Nit: It's probably worth calling >content::RunAllPendingInMessageLoop() to ensure >that ContextMenuWaiter::Cancel() has been called by the >time that we return in >the case that |menu_visible_| is false on line 85. Do I need to call content::RunAllPendingInMessageLoop() ? I'd already called it in IN_PROC_BROWSER_TEST_F(MouseLeaveTest, MAYBE_ContextMenu) as you suggested. If I nedd it, should I remove it in IN_PROC_BROWSER_TEST_F(MouseLeaveTest, MAYBE_ContextMenu) ? https://codereview.chromium.org/891213002/diff/100001/chrome/browser/mouselea... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/100001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:123: On 2015/02/17 19:59:29, pkotwicz wrote: > It sounds like this test will pass / fail based on the new browser's bounds? If > that's correct, it is worth ensuring that the new browser has "correct bounds" > Yes, right. > It looks like Browser::CreateParams allows you to specify the browser window's > initial bounds IIUC, initial bounds relies on the old window's bounds if we don't set it, and I think the new window covers the old window. If you have any concern about that, please let me know. https://codereview.chromium.org/891213002/diff/120001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.h (right): https://codereview.chromium.org/891213002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.h:46: // Wait until opening the context menu and closing. On 2015/02/17 19:59:29, pkotwicz wrote: > Nit: "Wait until the context menu is opened and closed." Done.
- Something weird that I noticed. I tried running MouseLeaveTest.NewWindow on Winodws 8 desktop (non Ash) and removed the parts of the test case which created the new window and the test still passed. This confuses me. It's likely that this is because I did not set up my environment correctly. Can you try it as well? I will look into this more tomorrow morning. - You are right. The new window's bounds are computed based on the old window. Do I need to call content::RunAllPendingInMessageLoop() ? I'd already called it in IN_PROC_BROWSER_TEST_F(MouseLeaveTest, MAYBE_ContextMenu) as you suggested. If I nedd it, should I remove it in IN_PROC_BROWSER_TEST_F(MouseLeaveTest, MAYBE_ContextMenu) ? I'd leave content::RunAllPendingInMessageLoop() in both places. My suggestion was for the sake of correctness if someone else would use ContextMenuWaiter not your particular test.
Sorry that I am slow reviewing today
On 2015/02/19 06:08:46, pkotwicz wrote: > - Something weird that I noticed. I tried running MouseLeaveTest.NewWindow on > Winodws 8 desktop (non Ash) and removed the parts of the test case which created > the new window and the test still passed. This confuses me. It's likely that > this is because I did not set up my environment correctly. Can you try it as > well? I will look into this more tomorrow morning. > Sorry. I don't have Windows 8 Environment.(Acutally I have Window7). I will try to set-up it but I can't make sure that I finish to do it in time. > - You are right. The new window's bounds are computed based on the old window. > > Do I need to call content::RunAllPendingInMessageLoop() ? > I'd already called it in IN_PROC_BROWSER_TEST_F(MouseLeaveTest, > MAYBE_ContextMenu) as you suggested. > If I nedd it, should I remove it in IN_PROC_BROWSER_TEST_F(MouseLeaveTest, > MAYBE_ContextMenu) ? > I'd leave content::RunAllPendingInMessageLoop() in both places. My suggestion > was for the sake of correctness if someone else would use ContextMenuWaiter not > your particular test. I got it.
I have both Windows 7 and Windows 8. I will do some investigating today :)
pkotwicz@chromium.org changed reviewers: + ananta@chromium.org
+ananta@ for his Windows expertise ananta@, what's the status of the --disable-legacy-window flag? In my testing the browser window in MouseLeaveTest.NewWindow gets a mouse leave even if a new window is opened if the --disable-legacy-window is not passed. Is there an easy way of fixing this? https://codereview.chromium.org/891213002/diff/120001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc (right): https://codereview.chromium.org/891213002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc:86: menu_observer.Wait(); Sorry I meant |menu_visible_| being true in line 85. We can call content::RunAllPendingInMessageLoop() both when |menu_visible_| is true and when |menu_visible_| is false. We still need to call menu_observer.Wait() though https://codereview.chromium.org/891213002/diff/160001/chrome/browser/mouselea... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/160001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:123: IN_PROC_BROWSER_TEST_F(MouseLeaveTest, MAYBE_NewWindow) { I looked a bit more and on Windows 8, this test passes even if I comment out the lines which create the new window (lines 130 - 134). The test does fail if I comment out the code which creates a new window if I run it with --disable-legacy-window (and it passes without the code commented out). I did not try on Windows 7 but I suspect that the behavior is the same. I looked at LegacyRenderWidgetHostHWND a while back and fixing it is difficult myid.shin@ can you check whether Windows 7 behaves the same when lines 130 - 134 are commented out? In this case, the test is correct but Chrome is buggy. +ananta for: 1) whether we are planning on turning on --disable-legacy-window by default (It was added in https://codereview.chromium.org/151083002) 2) whether the buggy behavior can be easily fixed - I don't think so but it's still worth asking https://codereview.chromium.org/891213002/diff/160001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:165: // Wait until opening context menu and closing one. How about: "Wait until the context menu is opened and closed."
https://codereview.chromium.org/891213002/diff/120001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc (right): https://codereview.chromium.org/891213002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc:86: > menu_observer.Wait(); > Sorry I meant |menu_visible_| being true in line 85. We can call > content::RunAllPendingInMessageLoop() both when |menu_visible_| is true and when > |menu_visible_| is false. > > We still need to call menu_observer.Wait() though Ah. I add to call content::RunAllPendingInMessageLoop() regardless of |menu_visible_|. Thanks. https://codereview.chromium.org/891213002/diff/160001/chrome/browser/mouselea... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/160001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:123: IN_PROC_BROWSER_TEST_F(MouseLeaveTest, MAYBE_NewWindow) { On 2015/02/20 03:44:21, pkotwicz wrote: > I looked a bit more and on Windows 8, this test passes even if I comment out the > lines which create the new window (lines 130 - 134). The test does fail if I > comment out the code which creates a new window if I run it with > --disable-legacy-window (and it passes without the code commented out). I did > not try on Windows 7 but I suspect that the behavior is the same. I looked at > LegacyRenderWidgetHostHWND a while back and fixing it is difficult > myid.shin@ can you check whether Windows 7 behaves the same when lines 130 - 134 > are commented out? OK. I will check. > > In this case, the test is correct but Chrome is buggy. > +ananta for: > 1) whether we are planning on turning on --disable-legacy-window by default (It > was added in https://codereview.chromium.org/151083002) > 2) whether the buggy behavior can be easily fixed - I don't think so but it's > still worth asking https://codereview.chromium.org/891213002/diff/160001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:165: // Wait until opening context menu and closing one. On 2015/02/20 03:44:21, pkotwicz wrote: > How about: "Wait until the context menu is opened and closed." Done.
https://codereview.chromium.org/891213002/diff/160001/chrome/browser/mouselea... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/160001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:123: IN_PROC_BROWSER_TEST_F(MouseLeaveTest, MAYBE_NewWindow) { On 2015/02/22 08:46:09, Miyoung Shin wrote: > On 2015/02/20 03:44:21, pkotwicz wrote: > > I looked a bit more and on Windows 8, this test passes even if I comment out > the > > lines which create the new window (lines 130 - 134). The test does fail if I > > comment out the code which creates a new window if I run it with > > --disable-legacy-window (and it passes without the code commented out). I did > > not try on Windows 7 but I suspect that the behavior is the same. I looked at > > LegacyRenderWidgetHostHWND a while back and fixing it is difficult > > myid.shin@ can you check whether Windows 7 behaves the same when lines 130 - > 134 > > are commented out? > > OK. I will check. I tested it on Windows7 as you asked. The result are, comment out(lines 130 - 134) with --disable-legacy-window : Test Fail comment out(lines 130 - 134) only : Test Pass without commented out and with --disable-legacy-window : Test Pass without commented out only : Test Pass
ananta@, Ping!
On 2015/02/24 16:04:54, pkotwicz wrote: > ananta@, Ping! Sorry missed this. It could be that the top level window gets a mouseleave due to the cursor moving to the legacy window. Will check and reply back.
On 2015/02/24 16:24:41, ananta wrote: > On 2015/02/24 16:04:54, pkotwicz wrote: > > ananta@, Ping! > > Sorry missed this. It could be that the top level window gets a mouseleave due > to the cursor moving to the legacy window. Will check and reply back. ananta@, is there any update ?
ananta@, Ping!
On 2015/03/10 15:55:00, pkotwicz wrote: > ananta@, Ping! The WM_MOUSELEAVE message is received by the main window because the cursor moves to the content area which is hosted by the legacy window. There is no good way to detect this and ignore the message. We could possibly try looking at the window under the cursor and ignore it but that is fraught with problems.
LGTM I think that it is worth removing the MouseLeaveTest.NewWindow test. As ananta@ mentioned, there is no good way of writing the test at this time. Sorry that I did not notice this earlier https://codereview.chromium.org/891213002/diff/180001/chrome/browser/mouselea... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/180001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:7: #include "chrome/app/chrome_command_ids.h" Can you please remove the new includes that you don't need. For instance, I don't think you need the above include anymore. https://codereview.chromium.org/891213002/diff/180001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:197: // Cancel the dialog. Nit: Move the comment between lines 198 and 199.
https://codereview.chromium.org/891213002/diff/180001/chrome/browser/mouselea... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/180001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:7: #include "chrome/app/chrome_command_ids.h" On 2015/03/11 00:47:39, pkotwicz wrote: > Can you please remove the new includes that you don't need. For instance, I > don't think you need the above include anymore. Done. I got rid of the unnecessary include. https://codereview.chromium.org/891213002/diff/180001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:197: // Cancel the dialog. On 2015/03/11 00:47:40, pkotwicz wrote: > Nit: Move the comment between lines 198 and 199. Done.
pkotwicz@chromium.org changed reviewers: + sky@chromium.org
sky@ can you please take a look? I think your CL is ready for OWNERS review
On 2015/03/11 20:35:45, pkotwicz wrote: > sky@ can you please take a look? > > I think your CL is ready for OWNERS review The description says: "However, when showing the context menu or the popup, the mouse leave event shouldn't be triggered." Can you clarify why menus/popups are special?
On 2015/03/11 21:51:13, sky wrote: > On 2015/03/11 20:35:45, pkotwicz wrote: > > sky@ can you please take a look? > > > > I think your CL is ready for OWNERS review > > The description says: "However, when showing the context menu or the popup, the > mouse leave event shouldn't be triggered." Can you clarify why menus/popups are > special? Because the mouse pointor stays on the web page when showing the context menu or the popup.
I assumed you were saying the popup menu was shown where the mouse was. On Wed, Mar 11, 2015 at 6:22 PM, <myid.shin@samsung.com> wrote: > On 2015/03/11 21:51:13, sky wrote: >> >> On 2015/03/11 20:35:45, pkotwicz wrote: >> > sky@ can you please take a look? >> > >> > I think your CL is ready for OWNERS review > > >> The description says: "However, when showing the context menu or the >> popup, > > the >> >> mouse leave event shouldn't be triggered." Can you clarify why >> menus/popups > > are >> >> special? > > > Because the mouse pointor stays on the web page when showing the context > menu or > the popup. > > https://codereview.chromium.org/891213002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:28: void LoadTestPageAndWaitForMouseOver(content::WebContents* tab) { This should either return a boolean that it's successful, or callers should wrap calls to it in EXPECT_NO_FATAL_FAILURE. https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:34: gfx::Point(tab_view_bounds.x() + tab_view_bounds.width() / 2, nit: use constructor that takes x,y. https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:77: gfx::Point above_content_; Add a description. https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:124: // Create an new window. an->a https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:127: browser_added_observer.WaitForSingleNewBrowser(); Seems like you're assuming the new window is opened where the mouse is. How do you know that'll happen? https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:131: content::RunAllPendingInMessageLoop(); Why do you need the run all pending? https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:163: content::RunAllPendingInMessageLoop(); Same comment about position of context menu and why you need the run all. https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:196: content::RunAllPendingInMessageLoop(); And same comment here.
https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:108: #if defined(OS_WIN) I think the MouseLeaveTest.NewWindow should be removed. Right now it is not possible to write a correct test for a mouseleave getting dispatched when a new window is opened. In particular, this test passes even if the code which creates the new window is removed. For more context see comments #29 and #37 https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:163: content::RunAllPendingInMessageLoop(); I suggested adding content::RunAllPendingInMessageLoop(); A test which tests that something does not occur is weird. I suggested adding content::RunAllPendingInMessageLoop() because it gives more opportunities for the test to fail. Calling content::RunAllPendingInMessageLoop() ensures that any pending synthetic mouse moves get dispatched.
https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:163: content::RunAllPendingInMessageLoop(); On 2015/03/12 16:51:57, pkotwicz wrote: > I suggested adding content::RunAllPendingInMessageLoop(); > > A test which tests that something does not occur is weird. I suggested adding > content::RunAllPendingInMessageLoop() because it gives more opportunities for > the test to fail. Calling content::RunAllPendingInMessageLoop() ensures that any > pending synthetic mouse moves get dispatched. Isn't the WaitAndGetTitle below effectively doing the waiting? If you're saying we need to wait for mouse events before getting to 165 then the code should do that rather than assuming RunAllPendingInMessageLoop is enough.
https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:163: content::RunAllPendingInMessageLoop(); I checked and on ChromeOS there aren't any delayed synthetic mouse moves sent to the web contents after the context menu / JS alert has opened. I am hoping that this test will stay valuable even if the logic which sends synthetic mouse events in WindowEventDispatcher changes. WaitAndGetTitle() just waits for the title set by the done() Javascript function to be set.
https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:163: content::RunAllPendingInMessageLoop(); On 2015/03/12 21:44:14, pkotwicz wrote: > I checked and on ChromeOS there aren't any delayed synthetic mouse moves sent to > the web contents after the context menu / JS alert has opened. I am hoping that > this test will stay valuable even if the logic which sends synthetic mouse > events in WindowEventDispatcher changes. > > WaitAndGetTitle() just waits for the title set by the done() Javascript function > to be set. > Certainly, but aren't those set based on mouse events, which is the whole point of the test? I'm still not seeing why the RunAll is needed.
https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:163: content::RunAllPendingInMessageLoop(); On 2015/03/12 22:34:06, sky wrote: > On 2015/03/12 21:44:14, pkotwicz wrote: > > I checked and on ChromeOS there aren't any delayed synthetic mouse moves sent > to > > the web contents after the context menu / JS alert has opened. I am hoping > that > > this test will stay valuable even if the logic which sends synthetic mouse > > events in WindowEventDispatcher changes. > > > > WaitAndGetTitle() just waits for the title set by the done() Javascript > function > > to be set. > > > > Certainly, but aren't those set based on mouse events, which is the whole point > of the test? > I'm still not seeing why the RunAll is needed. sky@, pkotwicz@, RunAll is called in WaitForMenuOpenAndClose as well. What do you think of removing it here ? https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:196: content::RunAllPendingInMessageLoop(); On 2015/03/12 15:37:37, sky wrote: > And same comment here. This is not set based on mouse events, so I think we can remove RunAll as well.
sky@ I do not understand what you are saying. Delayed synthetic mouse events are generated as a result of things which are not mouse events (e.g. bounds changing, visibility changing) MouseLeaveTest.ModalDialog is testing that regressions such as crbug.com/450138 do not occur. crbug.com/450138 occurred as a result of posting a non-delayed synthetic mouse event. It is easy to see how the the "regressing code" could have posted a delayed synthetic mouse event and have resulted in the same regression
Sorry, wrong crbug number. Correct crbug number: http://crbug.com/394672
On 2015/03/16 19:20:41, pkotwicz wrote: > sky@ I do not understand what you are saying. Delayed synthetic mouse events are > generated as a result of things which are not mouse events (e.g. bounds > changing, visibility changing) My point is we should be waiting for the events we care about, and not relying on arbitrary message ordering. If this, or any tests, needs assertions after a mouse event has been received then it should wait for the mouse event and then do the assertions. Arbitrarily pumping events is error prone. > MouseLeaveTest.ModalDialog is testing that regressions such as crbug.com/450138 > do not occur. crbug.com/450138 occurred as a result of posting a non-delayed > synthetic mouse event. It is easy to see how the the "regressing code" could > have posted a delayed synthetic mouse event and have resulted in the same > regression
sky@, I did some investigation. On Windows, you are right, WM_CAPTURECHANGED is not sent synchronously as a result of calling SetCapture() which makes things hard. On CrOS, we get synthetic mouse moves in the MouseLeaveTest.ModalDialog test, so we cannot check that |WindowEventDispatcher::synthesize_mouse_move_| is false prior to calling "alert->CloseModalDialog();" I think that we can remove the calls to "content::RunAllPendingInMessageLoop()" and worry about catching Windows regressions once the tests are enabled on Windows. Scott, what do you think?
SGTM On Mon, Mar 16, 2015 at 5:05 PM, <pkotwicz@chromium.org> wrote: > sky@, I did some investigation. On Windows, you are right, WM_CAPTURECHANGED > is > not sent synchronously as a result of calling SetCapture() which makes > things > hard. > On CrOS, we get synthetic mouse moves in the MouseLeaveTest.ModalDialog > test, so > we cannot check that |WindowEventDispatcher::synthesize_mouse_move_| is > false > prior to calling "alert->CloseModalDialog();" > > I think that we can remove the calls to > "content::RunAllPendingInMessageLoop()" > and worry about catching Windows regressions once the tests are enabled on > Windows. Scott, what do you think? > > https://codereview.chromium.org/891213002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Just be sure to add a good comment for the windows side. On Tue, Mar 17, 2015 at 10:13 AM, Scott Violet <sky@chromium.org> wrote: > SGTM > > On Mon, Mar 16, 2015 at 5:05 PM, <pkotwicz@chromium.org> wrote: >> sky@, I did some investigation. On Windows, you are right, WM_CAPTURECHANGED >> is >> not sent synchronously as a result of calling SetCapture() which makes >> things >> hard. >> On CrOS, we get synthetic mouse moves in the MouseLeaveTest.ModalDialog >> test, so >> we cannot check that |WindowEventDispatcher::synthesize_mouse_move_| is >> false >> prior to calling "alert->CloseModalDialog();" >> >> I think that we can remove the calls to >> "content::RunAllPendingInMessageLoop()" >> and worry about catching Windows regressions once the tests are enabled on >> Windows. Scott, what do you think? >> >> https://codereview.chromium.org/891213002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... File chrome/browser/mouseleave_browsertest.cc (right): https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:28: void LoadTestPageAndWaitForMouseOver(content::WebContents* tab) { On 2015/03/12 15:37:37, sky wrote: > This should either return a boolean that it's successful, or callers should wrap > calls to it in EXPECT_NO_FATAL_FAILURE. Done. https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:34: gfx::Point(tab_view_bounds.x() + tab_view_bounds.width() / 2, On 2015/03/12 15:37:37, sky wrote: > nit: use constructor that takes x,y. Done. https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:77: gfx::Point above_content_; On 2015/03/12 15:37:37, sky wrote: > Add a description. Done. https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:108: #if defined(OS_WIN) On 2015/03/12 16:51:57, pkotwicz wrote: > I think the MouseLeaveTest.NewWindow should be removed. Right now it is not > possible to write a correct test for a mouseleave getting dispatched when a new > window is opened. In particular, this test passes even if the code which creates > the new window is removed. > > For more context see comments #29 and #37 > Done. https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:124: // Create an new window. On 2015/03/12 15:37:37, sky wrote: > an->a Done. I removed this TC. https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:127: browser_added_observer.WaitForSingleNewBrowser(); On 2015/03/12 15:37:37, sky wrote: > Seems like you're assuming the new window is opened where the mouse is. How do > you know that'll happen? Done. I removed this TC. https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:131: content::RunAllPendingInMessageLoop(); On 2015/03/12 15:37:37, sky wrote: > Why do you need the run all pending? Done. I removed this TC. https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:163: content::RunAllPendingInMessageLoop(); Done. I removed to call content::RunAllPendingInMessageLoop(). https://codereview.chromium.org/891213002/diff/200001/chrome/browser/mouselea... chrome/browser/mouseleave_browsertest.cc:196: content::RunAllPendingInMessageLoop(); Done. I removed to call content::RunAllPendingInMessageLoop()
LGTM
The CQ bit was checked by myid.shin@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/891213002/#ps220001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/891213002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/3e66e1af60b2f5b2f488ab8f4c3ca3532e57f049 Cr-Commit-Position: refs/heads/master@{#321280} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
