Last Comment Bug 661991 - Start using the new Common Item Dialogs in Vista and later in nsFilePicker
: Start using the new Common Item Dialogs in Vista and later in nsFilePicker
Status: VERIFIED FIXED
[qa!]
: addon-compat, qawanted, verified-beta
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86 Windows 7
: -- normal with 1 vote (vote)
: mozilla11
Assigned To: Jim Mathies [:jimm]
:
: Jim Mathies [:jimm]
Mentors:
http://msdn.microsoft.com/en-us/libra...
Depends on: CVE-2012-0454 711073 711654 712571 715129 717835 718752 720071 739156
Blocks: 465198 702743 711028
  Show dependency treegraph
 
Reported: 2011-06-03 18:31 PDT by Brian R. Bondy [:bbondy]
Modified: 2013-05-08 22:11 PDT (History)
21 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
Old folder picker using SHBrowseForFolder (24.74 KB, image/png)
2011-08-08 07:39 PDT, Brian R. Bondy [:bbondy]
no flags Details
Old folder picker using SHBrowseForFolder w/ BIF_NEWDIALOGSTYLE (27.59 KB, image/png)
2011-08-08 07:40 PDT, Brian R. Bondy [:bbondy]
no flags Details
New folder picker UI by using IFileDialog with the FOS_PICKFOLDERS (84.43 KB, image/png)
2011-08-08 07:42 PDT, Brian R. Bondy [:bbondy]
faaborg: ui‑review+
Details
prelim cleanup v.1 (31.65 KB, patch)
2011-12-02 13:37 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
prelim cleanup v.1 (32.66 KB, patch)
2011-12-03 09:26 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
part1 - qi, includes, basic file events (11.31 KB, patch)
2011-12-05 02:52 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
part1 - qi, includes, basic file events (11.65 KB, patch)
2011-12-05 02:56 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
part2 - folder picker (2.77 KB, patch)
2011-12-05 11:02 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
part3 - file picker (12.05 KB, patch)
2011-12-05 11:03 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
part3 - file picker (12.11 KB, patch)
2011-12-05 11:09 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
part4 - close with parent (16.13 KB, patch)
2011-12-06 13:02 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
part6 - cifpn (9.96 KB, patch)
2011-12-07 13:05 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
part5 - cifpn (9.96 KB, patch)
2011-12-07 13:06 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
prelim cleanup v.1 (36.68 KB, patch)
2011-12-09 10:25 PST, Jim Mathies [:jimm]
ehsan: review+
neil: review-
Details | Diff | Splinter Review
part1 - qi, includes, file events (12.56 KB, patch)
2011-12-09 10:30 PST, Jim Mathies [:jimm]
ehsan: review+
Details | Diff | Splinter Review
part2 - folder picker (4.88 KB, patch)
2011-12-09 10:32 PST, Jim Mathies [:jimm]
ehsan: review+
Details | Diff | Splinter Review
part3 - file picker (12.87 KB, patch)
2011-12-09 10:33 PST, Jim Mathies [:jimm]
ehsan: review+
Details | Diff | Splinter Review
part4 - close with parent (15.26 KB, patch)
2011-12-09 10:33 PST, Jim Mathies [:jimm]
ehsan: review+
Details | Diff | Splinter Review
part5 - cifpn (9.98 KB, patch)
2011-12-09 10:34 PST, Jim Mathies [:jimm]
ehsan: review+
Details | Diff | Splinter Review
prelim cleanup v.2 (36.64 KB, patch)
2011-12-12 11:49 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
prelim cleanup v.2 (36.66 KB, patch)
2011-12-12 12:19 PST, Jim Mathies [:jimm]
neil: review+
Details | Diff | Splinter Review
rollup (66.44 KB, patch)
2011-12-14 12:44 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
clear mFiles (1.52 KB, patch)
2011-12-15 14:06 PST, Jim Mathies [:jimm]
netzen: review+
Details | Diff | Splinter Review

Description Brian R. Bondy [:bbondy] 2011-06-03 18:31:44 PDT
+++ This bug was initially created as a clone of Bug #660833 +++

We should use the new Common Item Dialog (file picker dialogs) in Vista and later.  
This code would be implemented in nsFilePicker.cpp instead of the current call to GetOpenFileNameW and GetSaveFileNameW.

From MSDN:
"Starting with Windows Vista, the Common Item Dialog supersedes the older Common File Dialog"

Documentation on the new Common Item Dialog can be found on MSDN here:
http://msdn.microsoft.com/en-us/library/bb776913%28v=vs.85%29.aspx


Steps to Reproduce:
<!DOCTYPE html>
<html>
<head>
    <title></title>
</head>
<body>
<form action='#' method='post' enctype='multipart/form-data'>
  <input size='40' name='uploads[]' type='file' multiple>
</form>
</body>
</html>

Actual Results:  
We use the old file picker dialogs in Vista and later.

Expected Results:  
We should use the new recommended file picker dialogs in Vista and later.
Comment 1 Kai Liu 2011-06-03 19:35:59 PDT
I don't see much benefit in using this.

The "Common Item" dialog does not really provide any additional functionality over the "Common File" dialog, and the file picker dialogs spawned by either method are actually the same.

The difference is in the interface.  The old OFN is a WinAPI-style interface, using simple calls to C-style functions exported by a DLL.  The new interface uses COM.  They ultimately provide interfaces to the same underlying OS component.

The biggest advantage to the new interface (and probably the reason why they created it) is that customizing the file dialogs (i.e., adding/removing custom UI elements) is more straightforward: you don't have to deal with dialog templates and message hooks like you did with the old OFN APIs (the old method of customization was so fragile that it often broke with each new version of Windows, which is why MSFT maintains different versions of the file dialog, to give programs that customized a file dialog from an older Windows an older file dialog).  But, like most programs in the Windows ecosystem, Firefox does not customize the file picker dialog (nor does it have any compelling reason to), so all this is moot (at least for now; maybe one day we might decide to jazz up the file picker).

This *might* have made bug 660833 easier to fix (I'm not 100% convinced that it would have, since it looks like we have to go manually resize the combo box's maxlength), but then that fix wouldn't work on NT5.  Which brings us to the biggest problem here: maintaining two separate code paths for NT5 and NT6 when we already have mature, working code using the legacy API.

This is something to consider when (1) we have a compelling reason to add custom UI elements to the file picker dialog and (2) XP's market share is at least an order of magnitude lower than what it is today. (I'm not going to hold my breath for that last one; maybe there will be a sharp die-off in 2014, but XP is like a cat with nine lives. :P)
Comment 2 Brian R. Bondy [:bbondy] 2011-06-03 20:03:03 PDT
Thanks for sharing Kai, I agree that most of the functionality is around customizing the dialog.  

I am fine if you want to mark this as invalid or lower priority until we have a better reason to do this task.
Comment 3 Brian R. Bondy [:bbondy] 2011-08-01 06:58:51 PDT
Bugs like Bug 629920 might be a good idea to do this task. Also Microsoft has put a notice on many pages about the old API should not be used any more.  Perhaps they know of some bugs with it.
Comment 4 Mounir Lamouri (:mounir) 2011-08-01 18:03:41 PDT
The new Vista/7 directory picker has a new UI, hasn't it? Our current directory picker on Windows is quite ugly and we could use a nicer one I believe.
Comment 5 Brian R. Bondy [:bbondy] 2011-08-02 03:52:07 PDT
I could be wrong, but I think the new dialogs are the same look & feel as we have now with the old API.  
http://msdn.microsoft.com/en-us/library/bb776913%28v=vs.85%29.aspx

I get almost the same looking dialogs on my Win7 by going to View -> Content on the list view.  The difference from the screenshot from what I have I think is just Win7 vs Vista.
Comment 6 Brian R. Bondy [:bbondy] 2011-08-02 04:01:26 PDT
The new dialogs do have:
- Ability to customize more, not sure if we have any use for that right now.
- Old API is deprecated by Microsoft, not sure if they will support the old forever.
- They do have state persistence, so it would solve Bug 465198.
Comment 7 Mounir Lamouri (:mounir) 2011-08-02 12:53:47 PDT
I'm not a Windows developer but according to http://msdn.microsoft.com/en-us/library/bb761832%28v=vs.85%29.aspx there is a FOS_PICKFOLDERS which probably means that the UI for picking a file or a folder is more or less the same (like on GTK). Which wasn't the case for Windows XP AFAIK.
Comment 8 Brian R. Bondy [:bbondy] 2011-08-03 19:09:28 PDT
I'm not sure about the look for folders, but the API is the same via that flag as you mentioned.

For folders we use: SHBrowseForFolderW 
http://msdn.microsoft.com/en-us/library/bb762115(v=vs.85).aspx
The recommended way is to use IFileDialog with the FOS_PICKFOLDERS 

For files we use: GetOpenFileNameW and GetSaveFileNameW
http://msdn.microsoft.com/en-us/library/ms646927(v=vs.85).aspx
http://msdn.microsoft.com/en-us/library/ms646928(v=vs.85).aspx
The recommended way is to use IFileOpenDialog and IFileSaveDialog (Which inherit from IFileDialog)
Comment 9 Mounir Lamouri (:mounir) 2011-08-03 21:33:46 PDT
Assuming the API is the same, I would expect the look to be the same but I might be wrong.

Though, I assume that the real issue here is that there is no way to have a code path for Windows XP and another for Windows Vista/7. Is that the case?
Comment 10 Brian R. Bondy [:bbondy] 2011-08-03 21:36:38 PDT
It would be pretty much a rewrite of the nsFilePicker code on Windows, which isn't that big.  

But yes we'd need to keep all of the old code as well, so basically 2x the code to support on Windows.

You can check if the method is available dynamically and load the methods dynamically for calling on post XP.  So I do not think that is an issue.
Comment 11 Mounir Lamouri (:mounir) 2011-08-03 21:42:54 PDT
We should do it then! :)
Comment 12 Jim Mathies [:jimm] 2011-08-04 04:25:06 PDT
(In reply to comment #11)
> We should do it then! :)

Depends on what we gain.. if it's the same dialog with a slightly updated look but the same functionality, it may not be worth it. We can migrate to this and dump the old code completely if and when we drop XP support. (five years from now maybe?)
Comment 13 Mounir Lamouri (:mounir) 2011-08-04 09:41:52 PDT
We would like to refresh the UI of the filepicker and add a few functionalities like selecting a directory but the directory picker of Windows is so bad that users might really not like that. If we have something better for Windows Vista and 7 at least, that would be great.
Comment 14 Brian R. Bondy [:bbondy] 2011-08-04 09:50:53 PDT
As a next step for this task I'll make a sample app to compare the dialogs to see exactly what visually changes if anything.  I'll probably do this early next week and I'll submit some screenshots if they differ.
Comment 15 Brian R. Bondy [:bbondy] 2011-08-08 07:39:51 PDT
Created attachment 551457 [details]
Old folder picker using SHBrowseForFolder
Comment 16 Brian R. Bondy [:bbondy] 2011-08-08 07:40:43 PDT
Created attachment 551458 [details]
Old folder picker using SHBrowseForFolder w/ BIF_NEWDIALOGSTYLE

There is also SHBrowseForFolder w/ BIF_USENEWUI, this is just the same as BIF_NEWDIALOGSTYLE but also with an edit box
Comment 17 Brian R. Bondy [:bbondy] 2011-08-08 07:42:33 PDT
Created attachment 551460 [details]
New folder picker UI by using IFileDialog with the FOS_PICKFOLDERS

Massive improvement for folder picker UI using new API.
Comment 18 Mounir Lamouri (:mounir) 2011-08-08 07:45:31 PDT
(In reply to Brian R. Bondy [:bbondy] from comment #17)
> Created attachment 551460 [details]
> New folder picker UI by using IFileDialog with the FOS_PICKFOLDERS
> 
> Massive improvement for folder picker UI using new API.

Indeed!
I believe there is no reason to argue about why we should use this API now :)
Comment 19 Brian R. Bondy [:bbondy] 2011-08-08 07:47:18 PDT
The file picker UI is exactly the same by the way.  
The folder picker UI as I mentioned is a significant advantage.  We currently use the old one with BIF_USENEWUI.
Comment 20 Jim Mathies [:jimm] 2011-08-08 07:54:09 PDT
(In reply to Mounir Lamouri (:volkmar) from comment #18)
> (In reply to Brian R. Bondy [:bbondy] from comment #17)
> > Created attachment 551460 [details]
> > New folder picker UI by using IFileDialog with the FOS_PICKFOLDERS
> > 
> > Massive improvement for folder picker UI using new API.
> 
> Indeed!
> I believe there is no reason to argue about why we should use this API now :)

I agree, the newer ui looks like a relevant win. We should seek ux approval though to be sure.
Comment 21 Jim Mathies [:jimm] 2011-08-08 08:19:31 PDT
The key issue here is that we will be implementing new code to support this ui on platforms that support it, while maintaining the old code for older platforms. So the open question is, is this a big enough win to justify the overhead.
Comment 22 Alex Faaborg [:faaborg] (Firefox UX) 2011-08-08 17:05:07 PDT
Comment on attachment 551460 [details]
New folder picker UI by using IFileDialog with the FOS_PICKFOLDERS

yep, very happy for us to get this fixed.
Comment 23 :Ehsan Akhgari 2011-08-08 21:08:18 PDT
Something to note here is that the code in question is completely untested in our automated tests, and given bugs like bug 483481 that we've had in the past, I would say that we should be very careful with changes to this code...
Comment 24 Brian R. Bondy [:bbondy] 2011-09-03 12:42:17 PDT
Another reason to use the new Common File Dialogs is the fix for Bug 660833 not being able to be used >=Vista.  See Comment 5 of Bug 684506.  Basically you can't set a hook which we need for Bug 660833 and get the new looking file picker dialogs (with search and new places bar).

As time goes on our code is starting to diverge anyway for >=Vista+ and <=XP.  As of the fix in Bug 684506 our code will start to diverge.
Comment 25 Jim Mathies [:jimm] 2011-12-02 13:37:15 PST
Created attachment 578706 [details] [diff] [review]
prelim cleanup v.1
Comment 26 Jim Mathies [:jimm] 2011-12-03 09:26:23 PST
Created attachment 578844 [details] [diff] [review]
prelim cleanup v.1
Comment 27 Jim Mathies [:jimm] 2011-12-05 02:52:02 PST
Created attachment 579016 [details] [diff] [review]
part1 - qi, includes, basic file events
Comment 28 Jim Mathies [:jimm] 2011-12-05 02:56:35 PST
Created attachment 579019 [details] [diff] [review]
part1 - qi, includes, basic file events
Comment 29 Jim Mathies [:jimm] 2011-12-05 11:02:01 PST
Created attachment 579122 [details] [diff] [review]
part2 - folder picker
Comment 30 Jim Mathies [:jimm] 2011-12-05 11:03:34 PST
Created attachment 579123 [details] [diff] [review]
part3 - file picker
Comment 31 Jim Mathies [:jimm] 2011-12-05 11:09:50 PST
Created attachment 579125 [details] [diff] [review]
part3 - file picker
Comment 32 Jim Mathies [:jimm] 2011-12-06 13:02:45 PST
Created attachment 579425 [details] [diff] [review]
part4 - close with parent
Comment 33 Jim Mathies [:jimm] 2011-12-07 13:05:30 PST
Created attachment 579807 [details] [diff] [review]
part6 - cifpn
Comment 34 Jim Mathies [:jimm] 2011-12-07 13:06:02 PST
Created attachment 579809 [details] [diff] [review]
part5 - cifpn
Comment 35 Jim Mathies [:jimm] 2011-12-08 12:10:06 PST
With a few updates these pass try fine. Unfortunately my close on parent close isn't working on XP, once I figure that out I'll post the final set and kick off reviews. I'm still confident we'll get this in before the 20th merge.
Comment 36 Jim Mathies [:jimm] 2011-12-09 10:25:33 PST
Created attachment 580457 [details] [diff] [review]
prelim cleanup v.1

Neil, this is purely code reformatting, no changes in functionality.
Comment 37 Jim Mathies [:jimm] 2011-12-09 10:26:52 PST
(In reply to Jim Mathies [:jimm] from comment #36)
> Created attachment 580457 [details] [diff] [review]
> prelim cleanup v.1
> 
> Neil, this is purely code reformatting, no changes in functionality.

(Actually, looking at it, there is some new code in the auto classes on the top of the cpp.)
Comment 38 Jim Mathies [:jimm] 2011-12-09 10:30:01 PST
Created attachment 580460 [details] [diff] [review]
part1 - qi, includes, file events

Brian, this is the basic QI set up patch. Note the elevation of some defines in the header. I prefer to avoid this if possible but in this case either I had to do it or would have had to place the header definitions in our code base. (Which is something I also try to avoid.) Thankfully there are no build problems as a result.
Comment 39 Jim Mathies [:jimm] 2011-12-09 10:32:33 PST
Created attachment 580461 [details] [diff] [review]
part2 - folder picker
Comment 40 Jim Mathies [:jimm] 2011-12-09 10:33:10 PST
Created attachment 580462 [details] [diff] [review]
part3 - file picker
Comment 41 Jim Mathies [:jimm] 2011-12-09 10:33:44 PST
Created attachment 580464 [details] [diff] [review]
part4 - close with parent
Comment 42 Jim Mathies [:jimm] 2011-12-09 10:34:26 PST
Created attachment 580465 [details] [diff] [review]
part5 - cifpn
Comment 43 :Ehsan Akhgari 2011-12-09 11:38:45 PST
Comment on attachment 580460 [details] [diff] [review]
part1 - qi, includes, file events

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

::: widget/src/windows/nsFilePicker.cpp
@@ +185,5 @@
> +STDMETHODIMP nsFilePicker::QueryInterface(REFIID refiid, void** ppvResult)
> +{
> +  *ppvResult = NULL;
> +  if (IID_IUnknown == refiid ||
> +      refiid == __uuidof(IFileDialogEvents)) {

I don't know if mingw supports __uuidof, but I won't hold this against you!
Comment 44 :Ehsan Akhgari 2011-12-09 11:45:30 PST
Comment on attachment 580461 [details] [diff] [review]
part2 - folder picker

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

::: widget/src/windows/nsFilePicker.cpp
@@ +450,5 @@
> +    return false;
> +
> +  nsRefPtr<IFileOpenDialog> dialog;
> +  if (FAILED(CoCreateInstance(CLSID_FileOpenDialog, NULL, CLSCTX_INPROC,
> +                              __uuidof(IFileOpenDialog),

Same comment about __uuidof.

@@ +459,5 @@
> +  dialog->Advise(this, &mFDECookie);
> +
> +  // options
> +  FILEOPENDIALOGOPTIONS fos = 0;
> +  fos |= FOS_PICKFOLDERS;

FILEOPENDIALOGOPTIONS fos = FOS_PICKFOLDERS;

@@ +467,5 @@
> +  dialog->SetTitle(mTitle.get());
> +  if (!aInitialDir->IsEmpty()) {
> +    nsRefPtr<IShellItem> folder;
> +    if (SUCCEEDED(SHCreateItemFromParsingName(aInitialDir->get(), NULL,
> +                                              __uuidof(IShellItem),

Ditto.

@@ +474,5 @@
> +    }
> +  }
> + 
> +  AutoDestroyTmpWindow adtw((HWND)(mParentWidget.get() ?
> +    mParentWidget->GetNativeData(NS_NATIVE_TMP_WINDOW) : NULL));

I find this not very well readable.  Can you please init the variable to NULL and then check for the condition on the next line?

@@ +493,5 @@
> +    return false;
> +  mUnicodeFile.Assign(str);
> +  CoTaskMemFree(str);
> + 
> +   return true;

Nit: indentation.

::: widget/src/windows/nsFilePicker.h
@@ +145,5 @@
>    nsCOMArray<nsILocalFile> mFiles;
>    static char            mLastUsedDirectory[];
>    nsString               mUnicodeFile;
>    static PRUnichar      *mLastUsedUnicodeDirectory;
> +  DWORD                  mFDECookie;

Please wrap this in an #ifdef MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN.
Comment 45 :Ehsan Akhgari 2011-12-09 11:56:44 PST
Comment on attachment 580462 [details] [diff] [review]
part3 - file picker

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

r=me with the comments addressed.

::: widget/src/windows/nsFilePicker.cpp
@@ +458,5 @@
>    // hook up event callbacks
>    dialog->Advise(this, &mFDECookie);
>  
>    // options
> +  FILEOPENDIALOGOPTIONS fos = FOS_PICKFOLDERS;

:)

@@ +741,5 @@
> +
> +  // options
> +
> +  FILEOPENDIALOGOPTIONS fos = 0;
> +  fos |= FOS_SHAREAWARE | FOS_OVERWRITEPROMPT | FOS_NOREADONLYRETURN;

:)

@@ +767,5 @@
> +      // Don't follow shortcuts when saving a shortcut, this can be used
> +      // to trick users (bug 271732)
> +      if (IsDefaultPathLink())
> +        fos |= FOS_NODEREFERENCELINKS;
> +      break;

Add an NS_NOTREACHED default label please?

@@ +810,5 @@
> +
> +  // display
> +
> +  AutoDestroyTmpWindow adtw((HWND)(mParentWidget.get() ?
> +    mParentWidget->GetNativeData(NS_NATIVE_TMP_WINDOW) : NULL));

Same comment as before.

@@ +991,2 @@
>  
>    //First, make sure the file name is not too long!

While you're here, can you please fix the spacing in the comments in this function?

@@ +1194,5 @@
> +    NS_WARNING("mSpecList realloc failed.");
> +    return;
> +  }
> +  COMDLG_FILTERSPEC* pSpecForward = (COMDLG_FILTERSPEC*)(((char*)mSpecList)
> +    + (size * mLength));

Why don't you cast mSpecList to COMDLG_FILTERSPEC*, and then add mLength to it?  It's really the compiler's job to do the pointer arithmetic for you.  :-)

@@ +1196,5 @@
> +  }
> +  COMDLG_FILTERSPEC* pSpecForward = (COMDLG_FILTERSPEC*)(((char*)mSpecList)
> +    + (size * mLength));
> +  memset(pSpecForward, 0, size);
> +  nsString* pStr = mStrings.AppendElement(aTitle);

AppendElement can return null.  I think you need to handle that case gracefully here.

::: widget/src/windows/nsFilePicker.h
@@ +160,5 @@
> +    ~ComDlgFilterSpec() {
> +      free(mSpecList);
> +    }
> +    
> +    PRUint32 Length() {

Make this method const please.

@@ +164,5 @@
> +    PRUint32 Length() {
> +      return mLength;
> +    }
> +
> +    const COMDLG_FILTERSPEC* get() {

Same here.
Comment 46 :Ehsan Akhgari 2011-12-09 12:03:42 PST
Comment on attachment 580465 [details] [diff] [review]
part5 - cifpn

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

r=me
Comment 47 :Ehsan Akhgari 2011-12-09 12:14:25 PST
Comment on attachment 580464 [details] [diff] [review]
part4 - close with parent

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

::: widget/src/windows/nsFilePicker.cpp
@@ +493,5 @@
> + * Close on parent close logic
> + */
> +
> +bool
> +nsFilePicker::ParentDestroyTest(bool aIsXPDialog)

Can you please choose a better name for this function?  Like, DestroyParentIfNeeded or something?

@@ +509,5 @@
> +    dlgWnd = mDlgWnd;
> +  if (IsWindow(dlgWnd) && IsWindowVisible(dlgWnd) && win->DestroyCalled()) {
> +    PRUnichar className[64];
> +    // Make sure we have the right window
> +    if (GetClassNameW(dlgWnd, className, sizeof(className)/sizeof(PRUnichar)) &&

Nit: use mozilla::ArrayLength.

::: widget/src/windows/nsFilePicker.h
@@ +138,5 @@
> +  void SetDialogHandle(HWND aWnd);
> +  bool ParentDestroyTest(bool aIsXPDialog);
> +  static void PickerCallbackTimerFunc(nsITimer *aTimer, void *aPicker);
> +  friend UINT_PTR CALLBACK MultiFilePickerHook(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam);
> +  friend UINT_PTR CALLBACK FilePickerHook(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam);

Hrm, why not just make these static?

@@ +153,5 @@
>    nsCOMArray<nsILocalFile> mFiles;
>    static char            mLastUsedDirectory[];
>    nsString               mUnicodeFile;
>    static PRUnichar      *mLastUsedUnicodeDirectory;
> +  HWND                   mDlgWnd;

I think this should also go inside the ifdef below.

@@ +174,5 @@
>      const COMDLG_FILTERSPEC* get() {
>        return mSpecList;
>      }
>      
> +    bool IsEmpty() {

Make this const please?

::: widget/src/windows/nsWindow.h
@@ +309,5 @@
>    // Open file picker tracking
>    void                    PickerOpen();
>    void                    PickerClosed();
>  
> +  bool                    DestroyCalled() { return mDestroyCalled; }

Make this const please?
Comment 48 :Ehsan Akhgari 2011-12-09 12:15:46 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #47)
> Comment on attachment 580464 [details] [diff] [review]
> part4 - close with parent
> 
> Review of attachment 580464 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/src/windows/nsFilePicker.cpp
> @@ +493,5 @@
> > + * Close on parent close logic
> > + */
> > +
> > +bool
> > +nsFilePicker::ParentDestroyTest(bool aIsXPDialog)
> 
> Can you please choose a better name for this function?  Like,
> DestroyParentIfNeeded or something?

Scratch that, use ClosePickerIfNeeded.  :)
Comment 49 :Ehsan Akhgari 2011-12-09 12:45:43 PST
Comment on attachment 580457 [details] [diff] [review]
prelim cleanup v.1

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

::: widget/src/windows/nsFilePicker.cpp
@@ +73,5 @@
> +// Manages matching SuppressBlurEvents calls on the parent widget.
> +class AutoSuppressEvents
> +{
> +public:
> +  AutoSuppressEvents(nsIWidget* aWidget) {

Make this explicit please?

@@ +122,5 @@
> +// in picker dialogs. We are responsible for destroying these.
> +class AutoDestroyTmpWindow
> +{
> +public:
> +  AutoDestroyTmpWindow(HWND aTmpWnd) :

explicit!

@@ +132,5 @@
> +    if (mWnd)
> +      DestroyWindow(mWnd);
> +  }
> +  
> +  inline HWND get() { return mWnd; }

const!

@@ +141,5 @@
> +// Manages matching PickerOpen/PickerClosed calls on the parent widget.
> +class AutoWidgetPickerState
> +{
> +public:
> +  AutoWidgetPickerState(nsIWidget* aWidget) {

ex!

@@ +150,5 @@
> +  ~AutoWidgetPickerState() {
> +    PickerState(false);
> +  }
> +private:
> +  void PickerState(bool aFlag) {

co!

@@ +319,3 @@
>    bool result = false;
> +
> +  nsAutoArrayPtr<PRUnichar> dirBuffer(new PRUnichar[FILE_BUFFER_SIZE+1]);

You don't need to allocate this dynamically, it's  better for it to be on the stack I think.

@@ +324,5 @@
> +
> +  AutoDestroyTmpWindow adtw((HWND)(mParentWidget.get() ?
> +    mParentWidget->GetNativeData(NS_NATIVE_TMP_WINDOW) : NULL));
> +
> +  BROWSEINFOW browserInfo;

I would do:

BROWSEINFOW browserInfo = {0};

@@ +379,3 @@
>  {
> +  OPENFILENAMEW ofn;
> +  memset(&ofn, 0, sizeof(ofn));

ofn = {0};

@@ +381,5 @@
> +  memset(&ofn, 0, sizeof(ofn));
> +  ofn.lStructSize = sizeof(ofn);
> +  nsString filterBuffer = mFilterList;
> +                                
> +  nsAutoArrayPtr<PRUnichar> fileBuffer(new PRUnichar[FILE_BUFFER_SIZE+1]);

This can live on the stack as well.

::: widget/src/windows/nsFilePicker.h
@@ +96,5 @@
> +  bool ShowFolderPicker(nsString* aInitialDir);
> +  void RememberLastUsedDirectory(nsCOMPtr<nsILocalFile>& file);
> +  bool IsPrivacyModeEnabled();
> +  bool IsDefaultPathLink();
> +  bool IsDefaultPathHtml();

Usual const comments apply here.  :-)
Comment 50 neil@parkwaycc.co.uk 2011-12-09 14:58:10 PST
Comment on attachment 580457 [details] [diff] [review]
prelim cleanup v.1

>+  AutoSuppressEvents(nsIWidget* aWidget) {
>+    mWidget = aWidget;
: mWidget(aWidget)

>+      nsWindow *win = static_cast<nsWindow *>(mWidget.get());
>+      win->SuppressBlurEvents(aFlag);
>+    }
>+  }
>+  nsCOMPtr<nsIWidget> mWidget;
Annoying. These should be cast to nsWindow somewhere early on.

>+  AutoDestroyTmpWindow(HWND aTmpWnd) :
>+    mWnd(NULL) {
: mWnd(aTmpWnd)
{}

>+  AutoWidgetPickerState(nsIWidget* aWidget) {
>+    mWidget = aWidget;
: mWidget(aWidget)

>+bool
>+nsFilePicker::ShowFolderPicker(nsString* aInitialDir)
> {
>+  if (!aInitialDir)
>+    return false;
Use const nsString& surely?

>+  nsAutoArrayPtr<PRUnichar> dirBuffer(new PRUnichar[FILE_BUFFER_SIZE+1]);
(Oops, FILE_BUFFER_SIZE is 4096. We _really_ don't want to allocate 4097...)

>+void
>+nsFilePicker::RememberLastUsedDirectory(nsCOMPtr<nsILocalFile>& file)
Why not nsILocalFile*?

>+  // InitWithPath() will convert UCS2 to FS path !!! corrupts unicode 
[No longer true, of course.]

>+  file->InitWithPath(mUnicodeFile);
Why are you doing this here and not in the caller?
It's unclear that the call changes the file.
Comment 51 neil@parkwaycc.co.uk 2011-12-09 15:23:05 PST
Comment on attachment 580462 [details] [diff] [review]
part3 - file picker

>-  FILEOPENDIALOGOPTIONS fos = 0;
>-  fos |= FOS_PICKFOLDERS;
>+  FILEOPENDIALOGOPTIONS fos = FOS_PICKFOLDERS;
Random cleanup of previous patches???
(Not a problem if you're landing these all in one changeset.)

>+      if (!file || NS_FAILED(file->InitWithPath(nsString(str)))) {
nsDependentString
>+        CoTaskMemFree(str);
>+        continue;
>+      }
>+      mFiles.AppendObject(file);
>+      CoTaskMemFree(str);
Also, why not write this
if (file && NS_SUCCEEDED(file->InitWithPath(nsDependentString(str)))
  mFiles.AppendObject(file);
CoTaskMemFree(str);

>+  PRUint32 size = sizeof(COMDLG_FILTERSPEC);
>+  PRUint32 hdrLen = size * (mLength + 1);
>+  mSpecList = (COMDLG_FILTERSPEC*)realloc(mSpecList, hdrLen);
>+  if (!mSpecList) {
>+    NS_WARNING("mSpecList realloc failed.");
>+    return;
>+  }
>+  COMDLG_FILTERSPEC* pSpecForward = (COMDLG_FILTERSPEC*)(((char*)mSpecList)
>+    + (size * mLength));
This looks very much like you're reimplementing an nsTArray...
Comment 52 neil@parkwaycc.co.uk 2011-12-09 15:49:33 PST
The Vista/7 folder and file picker code looks quite similar;
is there any way to share code somehow?
Comment 53 Jim Mathies [:jimm] 2011-12-09 15:56:38 PST
(In reply to neil@parkwaycc.co.uk from comment #52)
> The Vista/7 folder and file picker code looks quite similar;
> is there any way to share code somehow?

Pretty much every step of the way there are variances in what you have to do. To blend them you would need to add a lot of 

if (newpickers) {
// do this
} else {
// do that
}

I felt it would add complexity to code that is already pretty crazy to start with, so I kept them separate.
Comment 54 Jim Mathies [:jimm] 2011-12-09 16:01:13 PST
Looking over the rest of the review comments, don't see anything I have any issues with. Will update over the weekend and post a new set on Monday.
Comment 55 Jim Mathies [:jimm] 2011-12-12 09:09:13 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #49)
> Comment on attachment 580457 [details] [diff] [review]
> prelim cleanup v.1
> 
> Review of attachment 580457 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +319,3 @@
> >    bool result = false;
> > +
> > +  nsAutoArrayPtr<PRUnichar> dirBuffer(new PRUnichar[FILE_BUFFER_SIZE+1]);
> 
> You don't need to allocate this dynamically, it's  better for it to be on
> the stack I think.
> 

One nit - this buffer can be freed in the callbacks and reallocated, which is why we initialize it this way.
Comment 56 Jim Mathies [:jimm] 2011-12-12 09:11:47 PST
comment in the code on the above:

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsFilePicker.cpp#391
Comment 57 Jim Mathies [:jimm] 2011-12-12 10:02:39 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #44)
> Comment on attachment 580461 [details] [diff] [review]
> part2 - folder picker
> 
> @@ +474,5 @@
> > +    }
> > +  }
> > + 
> > +  AutoDestroyTmpWindow adtw((HWND)(mParentWidget.get() ?
> > +    mParentWidget->GetNativeData(NS_NATIVE_TMP_WINDOW) : NULL));
> 
> I find this not very well readable.  Can you please init the variable to
> NULL and then check for the condition on the next line?

Not sure what to do here that's cleaner looking. For example:

HWND hwnd = NULL;
if (mParentWidget.get())
  hwnd = mParentWidget->GetNativeData(NS_NATIVE_TMP_WINDOW);
AutoDestroyTmpWindow adtw(hwnd);

But that doesn't seem as nice as the single line statement I currently have.

> ::: widget/src/windows/nsFilePicker.h
> @@ +145,5 @@
> >    nsCOMArray<nsILocalFile> mFiles;
> >    static char            mLastUsedDirectory[];
> >    nsString               mUnicodeFile;
> >    static PRUnichar      *mLastUsedUnicodeDirectory;
> > +  DWORD                  mFDECookie;
> 
> Please wrap this in an #ifdef MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN.

This gets updated in one of the patches farther up in the queue.
Comment 58 Jim Mathies [:jimm] 2011-12-12 11:49:15 PST
Created attachment 580992 [details] [diff] [review]
prelim cleanup v.2
Comment 59 Jim Mathies [:jimm] 2011-12-12 12:19:08 PST
Created attachment 581003 [details] [diff] [review]
prelim cleanup v.2

resubmitting the first patch for review due to the previous r-.
Comment 60 Jim Mathies [:jimm] 2011-12-13 10:34:12 PST
Landing this on inbound is on hold until the pgo build problem (bug 709193) gets worked out.
Comment 61 Jim Mathies [:jimm] 2011-12-14 12:44:41 PST
Created attachment 581753 [details] [diff] [review]
rollup
Comment 63 neil@parkwaycc.co.uk 2011-12-15 07:26:40 PST
The Vista stuff doesn't compile on the Vista SDK - ShObjIdl.h needs NTDDI_VERSION set to NTDDI_LONGHORN or better, but only MOZ_WINSDK_TARGETVER is set, so we try to compile against interfaces that haven't been defined...

Also the Vista SDK doesn't define FILEOPENDIALOGOPTIONS; newer SDKs typedef it as equivalent to DWORD.

And you don't seem to have spelled Shell correctly in one of the patches.
Comment 64 Jim Mathies [:jimm] 2011-12-15 07:58:12 PST
(In reply to neil@parkwaycc.co.uk from comment #63)
> The Vista stuff doesn't compile on the Vista SDK - ShObjIdl.h needs
> NTDDI_VERSION set to NTDDI_LONGHORN or better, but only MOZ_WINSDK_TARGETVER
> is set, so we try to compile against interfaces that haven't been defined...
> 
> Also the Vista SDK doesn't define FILEOPENDIALOGOPTIONS; newer SDKs typedef
> it as equivalent to DWORD.
> 
> And you don't seem to have spelled Shell correctly in one of the patches.

filed bug 711073.
Comment 65 neil@parkwaycc.co.uk 2011-12-15 08:09:18 PST
Aha, it's because the Vista SDK doesn't define _WIN32_WINNT_VISTA...
Comment 66 Jim Mathies [:jimm] 2011-12-15 14:06:24 PST
Created attachment 582105 [details] [diff] [review]
clear mFiles

Minor fix for the vista & up code path, I reintroduced bug 665987. I went ahead and moved this down with the single file truncate call, which seemed better.
Comment 67 Brian R. Bondy [:bbondy] 2011-12-15 19:56:28 PST
Comment on attachment 582105 [details] [diff] [review]
clear mFiles

Looks good!
Comment 68 Jim Mathies [:jimm] 2011-12-16 11:28:46 PST
(In reply to Brian R. Bondy [:bbondy] from comment #67)
> Comment on attachment 582105 [details] [diff] [review]
> clear mFiles
> 
> Looks good!

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c4be303da94
Comment 69 Matt Brubeck (:mbrubeck) 2011-12-17 09:08:30 PST
https://hg.mozilla.org/mozilla-central/rev/7c4be303da94
Comment 70 Alex Keybl [:akeybl] 2012-01-06 12:57:11 PST
We've now uplifted bug 712571 and bug 715129 to Aurora, both regressions from this bug. I'm adding the qawanted keyword here to do some exploratory testing around the file picker, and try to find any other regressions while 11 is still on Aurora.
Comment 71 grebensown 2012-01-12 18:45:00 PST
Dialog windows begun to not open for me since 12.15.2011 Nightly update. 
OS: Win 7
Comment 72 Jim Mathies [:jimm] 2012-01-13 03:16:24 PST
(In reply to grebensown from comment #71)
> Dialog windows begun to not open for me since 12.15.2011 Nightly update. 
> OS: Win 7

Can you post steps to reproduce?
Comment 73 Brian R. Bondy [:bbondy] 2012-01-13 04:41:51 PST
Maybe we could fall back to the old dialog on error since we need to have that other code working anyway for the XP case?  (I'd prefer to try to find out what the issue is here though for grebensown@hotmail.com first).

By the way it works great for me, thanks for doing this :D
Comment 74 Jim Mathies [:jimm] 2012-01-13 05:00:03 PST
(In reply to Brian R. Bondy [:bbondy] from comment #73)
> Maybe we could fall back to the old dialog on error since we need to have
> that other code working anyway for the XP case?  (I'd prefer to try to find
> out what the issue is here though for grebensown@hotmail.com first).

I'm not going to fall back since at some point we want the old code to go away.

My best guess on grebensown's problem is that it might be caused by an invalid path passed as a parameter, and the newer dialogs react to this differently. bug 717835 been filed on this, hopefully it will get fleshed out. Whatever the cause is it doesn't appear to be a common problem.
 
> 
> By the way it works great for me, thanks for doing this :D

thx!
Comment 75 Brian R. Bondy [:bbondy] 2012-01-13 05:16:30 PST
> I'm not going to fall back since at some point we want the old code to go away.

I thought maybe only temporarily, you could get telemetry data to see the number of times opened vs the number of times failed to open so you don't lose visibility you get from bug reports.  But I think you're right this probably isn't worth it and its better to just fix anything that comes up.
Comment 76 Stefan Sitter 2012-01-17 12:21:44 PST
I think this change might have regressed Bug 718752 too.
Comment 77 Trif Andrei-Alin[:AlinT] 2012-02-10 08:14:21 PST
QA Verified on:
 Mozilla/5.0 (Windows NT 6.1; rv:11.0) Gecko/20100101 Firefox/11.0
Comment 78 Eric Shepherd [:sheppy] 2012-03-24 06:07:00 PDT
This doesn't appear to involve any API changes, correct? It's just the underlying code using a new API to present the file pickers?
Comment 79 Mounir Lamouri (:mounir) 2012-03-25 17:09:34 PDT
(In reply to Eric Shepherd [:sheppy] from comment #78)
> This doesn't appear to involve any API changes, correct? It's just the
> underlying code using a new API to present the file pickers?

Exact. I'm not sure why "addon-compat" has been set also.

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