Last Comment Bug 715129 - fp.appendFilters(nsIFilePicker.filterApps) does not show ".exe" file in the file picker
: fp.appendFilters(nsIFilePicker.filterApps) does not show ".exe" file in the f...
Status: VERIFIED FIXED
[qa!]
: regression
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: 11 Branch
: x86 Windows 7
: -- normal (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
:
:
Mentors:
Depends on:
Blocks: 661991
  Show dependency treegraph
 
Reported: 2012-01-04 07:10 PST by Alice0775 White
Modified: 2012-02-02 09:50 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
Proposed patch (2.67 KB, patch)
2012-01-04 07:56 PST, neil@parkwaycc.co.uk
jmathies: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Alice0775 White 2012-01-04 07:10:08 PST
Build Identifier:
http://hg.mozilla.org/releases/mozilla-aurora/rev/e2fe885dce01
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a2) Gecko/20120102 Firefox/11.0a2 ID:20120102042006

fp.appendFilters(nsIFilePicker.filterApps) does not show ".exe" file in the file picker

Reproducible: Always

Steps to Reproduce:
1. Start Firefox with clean profile
2. Open Error Console (Ctrl+Shift+J)
3. Paste the following code

const nsIFilePicker = Components.interfaces.nsIFilePicker;
var fp = Components.classes["@mozilla.org/filepicker;1"]
	           .createInstance(nsIFilePicker);
fp.init(window, "Dialog Title", nsIFilePicker.modeOpen);
fp.appendFilters(nsIFilePicker.filterApps);
var rv = fp.show();

4. Click Evaluate
5. Go to folder which contain ".exe" file (Ex. "C:\Program Files (x86)\Mozilla Firefox")
   and Try to select any ".exe" file

Actual Results:
  No ".exe" file is listed in the file picker

Expected Results:
  ".exe" file should be listed in the file picker


Regression window
Works:
http://hg.mozilla.org/mozilla-central/rev/d508455660d3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111214 Firefox/11.0a1 ID:20111214125611
Fails:
http://hg.mozilla.org/mozilla-central/rev/5131c0b1982f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111214 Firefox/11.0a1 ID:20111214132414
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d508455660d3&tochange=5131c0b1982f

Suspected : Bug 661991
Comment 1 neil@parkwaycc.co.uk 2012-01-04 07:56:31 PST
Created attachment 585758 [details] [diff] [review]
Proposed patch
Comment 2 Jim Mathies [:jimm] 2012-01-04 08:53:38 PST
Comment on attachment 585758 [details] [diff] [review]
Proposed patch

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

::: widget/src/windows/nsFilePicker.cpp
@@ +1356,5 @@
> +  else {
> +    pStr->StripWhitespace();
> +    if (pStr->EqualsLiteral("*"))
> +      pStr->AppendLiteral(".*");
> +  }

In the second part you're missing the initial assignment - 

pStr->Assign(aFilter);
pStr->StripWhitespace();
..


r+ with this addressed.
Comment 3 neil@parkwaycc.co.uk 2012-01-04 09:42:08 PST
Comment on attachment 585758 [details] [diff] [review]
Proposed patch

>-  pStr = mStrings.AppendElement(aFilter);
>+  pStr = mStrings.AppendElement();
Or I could alternatively revert this change. Which would you prefer?
Comment 4 Jim Mathies [:jimm] 2012-01-04 09:56:20 PST
(In reply to neil@parkwaycc.co.uk from comment #3)
> Comment on attachment 585758 [details] [diff] [review]
> Proposed patch
> 
> >-  pStr = mStrings.AppendElement(aFilter);
> >+  pStr = mStrings.AppendElement();
> Or I could alternatively revert this change. Which would you prefer?

Thought we might still get the '..apps' in the drop down here, but we over write the value, so yeah this is fine by me.
Comment 5 Jim Mathies [:jimm] 2012-01-04 17:09:29 PST
Comment on attachment 585758 [details] [diff] [review]
Proposed patch

https://hg.mozilla.org/mozilla-central/rev/3ec9d6539335
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-01-05 01:57:00 PST
Comment on attachment 585758 [details] [diff] [review]
Proposed patch

> nsFilePicker::ComDlgFilterSpec::Append(const nsAString& aTitle, const nsAString& aFilter)
> {
>-  PRUint32 size = sizeof(COMDLG_FILTERSPEC);
>-  PRUint32 hdrLen = size * (mLength + 1);
>-  mSpecList = (COMDLG_FILTERSPEC*)realloc(mSpecList, hdrLen);
>-  if (!mSpecList) {
>+  COMDLG_FILTERSPEC* pSpecForward = mSpecList.AppendElement();
>+  if (!pSpecForward) {
>     NS_WARNING("mSpecList realloc failed.");
>     return;
>   }

This is infallible; if it shouldn't be, the declaration should use an explicitly fallible nsAutoTArray
Comment 7 neil@parkwaycc.co.uk 2012-01-05 11:57:41 PST
(In reply to Ms2ger from comment #6)
> This is infallible
Sorry, I was copying the style of the rest of the function, which also checks.
Comment 8 Alex Keybl [:akeybl] 2012-01-06 12:56:31 PST
Comment on attachment 585758 [details] [diff] [review]
Proposed patch

[Triage Comment]
Approving for Aurora along with bug 712571. As noted there, it would be higher risk to back out all or part of 661991.
Comment 9 Jim Mathies [:jimm] 2012-01-09 02:54:06 PST
Neil, would you like me to land 3ec9d6539335 on Aurora?
Comment 10 neil@parkwaycc.co.uk 2012-01-09 05:32:17 PST
Yes please, I don't have an aurora tree.
Comment 12 Ioana (away) 2012-02-02 09:50:37 PST
Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:11.0) Gecko/20100101 Firefox/11.0

The .exe files are displayed in the file picker.

Note You need to log in before you can comment on or make changes to this bug.