50130 - A more efficient way for implementing widget/src/gtk/GetShellWidget()

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Jih-Shien Lu]

To find the shell widget, we can use gdk_window_get_toplevel() to avoid
repeatedly retrieving user_data by gdk_window_get_user_data() and
gtk_object_get_data():


static nsWidget *
GetShellWidget(GdkWindow *gdkWindow)
{
  GdkWindow *parent = gdk_window_get_toplevel(gdkWindow);
  nsWidget *root_win = nsnull;
  gpointer data;

  if (parent != gdkWindow) {
    gdk_window_get_user_data(parent, &data);
    if (GTK_IS_OBJECT(data)) {
      root_win = (nsWidget *) gtk_object_get_data(GTK_OBJECT(data), "nsWindow");
      if (root_win)
        return root_win;
    }
  }
  return nsnull;
}

This code seems to work for me, please take a look and decide if it's ok to use
it.
Thank you.

Comment 1

[Asa Dotzler [:asa]]

unsetting target milestone. Target Milestone is reserved for developer triage.

Comment 2

[Hidetoshi Tajima]

The code in the comment has been tested. Ready for check-in.
r=[e-mail] is wanted.

Comment 3

[Stuart Parmenter]

the code looks fine to me.  r=pavlov

Comment 4

[Christopher Blizzard (:blizzard)]

Interesting.  It looks ok for what it is supposed to do.

While I have the chance though I'd like to ask what happens in the case where
the top level window is not a mozilla window.  This happens in the case where
mozilla is embedded in another application.  I get tons + tons of asserts from
the GetShellWidget() code in my embedding app and it drives me up the wall.  Is
there any way that we can clean up those problems while we are at it?  Maybe at
least not make it an error condition?

Comment 5

[Hidetoshi Tajima]

blizzard, what kind of application are you
embedding mozilla? Nautilus?

Comment 6

[Jih-Shien Lu]

I guess that every gtk embedding application will be affected, since
gdk_window_get_toplevel() will return the outmost GdkWindow, which is
not necessarily owned by mozilla.
I not quite familiar with XIM programming model, therefore I don't know
whether the shell window responsible for handling XIM should be the
outmost window of the *entire* application or just that of mozilla. I will
propose a better solution after I figure out how to get the correct shell
window that mozilla needs for XIM input.

Comment 7

[Christopher Blizzard (:blizzard)]

Nautilus is one of the conusmers of it, yes.

Comment 8

[Masaki Katakai]

I tried TestGtkEmbed, but IME didn't work on it.
XIC is never created because GetShellWidget()
returns null as you commented. GetShellWidget() should
be changed for GtkEmbed.

I talked with Tajima-san (he is in japan now) and
we found a solution that we can use the most parent
(ancestral) nsWidget as nsIMEShellWidget. That nsWidget
has a native GtkWindow which will be embedded. I will attach
the patch for nsWidget.cpp. Does this make sense?

I'm not sure that embed feature also will be into RTM.
Do you think this problem should be fixed by RTM?
Should we continue to use this bug id? or need new one?

Comment 9

[Masaki Katakai]

Attachment: patch for GetShellWidget() for GtkEmbed

Comment 10

[Christopher Blizzard (:blizzard)]

The best way to actually get the outermost mozilla window is to use
nsWindow::GetMozArea().  Not only does it locate the outermost Mozilla window
but it's cached after it is called once so repeated calls will be very fast.

Why isn't this code in nsWindow, anyway?  Is there some reason all of this XIM
code isn't?

Comment 11

[Masaki Katakai]

Thanks for suggestion. I've verified GetShellWidget() and
GetMozArea() (+ get nsWindow object) return the same result.
We should use GetMozArea(). I'll make a patch.

Comment 12

[Masaki Katakai]

Attachment: 2nd patch

Comment 13

[Masaki Katakai]

I've attached the 2nd patch,

mozAreaWindow = (nsWindow *)gtk_object_get_data(GTK_OBJECT(top_mozarea),
"nsWindow");

GetMozArea() and gtk_object_get_data() can return the shell
widget, which can be used for GtkEmbed also. We can remove
GetShellWidget() codes from nsWidget.cpp now.

Comment 14

[Christopher Blizzard (:blizzard)]

Adding Adrian.  Adrian, can you have a look at this patch, too?

Comment 15

[Christopher Blizzard (:blizzard)]

It looks reasonable to me.

Comment 16

[Adrian Havill]

Code appears to work o.k. for me, and the IME problem appears to be fixed.

Comment 17

[Masaki Katakai]

I've checked in the patch right now of bug 53989 which can solve this problem.

Please assign to me or please mark as DUP of bug 53989.

Thank you.

Comment 18

[Hidetoshi Tajima]

reassign..

Comment 19

[Masaki Katakai]

fix checked in as part of bug 53989.

Comment 20

[Masaki Katakai]

*** Bug 50403 has been marked as a duplicate of this bug. ***

Comment 21

[Masaki Katakai]

*** Bug 82138 has been marked as a duplicate of this bug. ***

Comment 22

[Boris Zbarsky [:bzbarsky]]

*** Bug 81902 has been marked as a duplicate of this bug. ***