Last Comment Bug 708033 - Native app icon in window (frame) replaced with Executable icon when closing the app
: Native app icon in window (frame) replaced with Executable icon when closing ...
Status: VERIFIED FIXED
[qa!]
: polish
Product: Firefox Graveyard
Classification: Graveyard
Component: Web Apps (show other bugs)
: 15 Branch
: x86_64 Windows 7
: P3 normal
: Firefox 16
Assigned To: Tim Abraldes [:TimAbraldes] [:tabraldes]
: Jason Smith [:jsmith]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-06 11:50 PST by Mohamed Dabbagh
Modified: 2016-02-04 15:00 PST (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 - For windows that are being destroyed, hide them before destroying their icons. (1.00 KB, patch)
2012-06-12 11:56 PDT, Tim Abraldes [:TimAbraldes] [:tabraldes]
no flags Details | Diff | Splinter Review
Patch v2 (2.80 KB, patch)
2012-06-27 17:43 PDT, Tim Abraldes [:TimAbraldes] [:tabraldes]
no flags Details | Diff | Splinter Review
Patch v3 (4.21 KB, patch)
2012-06-28 17:44 PDT, Tim Abraldes [:TimAbraldes] [:tabraldes]
jmathies: review+
Details | Diff | Splinter Review

Description Mohamed Dabbagh 2011-12-06 11:50:59 PST
When the user launches a native app, the correct app icon is shown in the top left corner of the window (frame), when they close the app, the app icon is replaced with the Firefox icon until the app is closed. 

This occurs with Windows 7, Vista and presumably XP (but cannot verify as app icon support is still broken).

Steps to Reproduce:
1. Launch Firefox - Make sure to use FF9 or greater
2. Install the Apps extension - openwebapps-23dc5022e9-12_02_2011.xpi from http://people.mozilla.com/~dclarke/openwebapps/Extension/
3. Go to apps.mozillalabs.com/appdir
4. Install an app - Make sure to choose 'Install (with Native app)'
5. Double click on the desktop icon to launch the app - Notice correct app icon is shown on the top right corner of the window (frame)
6. Close the app and make sure to notice the app icon on the top left corner of the window (frame)

Actual Results:
The app icon is replaced with the Firefox icon while the app is closing

Expected Results:
The Firefox icon should not replace the app icon at any time in the window (frame)
Comment 1 Jason Smith [:jsmith] 2012-02-11 16:14:40 PST
A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/24808727
Comment 2 Jason Smith [:jsmith] 2012-03-13 18:37:22 PDT
Definitely mozilla-central worthy.
Comment 3 Jason Smith [:jsmith] 2012-03-15 09:09:10 PDT
I think we establish what to do with the existing tracking bug. If it's going to be used, then this bug still needs to block the tracking bug.
Comment 4 Jason Smith [:jsmith] 2012-04-24 21:23:27 PDT
On the new mozilla-central implementation on Win 7, I don't see the firefox icon appear on close, but I do see a different icon appearing than the app icon on close. The icon appearing on close looks like the icon for an executable for windows. This bug is still valid, except its a different icon being shown.
Comment 5 Tim Abraldes [:TimAbraldes] [:tabraldes] 2012-05-01 09:57:42 PDT
I'm unable to reproduce this issue on latest Nightly.  Is this still an issue?
Comment 6 Jason Smith [:jsmith] 2012-05-01 11:27:20 PDT
(In reply to Tim Abraldes from comment #5)
> I'm unable to reproduce this issue on latest Nightly.  Is this still an
> issue?

Yes. Just re-tested it. Try this test case:

1. Go to apps.mozillalabs.com/appdir
2. Install Favimon
3. Launch Favimon
4. Watch the icon in the top left of the window - Close the app window

You should see the favimon icon switch from that icon to the executable icon.

Note - Does not look like this happens with every single application.

Screenshot of issue: http://screencast.com/t/XuvoxPxUbF
Comment 7 Tim Abraldes [:TimAbraldes] [:tabraldes] 2012-05-23 14:10:10 PDT
Discussed this with rs: It's possible that simply hiding the window before the icon resources get removed/destroyed would solve this issue.
Comment 8 Tim Abraldes [:TimAbraldes] [:tabraldes] 2012-05-23 14:10:56 PDT
I meant to add this link [https://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp?rev=f4157e8c4107#6874]. This is where we would want to hide the window.
Comment 9 Tim Abraldes [:TimAbraldes] [:tabraldes] 2012-06-12 11:56:06 PDT
Created attachment 632357 [details] [diff] [review]
Patch v1 - For windows that are being destroyed, hide them before destroying their icons.

The attached patch fixes this issue in my local testing.

Pushed to try:
  https://tbpl.mozilla.org/?tree=Try&rev=670127177d1e

Jimm: Do you see any danger with this proposed patch?
Comment 10 Jim Mathies [:jimm] 2012-06-13 08:59:34 PDT
Isn't this going to get rid of the nice fade out windows experience on Vista+? A better solution might be to move the DestroyIcon code to the OnDestroy() handler. According to msdn windows that receive WM_DESTROY have already been removed from the screen.
Comment 11 Jim Mathies [:jimm] 2012-06-13 09:02:08 PDT
(In reply to Jim Mathies [:jimm] from comment #10)
> Isn't this going to get rid of the nice fade out windows experience on
> Vista+? A better solution might be to move the DestroyIcon code to the
> OnDestroy() handler. According to msdn windows that receive WM_DESTROY have
> already been removed from the screen.

Oh, pfft, never mind, it's already in OnDestroy(). This is weird, I don't see this problem with Fx, I'm guessing that because we don't use SetIcon and web apps do.
Comment 12 Jim Mathies [:jimm] 2012-06-13 09:05:02 PDT
My main concern with the hide call is that it will likely fire gecko events, which I'd like to avoid. How about caching the two icons we set in a member variable and free them in the dtor?
Comment 13 Tim Abraldes [:TimAbraldes] [:tabraldes] 2012-06-27 17:43:04 PDT
Created attachment 637328 [details] [diff] [review]
Patch v2

This patch uses jimm's suggestion (in `nsWindow::OnDestroy` cache the window's big and small icon, then in `nsWindow::~nsWindow` free them).

The member variables could probably use better names, but this patch works in local testing.  What do you think, Jim?
Comment 14 Jim Mathies [:jimm] 2012-06-28 08:00:54 PDT
Comment on attachment 637328 [details] [diff] [review]
Patch v2

Review of attachment 637328 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindow.cpp
@@ +6953,5 @@
>    }
>  
> +  // Store icon resources to be freed
> +  mIconTempBig = (HICON) ::SendMessageW(mWnd, WM_GETICON, (WPARAM)ICON_BIG, (LPARAM) 0);
> +  mIconTempSmall = (HICON) ::SendMessageW(mWnd, WM_GETICON, (WPARAM)ICON_SMALL, (LPARAM) 0);

Instead of doing this in OnDestroy, why not store the icons in the member variables when they are set in SetIcon?
Comment 15 Tim Abraldes [:TimAbraldes] [:tabraldes] 2012-06-28 17:44:40 PDT
Created attachment 637741 [details] [diff] [review]
Patch v3

This patch initializes the member variables (patch v2 omitted the member initialization) and stores the icons during `nsWindow::SetIcon()` rather than during `nsWindow::OnDestroy`

This patch is running through tryserver:
  https://tbpl.mozilla.org/?tree=Try&rev=031d0e649054

(In reply to Jim Mathies [:jimm] from comment #14)
> Comment on attachment 637328 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 637328 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/nsWindow.cpp
> @@ +6953,5 @@
> >    }
> >  
> > +  // Store icon resources to be freed
> > +  mIconTempBig = (HICON) ::SendMessageW(mWnd, WM_GETICON, (WPARAM)ICON_BIG, (LPARAM) 0);
> > +  mIconTempSmall = (HICON) ::SendMessageW(mWnd, WM_GETICON, (WPARAM)ICON_SMALL, (LPARAM) 0);
> 
> Instead of doing this in OnDestroy, why not store the icons in the member
> variables when they are set in SetIcon?

I was trying to avoid edge cases where someone uses `WM_SETICON` to set our window's icon without our knowledge (i.e. not through `nsWindow::SetIcon`): If we get the icon during `OnDestroy()` we can be sure that we're destroying the right icon.  On the other hand, our icon should only ever be set through `nsWindow::SetIcon()` so that might be overly cautious.  I've updated the patch to set the member variables during `nsWindow::SetIcon()`
Comment 16 Tim Abraldes [:TimAbraldes] [:tabraldes] 2012-07-05 16:08:42 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/709db7ae0dbc
Comment 18 Mohamed Dabbagh 2012-07-06 17:55:43 PDT
Tried with:
- Windows 7
- Firefox Nightly Build identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0 (updated on July 6, 2012)

Installed the following apps from apps.mozillalabs.com/appdir:
- Favimon (beta)
- BarFight
- Lucidchart

Also installed from https://marketplace.mozilla.org/en-US/ (apps-preview):
- Jauntly
- Galactians 2

Launched apps from desktop and closed each one and the problem does not occur anymore. The app icon remains in the window frame until the app closes. Tried with having FF nightly and beta opened while the app was running and without having any FF open and worked in all cases. Also tried with windows maximized and still no issue appeared. 

The issue seems fixed but I only tested on Window 7 as that's all I have. I'll keep the status as is since testing on Vista and XP still needs to be done.
Comment 19 Jason Smith [:jsmith] 2012-07-09 20:25:56 PDT
Thanks Mohamed. I finished the testing on this for the other pieces. Marking as verified.

Note You need to log in before you can comment on or make changes to this bug.