Native app icon in window (frame) replaced with Executable icon when closing the app

VERIFIED FIXED in Firefox 16

Status

Firefox Graveyard
Web Apps
P3
normal
VERIFIED FIXED
6 years ago
2 years ago

People

(Reporter: Mohamed Dabbagh, Assigned: TimAbraldes)

Tracking

({polish})

15 Branch
Firefox 16
x86_64
Windows 7
polish

Details

(Whiteboard: [qa!])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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)
Whiteboard: devPreviewNonBlocker
A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/24808727
Definitely mozilla-central worthy.
Blocks: 731054

Updated

6 years ago
Component: Extension → General
Product: Web Apps → Firefox
QA Contact: extension → general
Target Milestone: --- → Firefox 14

Updated

6 years ago
Component: General → Web Apps
QA Contact: general → webapps
No longer blocks: 731054
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.

Updated

6 years ago
Blocks: 731054

Updated

5 years ago
Whiteboard: devPreviewNonBlocker

Updated

5 years ago
Keywords: qawanted
Whiteboard: [marketplace-beta-]
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.
Keywords: qawanted
Summary: Native app icon in window (frame) replaced with Firefox icon when closing the app → Native app icon in window (frame) replaced with Executable icon when closing the app
I'm unable to reproduce this issue on latest Nightly.  Is this still an issue?

Updated

5 years ago
Keywords: qawanted
(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
Keywords: qawanted

Updated

5 years ago
Priority: -- → P3

Updated

5 years ago
Target Milestone: Firefox 14 → Future

Updated

5 years ago
No longer blocks: 731054

Updated

5 years ago
Version: unspecified → 15 Branch

Updated

5 years ago
Whiteboard: [marketplace-beta-]
Discussed this with rs: It's possible that simply hiding the window before the icon resources get removed/destroyed would solve this issue.
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.
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?
Assignee: nobody → tabraldes
Status: NEW → ASSIGNED
Attachment #632357 - Flags: review?(jmathies)

Updated

5 years ago
Keywords: polish

Comment 10

5 years ago
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

5 years ago
(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

5 years ago
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?
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?
Attachment #632357 - Attachment is obsolete: true
Attachment #632357 - Flags: review?(jmathies)
Attachment #637328 - Flags: review?(jmathies)

Comment 14

5 years ago
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?
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()`
Attachment #637328 - Attachment is obsolete: true
Attachment #637328 - Flags: review?(jmathies)
Attachment #637741 - Flags: review?(jmathies)

Updated

5 years ago
Attachment #637741 - Flags: review?(jmathies) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/709db7ae0dbc
Target Milestone: Future → Firefox 16

Updated

5 years ago
QA Contact: jsmith
Whiteboard: [qa+]
https://hg.mozilla.org/mozilla-central/rev/709db7ae0dbc
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 18

5 years ago
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.
Thanks Mohamed. I finished the testing on this for the other pieces. Marking as verified.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.