488620 - GTK2 test plugin should not use GDK methods to inspect the ancestors of its window

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

The ancestor windows of a plugin might not have associated GDK widgets. We should use X functions instead.

Comment 1

[Karl Tomlinson (:karlt)]

Determining the top-left of the window manager frame is tricky.
gdk_window_get_frame_extents has some complex logic for asking the window manager about this.

NPN_GetValue NPNVnetscapeWindow will give the toplevel window (not the frame),
and gdk_window_foreign_new, and gdk_window_get_frame_extents can be used on
that without any assumptions about which toolkit the browser uses.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://www.google.com/codesearch/p?hl=en#_EBSL1YmJGU/gtk%2B-2.4.14/gdk/x11/gdkwindow-x11.c&q=gdk_window_get_frame_extents

It doesn't look that complex to me. It finds the ancestor whose parent is the root, and returns its bounds as obtained by XGetGeometry. My code here returns coordinates relative to the ancestor whose parent is the root. So I think it's returning coordinates relative to the window frame that gdk_window_get_frame_extents would return. Am I missing something?

Comment 3

[Karl Tomlinson (:karlt)]

More recent versions just fall back to that method when other methods fail.
http://git.gnome.org/cgit/gtk+/tree/gdk/x11/gdkwindow-x11.c#n3118

I think virtual roots may be related to how some window managers do desktops.
I'm not sure whether a compositing window manager even needs to have a window for the frame.

The issue here is probably that the window manager frame top-left is not a natural origin for X11 applications.  There is related discussion here:
http://doc.trolltech.com/4.5/geometry.html#x11-peculiarities

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Then I suppose I need to get coordinates relative to the toplevel window's origin, then make them relative to the window frame by calling gdk_window_get_frame_extents on the gdk_window_foreign_new for the toplevel window. That way, the plugin's results will be guaranteed to be consistent with what window.screenX/screenY return in the tests.

I agree that using coordinates relative to the window manager frame is suboptimal, but as far as I know on Mac we have no choice, because I can't find a way for the plugin to convert its coordinates to screen coordinates that works without Carbon.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I suppose we could do something different on each platform. Then the problem is I don't know what the coordinate system should actually be on GTK2/X. The problem is that the only thing Web content knows about the top-level window is screenX/Y, which return the screen coordinates of the top-level window's window manager frame top-left... We don't have any way for a test to discover the offset from the top-level window's origin to the test's window origin, for example.

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to comment #5)
> I suppose we could do something different on each platform. Then the problem is
> I don't know what the coordinate system should actually be on GTK2/X. The
> problem is that the only thing Web content knows about the top-level window is
> screenX/Y, which return the screen coordinates of the top-level window's window
> manager frame top-left... We don't have any way for a test to discover the
> offset from the top-level window's origin to the test's window origin, for
> example.

I guess that means that the plugin needs to use some notion of the top-level window's window manager frame top-left, preferably the same as what the browser uses.

(In reply to comment #4)
> Then I suppose I need to get coordinates relative to the toplevel window's
> origin, then make them relative to the window frame by calling
> gdk_window_get_frame_extents on the gdk_window_foreign_new for the toplevel
> window. That way, the plugin's results will be guaranteed to be consistent with
> what window.screenX/screenY return in the tests.

Yes, I think that would be best.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 385561 [details] [diff] [review]
fix v2

+  for (;;) {

+    if (parent == root)
+      break;
+
+    window = parent;
+  }

The child of the root window is usually (but not necessarily) the window
manager's frame window.

+  GdkWindow* toplevel = gdk_window_foreign_new(window);

+  gdk_window_get_frame_extents(toplevel, &toplevelFrameExtents);

So toplevel is usually the window manager's frame, but
gdk_window_get_frame_extents expects the app's toplevel window.

I think NPN_GetValue NPNVnetscapeWindow is the best way to get the toplevel
window.  (You may find XTranslateCoordinates useful for pluginGetEdge too.)

+  var p = document.getElementById("p");
+  if (p.getClipRegionRectEdge(0, 2) - p.getClipRegionRectEdge(0, 0) != 200) {
+    // Plugin not set up yet
+    setTimeout(runTests, 100);
+    return;
+  }

I wonder whether the failure mode can be better than an infinite loop?
What is this waiting for?
Isn't the plugin loaded at onload?
Hasn't it been positioned and sized after a reflow?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v3

> I wonder whether the failure mode can be better than an infinite loop?

Yes it can.

> What is this waiting for?

See the comment I added. We need to wait for painting to unsuppress, since plugins are hidden and their geometry is not set up correctly, at least with my UpdatePluginGeometry changes.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/d11e97d7384d