Defer window destruction until GPU finished drawing.

Defer window destruction until GPU finished drawing.

When a tab is closed, it takes a while before the GPU stops drawing into the window. Destroying the window before the GPU has flushed its drawing pipeline causes unsightly X11 errors. The custom widget class that we use for drawing
tab contents has a lock that is set when the GPU process is drawing to that widget. We use this lock to determine who should delete the associated window:
- if the lock is clear at the time of widget destruction, the widget destroys the window
- if the lock is set, the GPU process signals to the browser to destroy the widget (it has to be done in the browser process b/c the X window is wrapped in a GdkWindow that resides in the browser address space)

Most the management is done in GtkNativeViewManager. I've added another map from XID to GtkWidget to facilitate this management.

BUG=55158
TEST=Open two windows with http://webkit.org/blog-files/3d-transforms/poster-circle.html Close one of them. There should be no X11 errors generated.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67719

[jonathan.backer, 2010-11-29 18:26:55]

Fixes a long standing bug. We may want this for M9 (it's a P2). Nat fixed this
same problem for Windows
(http://code.google.com/p/chromium/issues/detail?id=61516).

[Evan Martin, 2010-11-29 18:28:34]

+man who designed some of this stuff FYI

[agl, 2010-11-29 18:58:55]

Oh man. I know so little about what's going on here these days... No red flags
however.

[piman, 2010-11-29 21:59:01]

Overall this looks pretty good modulo some comments.

However I have a question: do we know for sure that the widget's XID is only
going to be "preserved" by a single client (GPU process command buffer stub) ?
Otherwise these semantics won't work, the first client will effectively destroy
the permanent XID (or at least unpin it) and it may go away under the other
client's feet. Would it be better/safer to have a refcount on the pin ?

http://codereview.chromium.org/5275009/diff/1/gfx/gtk_native_view_id_manager.cc
File gfx/gtk_native_view_id_manager.cc (right):

http://codereview.chromium.org/5275009/diff/1/gfx/gtk_native_view_id_manager....
gfx/gtk_native_view_id_manager.cc:107:
CHECK(GTK_IS_PRESERVE_WINDOW(i->second.widget));
I'm not sure that this CHECK (and other below) really need to be present in
release builds, you probably should change them to DCHECKs.

http://codereview.chromium.org/5275009/diff/1/gfx/gtk_native_view_id_manager....
gfx/gtk_native_view_id_manager.cc:183: xid_to_native_view_[xid] = widget;
We're effectively doing the lookup into the map twice, only for checks that
should always be true.
How about using xid_to_native_view_.insert(std::make_pair(xid, widget)) and
testing the return value for the checks ?

http://codereview.chromium.org/5275009/diff/1/gfx/gtk_native_view_id_manager.h
File gfx/gtk_native_view_id_manager.h (right):

http://codereview.chromium.org/5275009/diff/1/gfx/gtk_native_view_id_manager....
gfx/gtk_native_view_id_manager.h:76: // If the widget associated with the XID is
still alive, release the lock
You're talking about a lock here, but it's unclear from the user's pov what is
that lock. I assume you mean this releases the permanent XID.

http://codereview.chromium.org/5275009/diff/1/gfx/gtk_native_view_id_manager....
gfx/gtk_native_view_id_manager.h:122: // it's X window, but was destroyed. In
this case, the destruction is deferred
it's->its

http://codereview.chromium.org/5275009/diff/1/gfx/gtk_native_view_id_manager....
gfx/gtk_native_view_id_manager.h:123: // until a call to ReleasePermXID.
ReleasePermXID -> ReleasePermanentXID

[jonathan.backer, 2010-11-30 13:09:31]

I am working on reference counting the calls to GetPermanentXIDForID and
ReleasePermanentXID. Expect another patch set shortly.

http://codereview.chromium.org/5275009/diff/1/gfx/gtk_native_view_id_manager.cc
File gfx/gtk_native_view_id_manager.cc (right):

http://codereview.chromium.org/5275009/diff/1/gfx/gtk_native_view_id_manager....
gfx/gtk_native_view_id_manager.cc:107:
CHECK(GTK_IS_PRESERVE_WINDOW(i->second.widget));
On 2010/11/29 21:59:01, piman wrote:
> I'm not sure that this CHECK (and other below) really need to be present in
> release builds, you probably should change them to DCHECKs.

Done.

http://codereview.chromium.org/5275009/diff/1/gfx/gtk_native_view_id_manager....
gfx/gtk_native_view_id_manager.cc:183: xid_to_native_view_[xid] = widget;
On 2010/11/29 21:59:01, piman wrote:
> We're effectively doing the lookup into the map twice, only for checks that
> should always be true.
> How about using xid_to_native_view_.insert(std::make_pair(xid, widget)) and
> testing the return value for the checks ?

Done.

http://codereview.chromium.org/5275009/diff/1/gfx/gtk_native_view_id_manager.h
File gfx/gtk_native_view_id_manager.h (right):

http://codereview.chromium.org/5275009/diff/1/gfx/gtk_native_view_id_manager....
gfx/gtk_native_view_id_manager.h:122: // it's X window, but was destroyed. In
this case, the destruction is deferred
On 2010/11/29 21:59:01, piman wrote:
> it's->its

Done.

http://codereview.chromium.org/5275009/diff/1/gfx/gtk_native_view_id_manager....
gfx/gtk_native_view_id_manager.h:123: // until a call to ReleasePermXID.
On 2010/11/29 21:59:01, piman wrote:
> ReleasePermXID -> ReleasePermanentXID

Done.

[jonathan.backer, 2010-11-30 14:36:53]

As suggested by Antoine, I've implemented reference counting on permanent XIDs.
This led me to rethink the semantics of the xid lookup table, which simplified
the code. I renamed it perm_xid_to_info_ to reflect it's new role.

[Antoine Labour, 2010-11-30 17:05:55]

LGTM !