Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(252)

Issue 1624793002: Make File-Picker modal on Linux (Closed)

Created:
1 year, 10 months ago by joone
Modified:
1 year, 2 months ago
Reviewers:
sadrul, Elliot Glaysher
CC:
chromium-reviews, tfarina, tdanderson+views_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Make File-Picker modal on Linux Chromium for Linux uses GtkFileChooserDialog for opening a file-picker, but it is not modal to the X11 host window because GtkFileChooserDialog can be modal only to the parent GtkWindow. This patch allows the X11 host window to disable input event handling to make a file-picker modal. Design doc and discussion: https://docs.google.com/document/d/12CfKVTpaonxxM3sNksq6vY6qb0J2qR3b7h_bLxzYanE This CL was reverted due to the UI freezing problem that happens when the users open a file-picker from a child window of the X11 host window: https://codereview.chromium.org/1594973009 BUG=408481, 579408 TEST=BrowserSelectFileDialogTest.ModalTest Committed: https://crrev.com/df7f78e35f95dfae098dd6197b012e0506f68e0f Cr-Commit-Position: refs/heads/master@{#422482}

Patch Set 1 : Reverted CL #

Total comments: 1

Patch Set 2 : Fix the freezing problem #

Total comments: 2

Patch Set 3 : [WIP] use of scoped_ptr<ScopedHandle> #

Total comments: 2

Patch Set 4 : Use of g_object_set_data_full #

Patch Set 5 : Fix a link error in component builds #

Total comments: 6

Patch Set 6 : Rebase and make EnableEventListening a private method #

Patch Set 7 : Can close the host window while opening the file-picker #

Total comments: 5

Patch Set 8 : fix typo #

Patch Set 9 : Update comment #

Patch Set 10 : Adapt to https://codereview.chromium.org/2165083002 #

Patch Set 11 : Add a WeakPtrFactory #

Total comments: 4

Patch Set 12 : Remove ScopedHandle #

Unified diffs Side-by-side diffs Delta from patch set Stats (+109 lines, -4 lines) Patch
M chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +18 lines, -2 lines 0 comments Download
M chrome/browser/ui/libgtk2ui/select_file_dialog_interactive_uitest.cc View 1 2 1 chunk +44 lines, -0 lines 0 comments Download
M ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +16 lines, -0 lines 0 comments Download
M ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +31 lines, -2 lines 0 comments Download
Trybot results:  win_chromium_compile_dbg_ng   win_clang   win_chromium_x64_rel_ng   win_chromium_rel_ng   mac_chromium_compile_dbg_ng   ios-device   ios-simulator   mac_chromium_rel_ng   linux_chromium_chromeos_compile_dbg_ng   linux_chromium_asan_rel_ng   chromeos_x86-generic_chromium_compile_only_ng   linux_chromium_rel_ng   linux_chromium_compile_dbg_ng   chromium_presubmit   cast_shell_linux   chromeos_daisy_chromium_compile_only_ng   chromeos_amd64-generic_chromium_compile_only_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_clobber_rel_ng   linux_android_rel_ng   android_n5x_swarming_rel   android_clang_dbg_recipe   android_arm64_dbg_recipe   android_compile_dbg   cast_shell_android   chromium_presubmit   chromium_presubmit   mac_chromium_compile_dbg_ng   ios-simulator   ios-device   mac_chromium_rel_ng   win_clang   win_chromium_compile_dbg_ng   win_chromium_x64_rel_ng   win_chromium_rel_ng   linux_android_rel_ng   android_n5x_swarming_rel   android_clang_dbg_recipe   android_arm64_dbg_recipe   android_compile_dbg   cast_shell_android   linux_chromium_asan_rel_ng   linux_chromium_chromeos_compile_dbg_ng   linux_chromium_rel_ng   chromeos_x86-generic_chromium_compile_only_ng   chromium_presubmit   linux_chromium_compile_dbg_ng   chromeos_amd64-generic_chromium_compile_only_ng   cast_shell_linux   chromeos_daisy_chromium_compile_only_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_clobber_rel_ng 

Messages

Total messages: 66 (32 generated)
joone
Hi sadrul@, erg@ I'm trying to fix the freezing problem, which happened due to my ...
1 year, 10 months ago (2016-01-23 02:52:57 UTC) #2
sadrul
It'd be better like this: . Introduce 'scoped_ptr<ScopedHandle> DesktopWindowTreeHostX11::DisableEvents(XID dialog);', where ScopedHandle looks like this: ...
1 year, 10 months ago (2016-01-26 17:47:03 UTC) #4
joone
g_object_set_data can't keep the scoped-handle so the destructor is called when SelectFileDialogImplGTK::SelectFileImpl() returns.
1 year, 9 months ago (2016-02-24 01:46:53 UTC) #5
joone
sadral@ Please see the issue: https://codereview.chromium.org/1624793002/diff/40001/chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1624793002/diff/40001/chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc#newcode176 chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:176: g_object_set_data(G_OBJECT(dialog), "scope_handle", scope_handle.get()); g_object_set_data ...
1 year, 9 months ago (2016-03-02 23:40:28 UTC) #6
joone
On 2016/03/02 23:40:28, joone wrote: > sadral@ Please see the issue: > > https://codereview.chromium.org/1624793002/diff/40001/chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc > ...
1 year, 8 months ago (2016-03-31 23:04:44 UTC) #7
sadrul
On 2016/02/24 01:46:53, joone wrote: > g_object_set_data can't keep the scoped-handle so the destructor is ...
1 year, 8 months ago (2016-04-13 16:28:36 UTC) #8
joone
On 2016/04/13 16:28:36, sadrul wrote: > On 2016/02/24 01:46:53, joone wrote: > > g_object_set_data can't ...
1 year, 8 months ago (2016-04-14 23:03:21 UTC) #9
sadrul
https://codereview.chromium.org/1624793002/diff/40001/chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1624793002/diff/40001/chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc#newcode176 chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:176: g_object_set_data(G_OBJECT(dialog), "scope_handle", scope_handle.get()); On 2016/03/02 23:40:28, joone wrote: > ...
1 year, 8 months ago (2016-04-15 09:26:15 UTC) #10
joone
On 2016/04/15 09:26:15, sadrul wrote: > https://codereview.chromium.org/1624793002/diff/40001/chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc > File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): > > https://codereview.chromium.org/1624793002/diff/40001/chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc#newcode176 > ...
1 year, 7 months ago (2016-04-18 20:54:08 UTC) #12
sadrul
https://codereview.chromium.org/1624793002/diff/100001/chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1624793002/diff/100001/chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc#newcode179 chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:179: owning_window->GetHost()->GetAcceleratedWidget()); owning_window can be null, right? (see code/comment above) ...
1 year, 7 months ago (2016-04-19 14:56:12 UTC) #13
joone
Other changes you asked was applied to the latest CL. https://codereview.chromium.org/1624793002/diff/100001/chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1624793002/diff/100001/chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc#newcode179 ...
1 year, 7 months ago (2016-04-19 22:29:56 UTC) #14
joone
sadrul@ can you take a look at the following change? https://codereview.chromium.org/1624793002/diff/160001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/1624793002/diff/160001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc#newcode363 ...
1 year, 7 months ago (2016-04-21 20:45:45 UTC) #16
joone
On 2016/04/21 20:45:45, joone wrote: > sadrul@ can you take a look at the following ...
1 year, 7 months ago (2016-04-28 19:46:52 UTC) #17
sadrul
https://codereview.chromium.org/1624793002/diff/100001/chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1624793002/diff/100001/chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc#newcode179 chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:179: owning_window->GetHost()->GetAcceleratedWidget()); On 2016/04/19 22:29:56, joone wrote: > On 2016/04/19 ...
1 year, 7 months ago (2016-05-11 05:27:47 UTC) #18
joone
https://codereview.chromium.org/1624793002/diff/100001/chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1624793002/diff/100001/chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc#newcode179 chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:179: owning_window->GetHost()->GetAcceleratedWidget()); On 2016/05/11 05:27:47, sadrul wrote: > On 2016/04/19 ...
1 year, 7 months ago (2016-05-11 19:18:05 UTC) #19
sadrul
https://codereview.chromium.org/1624793002/diff/160001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/1624793002/diff/160001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc#newcode363 ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:363: scoped_handle_->CancelCallback(); On 2016/05/11 19:18:05, joone wrote: > On 2016/05/11 ...
1 year, 7 months ago (2016-05-11 19:20:36 UTC) #20
joone
> > Then, the instance > > of the host window is still alive after ...
1 year, 7 months ago (2016-05-11 21:13:18 UTC) #21
joone
Hi, sadrul@ I draw up a document to explain my CL: https://docs.google.com/document/d/12CfKVTpaonxxM3sNksq6vY6qb0J2qR3b7h_bLxzYanE/edit?usp=sharing It is helpful ...
1 year, 4 months ago (2016-07-22 20:40:03 UTC) #25
joone
sadrul@ ping?
1 year, 4 months ago (2016-08-10 17:19:30 UTC) #30
sadrul
On 2016/08/10 17:19:30, joone wrote: > sadrul@ ping? I have responded on the doc.
1 year, 4 months ago (2016-08-11 05:17:51 UTC) #31
joone
On 2016/08/11 05:17:51, sadrul wrote: > On 2016/08/10 17:19:30, joone wrote: > > sadrul@ ping? ...
1 year, 3 months ago (2016-08-31 22:28:26 UTC) #34
sadrul
On 2016/08/31 22:28:26, joone wrote: > On 2016/08/11 05:17:51, sadrul wrote: > > On 2016/08/10 ...
1 year, 3 months ago (2016-09-06 19:30:33 UTC) #35
joone
On 2016/09/06 19:30:33, sadrul wrote: > On 2016/08/31 22:28:26, joone wrote: > > On 2016/08/11 ...
1 year, 3 months ago (2016-09-07 20:06:45 UTC) #36
joone
Hi sadrul@ I've rebased the CL. It became a bit simpler due to this change: ...
1 year, 2 months ago (2016-09-22 21:27:43 UTC) #42
joone
On 2016/09/22 21:27:43, joone wrote: > Hi sadrul@ > > I've rebased the CL. It ...
1 year, 2 months ago (2016-09-30 07:32:18 UTC) #45
sadrul
LGTM with the comments addressed. https://codereview.chromium.org/1624793002/diff/260001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/1624793002/diff/260001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc#newcode2331 ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2331: return base::WrapUnique(scoped_handle_); return base::MakeUnique<...>(base::Bind...) ...
1 year, 2 months ago (2016-09-30 12:22:27 UTC) #48
joone
Thanks for review! https://codereview.chromium.org/1624793002/diff/260001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/1624793002/diff/260001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc#newcode2331 ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2331: return base::WrapUnique(scoped_handle_); On 2016/09/30 12:22:27, sadrul ...
1 year, 2 months ago (2016-10-01 00:03:17 UTC) #53
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1624793002/360001
1 year, 2 months ago (2016-10-01 00:03:53 UTC) #56
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/271207)
1 year, 2 months ago (2016-10-01 00:15:34 UTC) #58
joone
erg@, could you review this CL?
1 year, 2 months ago (2016-10-01 00:35:53 UTC) #59
Elliot Glaysher
lgtm
1 year, 2 months ago (2016-10-03 17:12:45 UTC) #60
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1624793002/360001
1 year, 2 months ago (2016-10-03 18:29:51 UTC) #62
commit-bot: I haz the power
Committed patchset #12 (id:360001)
1 year, 2 months ago (2016-10-03 19:06:39 UTC) #64
commit-bot: I haz the power
1 year, 2 months ago (2016-10-03 19:09:47 UTC) #66
Message was sent while issue was closed.
Patchset 12 (id:??) landed as
https://crrev.com/df7f78e35f95dfae098dd6197b012e0506f68e0f
Cr-Commit-Position: refs/heads/master@{#422482}

Powered by Google App Engine
This is Rietveld 0eb02b776