Make File-Picker modal on Linux

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}

[joone, 2016-01-23 02:29:21]

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=BrowserSelectFileDialogTest, ModalTest

Review URL: https://codereview.chromium.org/1233913009

Cr-Commit-Position: refs/heads/master@{#360961}
==========

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
    
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
==========

[joone, 2016-01-23 02:52:57]

Hi sadrul@, erg@

I'm trying to fix the freezing problem, which happened due to my commit.
https://code.google.com/p/chromium/issues/detail?id=579408

Could you take a look at this CL? It could fix the problem.
Here is the detail:

https://codereview.chromium.org/1624793002/diff/1/chrome/browser/ui/libgtk2ui...
File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right):

https://codereview.chromium.org/1624793002/diff/1/chrome/browser/ui/libgtk2ui...
chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:432: 
when the users open a file-picker from a child window of the X11 host window,
parent can be null. As a result, event listening is not enabled again for the
X11 host window.

https://codereview.chromium.org/1624793002/diff/20001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right):

https://codereview.chromium.org/1624793002/diff/20001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:174:
SetDesktopWindowTreeHost(dialog, host);

SetDesktopWindowTreeHost method was added to store |host| in |dialog|.

https://codereview.chromium.org/1624793002/diff/20001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:426: 

Instead of getting the X11 host window from |parent|, we can directly get it
from |dialog|. Because |parent| can be null when it closes after opening a
file-picker.

[joone, 2016-01-23 02:55:11]

joone.hur@intel.com changed reviewers:
+ erg@chromium.org, sadrul@chromium.org

[sadrul, 2016-01-26 17:47:03]

It'd be better like this:

. Introduce 'scoped_ptr<ScopedHandle>
DesktopWindowTreeHostX11::DisableEvents(XID dialog);', where ScopedHandle looks
like this:

  class ScopedHandle {
   public:
    explicit ScopedHandle(const base::Closure& destroy_callback)
        : destroy_callback_(destroy_callback) {}
    ~ScopedHandle() { if (!destroy_calback_.is_null()) destroy_callback_.Run();
}

    private:
      base::Closure destroy_callback_;
      DISALLOW_COPY_AND_ASSIGNMENT...
  };

and at the end of DesktopWindowTreeHostX11::DisableEvents:

  return make_scoped_ptr(
      ScopedHandle(base::Bind(&DesktopWindowTreeHostX11::EnableEvents,
                              weak_ptr_factory_.GetWeakPtr())));

Maybe ScopedEventDisableHandle is a better name than ScopedHandle.

Additionally:
. DesktopWindowTreeHostX11::EnableEvents() should be a private method.
. DesktopWindowTreeHostX11::close_widget_factory_ should be renamed to
weak_ptr_factory_
. SelectFileDialogImplGTK::SelectFileImpl() can store the scoped-handle using
g_object_set_data() as you are doing now, or it could use
g_object_set_data_full() so SelectFileDialogImplGTK::FileDialogDestroyed()
doesn't need to do anything.

This way, you don't have to worry about whether the DesktopWindowTreeHostX11 has
already died or not.

[joone, 2016-02-24 01:46:53]

g_object_set_data can't keep the scoped-handle so the destructor is called when
SelectFileDialogImplGTK::SelectFileImpl() returns.

[joone, 2016-03-02 23:40:28]

sadral@ Please see the issue:

https://codereview.chromium.org/1624793002/diff/40001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right):

https://codereview.chromium.org/1624793002/diff/40001/chrome/browser/ui/libgt...
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 can't retain the scoped_handle so the destructor is called
when
SelectFileDialogImplGTK::SelectFileImpl() returns.

I think scoped_ptr cannot be used for this case because it can be used to retain
ownership solely within the current scope.

[joone, 2016-03-31 23:04:44]

On 2016/03/02 23:40:28, joone wrote:
> sadral@ Please see the issue:
> 
>
https://codereview.chromium.org/1624793002/diff/40001/chrome/browser/ui/libgt...
> File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right):
> 
>
https://codereview.chromium.org/1624793002/diff/40001/chrome/browser/ui/libgt...
> 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 can't retain the scoped_handle so the destructor is called
> when
> SelectFileDialogImplGTK::SelectFileImpl() returns.
> 
> I think scoped_ptr cannot be used for this case because it can be used to
retain
> ownership solely within the current scope.

sadrul@ do you have any idea?

[sadrul, 2016-04-13 16:28:36]

On 2016/02/24 01:46:53, joone wrote:
> g_object_set_data can't keep the scoped-handle so the destructor is called
when
> SelectFileDialogImplGTK::SelectFileImpl() returns.

Use g_object_set_data_full then?

[joone, 2016-04-14 23:03:21]

On 2016/04/13 16:28:36, sadrul wrote:
> On 2016/02/24 01:46:53, joone wrote:
> > g_object_set_data can't keep the scoped-handle so the destructor is called
> when
> > SelectFileDialogImplGTK::SelectFileImpl() returns.
> 
> Use g_object_set_data_full then?

I tried, but it also doesn't keep the scoped_handle.

[sadrul, 2016-04-15 09:26:15]

https://codereview.chromium.org/1624793002/diff/40001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right):

https://codereview.chromium.org/1624793002/diff/40001/chrome/browser/ui/libgt...
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:
> 
> g_object_set_data can't retain the scoped_handle so the destructor is called
> when
> SelectFileDialogImplGTK::SelectFileImpl() returns.
> 
> I think scoped_ptr cannot be used for this case because it can be used to
retain
> ownership solely within the current scope. 

You would have to do scope_handle.release() there, and use
g_object_set_data_full() version, so that you can specify a callback that
deletes the handle.

[joone, 2016-04-16 23:06:35]

Patchset #5 (id:80001) has been deleted

[joone, 2016-04-18 20:54:08]

On 2016/04/15 09:26:15, sadrul wrote:
>
https://codereview.chromium.org/1624793002/diff/40001/chrome/browser/ui/libgt...
> File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right):
> 
>
https://codereview.chromium.org/1624793002/diff/40001/chrome/browser/ui/libgt...
> 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:
> > 
> > g_object_set_data can't retain the scoped_handle so the destructor is called
> > when
> > SelectFileDialogImplGTK::SelectFileImpl() returns.
> > 
> > I think scoped_ptr cannot be used for this case because it can be used to
> retain
> > ownership solely within the current scope. 
> 
> You would have to do scope_handle.release() there, and use
> g_object_set_data_full() version, so that you can specify a callback that
> deletes the handle.

sadrul@ g_object_set_data_full() is now working. The other change is that
ScopedHandle became a nested class of DesktopWindowTreeHostX11 so that
SelectFileDialogImplGTK can access it as shared library. Thanks!

[sadrul, 2016-04-19 14:56:12]

https://codereview.chromium.org/1624793002/diff/100001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right):

https://codereview.chromium.org/1624793002/diff/100001/chrome/browser/ui/libg...
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)

https://codereview.chromium.org/1624793002/diff/100001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:185:
reinterpret_cast<GDestroyNotify>(OnFilePickerDestroy));
Add a comment here:

  // OnFilePickerDestroy() is called when |dialog| destroyed. The
  // callback destroys the ScopedHandle instance, this re-enabling
  // events on the owning window.

https://codereview.chromium.org/1624793002/diff/100001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right):

https://codereview.chromium.org/1624793002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:104: void
EnableEventListening();
Make this a private method.

[joone, 2016-04-19 22:29:56]

Other changes you asked was applied to the latest CL.

https://codereview.chromium.org/1624793002/diff/100001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right):

https://codereview.chromium.org/1624793002/diff/100001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:179:
owning_window->GetHost()->GetAcceleratedWidget());
On 2016/04/19 14:56:11, sadrul wrote:
> owning_window can be null, right? (see code/comment above)

I tried to reproduce the bug, but the file-picker is shown up well without any
crash. I think this bug was already fixed.

[joone, 2016-04-21 18:32:44]

Patchset #7 (id:140001) has been deleted

[joone, 2016-04-21 20:45:45]

sadrul@ can you take a look at the following change?

https://codereview.chromium.org/1624793002/diff/160001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1624793002/diff/160001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:363:
scoped_handle_->CancelCallback();

I added CancelCallback method to fix the problem that the file-picker is not
closed when trying to close the host window while opening the file-picker. The
reason is that |close_widget_factory_| has weak pointers at this situation so
the CloseNow callback cannot be called.

[joone, 2016-04-28 19:46:52]

On 2016/04/21 20:45:45, joone wrote:
> sadrul@ can you take a look at the following change?
> 
>
https://codereview.chromium.org/1624793002/diff/160001/ui/views/widget/deskto...
> File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):
> 
>
https://codereview.chromium.org/1624793002/diff/160001/ui/views/widget/deskto...
> ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:363:
> scoped_handle_->CancelCallback();
> 
> I added CancelCallback method to fix the problem that the file-picker is not
> closed when trying to close the host window while opening the file-picker. The
> reason is that |close_widget_factory_| has weak pointers at this situation so
> the CloseNow callback cannot be called.

sadrul@ ping?

[sadrul, 2016-05-11 05:27:47]

https://codereview.chromium.org/1624793002/diff/100001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right):

https://codereview.chromium.org/1624793002/diff/100001/chrome/browser/ui/libg...
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 14:56:11, sadrul wrote:
> > owning_window can be null, right? (see code/comment above)
> 
> I tried to reproduce the bug, but the file-picker is shown up well without any
> crash. I think this bug was already fixed.

Update the code/comment from above too (lines 128:134). It should probably be a
separate CL that lands first.

https://codereview.chromium.org/1624793002/diff/160001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right):

https://codereview.chromium.org/1624793002/diff/160001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:184: // destroys the
ScopedHandle instance, this re-enabling events on the
*thus

https://codereview.chromium.org/1624793002/diff/160001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1624793002/diff/160001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:363:
scoped_handle_->CancelCallback();
On 2016/04/21 20:45:45, joone wrote:
> 
> I added CancelCallback method to fix the problem that the file-picker is not
> closed when trying to close the host window while opening the file-picker. The
> reason is that |close_widget_factory_| has weak pointers at this situation so
> the CloseNow callback cannot be called.

I am not sure I understand this well. What happens if we don't do this
CancelCallback()?

[joone, 2016-05-11 19:18:05]

https://codereview.chromium.org/1624793002/diff/100001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right):

https://codereview.chromium.org/1624793002/diff/100001/chrome/browser/ui/libg...
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 22:29:56, joone wrote:
> > On 2016/04/19 14:56:11, sadrul wrote:
> > > owning_window can be null, right? (see code/comment above)
> > 
> > I tried to reproduce the bug, but the file-picker is shown up well without
any
> > crash. I think this bug was already fixed.
> 
> Update the code/comment from above too (lines 128:134). It should probably be
a
> separate CL that lands first.

I already did it: https://codereview.chromium.org/1912873002

https://codereview.chromium.org/1624793002/diff/160001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1624793002/diff/160001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:363:
scoped_handle_->CancelCallback();
On 2016/05/11 05:27:47, sadrul wrote:
> On 2016/04/21 20:45:45, joone wrote:
> > 
> > I added CancelCallback method to fix the problem that the file-picker is not
> > closed when trying to close the host window while opening the file-picker.
The
> > reason is that |close_widget_factory_| has weak pointers at this situation
so
> > the CloseNow callback cannot be called.
> 
> I am not sure I understand this well. What happens if we don't do this
> CancelCallback()?

This file-pick is still opened after closing the host window. Then, the instance
of the host window is still alive after closing the file-picker.

[sadrul, 2016-05-11 19:20:36]

https://codereview.chromium.org/1624793002/diff/160001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1624793002/diff/160001/ui/views/widget/deskto...
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 05:27:47, sadrul wrote:
> > On 2016/04/21 20:45:45, joone wrote:
> > > 
> > > I added CancelCallback method to fix the problem that the file-picker is
not
> > > closed when trying to close the host window while opening the file-picker.
> The
> > > reason is that |close_widget_factory_| has weak pointers at this situation
> so
> > > the CloseNow callback cannot be called.
> > 
> > I am not sure I understand this well. What happens if we don't do this
> > CancelCallback()?
> 
> This file-pick is still opened after closing the host window.

If this host-window is closed, then that would invalidate the weak-ptr to this
DWTHX11 object, and the callback would not be run when the file-dialog is
closed.

> Then, the instance
> of the host window is still alive after closing the file-picker. 

?? Not sure what this means.

[joone, 2016-05-11 21:13:18]

> > Then, the instance
> > of the host window is still alive after closing the file-picker. 
> 
> ?? Not sure what this means.

Chromium processes are running behind so we can only kill the processes by
running kill command.

[joone, 2016-05-27 17:01:51]

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
    
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
==========

to

==========
Make File-Picker modal on Linux
    
Chromium for Linux opens a Gtk+ file-picker for file browsing, but it is not
modal to the X11 host window because its native widget is not a GtkWindow
but a X11 window. However, GtK+ file-picker 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
    
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
==========

[joone, 2016-07-22 18:35:45]

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 to the X11 host window because its native widget is not a GtkWindow
but a X11 window. However, GtK+ file-picker 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
    
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
==========

to

==========
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
    
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
==========

[joone, 2016-07-22 18:36:17]

Description was changed from

==========
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
    
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
==========

to

==========
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.
    
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
==========

[joone, 2016-07-22 20:40:03]

Hi, sadrul@

I draw up a document to explain my CL:
https://docs.google.com/document/d/12CfKVTpaonxxM3sNksq6vY6qb0J2qR3b7h_bLxzYa...

It is helpful to understand the changes.

[joone, 2016-07-22 21:07:30]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 21:08:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 21:49:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 21:49:31]

Dry run: This issue passed the CQ dry run.

[joone, 2016-08-10 17:19:30]

sadrul@ ping?

[sadrul, 2016-08-11 05:17:51]

On 2016/08/10 17:19:30, joone wrote:
> sadrul@ ping?

I have responded on the doc.

[joone, 2016-08-16 01:27:46]

Description was changed from

==========
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.
    
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
==========

to

==========
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
==========

[joone, 2016-08-16 01:28:25]

Description was changed from

==========
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
==========

to

==========
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
==========

[joone, 2016-08-31 22:28:26]

On 2016/08/11 05:17:51, sadrul wrote:
> On 2016/08/10 17:19:30, joone wrote:
> > sadrul@ ping?
> 
> I have responded on the doc.

sadrul@ Please take a look at my comments on the doc.

[sadrul, 2016-09-06 19:30:33]

On 2016/08/31 22:28:26, joone wrote:
> On 2016/08/11 05:17:51, sadrul wrote:
> > On 2016/08/10 17:19:30, joone wrote:
> > > sadrul@ ping?
> > 
> > I have responded on the doc.
> 
> sadrul@ Please take a look at my comments on the doc.

I think a quick real-time conversation (IM, or VC) would be useful (I have sent
out an email about it last week). Would that be possible?

[joone, 2016-09-07 20:06:45]

On 2016/09/06 19:30:33, sadrul wrote:
> On 2016/08/31 22:28:26, joone wrote:
> > On 2016/08/11 05:17:51, sadrul wrote:
> > > On 2016/08/10 17:19:30, joone wrote:
> > > > sadrul@ ping?
> > > 
> > > I have responded on the doc.
> > 
> > sadrul@ Please take a look at my comments on the doc.
> 
> I think a quick real-time conversation (IM, or VC) would be useful (I have
sent
> out an email about it last week). Would that be possible?

I sent you an invitation.

[joone, 2016-09-22 20:10:29]

Patchset #10 (id:220001) has been deleted

[joone, 2016-09-22 20:32:17]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 20:33:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 21:19:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 21:19:58]

Dry run: This issue passed the CQ dry run.

[joone, 2016-09-22 21:27:43]

Hi sadrul@

I've rebased the CL. It became a bit simpler due to this change:
https://codereview.chromium.org/2165083002

[joone, 2016-09-30 06:59:38]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 06:59:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joone, 2016-09-30 07:32:18]

On 2016/09/22 21:27:43, joone wrote:
> Hi sadrul@
> 
> I've rebased the CL. It became a bit simpler due to this change:
> https://codereview.chromium.org/2165083002

Please review the updated CL.

[commit-bot: I haz the power, 2016-09-30 07:33:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 07:33:20]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-09-30 12:22:27]

LGTM with the comments addressed.

https://codereview.chromium.org/1624793002/diff/260001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1624793002/diff/260001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2331: return
base::WrapUnique(scoped_handle_);
return base::MakeUnique<...>(base::Bind...)

https://codereview.chromium.org/1624793002/diff/260001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right):

https://codereview.chromium.org/1624793002/diff/260001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:440: ScopedHandle*
scoped_handle_;
You shouldn't need this?

[joone, 2016-09-30 23:46:21]

Patchset #12 (id:280001) has been deleted

[joone, 2016-09-30 23:48:39]

Patchset #12 (id:300001) has been deleted

[joone, 2016-09-30 23:56:51]

Patchset #12 (id:320001) has been deleted

[joone, 2016-09-30 23:59:03]

Patchset #12 (id:340001) has been deleted

[joone, 2016-10-01 00:03:17]

Thanks for review!

https://codereview.chromium.org/1624793002/diff/260001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1624793002/diff/260001/ui/views/widget/deskto...
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 wrote:
> return base::MakeUnique<...>(base::Bind...)

Done.

https://codereview.chromium.org/1624793002/diff/260001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right):

https://codereview.chromium.org/1624793002/diff/260001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:440: ScopedHandle*
scoped_handle_;
On 2016/09/30 12:22:27, sadrul wrote:
> You shouldn't need this?

Removed.

[joone, 2016-10-01 00:03:24]

The CQ bit was checked by joone.hur@intel.com

[joone, 2016-10-01 00:03:25]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1624793002/#ps360001
(title: "Remove ScopedHandle")

[commit-bot: I haz the power, 2016-10-01 00:03:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-01 00:15:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-01 00:15:34]

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_presub...)

[joone, 2016-10-01 00:35:53]

erg@, could you review this CL?

[Elliot Glaysher, 2016-10-03 17:12:45]

lgtm

[joone, 2016-10-03 18:28:49]

The CQ bit was checked by joone.hur@intel.com

[commit-bot: I haz the power, 2016-10-03 18:29:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 19:06:37]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-10-03 19:06:39]

Committed patchset #12 (id:360001)

[commit-bot: I haz the power, 2016-10-03 19:09:44]

Description was changed from

==========
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
==========

to

==========
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}
==========

[commit-bot: I haz the power, 2016-10-03 19:09:47]

Patchset 12 (id:??) landed as
https://crrev.com/df7f78e35f95dfae098dd6197b012e0506f68e0f
Cr-Commit-Position: refs/heads/master@{#422482}