933580 - Defect - Not switching to desktop if Adobe Reader set as default application

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: File Handling, defect, P2)

Description

[Kamil Jozwiak [:kjozwiak]]

If the Adobe Reader is set as the default application, Firefox Metro doesn't switch to the desktop when the PDF is opened. So we have a situation where a user downloads a PDF, it opens on the desktop but Firefox Metro just display's a blank screen and does nothing.

If the Windows Reader App is set to the default application, Firefox Metro correctly switches over without issues. Think this should be the same behavior if the Adobe Reader is the default PDF application.

Steps to reproduce the issue:

Prerequisites:
- Download Adobe Reader for the desktop
- Remove the default "Reader" from Windows 8 App Store

1) Open Firefox Metro
2) Search for PDF under Google and select any trusted link that's a [PDF]
3) The PDF will start downloading and once completed, will open on the desktop PDF reader but you'll just have a "blank" page under Firefox Metro

Current Behavior:

- PDF opens on the desktop but the user is never taken there. User just see's a blank page with the "download app bar" at the bottom indicating the PDF was downloaded successfully.

Expected Behavior:

- I think Firefox Metro should switch to the desktop so the PDF that was downloaded is visible & displayed. User can always switch back to Firefox Metro once they are done with the PDF.

Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-10-31-03-02-03-mozilla-central/

Comment 1

[Brian R. Bondy [:bbondy]]

Sounds like we have a ShellExecute call somewhere that's not passing:
seinfo.fMask = SEE_MASK_FLAG_LOG_USAGE;

Comment 2

[Brian R. Bondy [:bbondy]]

That flag allows windows to make an environment switch when opening files.

Comment 3

[Kamil Jozwiak [:kjozwiak]]

Attachment: Switches to the Desktop if the default application is not a MS App Reader

Used Brians advice from comment #1 and tried each of the shell executes and this is the one that fixed the problem. Found the ShellExecute calls by using dxr.mozilla.org

Also went through a few test cases to make sure everything was still working:
- Switched the default PDF reader to Adobe and ensured that once downloaded it would switch to the desktop after the PDF has been opened
- Switched the default PDF reader to MS App Reader and ensured that the PDF is opened in the app once it has been downloaded
- Downloaded about 10 PDF's just to make sure things worked

Comment 4

[Jim Mathies [:jimm]]

From the metro enabled desktop browser doc ms produced, which explains this change - 

'Should a new experience enabled desktop browser try to launch a desktop app and have that app come to the foreground (for example, downloading and launching a file such as a PDF reader), it may do so by using ShellExecuteEx() and specifying the SEE_MASK_FLAG_LOG_USAGE flag in the fMask field. If this flag is not specified, the desktop app launches, but won’t be able to come to the foreground.   There is no automatic switching between the new experience and the desktop.'

Comment 5

[Jim Mathies [:jimm]]

Comment on attachment 827183 [details] [diff] [review]
Switches to the Desktop if the default application is not a MS App Reader

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

::: xpcom/io/nsLocalFileWin.cpp
@@ +206,5 @@
>          memset(&seinfo, 0, sizeof(seinfo));
>          seinfo.cbSize = sizeof(SHELLEXECUTEINFOW);
> +        if (XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Metro) {
> +          seinfo.fMask  = SEE_MASK_FLAG_LOG_USAGE;
> +        } else {

seinfo was zeroed out above using memset, so there's no need to set it to zero. You can remove the |else {...} | code block.

Comment 6

[Kamil Jozwiak [:kjozwiak]]

Attachment: Switches to the Desktop if the default application is not a MS App Reader (with changes)

Attached the new patch with the changes from comment #5. Also went through the test cases from comment #3 and everything worked without any issues.

Jim, would you be able to push this to the try server?

Comment 7

[Jim Mathies [:jimm]]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #6)
> Created attachment 827766 [details] [diff] [review]
> Switches to the Desktop if the default application is not a MS App Reader
> (with changes)
> 
> Attached the new patch with the changes from comment #5. Also went through
> the test cases from comment #3 and everything worked without any issues.
> 
> Jim, would you be able to push this to the try server?

Nothing on try will massage this, and the msdn page says this constant was introduced in xp, so I think you should be ok to push.

Comment 8

[Brian R. Bondy [:bbondy]]

Pushed to try just in case since nsILocalFile is used at various places still.
It passed as expected.

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4036edfda952

Thanks for the patch, one more block28 down! \o/

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4036edfda952

Comment 10

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
Build ID: 20131117030203

Verified as fixed on latest Nightly using the steps from comment 0 and comment 3. 
Tested with several PDF files of various size and got the expected results each time.