Start consolidating cross-port file selection code into ui/base/dialogs.

Start consolidating cross-port file selection code into ui/base/dialogs.

Part 2: Split the chrome specific policy code into a chrome class that
implements a general interface. In the next patch, we'll actually move SelectFileDialog to ui/base/dialogs/.

BUG=134529
TEST=none


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

[sky, 2012-06-26 00:07:18]

https://chromiumcodereview.appspot.com/10667026/diff/1/chrome/browser/chrome_...
File chrome/browser/chrome_select_file_policy.cc (right):

https://chromiumcodereview.appspot.com/10667026/diff/1/chrome/browser/chrome_...
chrome/browser/chrome_select_file_policy.cc:47: TabContents* tab_contents =
TabContents::FromWebContents(source_contents);
Can't this return NULL?

https://chromiumcodereview.appspot.com/10667026/diff/1/chrome/browser/chrome_...
File chrome/browser/chrome_select_file_policy.h (right):

https://chromiumcodereview.appspot.com/10667026/diff/1/chrome/browser/chrome_...
chrome/browser/chrome_select_file_policy.h:17: class ChromeSelectFilePolicy :
public ui::SelectFilePolicy {
If this extends a class in ui, shouldn't it live in chrome/browser/ui ?

https://chromiumcodereview.appspot.com/10667026/diff/1/chrome/browser/chrome_...
chrome/browser/chrome_select_file_policy.h:35: };
DISALLOW_COPY_AND_ASSIGN

https://chromiumcodereview.appspot.com/10667026/diff/1/chrome/browser/chrome_...
File chrome/browser/chrome_select_file_policy_unittest.cc (right):

https://chromiumcodereview.appspot.com/10667026/diff/1/chrome/browser/chrome_...
chrome/browser/chrome_select_file_policy_unittest.cc:12: #include
"chrome/browser/chrome_select_file_policy.h"
test include order should match that of .cc include. So, this should be the
first include.

https://chromiumcodereview.appspot.com/10667026/diff/1/chrome/browser/ui/sele...
File chrome/browser/ui/select_file_dialog.cc (right):

https://chromiumcodereview.appspot.com/10667026/diff/1/chrome/browser/ui/sele...
chrome/browser/ui/select_file_dialog.cc:18: void
SelectFileDialog::SetFilePolicy(ui::SelectFilePolicy* policy) {
How come this is static and not per instance?

https://chromiumcodereview.appspot.com/10667026/diff/1/ui/ui.gyp
File ui/ui.gyp (right):

https://chromiumcodereview.appspot.com/10667026/diff/1/ui/ui.gyp#newcode109
ui/ui.gyp:109: 'base/cursor/cursor_x11.cc',
Add select_file_policy too.

[Elliot Glaysher, 2012-06-28 20:09:52]

First version for review.

Based on sky's comments, I changed every callsite to SelectFileDialog::Create()
so it takes the policy instead of setting it as a client class.

https://chromiumcodereview.appspot.com/10667026/diff/1/chrome/browser/chrome_...
File chrome/browser/chrome_select_file_policy.cc (right):

https://chromiumcodereview.appspot.com/10667026/diff/1/chrome/browser/chrome_...
chrome/browser/chrome_select_file_policy.cc:47: TabContents* tab_contents =
TabContents::FromWebContents(source_contents);
On 2012/06/26 00:07:18, sky wrote:
> Can't this return NULL?

It can return NULL only if |source_contents| is invalid. This usage is OK
because the Closure isn't stored and is used down stack of where we have to have
a valid WebContents.

Still, I've added a DCHECK().

[sky, 2012-06-28 21:01:40]

LGTM

https://chromiumcodereview.appspot.com/10667026/diff/37003/chrome/browser/ui/...
File chrome/browser/ui/chrome_select_file_policy.h (right):

https://chromiumcodereview.appspot.com/10667026/diff/37003/chrome/browser/ui/...
chrome/browser/ui/chrome_select_file_policy.h:20: explicit
ChromeSelectFilePolicy(content::WebContents* source_contents);
Document ownership.

[commit-bot: I haz the power, 2012-06-28 21:22:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/10667026/36004

[commit-bot: I haz the power, 2012-06-28 21:22:54]

Presubmit check for 10667026-36004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  chrome/browser/chromeos/extensions/file_browser_handler_api.cc:61
    virtual void set_function_for_test( \
  chrome/browser/chromeos/extensions/file_browser_handler_api.cc:317
    result->set_function_for_test(this);

** Presubmit Warnings **
New code should not use wstrings.  If you are calling an API that accepts a
wstring, fix the API.
    chrome/browser/ui/views/user_data_dir_dialog_view.cc:65

Presubmit checks took 2.6s to calculate.

[Elliot Glaysher, 2012-06-28 23:05:26]

On 2012/06/28 21:22:54, I haz the power (commit-bot) wrote:
> Presubmit check for 10667026-36004 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit Messages **
> You might be calling functions intended only for testing from
> production code.  It is OK to ignore this warning if you know what
> you are doing, as the heuristics used to detect the situation are
> not perfect.  The commit queue will not block on this warning.
> Email mailto:joi@chromium.org if you have questions.
>   chrome/browser/chromeos/extensions/file_browser_handler_api.cc:61
>     virtual void set_function_for_test( \
>   chrome/browser/chromeos/extensions/file_browser_handler_api.cc:317
>     result->set_function_for_test(this);
> 
> ** Presubmit Warnings **
> New code should not use wstrings.  If you are calling an API that accepts a
> wstring, fix the API.
>     chrome/browser/ui/views/user_data_dir_dialog_view.cc:65
> 
> Presubmit checks took 2.6s to calculate.

Adding NOTRY= as I have trybots and the CQ is broken for blocking on a Message.

[commit-bot: I haz the power, 2012-06-28 23:05:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/10667026/33002

[commit-bot: I haz the power, 2012-06-28 23:05:59]

Presubmit check for 10667026-33002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  chrome/browser/chromeos/extensions/file_browser_handler_api.cc:61
    virtual void set_function_for_test( \
  chrome/browser/chromeos/extensions/file_browser_handler_api.cc:317
    result->set_function_for_test(this);

** Presubmit Warnings **
New code should not use wstrings.  If you are calling an API that accepts a
wstring, fix the API.
    chrome/browser/ui/views/user_data_dir_dialog_view.cc:66

Presubmit checks took 2.6s to calculate.

[commit-bot: I haz the power, 2012-06-28 23:19:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/10667026/23012

[commit-bot: I haz the power, 2012-06-29 00:18:16]

Try job failure for 10667026-23012 (retry) (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-06-29 16:34:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/10667026/48002

[commit-bot: I haz the power, 2012-06-29 18:35:15]

Change committed as 144938