504385 - "Save Link As..." displays broken file picker when file type is unknown on Windows CE

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect, P4)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

HTML pages, images seem to be fine, but unknown types (tested .mov) aren't.

STR:
1. Go to a page that has file links. I happened to be testing movie trailers.
2. Try to right click & "Save file as..."

Results:
See a broken file picker

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

So it looks like specifically this is happening when the nsMIMEInfo object doesn't have a description set. I'm pretty sure this is *supposed* to be being pulled from the registry, but for some reason it's bailing out before that happens. So we fall back to a static list of mimetype descriptions http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#507).

And then the file dialog chokes because it has a bad parameter.

So right now you can only download files that have a mime type in that list... (I think)

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1 (WIP)

#ifdef's some stuff that doesn't work on CE (system variables are a no go).

Not sure if this is entirely the correct approach, but we still want an nsMIMEInfo object returned instead of the NS_ERROR case.

This *might* be doable by just setting a bogus mime description instead of that + returning a null file.

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

So the minimum needed to make this work is just the changes in nsExternalHelperAppService.cpp. It makes the file picker usable again, but still doesn't make it quite right. Even when the file type is in the registry with a description (eg .mov ends up picking up "Video File"), ExpandEnvironmentStringsW is failing and so we bailing out happens up the chain and we through the registry value away.

Specifically, ExpandEnvironmentStringsW is not handling parameters well (I think). Eg, "ceplayer.exe %1" should be fine, but since the % doesn't have a matching % it returns -1.

So I think I might spin that out & take the "simple" solution here (which still should be localized)

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Even with the fix here and in bug 506394, there's still another problem. This comes about when the registry returns a reference to a file path that doesn't exist. For example, keeping with. mov, [HKEY_CLASSES_ROOT\videofile\shell\open\command] references ceplayer.exe, which we then try to open to get app info out (http://hg.mozilla.org/mozilla-central/file/58d6884a3401/uriloader/exthandler/win/nsOSHelperAppService.cpp#l488)

So the dialog works, but since we return an error at above, we end up throwing away the mime info we got from the registry. So we end up falling back to the default "File" description in this patch.

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.2

The simple solution. We still have the problem I mentioned in comment #4, but at least the dialog works now.

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 391474 [details] [diff] [review]
Patch v0.2

>+    if (NS_FAILED(rv)) {
should be
>+    if (NS_FAILED(rv) && !aFileExt.IsEmpty()) {
if this is going to pass tests.

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.3

With change as promised in comment #6. Passes tests now.

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

pushed http://hg.mozilla.org/mozilla-central/rev/4784832ab52b

Comment 9

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Windows; U; WindowsCE 6.0; en-US; rv:1.9.2a1pre)
Gecko/20090806 Minefield/3.6a1pre.  Bug 506491 is happening on the tegra builds as well, but that's tracked over there.