162507 - Default icon should not be set for embedding applications

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Marco Pesenti Gritti]

The new gtk2 icon code override the icon set by galeon with the mozilla icons.
To reproduce just try TestGtkEmbed and see that it use the mozilla icon.

Comment 1

[Marco Pesenti Gritti]

Attachment: proposed fix

The patch fix the problem by setting the default icon on Create, only when the
window type is not a child. So it will set it on the mozilla toplevel creation
but not on the gtkmozembed child creation.
I couldnt think to a cleaner fix ... hope it's ok.

Comment 2

[Marco Pesenti Gritti]

Just noticed that the patch doesnt fix the problem entirely, will try to
investigate more.

Comment 3

[Marco Pesenti Gritti]

Ok on some sites (www.linuxtoday.com, this bug page, www.slashdot.org), the
patch doesnt work because a toplevel (popup type) is created and immediately
destroyed ... Is this supposed to happen ?
Also I should have used mIsTopLevel instead of testing the type directly I guess.

Comment 4

[Marco Pesenti Gritti]

Attachment: working patch

The patch leave the popup toplevel out of our test. Appear to work this time.

Comment 5

[Marco Pesenti Gritti]

The problem is more serious than I thought. Overriding the icon cause critical
g_object_unref warnings and crashes when closing windows. (Changing severity)

Comment 6

[Christopher Blizzard (:blizzard)]

Maybe using the default icon is a bad idea.  Maybe we just need to set it for
toplevel windows every time.  That's reasonable.  This patch just sets the
default icon for the app once someone opens a toplevel window, which an app
might not want mozilla to do. :)

Comment 7

[Marco Pesenti Gritti]

I was assuming we had two cases
1 Embedding apps. A toplevel window is never created (through nsWindow) and we
dont want the default icon to be set.
2 Mozilla. A toplevel window is created and we want to set the default icon.

So, I'm sure I'm missing something, I cant get what is "which an app
might not want mozilla to do" referred to.

Anyway, setting the icon every time sounds like a good solution to me. Thanks
for looking into this.

Comment 8

[Christopher Blizzard (:blizzard)]

There's no way for the code to tell when Mozilla is embedded and when it isn't.
 And, frankly, I don't think that we should have to worry about it.  If we
create a toplevel window, we should be setting the icon, and we shouldn't use
the set_default at all.

Comment 9

[Marco Pesenti Gritti]

Attachment: set the icon only on toplevel creation (eWindowType_dialog and eWindowType_toplevel(

Note that these days another icon is set outside the widget implementation
using SetIcon. 
So default.xpm is not visible (at least in the cases I verified). 
I wonder if we still need to set the icon in nsWindow at all ...

Comment 10

[Tommi Komulainen]

Attachment: The previous patch rediffed for current trunk

The previous patch didn't apply anymore.  I've updated it for current trunk.

Comment 11

[Christopher Blizzard (:blizzard)]

Attachment: patch

Fixing icon handling.

Comment 12

[Christopher Blizzard (:blizzard)]

Checked in.