Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1363093004:
Add BrowserSelectFileDialogTest.OpenCloseFileDialog (Closed)
DescriptionAdd BrowserSelectFileDialogTest.OpenCloseFileDialog.
This test checks if file-picker opens and closes without any memory leaks.
However, GtkFileChooserDialog has memory leaks. Therefore, we annotate this
memory leaks so that LeakSanitizer can skip them.
BUG=537468
TEST=BrowserSelectFileDialogTest.OpenCloseFileDialog
Patch Set 1 #Patch Set 2 : try to deallocate memory #Patch Set 3 : fix build errors #Patch Set 4 : fix CrOS build error and disable the test case #Patch Set 5 : enable the test and skip libglib-2.0 from LSan report #
Total comments: 10
Patch Set 6 : remove CloseImpl() method and skip interactive_ui_tests from LSan report #Patch Set 7 : check if the focused window is the file-picker dialog #
Total comments: 8
Patch Set 8 : skip libgtk-x11-2.0 from LSan report #
Total comments: 4
Patch Set 9 : use ANNOTATE_LEAKING_OBJECT_PTR #Patch Set 10 : use ANNOTATE_SCOPED_MEMORY_LEAK #
Total comments: 14
Patch Set 11 : initialize the variables with 0, fix coding style issues. #
Total comments: 1
Patch Set 12 : move the comment into the body of the function. #
Total comments: 2
Patch Set 13 : directly create the SelectFileDialog #
Total comments: 1
Patch Set 14 : call gtk_widget_destroy() to close the file-picker #
Total comments: 1
Patch Set 15 : close all file-pickers #Patch Set 16 : expose the GtkWidget via a friend #Patch Set 17 : fix gn and gyp files for chromeos #
Total comments: 30
Patch Set 18 : Remove the page loading #Patch Set 19 : add comments #
Total comments: 6
Patch Set 20 : do not run the test-server #
Total comments: 2
Messages
Total messages: 73 (18 generated)
joone.hur@intel.com changed reviewers: + msw@chromium.org
msw@ I added a test case to see only if there are any memory leaks while opening and closing file-picker. Please trigger the linux_chromium_asan_rel_ng bot with the CL.
On 2015/09/24 23:01:53, joone wrote: > msw@ I added a test case to see only if there are any memory leaks while opening > and closing file-picker. Please trigger the linux_chromium_asan_rel_ng bot with > the CL. Done.
On 2015/09/24 23:11:04, msw wrote: > On 2015/09/24 23:01:53, joone wrote: > > msw@ I added a test case to see only if there are any memory leaks while > opening > > and closing file-picker. Please trigger the linux_chromium_asan_rel_ng bot > with > > the CL. > > 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
On 2015/09/30 01:26:27, joone wrote: > Bug report: https://code.google.com/p/chromium/issues/detail?id=537468 msw@ according to the above bug report, the test case fails due to the memory leak in GtkFileChooserDialog. So, how about disabling the test case and then enabling it once the memory leak is fixed in Gtk?
On 2015/10/15 21:53:36, joone wrote: > On 2015/09/30 01:26:27, joone wrote: > > Bug report: https://code.google.com/p/chromium/issues/detail?id=537468 > > msw@ according to the above bug report, the test case fails due to the memory > leak in GtkFileChooserDialog. > So, how about disabling the test case and then enabling it once the memory leak > is fixed in Gtk? Leave the test enabled and add a minimally-encompassing entry to: build/sanitizers/lsan_suppressions.cc
On 2015/10/15 22:20:58, msw wrote: > On 2015/10/15 21:53:36, joone wrote: > > On 2015/09/30 01:26:27, joone wrote: > > > Bug report: https://code.google.com/p/chromium/issues/detail?id=537468 > > > > msw@ according to the above bug report, the test case fails due to the memory > > leak in GtkFileChooserDialog. > > So, how about disabling the test case and then enabling it once the memory > leak > > is fixed in Gtk? > > Leave the test enabled and add a minimally-encompassing entry to: > build/sanitizers/lsan_suppressions.cc Done.
Do you want to land this separately from https://codereview.chromium.org/1233913009 ? If so, you should add some of those reviewers here. https://codereview.chromium.org/1363093004/diff/80001/build/sanitizers/lsan_s... File build/sanitizers/lsan_suppressions.cc (right): https://codereview.chromium.org/1363093004/diff/80001/build/sanitizers/lsan_s... build/sanitizers/lsan_suppressions.cc:58: "leak:libglib-2.0\n" I wonder if this is too wide of a suppression. Would this ignore any leaks of memory allocated by glib (even if the fault was in our ownership/usage)? You should ping someone familiar with lsan/glib. https://codereview.chromium.org/1363093004/diff/80001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1363093004/diff/80001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:297: // Get the focused window. Can we ensure this is the dialog window somehow? Otherwise, this function just sends the Esc key to whatever window has input focus, and that seems quite wrong. https://codereview.chromium.org/1363093004/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view_focus_uitest.cc (right): https://codereview.chromium.org/1363093004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view_focus_uitest.cc:34: // Spins a run loop until a Widget becomes inactive. nit: "Spins a run loop until a Widget's activation reaches the desired state." https://codereview.chromium.org/1363093004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view_focus_uitest.cc:166: nit: remove blank line https://codereview.chromium.org/1363093004/diff/80001/ui/shell_dialogs/select... File ui/shell_dialogs/select_file_dialog.h (right): https://codereview.chromium.org/1363093004/diff/80001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.h:198: virtual void CloseImpl() = 0; What's the point of splitting up Close and CloseImpl?
https://codereview.chromium.org/1363093004/diff/80001/build/sanitizers/lsan_s... File build/sanitizers/lsan_suppressions.cc (right): https://codereview.chromium.org/1363093004/diff/80001/build/sanitizers/lsan_s... build/sanitizers/lsan_suppressions.cc:58: "leak:libglib-2.0\n" On 2015/10/16 17:01:03, msw wrote: > I wonder if this is too wide of a suppression. Would this ignore any leaks of > memory allocated by glib (even if the fault was in our ownership/usage)? You > should ping someone familiar with lsan/glib. How about adding interactive_ui_tests to the suppression list? https://codereview.chromium.org/1363093004/diff/80001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1363093004/diff/80001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:297: // Get the focused window. On 2015/10/16 17:01:04, msw wrote: > Can we ensure this is the dialog window somehow? Otherwise, this function just > sends the Esc key to whatever window has input focus, and that seems quite > wrong. Yes, because the file-picker is modal. https://codereview.chromium.org/1363093004/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view_focus_uitest.cc (right): https://codereview.chromium.org/1363093004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view_focus_uitest.cc:34: // Spins a run loop until a Widget becomes inactive. On 2015/10/16 17:01:04, msw wrote: > nit: "Spins a run loop until a Widget's activation reaches the desired state." Done. https://codereview.chromium.org/1363093004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view_focus_uitest.cc:166: On 2015/10/16 17:01:04, msw wrote: > nit: remove blank line Done. https://codereview.chromium.org/1363093004/diff/80001/ui/shell_dialogs/select... File ui/shell_dialogs/select_file_dialog.h (right): https://codereview.chromium.org/1363093004/diff/80001/ui/shell_dialogs/select... ui/shell_dialogs/select_file_dialog.h:198: virtual void CloseImpl() = 0; On 2015/10/16 17:01:04, msw wrote: > What's the point of splitting up Close and CloseImpl? We don't need CloseImpl() so I removed it.
joone.hur@intel.com changed reviewers: + glider@chromium.org
glider@ could you take a look at the change in lsan_suppressions.cc if it makes
sense?
There are memory leaks in GtkFileChooserDialog as follows,
so we need to skip this memory leaks.
==5623==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 32 byte(s) in 1 object(s) allocated from:
#0 0x513e4b in __interceptor_malloc
(/tmp/run8sclYK/out/Release/interactive_ui_tests+0x513e4b)
#1 0x7f340b9bea38 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 0x513e4b in __interceptor_malloc
(/tmp/run8sclYK/out/Release/interactive_ui_tests+0x513e4b)
#1 0x7f340b9bea38 in g_malloc
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4da38)
joone.hur@intel.com changed reviewers: + sadrul@chromium.org
+CC more build/sanitizers/OWNERS for suppression advice. https://codereview.chromium.org/1363093004/diff/120001/build/sanitizers/lsan_... File build/sanitizers/lsan_suppressions.cc (right): https://codereview.chromium.org/1363093004/diff/120001/build/sanitizers/lsan_... build/sanitizers/lsan_suppressions.cc:58: "leak:interactive_ui_tests\n" This is worse! We definitely don't want to ignore any leaks in any of our interactive tests! Ideally we cold annotate the specific test fixture with a suppression for the leak in the most granular glib stack signature. https://codereview.chromium.org/1363093004/diff/120001/chrome/browser/ui/libg... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1363093004/diff/120001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:303: // Check if the focused window is the file-picker dailog. nit: 'dialog' https://codereview.chromium.org/1363093004/diff/120001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:305: for (std::set<GtkWidget*>::iterator it = dialogs_.begin(); nit: "auto it" https://codereview.chromium.org/1363093004/diff/120001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:314: LOG(ERROR) << "Cannot find the focused dialog"; nit: make this a "NOTREACHED() << ...;" to crash in debug
https://codereview.chromium.org/1363093004/diff/120001/build/sanitizers/lsan_... File build/sanitizers/lsan_suppressions.cc (right): https://codereview.chromium.org/1363093004/diff/120001/build/sanitizers/lsan_... build/sanitizers/lsan_suppressions.cc:58: "leak:interactive_ui_tests\n" On 2015/10/20 23:18:14, msw wrote: > This is worse! We definitely don't want to ignore any leaks in any of our > interactive tests! Ideally we cold annotate the specific test fixture with a > suppression for the leak in the most granular glib stack signature. Instead, I added libgtk-x11-2.0 to the suppression list, but it doesn't help to ignore the memory leaks. https://codereview.chromium.org/1363093004/diff/120001/chrome/browser/ui/libg... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1363093004/diff/120001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:303: // Check if the focused window is the file-picker dailog. On 2015/10/20 23:18:14, msw wrote: > nit: 'dialog' Done. https://codereview.chromium.org/1363093004/diff/120001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:305: for (std::set<GtkWidget*>::iterator it = dialogs_.begin(); On 2015/10/20 23:18:14, msw wrote: > nit: "auto it" Done. https://codereview.chromium.org/1363093004/diff/120001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:314: LOG(ERROR) << "Cannot find the focused dialog"; On 2015/10/20 23:18:14, msw wrote: > nit: make this a "NOTREACHED() << ...;" to crash in debug Done.
https://codereview.chromium.org/1363093004/diff/140001/build/sanitizers/lsan_... File build/sanitizers/lsan_suppressions.cc (right): https://codereview.chromium.org/1363093004/diff/140001/build/sanitizers/lsan_... build/sanitizers/lsan_suppressions.cc:58: "leak:libgtk-x11-2.0\n" Yeah, it should be 'glib2.0-2.32.4', according to the failure: Direct leak of 32 byte(s) in 1 object(s) allocated from: #0 0x515abb in __interceptor_malloc (/tmp/rundtqsbH/out/Release/interactive_ui_tests+0x515abb) #1 0x7f694e6d1a38 in g_malloc /build/buildd/glib2.0-2.32.4/./glib/gmem.c:159 Indirect leak of 25 byte(s) in 1 object(s) allocated from: #0 0x515abb in __interceptor_malloc (/tmp/rundtqsbH/out/Release/interactive_ui_tests+0x515abb) #1 0x7f694e6d1a38 in g_malloc /build/buildd/glib2.0-2.32.4/./glib/gmem.c:159 But that's a very wide suppression, which we should try to avoid. Try remove the suppression altogether and just using either ANNOTATE_LEAKING_OBJECT_PTR or ANNOTATE_SCOPED_MEMORY_LEAK within the test itself. If neither of those work, I'll approve the 'glib2.0-2.32.4' suppression. https://codereview.chromium.org/1363093004/diff/140001/chrome/browser/ui/libg... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1363093004/diff/140001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:306: it != dialogs_.end(); ++it) { nit: fits on line above now.
Patchset #9 (id:160001) has been deleted
Description was changed from ========== Add BrowserViewFocusTest.BrowserFileDialogTest. This test checks if file-picker opens and closes without any memory leaks. However, GtkFileChooserDialog has memory leaks so an encompassing entry was added to build/sanitizers/lsan_suppressions.cc to skip the memory leak error. BUG=537468 TEST=BrowserViewFocusTest.BrowserFileDialogTest ========== to ========== Add BrowserViewFocusTest.BrowserFileDialogTest. This test checks if file-picker opens and closes without any memory leaks. However, GtkFileChooserDialog has memory leaks so we annotate this memory leaks to skip the LSan report. BUG=537468 TEST=BrowserViewFocusTest.BrowserFileDialogTest ==========
Description was changed from ========== Add BrowserViewFocusTest.BrowserFileDialogTest. This test checks if file-picker opens and closes without any memory leaks. However, GtkFileChooserDialog has memory leaks so we annotate this memory leaks to skip the LSan report. BUG=537468 TEST=BrowserViewFocusTest.BrowserFileDialogTest ========== to ========== Add BrowserViewFocusTest.BrowserFileDialogTest. This test checks if file-picker opens and closes without any memory leaks. However, GtkFileChooserDialog has memory leaks so we annotate this memory leaks so that LeakSanitizer can skip them. BUG=537468 TEST=BrowserViewFocusTest.BrowserFileDialogTest ==========
Description was changed from ========== Add BrowserViewFocusTest.BrowserFileDialogTest. This test checks if file-picker opens and closes without any memory leaks. However, GtkFileChooserDialog has memory leaks so we annotate this memory leaks so that LeakSanitizer can skip them. BUG=537468 TEST=BrowserViewFocusTest.BrowserFileDialogTest ========== to ========== Add BrowserViewFocusTest.BrowserFileDialogTest. This test checks if file-picker opens and closes without any memory leaks. However, GtkFileChooserDialog has memory leaks. Therefore, we annotate this memory leaks so that LeakSanitizer can skip them. BUG=537468 TEST=BrowserViewFocusTest.BrowserFileDialogTest ==========
Description was changed from ========== Add BrowserViewFocusTest.BrowserFileDialogTest. This test checks if file-picker opens and closes without any memory leaks. However, GtkFileChooserDialog has memory leaks. Therefore, we annotate this memory leaks so that LeakSanitizer can skip them. BUG=537468 TEST=BrowserViewFocusTest.BrowserFileDialogTest ========== to ========== Add BrowserViewFocusTest.BrowserFileDialogTest. This test checks if file-picker opens and closes without any memory leaks. However, GtkFileChooserDialog has memory leaks. Therefore, we annotate this memory leaks so that LeakSanitizer can skip them. BUG=537468 TEST=BrowserViewFocusTest.BrowserFileDialogTest ==========
https://codereview.chromium.org/1363093004/diff/140001/build/sanitizers/lsan_... File build/sanitizers/lsan_suppressions.cc (right): https://codereview.chromium.org/1363093004/diff/140001/build/sanitizers/lsan_... build/sanitizers/lsan_suppressions.cc:58: "leak:libgtk-x11-2.0\n" On 2015/10/22 22:23:05, msw wrote: > Yeah, it should be 'glib2.0-2.32.4', according to the failure: > > Direct leak of 32 byte(s) in 1 object(s) allocated from: > #0 0x515abb in __interceptor_malloc > (/tmp/rundtqsbH/out/Release/interactive_ui_tests+0x515abb) > #1 0x7f694e6d1a38 in g_malloc /build/buildd/glib2.0-2.32.4/./glib/gmem.c:159 > > Indirect leak of 25 byte(s) in 1 object(s) allocated from: > #0 0x515abb in __interceptor_malloc > (/tmp/rundtqsbH/out/Release/interactive_ui_tests+0x515abb) > #1 0x7f694e6d1a38 in g_malloc /build/buildd/glib2.0-2.32.4/./glib/gmem.c:159 > > But that's a very wide suppression, which we should try to avoid. Try remove the > suppression altogether and just using either ANNOTATE_LEAKING_OBJECT_PTR or > ANNOTATE_SCOPED_MEMORY_LEAK within the test itself. If neither of those work, > I'll approve the 'glib2.0-2.32.4' suppression. ANNOTATE_SCOPED_MEMORY_LEAK works! https://codereview.chromium.org/1363093004/diff/140001/chrome/browser/ui/libg... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1363093004/diff/140001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:306: it != dialogs_.end(); ++it) { On 2015/10/22 22:23:05, msw wrote: > nit: fits on line above now. Done.
On 2015/10/23 10:51:42, joone wrote: > https://codereview.chromium.org/1363093004/diff/140001/build/sanitizers/lsan_... > File build/sanitizers/lsan_suppressions.cc (right): > > https://codereview.chromium.org/1363093004/diff/140001/build/sanitizers/lsan_... > build/sanitizers/lsan_suppressions.cc:58: "leak:libgtk-x11-2.0\n" > On 2015/10/22 22:23:05, msw wrote: > > Yeah, it should be 'glib2.0-2.32.4', according to the failure: > > > > Direct leak of 32 byte(s) in 1 object(s) allocated from: > > #0 0x515abb in __interceptor_malloc > > (/tmp/rundtqsbH/out/Release/interactive_ui_tests+0x515abb) > > #1 0x7f694e6d1a38 in g_malloc > /build/buildd/glib2.0-2.32.4/./glib/gmem.c:159 > > > > Indirect leak of 25 byte(s) in 1 object(s) allocated from: > > #0 0x515abb in __interceptor_malloc > > (/tmp/rundtqsbH/out/Release/interactive_ui_tests+0x515abb) > > #1 0x7f694e6d1a38 in g_malloc > /build/buildd/glib2.0-2.32.4/./glib/gmem.c:159 > > > > But that's a very wide suppression, which we should try to avoid. Try remove > the > > suppression altogether and just using either ANNOTATE_LEAKING_OBJECT_PTR or > > ANNOTATE_SCOPED_MEMORY_LEAK within the test itself. If neither of those work, > > I'll approve the 'glib2.0-2.32.4' suppression. > > ANNOTATE_SCOPED_MEMORY_LEAK works! > > https://codereview.chromium.org/1363093004/diff/140001/chrome/browser/ui/libg... > File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): > > https://codereview.chromium.org/1363093004/diff/140001/chrome/browser/ui/libg... > chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:306: it != > dialogs_.end(); ++it) { > On 2015/10/22 22:23:05, msw wrote: > > nit: fits on line above now. > > Done. Thanks for using ANNOTATE_SCOPED_MEMORY_LEAK. This is the best solution in the case we don't have frame pointers for the library. Sorry for not responding earlier.
Glad that worked; lgtm with nits. https://codereview.chromium.org/1363093004/diff/200001/chrome/browser/ui/libg... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1363093004/diff/200001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:61: // Close the file dialog. nit: add a blank line above. https://codereview.chromium.org/1363093004/diff/200001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:296: // Close the file-picker by sending an ESC key event to it. nit: move this comment to the function declaration or inside the body. https://codereview.chromium.org/1363093004/diff/200001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:299: XID focused_window; nit: init to 0 (or None from usr/include/X11/X.h) https://codereview.chromium.org/1363093004/diff/200001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:300: int revert; nit: init to RevertToParent or 0? (maybe nix this and pass nullptr?) https://codereview.chromium.org/1363093004/diff/200001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:317: // Send an ESC key event to the focused_window. nit: bars around |focused_window| or use the generic "focused dialog". https://codereview.chromium.org/1363093004/diff/200001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:318: XKeyEvent event; nit: init with {0} and drop the following memset. https://codereview.chromium.org/1363093004/diff/200001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:326: reinterpret_cast<XEvent*>(&event)); nit: indent to match "gfx::" inside the open parens. Use "git cl format"
Thanks for the reviews. https://codereview.chromium.org/1363093004/diff/200001/chrome/browser/ui/libg... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1363093004/diff/200001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:61: // Close the file dialog. On 2015/10/23 19:01:19, msw wrote: > nit: add a blank line above. Done. https://codereview.chromium.org/1363093004/diff/200001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:296: // Close the file-picker by sending an ESC key event to it. On 2015/10/23 19:01:19, msw wrote: > nit: move this comment to the function declaration or inside the body. Done. https://codereview.chromium.org/1363093004/diff/200001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:299: XID focused_window; On 2015/10/23 19:01:19, msw wrote: > nit: init to 0 (or None from usr/include/X11/X.h) Done. https://codereview.chromium.org/1363093004/diff/200001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:300: int revert; On 2015/10/23 19:01:19, msw wrote: > nit: init to RevertToParent or 0? (maybe nix this and pass nullptr?) When I pass nullptr, it crashes. https://codereview.chromium.org/1363093004/diff/200001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:317: // Send an ESC key event to the focused_window. On 2015/10/23 19:01:19, msw wrote: > nit: bars around |focused_window| or use the generic "focused dialog". Done. https://codereview.chromium.org/1363093004/diff/200001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:318: XKeyEvent event; On 2015/10/23 19:01:19, msw wrote: > nit: init with {0} and drop the following memset. Done. https://codereview.chromium.org/1363093004/diff/200001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:326: reinterpret_cast<XEvent*>(&event)); On 2015/10/23 19:01:19, msw wrote: > nit: indent to match "gfx::" inside the open parens. Use "git cl format" Done.
lgtm with a minor comment nit https://codereview.chromium.org/1363093004/diff/220001/ui/shell_dialogs/selec... File ui/shell_dialogs/select_file_dialog.h (right): https://codereview.chromium.org/1363093004/diff/220001/ui/shell_dialogs/selec... ui/shell_dialogs/select_file_dialog.h:174: // Close the file-picker by sending an ESC key event to it. nit: technically the ESC key technique is specific to the GTK2 impl, so I'd remove that part of this comment (or move it into the body of the gtk2 impl).
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 Link to the patchset: https://codereview.chromium.org/1363093004/#ps240001 (title: "move the comment into the body of the function.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1363093004/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1363093004/240001
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/10/24 02:51:04, 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...) sadrul@ I need your LGTM.
joone.hur@intel.com changed reviewers: + sky@chromium.org
sky@ could you review this CL?
https://codereview.chromium.org/1363093004/diff/240001/chrome/browser/ui/brow... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/1363093004/diff/240001/chrome/browser/ui/brow... chrome/browser/ui/browser.h:400: ui::SelectFileDialog* OpenFile(); Document ownership. That said, AFAICT all you want to test is the open dialog, and only for gtk. Why don't you have your test directly create the selectfiledialog and do the dance you need to do to close it? That way no changes to browser and the like.
https://codereview.chromium.org/1363093004/diff/240001/chrome/browser/ui/brow... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/1363093004/diff/240001/chrome/browser/ui/brow... chrome/browser/ui/browser.h:400: ui::SelectFileDialog* OpenFile(); On 2015/10/30 15:31:17, sky wrote: > Document ownership. > That said, AFAICT all you want to test is the open dialog, and only for gtk. Why > don't you have your test directly create the selectfiledialog and do the dance > you need to do to close it? That way no changes to browser and the like. Because this CL was separated from https://codereview.chromium.org/1233913009/ (Make file-picker modal on Linux) This method was used in the CL and I was asked to add support for the file-picker modal test for Windows ad CrOS. https://codereview.chromium.org/1233913009/#msg35
That motivates writing the test for all platforms, but why do you need to change browser? Why can't you directly test selectfiledialog? On Fri, Oct 30, 2015 at 8:59 AM, <joone.hur@intel.com> wrote: > > https://codereview.chromium.org/1363093004/diff/240001/chrome/browser/ui/brow... > File chrome/browser/ui/browser.h (right): > > https://codereview.chromium.org/1363093004/diff/240001/chrome/browser/ui/brow... > chrome/browser/ui/browser.h:400: ui::SelectFileDialog* OpenFile(); > On 2015/10/30 15:31:17, sky wrote: >> >> Document ownership. >> That said, AFAICT all you want to test is the open dialog, and only > > for gtk. Why >> >> don't you have your test directly create the selectfiledialog and do > > the dance >> >> you need to do to close it? That way no changes to browser and the > > like. > > Because this CL was separated from > https://codereview.chromium.org/1233913009/ (Make file-picker modal on > Linux) > > This method was used in the CL and I was asked to add support for the > file-picker modal test for Windows ad CrOS. > https://codereview.chromium.org/1233913009/#msg35 > > https://codereview.chromium.org/1363093004/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #13 (id:260001) has been deleted
On 2015/10/30 16:59:16, sky wrote: > That motivates writing the test for all platforms, but why do you need > to change browser? Why can't you directly test selectfiledialog? > sky@ The updated CL allows the test case to directly create the SelectFileDialog. Thanks for the review.
https://codereview.chromium.org/1363093004/diff/280001/chrome/browser/ui/libg... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1363093004/diff/280001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:319: // Send an ESC key event to the focused dialog. Whoa .. not lgtm You should close the dialog using the gtk+ api (e.g. gtk_widget_destroy())
On 2015/10/31 16:36:17, sadrul wrote: > https://codereview.chromium.org/1363093004/diff/280001/chrome/browser/ui/libg... > File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): > > https://codereview.chromium.org/1363093004/diff/280001/chrome/browser/ui/libg... > chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:319: // Send an ESC > key event to the focused dialog. > Whoa .. not lgtm > > You should close the dialog using the gtk+ api (e.g. gtk_widget_destroy()) https://codereview.chromium.org/1233913009/#msg60 Because of the memory leaks, I tried to follow the user behavior by sending an ESC in order to close the file-picker. I thought that programmatic way didn't help to fix this memory leaks.
sadrul@ could you take a look at the updated CL?
https://codereview.chromium.org/1363093004/diff/300001/chrome/browser/ui/libg... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1363093004/diff/300001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:311: } This seems unnecessarily complex (e.g. there's no reason the file-dialog has to be focused when Close() is called). Why can't this just close all of dialogs_? (I am not even convinced we still need to keep a set of dialogs_ here; I think we can do away with tracking just one GtkWidget* for the dialog).
Also, if this is only for a test there is no reason to put the code in SelectFileDialogImplGTK. Keep it in the test. On Sun, Nov 1, 2015 at 5:31 PM, <sadrul@chromium.org> wrote: > > https://codereview.chromium.org/1363093004/diff/300001/chrome/browser/ui/libg... > File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc > (right): > > https://codereview.chromium.org/1363093004/diff/300001/chrome/browser/ui/libg... > chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:311: } > This seems unnecessarily complex (e.g. there's no reason the file-dialog > has to be focused when Close() is called). Why can't this just close all > of dialogs_? (I am not even convinced we still need to keep a set of > dialogs_ here; I think we can do away with tracking just one GtkWidget* > for the dialog). > > https://codereview.chromium.org/1363093004/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/11/02 16:36:32, sky wrote: > Also, if this is only for a test there is no reason to put the code in > SelectFileDialogImplGTK. Keep it in the test. > sky@ do you know any way to close the file-picker in the test?
On 2015/11/03 03:46:45, joone wrote: > On 2015/11/02 16:36:32, sky wrote: > > Also, if this is only for a test there is no reason to put the code in > > SelectFileDialogImplGTK. Keep it in the test. > > > > sky@ do you know any way to close the file-picker in the test? I'm not familiar enough to say for sure. My concern was that you shouldn't add a close function to the public class purely for a test. Keep it in the test.
On 2015/11/03 17:09:15, sky wrote: > On 2015/11/03 03:46:45, joone wrote: > > On 2015/11/02 16:36:32, sky wrote: > > > Also, if this is only for a test there is no reason to put the code in > > > SelectFileDialogImplGTK. Keep it in the test. > > > > > > > sky@ do you know any way to close the file-picker in the test? > > I'm not familiar enough to say for sure. My concern was that you shouldn't add a > close function to the public class purely for a test. Keep it in the test. However, there seems no way to close the file-picker in the test.
Expose the GtkWidget via a friend. On Tue, Nov 3, 2015 at 1:31 PM, <joone.hur@intel.com> wrote: > On 2015/11/03 17:09:15, sky wrote: >> >> On 2015/11/03 03:46:45, joone wrote: >> > On 2015/11/02 16:36:32, sky wrote: >> > > Also, if this is only for a test there is no reason to put the code in >> > > SelectFileDialogImplGTK. Keep it in the test. >> > > >> > >> > sky@ do you know any way to close the file-picker in the test? > > >> I'm not familiar enough to say for sure. My concern was that you shouldn't >> add > > a >> >> close function to the public class purely for a test. Keep it in the test. > > > However, there seems no way to close the file-picker in the test. > > https://codereview.chromium.org/1363093004/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #16 (id:340001) has been deleted
Description was changed from ========== Add BrowserViewFocusTest.BrowserFileDialogTest. This test checks if file-picker opens and closes without any memory leaks. However, GtkFileChooserDialog has memory leaks. Therefore, we annotate this memory leaks so that LeakSanitizer can skip them. BUG=537468 TEST=BrowserViewFocusTest.BrowserFileDialogTest ========== to ========== Add BrowserSelectFileDialogTest.OpenCloseFileDialog. This test checks if file-picker opens and closes without any memory leaks. However, GtkFileChooserDialog has memory leaks. Therefore, we annotate this memory leaks so that LeakSanitizer can skip them. BUG=537468 TEST=BrowserSelectFileDialogTest.OpenCloseFileDialog ==========
On 2015/11/03 23:35:10, sky wrote: > Expose the GtkWidget via a friend. > sky@ There were some changes in the updated CL: * SelectFileDialogImplGTK class declaration was separated from select_file_dialog_impl_gtk2.cc. * GtkWidget was exposed to the test case via a friend so the close method was removed from SelectFileDialogImplGTK * The test case was moved to chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc
https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.h (right): https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. no (c) and 2015 https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.h:19: friend class FilePicker; move friend declaration into private section and indent one more space. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.h:21: explicit SelectFileDialogImplGTK(Listener* listener, no explicit. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc (right): https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:9: #include "chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.h" This should be the first include. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:23: class BrowserSelectFileDialogTest : public InProcessBrowserTest { Use a using statement. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:29: explicit WidgetActivationWaiter(views::Widget* widget, bool active) no explicit https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:40: void OnWidgetActivationChanged(views::Widget* widget, bool active) override { prefix this with // views::WidgetObserver: and move to private section. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:60: class FilePicker: public ui::SelectFileDialog::Listener { 'FilePicker:' => 'FilePicker :' https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:60: class FilePicker: public ui::SelectFileDialog::Listener { Add description. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:64: this, new ChromeSelectFilePolicy(NULL)); nullptr. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:89: while (file_dialog->dialogs_.begin() != file_dialog->dialogs_.end()) { nit: no {} https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:89: while (file_dialog->dialogs_.begin() != file_dialog->dialogs_.end()) { !dialogs_.empty(). https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:107: IN_PROC_BROWSER_TEST_F(BrowserSelectFileDialogTest, OpenCloseFileDialog) { Document what that test is asserting. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:112: ui_test_utils::NavigateToURL(browser(), url); Why do you need to navigate? https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:123: // Run a nested loop until the browser window becomes inactive. Document why this is done.
sky@ Thanks for the review. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.h (right): https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2015/11/06 21:04:09, sky wrote: > no (c) and 2015 Done. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.h:19: friend class FilePicker; On 2015/11/06 21:04:09, sky wrote: > move friend declaration into private section and indent one more space. Done. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.h:21: explicit SelectFileDialogImplGTK(Listener* listener, On 2015/11/06 21:04:09, sky wrote: > no explicit. Done. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc (right): https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:9: #include "chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.h" On 2015/11/06 21:04:10, sky wrote: > This should be the first include. It causes a #include order problem. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:23: class BrowserSelectFileDialogTest : public InProcessBrowserTest { On 2015/11/06 21:04:10, sky wrote: > Use a using statement. Done. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:29: explicit WidgetActivationWaiter(views::Widget* widget, bool active) On 2015/11/06 21:04:10, sky wrote: > no explicit Done. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:40: void OnWidgetActivationChanged(views::Widget* widget, bool active) override { On 2015/11/06 21:04:10, sky wrote: > prefix this with > // views::WidgetObserver: > and move to private section. Done. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:60: class FilePicker: public ui::SelectFileDialog::Listener { On 2015/11/06 21:04:09, sky wrote: > Add description. Done. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:60: class FilePicker: public ui::SelectFileDialog::Listener { On 2015/11/06 21:04:09, sky wrote: > Add description. Done. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:64: this, new ChromeSelectFilePolicy(NULL)); On 2015/11/06 21:04:09, sky wrote: > nullptr. Done. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:89: while (file_dialog->dialogs_.begin() != file_dialog->dialogs_.end()) { On 2015/11/06 21:04:09, sky wrote: > nit: no {} Done. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:89: while (file_dialog->dialogs_.begin() != file_dialog->dialogs_.end()) { On 2015/11/06 21:04:10, sky wrote: > !dialogs_.empty(). Done. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:107: IN_PROC_BROWSER_TEST_F(BrowserSelectFileDialogTest, OpenCloseFileDialog) { On 2015/11/06 21:04:10, sky wrote: > Document what that test is asserting. Done. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:112: ui_test_utils::NavigateToURL(browser(), url); On 2015/11/06 21:04:10, sky wrote: > Why do you need to navigate? Without the page loading, the test works fine so I removed it. https://codereview.chromium.org/1363093004/diff/380001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:123: // Run a nested loop until the browser window becomes inactive. On 2015/11/06 21:04:10, sky wrote: > Document why this is done. Done.
sky@ could you review the latest CL?
Sadrul explicitly said not lgtm earlier in the thread. You need to get him happy.
On 2015/11/13 16:06:11, sky wrote: > Sadrul explicitly said not lgtm earlier in the thread. You need to get him > happy. sadrul@ According to your comment, gtk_widget_destroy() was used to close the file-picker in the updated CL. Please take a look at the CL.
lgtm https://codereview.chromium.org/1363093004/diff/420001/chrome/browser/ui/libg... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc (right): https://codereview.chromium.org/1363093004/diff/420001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:5: - extra new line https://codereview.chromium.org/1363093004/diff/420001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:79: if (select_file_dialog_.get()) When can |select_file_dialog_| be null? https://codereview.chromium.org/1363093004/diff/420001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:110: ASSERT_TRUE(embedded_test_server()->Start()); You don't need to start the test-server, right?
sadrul@ thanks for the review! https://codereview.chromium.org/1363093004/diff/420001/chrome/browser/ui/libg... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc (right): https://codereview.chromium.org/1363093004/diff/420001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:5: On 2015/11/17 02:10:15, sadrul wrote: > - extra new line Done. https://codereview.chromium.org/1363093004/diff/420001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:79: if (select_file_dialog_.get()) On 2015/11/17 02:10:15, sadrul wrote: > When can |select_file_dialog_| be null? There is no case where |select_file_dialog_| is null, so this condition will be removed. https://codereview.chromium.org/1363093004/diff/420001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:110: ASSERT_TRUE(embedded_test_server()->Start()); On 2015/11/17 02:10:15, sadrul wrote: > You don't need to start the test-server, right? Right, we don't need to run it because the page loading was removed.
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 Link to the patchset: https://codereview.chromium.org/1363093004/#ps440001 (title: "do not run the test-server")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1363093004/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1363093004/440001
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: sky@chromium.org. Please make sure to get positive review before another CQ attempt.
On 2015/11/18 00:53:36, commit-bot: I haz the power wrote: > A disapproval has been posted by following reviewers: mailto:sky@chromium.org. > Please make sure to get positive review before another CQ attempt. sky@ This CL needs your LGTM.
Can you elaborate on the point of this test? AFAICT it is to verify gtk itself doesn't have any leaks. Why should Chrome be verifying gtk doesn't have leaks? https://codereview.chromium.org/1363093004/diff/440001/chrome/browser/ui/libg... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc (right): https://codereview.chromium.org/1363093004/diff/440001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. This file name should end in either interactive_uitest. https://codereview.chromium.org/1363093004/diff/440001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:9: #include "chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.h" This should be your first include.
On 2015/11/18 16:17:09, sky wrote: > Can you elaborate on the point of this test? AFAICT it is to verify gtk itself > doesn't have any leaks. Why should Chrome be verifying gtk doesn't have leaks? > I have been working on https://codereview.chromium.org/1233913009/ This CL has not landed due to the memory leak so I created this test case to figure out what causes this memory leak by just opening and closing the file-picker. Currently, gtk has the memory leak so this CL lets ASAN know the memory leak by setting ANNOTATE_SCOPED_MEMORY_LEAK as follows: // Leaks in GtkFileChooserDialog. http://crbug.com/537468 ANNOTATE_SCOPED_MEMORY_LEAK; libgtk2ui::FilePicker file_picker(browser()->window()); > > https://codereview.chromium.org/1363093004/diff/440001/chrome/browser/ui/libg... > chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:9: #include > "chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.h" > This should be your first include. It causes a #include order problem. If we can ignore this style check error, this head file could be the first include.
Please clarify, is the leak in gtk or chrome? On Wed, Nov 18, 2015 at 9:41 AM, <joone.hur@intel.com> wrote: > On 2015/11/18 16:17:09, sky wrote: >> >> Can you elaborate on the point of this test? AFAICT it is to verify gtk >> itself >> doesn't have any leaks. Why should Chrome be verifying gtk doesn't have >> leaks? > > > > I have been working on https://codereview.chromium.org/1233913009/ > This CL has not landed due to the memory leak so I created this test case to > figure out what causes this memory leak > by just opening and closing the file-picker. Currently, gtk has the memory > leak > so this CL lets ASAN know the memory leak by setting > ANNOTATE_SCOPED_MEMORY_LEAK > as follows: > > // Leaks in GtkFileChooserDialog. http://crbug.com/537468 > ANNOTATE_SCOPED_MEMORY_LEAK; > libgtk2ui::FilePicker file_picker(browser()->window()); > > > > https://codereview.chromium.org/1363093004/diff/440001/chrome/browser/ui/libg... >> >> chrome/browser/ui/libgtk2ui/select_file_dialog_impl_test.cc:9: #include >> "chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.h" >> This should be your first include. > > > It causes a #include order problem. If we can ignore this style check error, > this head file could be the first include. https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > https://codereview.chromium.org/1363093004/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/11/18 20:43:19, sky wrote: > Please clarify, is the leak in gtk or chrome? > There is the leak in gtk.
Writing tests for gtk is outside the scope of the Chromium project. The bug you pointed to has also been closed with this. NOT LGTM On Wed, Nov 18, 2015 at 3:17 PM, <joone.hur@intel.com> wrote: > On 2015/11/18 20:43:19, sky wrote: >> >> Please clarify, is the leak in gtk or chrome? > > > > There is the leak in gtk. > > > https://codereview.chromium.org/1363093004/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/11/19 00:40:44, sky wrote: > Writing tests for gtk is outside the scope of the Chromium project. > The bug you pointed to has also been closed with this. > > NOT LGTM > I think that this test case has two purposes: 1) Testing the file-picker of libgtk2ui to see if it works fine. 2) Aware of the gtk+ memory leaks in file-picker. Without this test case, someone may suffer from the same memory leak.
On Wed, Nov 18, 2015 at 5:58 PM, <joone.hur@intel.com> wrote: > On 2015/11/19 00:40:44, sky wrote: >> >> Writing tests for gtk is outside the scope of the Chromium project. >> The bug you pointed to has also been closed with this. > > >> NOT LGTM > > > > I think that this test case has two purposes: > 1) Testing the file-picker of libgtk2ui to see if it works fine. This test exercises showing and closing the file picker. That isn't a particularly compelling test. > 2) Aware of the gtk+ memory leaks in file-picker. Without this test case, > someone may suffer from the same memory leak. See earlier comments. -Scott > > > > > https://codereview.chromium.org/1363093004/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/11/19 17:27:01, sky wrote: > On Wed, Nov 18, 2015 at 5:58 PM, <mailto:joone.hur@intel.com> wrote: > > On 2015/11/19 00:40:44, sky wrote: > >> > >> Writing tests for gtk is outside the scope of the Chromium project. > >> The bug you pointed to has also been closed with this. > > > > > >> NOT LGTM > > > > > > > > I think that this test case has two purposes: > > 1) Testing the file-picker of libgtk2ui to see if it works fine. > > This test exercises showing and closing the file picker. That isn't a > particularly compelling test. > > > 2) Aware of the gtk+ memory leaks in file-picker. Without this test case, > > someone may suffer from the same memory leak. > > See earlier comments. > > -Scott > I see. Due to the reviews, the test case was really improved so I will apply this code to https://codereview.chromium.org/1233913009/ sky@ msw@ sadrul@ thanks for the reviews. |
