Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1233913009:
Make File-Picker modal on Linux (Closed)
DescriptionMake File-Picker modal on Linux
Chromium for Linux opens a Gtk file-picker for file browsing, but it is not
modal because there is no GtkWindow as parent of file-picker.
This patch allows the X11 host window to disable input event handling to make
a file-picker modal
The original CL was reverted due to wrong DCHECK with modal_dialog_xid_:
https://codereview.chromium.org/1243503002
BUG=408481
TEST=BrowserSelectFileDialogTest, ModalTest
Committed: https://crrev.com/3bc35de4ff086027d50519c9fb40122f27eb82f4
Cr-Commit-Position: refs/heads/master@{#360961}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Add DCHECK for |dialog| #Patch Set 3 : Add a test case #
Total comments: 6
Patch Set 4 : Add a nested runloop #
Total comments: 2
Patch Set 5 : do not use the interstitial page #
Total comments: 2
Patch Set 6 : add the comment and mouse-up event #
Total comments: 12
Patch Set 7 : add a timeout and more tests #
Total comments: 2
Patch Set 8 : add a WidgetObserver to know when the browser window becomes inactive #
Total comments: 7
Patch Set 9 : update the comment #Patch Set 10 : Move the test to browser_view_focus_uitest.cc #
Total comments: 6
Patch Set 11 : directly call ui_test_utils::ClickOnView and add EXPECT_TRUE(widget->IsActive()) #Patch Set 12 : initialize the test server #Patch Set 13 : do not feed key events to the input method when the window lost focus #
Total comments: 5
Patch Set 14 : inline native_widget_delegate_->AsWidget() in the if statement. #Patch Set 15 : close the file-picker when the test is done. #
Total comments: 2
Patch Set 16 : [For only testing] the body of the test case is commented out. #Patch Set 17 : update the test case and use ANNOTATE_SCOPED_MEMORY_LEAK #Patch Set 18 : remove the test infrastructure and rebase on master #
Messages
Total messages: 86 (18 generated)
joone.hur@intel.com changed reviewers: + erg@chromium.org, juncai@chromium.org, sadrul@chromium.org
erg@, sadrul@: please review this CL. juncai@: Could you check the DCHECK failure with this CL? https://codereview.chromium.org/1233913009/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/1233913009/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2043: DCHECK(!modal_dialog_xid_); The modal_dialog_xid_ should be null so this line was changed from DCHECK(modal_dialog_xid_) to DCHECK(!modal_dialog_xid_) https://codereview.chromium.org/1233913009/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2052: DCHECK(modal_dialog_xid_); modal_dialog_xid_ should be not null so it was changed from DCHECK(!modal_dialog_xid_) to DCHECK(modal_dialog_xid_)
On 2015/07/17 18:15:00, joone wrote: > erg@, sadrul@: please review this CL. > juncai@: Could you check the DCHECK failure with this CL? > > https://codereview.chromium.org/1233913009/diff/1/ui/views/widget/desktop_aur... > File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): > > https://codereview.chromium.org/1233913009/diff/1/ui/views/widget/desktop_aur... > ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2043: > DCHECK(!modal_dialog_xid_); > The modal_dialog_xid_ should be null so this line was changed from > DCHECK(modal_dialog_xid_) to DCHECK(!modal_dialog_xid_) > > https://codereview.chromium.org/1233913009/diff/1/ui/views/widget/desktop_aur... > ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2052: > DCHECK(modal_dialog_xid_); > modal_dialog_xid_ should be not null so it was changed from > DCHECK(!modal_dialog_xid_) to DCHECK(modal_dialog_xid_) lgtm
https://codereview.chromium.org/1233913009/diff/1/chrome/browser/ui/libgtk2ui... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1233913009/diff/1/chrome/browser/ui/libgtk2ui... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:281: owning_window->GetHost()->GetAcceleratedWidget()); From the code and comment above, looks like owning_window can be NULL? Should this have a null-check? https://codereview.chromium.org/1233913009/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/1233913009/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2043: DCHECK(!modal_dialog_xid_); On 2015/07/17 18:14:59, joone wrote: > The modal_dialog_xid_ should be null so this line was changed from > DCHECK(modal_dialog_xid_) to DCHECK(!modal_dialog_xid_) Sorry I missed this during review. Looks like something that would be easily caught during manual testing. https://codereview.chromium.org/1233913009/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2044: modal_dialog_xid_ = dialog; Add a DCHECK that |dialog| is non-zero too? https://codereview.chromium.org/1233913009/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right): https://codereview.chromium.org/1233913009/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/x11_desktop_handler.cc:235: RevertToParent, CurrentTime); Hm, does this work correctly? How are you testing? My understanding is, the file-selector dialog that was created by GTK+ is using a different Display connection (!= xdisplay_, which is created by chrome). If that is the case, does XSetInputFocus() do the right thing when the window does not belong to the display?
https://codereview.chromium.org/1233913009/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/1233913009/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2044: modal_dialog_xid_ = dialog; On 2015/07/19 01:25:36, sadrul wrote: > Add a DCHECK that |dialog| is non-zero too? I will add a DECHECK. https://codereview.chromium.org/1233913009/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right): https://codereview.chromium.org/1233913009/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/x11_desktop_handler.cc:235: RevertToParent, CurrentTime); On 2015/07/19 01:25:36, sadrul wrote: > Hm, does this work correctly? How are you testing? > > My understanding is, the file-selector dialog that was created by GTK+ is using > a different Display connection (!= xdisplay_, which is created by chrome). If > that is the case, does XSetInputFocus() do the right thing when the window does > not belong to the display? Each x-server has one xdisplay so it is shared between Gtk+ file-picker and DWTH in Chrome.
sadrul@ Could you take a look the test case I added?
ping sadrul
https://codereview.chromium.org/1233913009/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/1233913009/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:252: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100)); Is this necessary here because you want to wait until the GTK+ dialog is opened and focus? If that's the case, then this can potentially be flaky. You should find a more deterministic way of doing this (e.g. a nested runloop until the browser window loses focus, or something like that). https://codereview.chromium.org/1233913009/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:255: ui_controls::LEFT, ui_controls::DOWN)); You want to make sure the cursor is located within the browser window, but outside the gtk+ dialog, right? https://codereview.chromium.org/1233913009/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:259: browser()->tab_strip_model()->GetActiveWebContents()); Why do you need to open the interstitial page here?
https://codereview.chromium.org/1233913009/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/1233913009/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:252: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100)); On 2015/08/20 06:24:20, sadrul wrote: > Is this necessary here because you want to wait until the GTK+ dialog is opened > and focus? If that's the case, then this can potentially be flaky. You should > find a more deterministic way of doing this (e.g. a nested runloop until the > browser window loses focus, or something like that). Ok, I will find another way of doing this. https://codereview.chromium.org/1233913009/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:255: ui_controls::LEFT, ui_controls::DOWN)); On 2015/08/20 06:24:20, sadrul wrote: > You want to make sure the cursor is located within the browser window, but > outside the gtk+ dialog, right? Yes, right. https://codereview.chromium.org/1233913009/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:259: browser()->tab_strip_model()->GetActiveWebContents()); On 2015/08/20 06:24:20, sadrul wrote: > Why do you need to open the interstitial page here? The interstitial page could only lose focus when file-picker pops-up.
sadrul@ could you review the updated test case?
https://codereview.chromium.org/1233913009/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/1233913009/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:259: ASSERT_TRUE(ui_test_utils::SendMouseMoveSync(gfx::Point(100, 100))); How can you tell this point is not on the open-dialog? https://codereview.chromium.org/1233913009/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:266: EXPECT_FALSE(interstitial_page->HasFocus()); Why is it necessary to open the interstitial page? Can you not check the activation/focus state of the browser itself?
On 2015/08/27 19:56:17, sadrul wrote: > https://codereview.chromium.org/1233913009/diff/60001/chrome/browser/ui/brows... > File chrome/browser/ui/browser_focus_uitest.cc (right): > > https://codereview.chromium.org/1233913009/diff/60001/chrome/browser/ui/brows... > chrome/browser/ui/browser_focus_uitest.cc:259: > ASSERT_TRUE(ui_test_utils::SendMouseMoveSync(gfx::Point(100, 100))); > How can you tell this point is not on the open-dialog? > > https://codereview.chromium.org/1233913009/diff/60001/chrome/browser/ui/brows... > chrome/browser/ui/browser_focus_uitest.cc:266: > EXPECT_FALSE(interstitial_page->HasFocus()); > Why is it necessary to open the interstitial page? Can you not check the > activation/focus state of the browser itself? sadrul@ A mouse down event sends to the top left of the browser window. It's more accurate point than (100, 100) In addition, browser()->window()->IsActive() is called to check if the window has focus instead of creating the interstitial page.
lgtm https://codereview.chromium.org/1233913009/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/1233913009/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:258: run_loop.Run(); Add a comment explaining why this is necessary. https://codereview.chromium.org/1233913009/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:264: ui_controls::LEFT, ui_controls::DOWN)); Can this send both (DOWN | UP) for a click?
(lgtm)
The CQ bit was checked by joone.hur@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from juncai@chromium.org, erg@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1233913009/#ps100001 (title: "add the comment and mouse-up event")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1233913009/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1233913009/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2015/09/02 22:02:51, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) This CL needs to get another LGTM: Missing LGTM from an OWNER for these files: chrome/browser/ui/browser_focus_uitest.cc
On 2015/09/02 22:24:30, joone wrote: > On 2015/09/02 22:02:51, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > This CL needs to get another LGTM: > > Missing LGTM from an OWNER for these files: > chrome/browser/ui/browser_focus_uitest.cc sadrul@, erg@ Do you know who is the owner of browser_focus_uitest.cc?
On 2015/09/04 17:50:03, joone wrote: > On 2015/09/02 22:24:30, joone wrote: > > On 2015/09/02 22:02:51, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > > > This CL needs to get another LGTM: > > > > Missing LGTM from an OWNER for these files: > > chrome/browser/ui/browser_focus_uitest.cc > > sadrul@, erg@ Do you know who is the owner of browser_focus_uitest.cc? Teach a man to fish: chrome/browser/ui/OWNERS is a file you can read. Documentation on owners files: https://www.chromium.org/developers/owners-files
joone.hur@intel.com changed reviewers: + msw@chromium.org
erg@ thanks! msw@ could you review the test case?
https://codereview.chromium.org/1233913009/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/1233913009/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:243: #if defined(OS_LINUX) && !defined(OS_CHROMEOS) && defined(USE_AURA) Remove |&& defined(USE_AURA)| from here and similar instances below; no such configuration exists, afaik. https://codereview.chromium.org/1233913009/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:246: #define MAYBE_BrowserDialogModalTest DISABLE_BrowserDialogModalTest "DISABLED_" https://codereview.chromium.org/1233913009/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:256: // Insert a delay to wait until the file-picker is opened and gets focus. Is there a better way to do this? If the nested run loop's quit closure isn't called because something is blocked, the test will hang until the harness aborts, after maybe 20 minutes (I think). Consider using something like ViewManagerTestBase::DoRunLoopWithTimeout; I also wonder if some intermediary step might post another task amidst the opening of the dialog, so the nested run loop might quit before the dialog is fully opened (but perhaps that's unlikely). Maybe consider observing deactivation of the browser window? https://codereview.chromium.org/1233913009/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:262: gfx::Rect rect = browser()->window()->GetBounds(); Right after the run loop, do EXPECT_FALSE(browser()->window()->IsActive()); https://codereview.chromium.org/1233913009/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:263: ASSERT_TRUE(ui_test_utils::SendMouseMoveSync(gfx::Point(rect.x() + 1, nit: maybe use ClickOnView instead? https://codereview.chromium.org/1233913009/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:265: ASSERT_TRUE(ui_test_utils::SendMouseEventsSync( I wonder if (1) Perhaps the dialog will actually cover the visible bounds of the browser window, so clicking there is actually focusing the dialog... (2) If other methods of window switching/interaction would actually cause the browser window to become active here (ie. can you also test BringBrowserWindowToFront, ShowAndFocusNativeWindow, SendKeyPress*, and/or pressing something like [Alt]+[Tab], etc.)
msw@ thanks for the review! https://codereview.chromium.org/1233913009/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/1233913009/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:243: #if defined(OS_LINUX) && !defined(OS_CHROMEOS) && defined(USE_AURA) On 2015/09/04 22:25:16, msw wrote: > Remove |&& defined(USE_AURA)| from here and similar instances below; no such > configuration exists, afaik. Done. https://codereview.chromium.org/1233913009/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:246: #define MAYBE_BrowserDialogModalTest DISABLE_BrowserDialogModalTest On 2015/09/04 22:25:16, msw wrote: > "DISABLED_" Done. https://codereview.chromium.org/1233913009/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:256: // Insert a delay to wait until the file-picker is opened and gets focus. On 2015/09/04 22:25:17, msw wrote: > Is there a better way to do this? If the nested run loop's quit closure isn't > called because something is blocked, the test will hang until the harness > aborts, after maybe 20 minutes (I think). Consider using something like > ViewManagerTestBase::DoRunLoopWithTimeout; I also wonder if some intermediary > step might post another task amidst the opening of the dialog, so the nested run > loop might quit before the dialog is fully opened (but perhaps that's unlikely). > Maybe consider observing deactivation of the browser window? I added a timeout(1 sec). https://codereview.chromium.org/1233913009/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:262: gfx::Rect rect = browser()->window()->GetBounds(); On 2015/09/04 22:25:17, msw wrote: > Right after the run loop, do EXPECT_FALSE(browser()->window()->IsActive()); Done. https://codereview.chromium.org/1233913009/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:263: ASSERT_TRUE(ui_test_utils::SendMouseMoveSync(gfx::Point(rect.x() + 1, On 2015/09/04 22:25:16, msw wrote: > nit: maybe use ClickOnView instead? Done. https://codereview.chromium.org/1233913009/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:265: ASSERT_TRUE(ui_test_utils::SendMouseEventsSync( On 2015/09/04 22:25:16, msw wrote: > I wonder if (1) Perhaps the dialog will actually cover the visible bounds of the > browser window, so clicking there is actually focusing the dialog... Calling ClickOnView() may be able to fix this problem. (2) If > other methods of window switching/interaction would actually cause the browser > window to become active here (ie. can you also test BringBrowserWindowToFront, > ShowAndFocusNativeWindow, SendKeyPress*, and/or pressing something like > [Alt]+[Tab], etc.) Done.
https://codereview.chromium.org/1233913009/diff/120001/chrome/browser/ui/brow... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/1233913009/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:261: base::TimeDelta::FromMilliseconds(1000)); Hmm, hardcoded timeouts aren't good either; now the test always takes at least 1 second, and if the dialog takes any longer the test will be flaky and imminently disabled. I asked sky@ and he strongly recommended using an observer that will be called back on browser deactivation. He said that the DoRunLoopWithTimeout alone wouldn't be sufficient either, but would be a decent addition to the observer. I'm not aware of a cross-platform window activation observer, but you can move this test to a Views-specific interactive ui test, and use WidgetObserver, since you only care about Linux desktop; that should minimize the work needed for a correct implementation.
https://codereview.chromium.org/1233913009/diff/120001/chrome/browser/ui/brow... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/1233913009/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:261: base::TimeDelta::FromMilliseconds(1000)); On 2015/09/11 00:13:57, msw wrote: > Hmm, hardcoded timeouts aren't good either; now the test always takes at least 1 > second, and if the dialog takes any longer the test will be flaky and imminently > disabled. I asked sky@ and he strongly recommended using an observer that will > be called back on browser deactivation. He said that the DoRunLoopWithTimeout > alone wouldn't be sufficient either, but would be a decent addition to the > observer. > > I'm not aware of a cross-platform window activation observer, but you can move > this test to a Views-specific interactive ui test, and use WidgetObserver, since > you only care about Linux desktop; that should minimize the work needed for a > correct implementation. The timeout was removed and a WidgetObserver was added in the updated CL. Please take a look at it.
https://codereview.chromium.org/1233913009/diff/140001/chrome/browser/ui/brow... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/1233913009/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:195: class WidgetActivationWaiter : public views::WidgetObserver { The WidgetActivationWaiter class was copied from ui/views/widget/widget_interactive_uitest.cc and was updated for BrowserDialogModalTest.
Sorry for the run around, but the test/helper looks good now. https://codereview.chromium.org/1233913009/diff/140001/chrome/browser/ui/brow... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/1233913009/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:40: #include "ui/views/widget/widget.h" c/b/ui/ can't always depend on views code; move this test to: chrome/browser/ui/views/frame/browser_view_focus_uitest.cc (or maybe a new c/b/ui/views/browser_focus_uitest.cc) https://codereview.chromium.org/1233913009/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:289: // Insert a delay to wait until the browser window becomes inactive. Update this comment.
Thank you for the review. https://codereview.chromium.org/1233913009/diff/140001/chrome/browser/ui/brow... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/1233913009/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:289: // Insert a delay to wait until the browser window becomes inactive. On 2015/09/14 16:56:16, msw wrote: > Update this comment. Done.
https://codereview.chromium.org/1233913009/diff/140001/chrome/browser/ui/brow... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/1233913009/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:40: #include "ui/views/widget/widget.h" On 2015/09/14 16:56:16, msw wrote: > c/b/ui/ can't always depend on views code; move this test to: > chrome/browser/ui/views/frame/browser_view_focus_uitest.cc > (or maybe a new c/b/ui/views/browser_focus_uitest.cc) Ping; I'm not sure if you saw this comment.
https://codereview.chromium.org/1233913009/diff/140001/chrome/browser/ui/brow... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/1233913009/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:40: #include "ui/views/widget/widget.h" On 2015/09/14 16:56:16, msw wrote: > c/b/ui/ can't always depend on views code; move this test to: > chrome/browser/ui/views/frame/browser_view_focus_uitest.cc > (or maybe a new c/b/ui/views/browser_focus_uitest.cc) Okay, I will move this test to browser_view_focus_uitest.cc or browser_focus_uitest.cc.
https://codereview.chromium.org/1233913009/diff/140001/chrome/browser/ui/brow... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/1233913009/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser_focus_uitest.cc:40: #include "ui/views/widget/widget.h" On 2015/09/14 18:23:39, joone wrote: > On 2015/09/14 16:56:16, msw wrote: > > c/b/ui/ can't always depend on views code; move this test to: > > chrome/browser/ui/views/frame/browser_view_focus_uitest.cc > > (or maybe a new c/b/ui/views/browser_focus_uitest.cc) > > Okay, I will move this test to browser_view_focus_uitest.cc or > browser_focus_uitest.cc. Done.
lgtm with nits; thanks for addressing my feedback. https://codereview.chromium.org/1233913009/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view_focus_uitest.cc (right): https://codereview.chromium.org/1233913009/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view_focus_uitest.cc:33: ui_test_utils::ClickOnView(browser(), vid); nit: InProcessBrowserTest::browser() is protected, so this helper (and IsViewFocused) aren't really needed, the test fixtures should be able to directly call ui_test_utils::ClickOnView(browser(), vid); https://codereview.chromium.org/1233913009/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view_focus_uitest.cc:41: widget->AddObserver(this); nit: add this expectation here: EXPECT_TRUE(widget->IsActive()); https://codereview.chromium.org/1233913009/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view_focus_uitest.cc:131: // This test is only for Linux Desktop. nit: as a follow-up should we try to enable this on other Views platforms (Windows and Chrome OS)?
Thanks! https://codereview.chromium.org/1233913009/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view_focus_uitest.cc (right): https://codereview.chromium.org/1233913009/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view_focus_uitest.cc:33: ui_test_utils::ClickOnView(browser(), vid); On 2015/09/14 23:04:59, msw wrote: > nit: InProcessBrowserTest::browser() is protected, so this helper (and > IsViewFocused) aren't really needed, the test fixtures should be able to > directly call ui_test_utils::ClickOnView(browser(), vid); Done. https://codereview.chromium.org/1233913009/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view_focus_uitest.cc:41: widget->AddObserver(this); On 2015/09/14 23:04:59, msw wrote: > nit: add this expectation here: EXPECT_TRUE(widget->IsActive()); Done. https://codereview.chromium.org/1233913009/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view_focus_uitest.cc:131: // This test is only for Linux Desktop. On 2015/09/14 23:04:58, msw wrote: > nit: as a follow-up should we try to enable this on other Views platforms > (Windows and Chrome OS)? Sure, I will try to do.
The CQ bit was checked by joone.hur@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, erg@chromium.org, juncai@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/1233913009/#ps200001 (title: "directly call ui_test_utils::ClickOnView and add EXPECT_TRUE(widget->IsActive())")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1233913009/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1233913009/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by joone.hur@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, sadrul@chromium.org, erg@chromium.org, juncai@chromium.org Link to the patchset: https://codereview.chromium.org/1233913009/#ps220001 (title: "initialize the test server")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1233913009/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1233913009/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
msw@ I fixed the crash problem in the test case. Please take a look at the updated CL. https://codereview.chromium.org/1233913009/diff/240001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/1233913009/diff/240001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1527: GetInputMethod()->DispatchKeyEvent(event); We don't need to feed key events to the input method when the browser window lost focus. [7141:7141:0915/183425:FATAL:input_method_auralinux.cc(48)] Check failed: system_toplevel_window_focused(). #0 0x0000004c31e1 __interceptor_backtrace #1 0x000002d924e8 base::debug::StackTrace::StackTrace() #2 0x000002dd4ff7 logging::LogMessage::~LogMessage() #3 0x000004f96725 ui::InputMethodAuraLinux::DispatchKeyEvent() #4 0x000002457cbf views::DesktopWindowTreeHostX11::DispatchEvent() #5 0x00000245a480 views::DesktopWindowTreeHostX11::DispatchEvent() #6 0x000003c4c55d ui::PlatformEventSource::DispatchEvent() #7 0x000003c4f875 ui::X11EventSource::DispatchEvent() #8 0x000003c4f351 ui::X11EventSource::DispatchXEvents() #9 0x000003c501cc ui::(anonymous namespace)::XSourceDispatch() #10 0x7f49a4935d13 g_main_context_dispatch
lgtm with a nit https://codereview.chromium.org/1233913009/diff/240001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/1233913009/diff/240001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1525: views::Widget* widget = native_widget_delegate_->AsWidget(); nit: inline this in the if statement below. https://codereview.chromium.org/1233913009/diff/240001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1527: GetInputMethod()->DispatchKeyEvent(event); On 2015/09/16 22:42:55, joone wrote: > > We don't need to feed key events to the input method when the browser window > lost focus. > > [7141:7141:0915/183425:FATAL:input_method_auralinux.cc(48)] Check failed: > system_toplevel_window_focused(). > #0 0x0000004c31e1 __interceptor_backtrace > #1 0x000002d924e8 base::debug::StackTrace::StackTrace() > #2 0x000002dd4ff7 logging::LogMessage::~LogMessage() > #3 0x000004f96725 ui::InputMethodAuraLinux::DispatchKeyEvent() > #4 0x000002457cbf views::DesktopWindowTreeHostX11::DispatchEvent() > #5 0x00000245a480 views::DesktopWindowTreeHostX11::DispatchEvent() > #6 0x000003c4c55d ui::PlatformEventSource::DispatchEvent() > #7 0x000003c4f875 ui::X11EventSource::DispatchEvent() > #8 0x000003c4f351 ui::X11EventSource::DispatchXEvents() > #9 0x000003c501cc ui::(anonymous namespace)::XSourceDispatch() > #10 0x7f49a4935d13 g_main_context_dispatch This seems reasonable, but other reviewers on the CL may know better.
sadrul@, erg@ could you take a look at the change of the test case? https://codereview.chromium.org/1233913009/diff/240001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/1233913009/diff/240001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1525: views::Widget* widget = native_widget_delegate_->AsWidget(); On 2015/09/16 23:10:04, msw wrote: > nit: inline this in the if statement below. Done. https://codereview.chromium.org/1233913009/diff/240001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1527: GetInputMethod()->DispatchKeyEvent(event); On 2015/09/16 23:10:04, msw wrote: > On 2015/09/16 22:42:55, joone wrote: > > > > We don't need to feed key events to the input method when the browser window > > lost focus. > > > > [7141:7141:0915/183425:FATAL:input_method_auralinux.cc(48)] Check failed: > > system_toplevel_window_focused(). > > #0 0x0000004c31e1 __interceptor_backtrace > > #1 0x000002d924e8 base::debug::StackTrace::StackTrace() > > #2 0x000002dd4ff7 logging::LogMessage::~LogMessage() > > #3 0x000004f96725 ui::InputMethodAuraLinux::DispatchKeyEvent() > > #4 0x000002457cbf views::DesktopWindowTreeHostX11::DispatchEvent() > > #5 0x00000245a480 views::DesktopWindowTreeHostX11::DispatchEvent() > > #6 0x000003c4c55d ui::PlatformEventSource::DispatchEvent() > > #7 0x000003c4f875 ui::X11EventSource::DispatchEvent() > > #8 0x000003c4f351 ui::X11EventSource::DispatchXEvents() > > #9 0x000003c501cc ui::(anonymous namespace)::XSourceDispatch() > > #10 0x7f49a4935d13 g_main_context_dispatch > > This seems reasonable, but other reviewers on the CL may know better. Acknowledged.
lgtm
The CQ bit was checked by joone.hur@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, juncai@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/1233913009/#ps260001 (title: "inline native_widget_delegate_->AsWidget() in the if statement.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1233913009/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1233913009/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/09/17 19:10:12, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) There is a memory leak in libfontconfig as follows. It looks like a new bug. ==186002==ERROR: LeakSanitizer: detected memory leaks Indirect leak of 3870 byte(s) in 320 object(s) allocated from: #0 0x5149fb (/home/joone/otc/chromium/src/out/Release/interactive_ui_tests+0x5149fb) #1 0x7f48b7924839 (/lib/x86_64-linux-gnu/libc.so.6+0x88839) ----------------------------------------------------- Suppressions used: count bytes template 438 22528 libfontconfig -----------------------------------------------------
On 2015/09/17 22:55:06, joone wrote: > On 2015/09/17 19:10:12, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > There is a memory leak in libfontconfig as follows. It looks like a new bug. > > ==186002==ERROR: LeakSanitizer: detected memory leaks > > Indirect leak of 3870 byte(s) in 320 object(s) allocated from: > #0 0x5149fb > (/home/joone/otc/chromium/src/out/Release/interactive_ui_tests+0x5149fb) > #1 0x7f48b7924839 (/lib/x86_64-linux-gnu/libc.so.6+0x88839) > > ----------------------------------------------------- > Suppressions used: > count bytes template > 438 22528 libfontconfig > ----------------------------------------------------- I don't see the cause of the leak offhand; maybe the dialog itself is left open?
On 2015/09/17 23:09:41, msw wrote: > On 2015/09/17 22:55:06, joone wrote: > > On 2015/09/17 19:10:12, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > > > There is a memory leak in libfontconfig as follows. It looks like a new bug. > > > > ==186002==ERROR: LeakSanitizer: detected memory leaks > > > > Indirect leak of 3870 byte(s) in 320 object(s) allocated from: > > #0 0x5149fb > > (/home/joone/otc/chromium/src/out/Release/interactive_ui_tests+0x5149fb) > > #1 0x7f48b7924839 (/lib/x86_64-linux-gnu/libc.so.6+0x88839) > > > > ----------------------------------------------------- > > Suppressions used: > > count bytes template > > 438 22528 libfontconfig > > ----------------------------------------------------- > > I don't see the cause of the leak offhand; maybe the dialog itself is left open? Many interactive_ui_tests have the same problem.
I ran the test cases in BrowserFocusTest without this CL, but every test failed
due to the same memory leak as follows:
..
..
==211689==ERROR: LeakSanitizer: detected memory leaks
Indirect leak of 2248 byte(s) in 188 object(s) allocated from:
#0 0x51487b
(/home/joone/otc/chromium/src/out/Release/interactive_ui_tests+0x51487b)
#1 0x7f8443305839 (/lib/x86_64-linux-gnu/libc.so.6+0x88839)
-----------------------------------------------------
Suppressions used:
count bytes template
261 15632 libfontconfig
-----------------------------------------------------
SUMMARY: AddressSanitizer: 2248 byte(s) leaked in 188 allocation(s).
[5/5] BrowserFocusTest.NavigateFromOmniboxIntoNewTab (1311 ms)
5 tests failed:
BrowserFocusTest.ClickingMovesFocus
(../../chrome/browser/ui/browser_focus_uitest.cc:198)
BrowserFocusTest.FindFocusTest
(../../chrome/browser/ui/browser_focus_uitest.cc:487)
BrowserFocusTest.FocusTraversal
(../../chrome/browser/ui/browser_focus_uitest.cc:428)
BrowserFocusTest.InterstitialFocus
(../../chrome/browser/ui/browser_focus_uitest.cc:460)
BrowserFocusTest.NavigateFromOmniboxIntoNewTab
(../../chrome/browser/ui/browser_focus_uitest.cc:632)
On 2015/09/18 00:22:21, joone wrote: > On 2015/09/17 23:09:41, msw wrote: > > On 2015/09/17 22:55:06, joone wrote: > > > On 2015/09/17 19:10:12, commit-bot: I haz the power wrote: > > > > Try jobs failed on following builders: > > > > linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > > > > > There is a memory leak in libfontconfig as follows. It looks like a new bug. > > > > > > ==186002==ERROR: LeakSanitizer: detected memory leaks > > > > > > Indirect leak of 3870 byte(s) in 320 object(s) allocated from: > > > #0 0x5149fb > > > (/home/joone/otc/chromium/src/out/Release/interactive_ui_tests+0x5149fb) > > > #1 0x7f48b7924839 (/lib/x86_64-linux-gnu/libc.so.6+0x88839) > > > > > > ----------------------------------------------------- > > > Suppressions used: > > > count bytes template > > > 438 22528 libfontconfig > > > ----------------------------------------------------- > > > > I don't see the cause of the leak offhand; maybe the dialog itself is left > open? > > Many interactive_ui_tests have the same problem. Are there other interactive_ui_tests failing with the same failure in the bots? (note that the specific failure the bot is showing for the new test is: ================================================================= ==13482==ERROR: LeakSanitizer: detected memory leaks Direct leak of 32 byte(s) in 1 object(s) allocated from: #0 0x513e9b in __interceptor_malloc (/tmp/run_tha_testTbaJAQ/out/Release/interactive_ui_tests+0x513e9b) #1 0x7f3d27584a38 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4da38) Indirect leak of 25 byte(s) in 1 object(s) allocated from: #0 0x513e9b in __interceptor_malloc (/tmp/run_tha_testTbaJAQ/out/Release/interactive_ui_tests+0x513e9b) #1 0x7f3d27584a38 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4da38) ) Seeing as how g_malloc is involved, it is highly likely that the leak is happening in glib/gtk land. As msw@ noted, it may very well be because the gtk+ dialog window is left open.
On 2015/09/18 21:25:00, sadrul wrote:
>
> Are there other interactive_ui_tests failing with the same failure in the
bots?
> (note that the specific failure the bot is showing for the new test is:
>
There are no other same failures in the bots, but I see all BrowserFocusTest
failed due to the same problem in my PC.
> =================================================================
> ==13482==ERROR: LeakSanitizer: detected memory leaks
>
> Direct leak of 32 byte(s) in 1 object(s) allocated from:
> #0 0x513e9b in __interceptor_malloc
> (/tmp/run_tha_testTbaJAQ/out/Release/interactive_ui_tests+0x513e9b)
> #1 0x7f3d27584a38 in g_malloc
> (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4da38)
>
> Indirect leak of 25 byte(s) in 1 object(s) allocated from:
> #0 0x513e9b in __interceptor_malloc
> (/tmp/run_tha_testTbaJAQ/out/Release/interactive_ui_tests+0x513e9b)
> #1 0x7f3d27584a38 in g_malloc
> (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4da38)
>
> )
>
> Seeing as how g_malloc is involved, it is highly likely that the leak is
> happening in glib/gtk land. As msw@ noted, it may very well be because the
gtk+
> dialog window is left open.
I tried to run ASSERT_TRUE(ui_test_utils::SendKeyPressSync(
browser(), ui::VKEY_ESCAPE, false, false, false, false));
But, the file-picker doesn't close.
Is there any way to close the file-picker?
In addition, SelectFileDialogImplGTK::~SelectFileDialogImplGTK is called when
the test case is done.
So the gtk+ dialog window seems to be removed when the test case is done.
On 2015/09/21 21:04:34, joone wrote: > On 2015/09/18 21:25:00, sadrul wrote: > > > > Are there other interactive_ui_tests failing with the same failure in the > bots? > > (note that the specific failure the bot is showing for the new test is: > > > > There are no other same failures in the bots, but I see all BrowserFocusTest > failed due to the same problem in my PC. > > > ================================================================= > > ==13482==ERROR: LeakSanitizer: detected memory leaks > > > > Direct leak of 32 byte(s) in 1 object(s) allocated from: > > #0 0x513e9b in __interceptor_malloc > > (/tmp/run_tha_testTbaJAQ/out/Release/interactive_ui_tests+0x513e9b) > > #1 0x7f3d27584a38 in g_malloc > > (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4da38) > > > > Indirect leak of 25 byte(s) in 1 object(s) allocated from: > > #0 0x513e9b in __interceptor_malloc > > (/tmp/run_tha_testTbaJAQ/out/Release/interactive_ui_tests+0x513e9b) > > #1 0x7f3d27584a38 in g_malloc > > (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4da38) > > > > ) > > > > Seeing as how g_malloc is involved, it is highly likely that the leak is > > happening in glib/gtk land. As msw@ noted, it may very well be because the > gtk+ > > dialog window is left open. > > I tried to run ASSERT_TRUE(ui_test_utils::SendKeyPressSync( > browser(), ui::VKEY_ESCAPE, false, false, false, false)); > > But, the file-picker doesn't close. > Is there any way to close the file-picker? > > In addition, SelectFileDialogImplGTK::~SelectFileDialogImplGTK is called when > the test case is done. > So the gtk+ dialog window seems to be removed when the test case is done. Maybe see how it's destroyed with the test TearDown and ensure that's done in the test itself? It might require tearing down the browser window?
msw@, sadrul@ I managed to close the file-picker by sending a XKeyEvent. could you take a look at the updated CL? Also, it would to nice to test the CL on buildbot(linux_chromium_asan_rel_ng) to see if there is any memory leak.
I had the asan bot run, it looks like the test still leaks... Perhaps this just needs a suppression if the leak is really in libfontconfig? build/sanitizers/lsan_suppressions.cc mentions http://crbug.com/39050 (+CC mattm and glider): https://code.google.com/p/chromium/codesearch#chromium/src/build/sanitizers/l... But first I would try to ascertain what's triggering the leak from our code (build ASAN locally or run tryjobs and comment out parts of the test's body, and/or the body of the functions it calls, until you find the culprit). https://codereview.chromium.org/1233913009/diff/280001/chrome/browser/ui/brow... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/1233913009/diff/280001/chrome/browser/ui/brow... chrome/browser/ui/browser.h:404: void CloseFileDialog(); Adding public API to Browser (especially test-specific API, which should be named "CloseFileDialogForTesting") is not really desirable. I suggest making OpenFile return a scoped_refptr<ui::SelectFileDialog> reference to the dialog, so that callers can interact with the dialog as needed. Otherwise, *maybe* adding something like: scoped_refptr<ui::SelectFileDialog> ui::SelectFileDialog::GetDialogForListener(ui::SelectFileDialog::Listener* listener) (which checks a cached map of listeners to their open dialogs) might be preferable to adding this new Browser::CloseFileDialogForTesting() function. I'd ask that others on this CL weigh in. https://codereview.chromium.org/1233913009/diff/280001/ui/shell_dialogs/selec... File ui/shell_dialogs/select_file_dialog.h (right): https://codereview.chromium.org/1233913009/diff/280001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog.h:198: virtual void CloseImpl() = 0; Give this base class a CloseImpl definition that just calls NOTIMPLEMENTED(), so each implementation doesn't have to define this.
On 2015/09/23 17:12:21, msw wrote: > I had the asan bot run, it looks like the test still leaks... Perhaps this just > needs a suppression if the leak is really in libfontconfig? > build/sanitizers/lsan_suppressions.cc mentions http://crbug.com/39050 (+CC mattm > and glider): > https://code.google.com/p/chromium/codesearch#chromium/src/build/sanitizers/l... > > But first I would try to ascertain what's triggering the leak from our code > (build ASAN locally or run tryjobs and comment out parts of the test's body, > and/or the body of the functions it calls, until you find the culprit). > > https://codereview.chromium.org/1233913009/diff/280001/chrome/browser/ui/brow... > File chrome/browser/ui/browser.h (right): > > https://codereview.chromium.org/1233913009/diff/280001/chrome/browser/ui/brow... > chrome/browser/ui/browser.h:404: void CloseFileDialog(); > Adding public API to Browser (especially test-specific API, which should be > named "CloseFileDialogForTesting") is not really desirable. > > I suggest making OpenFile return a scoped_refptr<ui::SelectFileDialog> reference > to the dialog, so that callers can interact with the dialog as needed. > > Otherwise, *maybe* adding something like: scoped_refptr<ui::SelectFileDialog> > ui::SelectFileDialog::GetDialogForListener(ui::SelectFileDialog::Listener* > listener) (which checks a cached map of listeners to their open dialogs) might > be preferable to adding this new Browser::CloseFileDialogForTesting() function. > > I'd ask that others on this CL weigh in. > > https://codereview.chromium.org/1233913009/diff/280001/ui/shell_dialogs/selec... > File ui/shell_dialogs/select_file_dialog.h (right): > > https://codereview.chromium.org/1233913009/diff/280001/ui/shell_dialogs/selec... > ui/shell_dialogs/select_file_dialog.h:198: virtual void CloseImpl() = 0; > Give this base class a CloseImpl definition that just calls NOTIMPLEMENTED(), so > each implementation doesn't have to define this. I'm not sure where the libfontconfig is coming into this discussion, it only shows up in the suppressions. You should ignore the "Suppressions used" part of the report and just look at the leak tracebacks. I second the recommendation to try doing some local debugging with an asan build.
On 2015/09/23 20:28:37, mattm wrote: > I'm not sure where the libfontconfig is coming into this discussion, it only > shows up in the suppressions. You should ignore the "Suppressions used" part of > the report and just look at the leak tracebacks. > > I second the recommendation to try doing some local debugging with an asan > build. Other test cases like BrowserFocusTest.* also have the memory leak on my local machine so it's hard to debug this leak.
On 2015/09/23 17:12:21, msw wrote: > I had the asan bot run, it looks like the test still leaks... Perhaps this just > needs a suppression if the leak is really in libfontconfig? > build/sanitizers/lsan_suppressions.cc mentions http://crbug.com/39050 (+CC mattm > and glider): > https://code.google.com/p/chromium/codesearch#chromium/src/build/sanitizers/l... > > But first I would try to ascertain what's triggering the leak from our code > (build ASAN locally or run tryjobs and comment out parts of the test's body, > and/or the body of the functions it calls, until you find the culprit). > Please try to run tryjobs after commenting out the body of the test code. If there are still memory leaks, there might be memory leaks in SelectFileDialogImplGtk.
On 2015/09/24 01:55:54, joone wrote: > Please try to run tryjobs after commenting out the body of the test code. > If there are still memory leaks, there might be memory leaks in > SelectFileDialogImplGtk. You can upload a patch set that you'd like to test and I'll trigger the bot... On 2015/09/24 01:48:22, joone wrote: > Other test cases like BrowserFocusTest.* also have the memory leak on my local > machine so it's hard to debug this leak. Run the test binary with this command line flag to only run your test: --gtest_filter=BrowserViewFocusTest.BrowserDialogModalTest
On 2015/09/24 17:25:42, msw wrote: > On 2015/09/24 01:55:54, joone wrote: > > Please try to run tryjobs after commenting out the body of the test code. > > If there are still memory leaks, there might be memory leaks in > > SelectFileDialogImplGtk. > > You can upload a patch set that you'd like to test and I'll trigger the bot... > Ok, I will upload a patch that the body of the test case is commented out. > On 2015/09/24 01:48:22, joone wrote: > > Other test cases like BrowserFocusTest.* also have the memory leak on my local > > machine so it's hard to debug this leak. > > Run the test binary with this command line flag to only run your test: > --gtest_filter=BrowserViewFocusTest.BrowserDialogModalTest I mean that it's hard to know if the memory leak is really fixed in the test case while debugging because most of test cases have the same memory leaks on my machine.
On 2015/09/24 17:25:42, msw wrote: > On 2015/09/24 01:55:54, joone wrote: > > Please try to run tryjobs after commenting out the body of the test code. > > If there are still memory leaks, there might be memory leaks in > > SelectFileDialogImplGtk. > > You can upload a patch set that you'd like to test and I'll trigger the bot... > msw@ could you trigger the linux_chromium_asan_rel_ng bot?
On 2015/09/24 18:07:30, joone wrote: > msw@ could you trigger the linux_chromium_asan_rel_ng bot? Done.
On 2015/09/24 18:12:30, msw wrote: > On 2015/09/24 18:07:30, joone wrote: > > msw@ could you trigger the linux_chromium_asan_rel_ng bot? > Done. There looks like memory leaks in GtkFileChooserDialog. The following memory leak fix was not applied to Gtk-2.24, but I'm not sure if this is root cause. Bug 554618 - GtkFileChooserDialog leaks memory(https://bugzilla.gnome.org/show_bug.cgi?id=554618)
Description was changed from ========== Make File-Picker modal on Linux Chromium for Linux opens a Gtk file-picker for file browsing, but it is not modal because there is no GtkWindow as parent of file-picker. This patch allows the X11 host window to disable input event handling to make a file-picker modal The original CL was reverted due to wrong DCHECK with modal_dialog_xid_: https://codereview.chromium.org/1243503002 BUG=408481 TEST=BrowserViewFocusTest.BrowserDialogModalTest ========== to ========== Make File-Picker modal on Linux Chromium for Linux opens a Gtk file-picker for file browsing, but it is not modal because there is no GtkWindow as parent of file-picker. This patch allows the X11 host window to disable input event handling to make a file-picker modal The original CL was reverted due to wrong DCHECK with modal_dialog_xid_: https://codereview.chromium.org/1243503002 BUG=408481 TEST=BrowserSelectFileDialogTest, ModalTest ==========
joone.hur@intel.com changed reviewers: + sky@chromium.org
sky@ could you take a look at the updated CL? It includes the code you suggested and reviewed in the below CL. https://codereview.chromium.org/1363093004/
Wow, talk about the bait and switch! Can you separate out the infrastructure needed for the test into its own patch. That is, the creation of the header you added here: https://codereview.chromium.org/1363093004/
On 2015/11/19 22:23:44, sky wrote: > Wow, talk about the bait and switch! > > Can you separate out the infrastructure needed for the test into its own patch. > That is, the creation of the header you added here: > https://codereview.chromium.org/1363093004/ The separate CL landed: https://crrev.com/3d1b89d8b07530fa86e102bb4f64b61527dceee4 sky@ can I commit this CL?
Elliot should be an owner of all these files. If he isn't, update the OWNERs file separately.
On 2015/11/20 15:58:26, sky wrote: > Elliot should be an owner of all these files. If he isn't, update the OWNERs > file separately. erg@ could you take a look the updated CL? The test case can directly open/close the file-picker using the test infrastructure that was already committed(https://crrev.com/3d1b89d8b07530fa86e102bb4f64b61527dceee4) and it was moved to libgtk2ui.
lgtm I patched this in locally; handles the click on chrome window when save dialog is up and the alt-tab to chrome when the save dialog is up cases.
The CQ bit was checked by joone.hur@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, sadrul@chromium.org, juncai@chromium.org Link to the patchset: https://codereview.chromium.org/1233913009/#ps340001 (title: "remove the test infrastructure and rebase on master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1233913009/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1233913009/340001
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/3bc35de4ff086027d50519c9fb40122f27eb82f4 Cr-Commit-Position: refs/heads/master@{#360961}
Message was sent while issue was closed.
A revert of this CL (patchset #18 id:340001) has been created in https://codereview.chromium.org/1594973009/ by sadrul@chromium.org. The reason for reverting is: Breaks input events in some cases (crbug.com/579408). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
