486600 - Fennec doesnt restore the window on Omnia running windows mobile 6

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[kantha kanchiraju]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090402 Shiretoko/3.5b4pre (.NET CLR 3.5.30729)
Build Identifier: Release build made from changeset:   26816:1a6d30255368

When the close button (X) is clicked window is hidden but the application keeps running in the background. Only way to bring the window back is to use the task manager and say switch to application.

If the fennec program is started again, it starts another window instead of bringing the existing window to front.

Reproducible: Always

Steps to Reproduce:
1.start fennec on omnia
2.browse to a page
3.hit close and start fennec again
4.In task manager you will see two fennec applications



Expected Results:  
1.Only one fennec window should be running

Comment 1

[kantha kanchiraju]

Attachment: patch for wince fennec window activate and restore

With this patch on Wince if fennec is already running, it is activated and brought to front. It also prevents fennec from opening another window.

the change to SetForegroundWindow is based on the following documentation.
http://msdn.microsoft.com/en-us/library/ms940024.aspx

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 375195 [details] [diff] [review]
patch for wince fennec window activate and restore

i am sure you really didn't want tabs, right.  please fix those up.

Also add a comment why you want to OR x01.  (cite the msdn link would be cool)

Should this just be WINCE_WINDOWS_MOBILE instead of WINCE?


with answers and corrections, r=me.

Comment 3

[kantha kanchiraju]

I am trying to prevent fennec from opening multiple windows not tabs. tabs are working with this patch.

msdn document says this applies to wince. I am not sure if this is windows mobile specific. do you want me to change it to WINCE_WINDOWS_MOBILE?

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

My reading of MSDN leads me to believe that it should be WINCE, not WINCE_WINDOWS_MOBILE.

Comment 5

[Doug Turner (:dougt)]

kantha, can you put up another patch?

Comment 6

[kantha kanchiraju]

Attachment: patch with comments added as per doug's suggestion

Comments are added citing the msdn article. 
this patch applies to WINCE builds.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 375245 [details] [diff] [review]
patch with comments added as per doug's suggestion

>diff --git a/toolkit/xre/nsNativeAppSupportWin.cpp b/toolkit/xre/nsNativeAppSupportWin.cpp
>-        ::SetForegroundWindow( mHandle );
>-        ::SendMessage( mHandle, WM_COPYDATA, 0, (LPARAM)&cds );
>+#ifdef WINCE
>+		// for activating the existing window on wince we need "| 0x01"
>+		// see http://msdn.microsoft.com/en-us/library/ms940024.aspx for details
>+		::SetForegroundWindow( (HWND)(((ULONG) mHandle) | 0x01) );
>+#else
>+		::SetForegroundWindow( mHandle );
>+		::SendMessage( mHandle, WM_COPYDATA, 0, (LPARAM)&cds );
>+#endif

Use spaces (2) instead of tabs (\t) and please make sure the WM_COPYDATA SendMessage is used for both desktop and WinCE. just move it out of the block:

>+#ifdef WINCE
>+        // for activating the existing window on wince we need "| 0x01"
>+        // see http://msdn.microsoft.com/en-us/library/ms940024.aspx for details
>+        ::SetForegroundWindow( (HWND)(((ULONG) mHandle) | 0x01) );
>+#else
>+        ::SetForegroundWindow( mHandle );
>+#endif
>+        ::SendMessage( mHandle, WM_COPYDATA, 0, (LPARAM)&cds );

Comment 8

[Doug Turner (:dougt)]

mfinkle, do we want to continue passing the command line to running instances of fennec?  I suppose it does make some sense.

Comment 9

[kantha kanchiraju]

When the message is posted, a new window will be opened. we are trying to avoid that. thats the reason SendMessage is in else block.
I am not sure, but is there a way to send command line parameters in Wince.
I thought passing command line makes sense in DDE. In wince we are not starting the DDE Server, I guess thats another reason not to pass command line.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

We want to be able to have the user click on a link in another app (like email) and open it in Fennec.  If Fennec isn't running, we can handle this as is. 

If Fennec is already running we want to pass the link (or any other command line params) to the running instance.  It seems the problem you are seeing is a bug in our command line handling code where we create a new window and not a new tab.

Comment 11

[kantha kanchiraju]

Attachment: patch with sendmessage out of else block

new patch in response to comment #7.
I am still left with the issue of new window popping.
Can I check the command line and if it is empty stop sending the message and prevent a new window from popping.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #11)

> I am still left with the issue of new window popping.
> Can I check the command line and if it is empty stop sending the message and
> prevent a new window from popping.

You can file a new bug for this part. We'll need to add a commandline handler to Fennec, much like nsBrowserGlue.js in Firefox.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/e0d67d0900ce

Comment 14

[Aakash Desai [:aakashd]]

verified FIXED on build:

Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.3a1pre) Gecko/20090818 Fennec/1.0a3pre