Refactoring; switch the |filter| parameter into something that is more amenab...

Refactoring; switch the |filter| parameter into something that is more amenable to cross-platform implementation.

BUG=9852
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=13573

[Avi (use Gerrit), 2009-04-08 14:53:07]

Sky- Main reviewer
Paul- FYI

Two notes:
- I'm not thrilled by the vector-within-a-vector approach, but that's the
minimum we need to allow for multiple extensions per choice. The Mac's type
system, for example, knows that .htm=.html, etc. Is Windows/Linux that smart, or
must we keep specifying all alternatives? (I'm OK with that, and that's as it's
currently written.)

- There's a curious memory corruption error that I get with my build of Chrome.
I'm not sure where it's coming from, so I'm doing a clean pull/rebuild and then
will track it down. I don't see anything with this code, so I'm sending it for
review. I won't commit it until I'm sure the memory issue isn't its fault.

[Avi (use Gerrit), 2009-04-08 14:56:27]

resend with reviewers set properly

[Avi (use Gerrit), 2009-04-08 15:19:08]

On 2009/04/08 14:53:07, Avi wrote:
> - There's a curious memory corruption error that I get with my build of
Chrome.

It's gone after a clobber, so I'm satisfied with this code now in that regard.

[sky, 2009-04-08 17:04:36]

http://codereview.chromium.org/63093/diff/1026/88
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/63093/diff/1026/88#newcode645
Line 645:
file_type_info.extensions[0].push_back(info->suggested_path.Extension());
What if the extension is empty?

http://codereview.chromium.org/63093/diff/1026/88#newcode646
Line 646: file_type_info.include_all_files = true;
Did the old path allow this?

http://codereview.chromium.org/63093/diff/1026/82
File chrome/browser/views/shell_dialogs_win.cc (right):

http://codereview.chromium.org/63093/diff/1026/82#newcode221
Line 221: : type(type), title(title), default_path(default_path),
nit: style guide says if you wrap, each variable must be on it's own line.

http://codereview.chromium.org/63093/diff/1026/82#newcode309
Line 309: ExecuteSelectParams execute_params(type, UTF16ToWide(title),
default_path,
I think it would be good to add a DCHECK on the size of
extension_description_overrides. It seems so easy to get
extension_description_overrides and extensions out of sync.

http://codereview.chromium.org/63093/diff/1026/82#newcode335
Line 335: for (size_t j=0; j<inner_exts.size(); ++j) {
nit: would be nice if this were split the inner loop into a separate function to
avoid the use of i/j, which is so easy to get wrong. Your call though.

http://codereview.chromium.org/63093/diff/1026/91
File chrome/common/win_util.cc (right):

http://codereview.chromium.org/63093/diff/1026/91#newcode229
Line 229: DCHECK(file_ext.size() == ext_desc.size());
I thought the description overrides were optional? Are they filled in some where
else if not provided?

http://codereview.chromium.org/63093/diff/1026/74
File webkit/glue/webview_delegate.h (left):

http://codereview.chromium.org/63093/diff/1026/74#oldcode566
Line 566: const std::wstring& filter,
How come we're getting rid of filter here? Are you changing something upstream?

[Avi (use Gerrit), 2009-04-08 18:08:48]

http://codereview.chromium.org/63093/diff/1026/88
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/63093/diff/1026/88#newcode645
Line 645:
file_type_info.extensions[0].push_back(info->suggested_path.Extension());
On 2009/04/08 17:04:36, sky wrote:
> What if the extension is empty?

Perfectly fine. That'll be caught by the filter building code and expanded to an
"include all files = true" just like is done by today's code.

http://codereview.chromium.org/63093/diff/1026/88#newcode646
Line 646: file_type_info.include_all_files = true;
On 2009/04/08 17:04:36, sky wrote:
> Did the old path allow this?

The old path implicitly did this. Read the old GetFileFilterFromPath. This code
change alters no behavior.

http://codereview.chromium.org/63093/diff/1026/82
File chrome/browser/views/shell_dialogs_win.cc (right):

http://codereview.chromium.org/63093/diff/1026/82#newcode221
Line 221: : type(type), title(title), default_path(default_path),
On 2009/04/08 17:04:36, sky wrote:
> nit: style guide says if you wrap, each variable must be on it's own line.

okay

http://codereview.chromium.org/63093/diff/1026/82#newcode309
Line 309: ExecuteSelectParams execute_params(type, UTF16ToWide(title),
default_path,
On 2009/04/08 17:04:36, sky wrote:
> I think it would be good to add a DCHECK on the size of
> extension_description_overrides. It seems so easy to get
> extension_description_overrides and extensions out of sync.

OK

http://codereview.chromium.org/63093/diff/1026/82#newcode335
Line 335: for (size_t j=0; j<inner_exts.size(); ++j) {
On 2009/04/08 17:04:36, sky wrote:
> nit: would be nice if this were split the inner loop into a separate function
to
> avoid the use of i/j, which is so easy to get wrong. Your call though.

I'll leave it for now.

http://codereview.chromium.org/63093/diff/1026/91
File chrome/common/win_util.cc (right):

http://codereview.chromium.org/63093/diff/1026/91#newcode229
Line 229: DCHECK(file_ext.size() == ext_desc.size());
On 2009/04/08 17:04:36, sky wrote:
> I thought the description overrides were optional? Are they filled in some
where
> else if not provided?

The *contents* of the overrides are optional, but if you choose to not use them
you still must provide a blank string to indicate this. (They're filled in on
line 244, below.) Tweaking comment to make this clearer.

http://codereview.chromium.org/63093/diff/1026/74
File webkit/glue/webview_delegate.h (left):

http://codereview.chromium.org/63093/diff/1026/74#oldcode566
Line 566: const std::wstring& filter,
On 2009/04/08 17:04:36, sky wrote:
> How come we're getting rid of filter here? Are you changing something
upstream?

Its only caller was glue/chrome_client_impl.cc (via a chain of callers), who
always passed an empty string. Since it's not used, let's just rip it out. If
it's ever needed in the future we can add it back properly.

[sky, 2009-04-08 18:15:11]

Thanks for the clarification. I'm still confused on one thing though.

http://codereview.chromium.org/63093/diff/1026/91
File chrome/common/win_util.cc (right):

http://codereview.chromium.org/63093/diff/1026/91#newcode229
Line 229: DCHECK(file_ext.size() == ext_desc.size());
On 2009/04/08 18:08:48, Avi wrote:
> On 2009/04/08 17:04:36, sky wrote:
> > I thought the description overrides were optional? Are they filled in some
> where
> > else if not provided?
> 
> The *contents* of the overrides are optional, but if you choose to not use
them
> you still must provide a blank string to indicate this. (They're filled in on
> line 244, below.) Tweaking comment to make this clearer.

I must be missing something. In bookmark manager and other places I don't see
you resizing the descriptions to be the same size as the extensions. What am I
missing?

[Avi (use Gerrit), 2009-04-08 18:40:09]

On 2009/04/08 18:15:11, sky wrote:
> In bookmark manager and other places I don't see
> you resizing the descriptions to be the same size as the extensions.

Wow. The API is broken when its own writer can't use it right. D'oh. It's now
valid to just leave the override vector empty if it's not needed.

[sky, 2009-04-08 19:30:56]

http://codereview.chromium.org/63093/diff/124/1059
File chrome/common/win_util.cc (right):

http://codereview.chromium.org/63093/diff/124/1059#newcode235
Line 235: std::wstring desc = ext_desc[i];
If that's the case, this needs to change too.

[Avi (use Gerrit), 2009-04-08 20:40:23]

This has a few new critical fixes. I found that in the last CL I'd changed the
RunFileChooser override on WebContents without changing it in the
RenderViewHostDelegate::View (damn, I miss @Override) so that broke the file
selection widget in the last review :(

This fixes that too. It all works now; please check the diff from the previous
fileset to get it all.

http://codereview.chromium.org/63093/diff/124/1059
File chrome/common/win_util.cc (right):

http://codereview.chromium.org/63093/diff/124/1059#newcode235
Line 235: std::wstring desc = ext_desc[i];
On 2009/04/08 19:30:57, sky wrote:
> If that's the case, this needs to change too.

I was going to write that he was a one trick pony, but it does make sense to
push that change all the way down the stack. The one thing you give up is the
stack trace to the caller.

[sky, 2009-04-08 21:02:42]

LGTM