Closed Bug 661991 Opened 13 years ago Closed 13 years ago

Start using the new Common Item Dialogs in Vista and later in nsFilePicker

Categories

(Core :: Widget: Win32, defect)

Product:
Core
Component:
Widget: Win32
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla11
Tracking Flags:
Tracking Status
firefox11 --- verified

People

(Reporter: bbondy, Assigned: jimm)

References

(
URL
)

Details

(Keywords: addon-compat, qawanted, verified-beta, Whiteboard: [qa!])

Attachments

(11 files, 12 obsolete files)

Old folder picker using SHBrowseForFolder
24.74 KB, image/png
Details
Old folder picker using SHBrowseForFolder w/ BIF_NEWDIALOGSTYLE
27.59 KB, image/png
Details
New folder picker UI by using IFileDialog with the FOS_PICKFOLDERS
84.43 KB, image/png
faaborg
: ui-review+
Details
part1 - qi, includes, file events
12.56 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
part2 - folder picker
4.88 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
part3 - file picker
12.87 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
part4 - close with parent
15.26 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
part5 - cifpn
9.98 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
prelim cleanup v.2
36.66 KB, patch
neil
: review+
Details | Diff | Splinter Review
rollup
66.44 KB, patch
Details | Diff | Splinter Review
clear mFiles
1.52 KB, patch
bbondy
: review+
Details | Diff | Splinter Review 
+++ 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.
Assignee: nobody → netzen
Summary: Forms with <input type='file' multiple> does not accept more than 253 files at once. → Start using the new Common Item Dialogs in Vista and later in nsFilePicker
No longer depends on: 660833
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)
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.
Assignee: netzen → nobody
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.
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.
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.
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.
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.
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)
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?
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.
We should do it then! :)
(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?)
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.
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.
There is also SHBrowseForFolder w/ BIF_USENEWUI, this is just the same as BIF_NEWDIALOGSTYLE but also with an edit box
Massive improvement for folder picker UI using new API.
(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 :)
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.
(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.
Assignee: nobody → netzen
Attachment #551460 - Flags: ui-review?(faaborg)
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 on attachment 551460 [details]
New folder picker UI by using IFileDialog with the FOS_PICKFOLDERS

yep, very happy for us to get this fixed.
Attachment #551460 - Flags: ui-review?(faaborg) → ui-review+
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...
Blocks: 465198
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.
Blocks: 702743
Assignee: netzen → jmathies
Depends on: CVE-2012-0454
Attached patch prelim cleanup v.1 (obsolete) — — Splinter Review 

    
    

    
  

      
      
      
      

        Attached patch
          
          
        prelim cleanup v.1 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #578706 -
        Attachment is obsolete: true

    
    

    
  


  

    
    

    
  


  

        Attachment #579016 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part2 - folder picker (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part3 - file picker (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part3 - file picker (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #579123 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part4 - close with parent (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part6 - cifpn (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part5 - cifpn (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #579807 -
        Attachment is obsolete: true

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        prelim cleanup v.1 (obsolete)
        — 
          — Splinter Review
      

    


  
Neil, this is purely code reformatting, no changes in functionality.

        Attachment #578844 -
        Attachment is obsolete: true

        Attachment #579019 -
        Attachment is obsolete: true

        Attachment #579122 -
        Attachment is obsolete: true

        Attachment #579125 -
        Attachment is obsolete: true

        Attachment #579425 -
        Attachment is obsolete: true

        Attachment #579809 -
        Attachment is obsolete: true

        Attachment #580457 -
        Flags: review?(neil)

    
    

    
  
(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.)

    
    

    
  


  
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.

        Attachment #580460 -
        Flags: review?(netzen)

    
    

    
  


  

    
    

    
  


  

    
    

    
  


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part5 - cifpn
        — 
          — Splinter Review
      

    


  

    
    

    
  
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!

        Attachment #580460 -
        Flags: review?(netzen) → review+

    
    

    
  
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.

        Attachment #580461 -
        Flags: review+

    
    

    
  
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.

        Attachment #580462 -
        Flags: review+

    
    

    
  
Comment on attachment 580465 [details] [diff] [review]
part5 - cifpn

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

r=me

        Attachment #580465 -
        Flags: review+

    
    

    
  
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?

        Attachment #580464 -
        Flags: review+

    
    

    
  
(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 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.  :-)

        Attachment #580457 -
        Flags: review?(neil) → review+

    
    

    
  
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.

        Attachment #580457 -
        Flags: review-

    
    

    
  
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...

    
    

    
  
The Vista/7 folder and file picker code looks quite similar;
is there any way to share code somehow?

    
    

    
  
(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.

    
    

    
  
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.

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        prelim cleanup v.2 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #580457 -
        Attachment is obsolete: true

    
    

    
  


  
resubmitting the first patch for review due to the previous r-.

        Attachment #580992 -
        Attachment is obsolete: true

        Attachment #581003 -
        Flags: review?(neil)

    
  

        Attachment #581003 -
        Flags: review?(neil) → review+

    
    

    
  
Landing this on inbound is on hold until the pgo build problem (bug 709193) gets worked out.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        rollup
        — 
          — Splinter Review
      

    


  

    
  

          status-firefox11:
          --- → fixed

    
  
Blocks: 711028

    
    

    
  
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.

    
  
Depends on: 711073

    
    

    
  
(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.

    
    

    
  
Aha, it's because the Vista SDK doesn't define _WIN32_WINNT_VISTA...

    
    

    
  

      
      
      
      

        Attached patch
          
          
        clear mFiles
        — 
          — Splinter Review
      

    


  
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.

        Attachment #582105 -
        Flags: review?(netzen)

    
    

    
  
Comment on attachment 582105 [details] [diff] [review]
clear mFiles

Looks good!

        Attachment #582105 -
        Flags: review?(netzen) → review+

    
    

    
  
(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

    
  
Depends on: 711654

    
  
Depends on: 712571

    
  
Depends on: 715129

    
    

    
  
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.
Keywords: qawanted

    
    

    
  
Dialog windows begun to not open for me since 12.15.2011 Nightly update. 
OS: Win 7

    
    

    
  
(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?

    
  
Depends on: 717835

    
    

    
  
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

    
    

    
  
(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!

    
    

    
  
> 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.

    
    

    
  
I think this change might have regressed Bug 718752 too.
Depends on: 718752
Depends on: 720071

    
  
Whiteboard: [qa+]

    
    

    
  
QA Verified on:
 Mozilla/5.0 (Windows NT 6.1; rv:11.0) Gecko/20100101 Firefox/11.0
Status: RESOLVED → VERIFIED

          status-firefox11:
          fixed → verified
Keywords: verified-beta
Whiteboard: [qa+] → [qa!]

    
    

    
  
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?

    
    

    
  
(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.
Keywords: dev-doc-needed
Depends on: 739156

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: