Closed Bug 504385 Opened 11 years ago Closed 10 years ago

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

Categories

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

ARM
Windows CE
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: zpao, Assigned: zpao)

References

()

Details

(Whiteboard: [nv])

Attachments

(1 file, 2 obsolete files)

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
Priority: -- → P4
Whiteboard: [nv]
Version: unspecified → Trunk
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)
Attached patch Patch v0.1 (WIP) (obsolete) — Splinter Review
#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.
Assignee: nobody → paul
Status: NEW → ASSIGNED
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)
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.
Attached patch Patch v0.2 (obsolete) — Splinter Review
The simple solution. We still have the problem I mentioned in comment #4, but at least the dialog works now.
Attachment #390360 - Attachment is obsolete: true
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.
Attached patch Patch v0.3Splinter Review
With change as promised in comment #6. Passes tests now.
Attachment #391474 - Attachment is obsolete: true
Attachment #392365 - Flags: review?(vladimir)
pushed http://hg.mozilla.org/mozilla-central/rev/4784832ab52b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.