Fix open dialog not remembering last opened folder on drive

Fix open dialog not remembering last opened folder on drive 

The problem is dialog passes file's local cache path in 
FileSelected. Last selected directory paths get set to cache 
dir, which file manager does not understand, so it opens 
default dir. In order to properly remember last selected 
dir, we should also pass file's drive path to dialog 
listeners and use it to remember last selected dir. 


BUG=126923
TEST=manual

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

[tbarzic, 2012-07-24 17:11:55]

Please, take a look:
achuith: chromeos part
asanka: downloads
sky: ui/*, chrome/browser/ui/*
jam: content/*

[achuithb, 2012-07-24 20:13:05]

http://codereview.chromium.org/10804026/diff/15001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10804026/diff/15001/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:1767: FilePath path = file_info.real_path;
nit: const FilePath& would save a copy.

http://codereview.chromium.org/10804026/diff/15001/chrome/browser/ui/views/se...
File chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc
(right):

http://codereview.chromium.org/10804026/diff/15001/chrome/browser/ui/views/se...
chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc:49: OVERRIDE
{
nit: don't think you're supposed to break before OVERRIDE. Maybe break after (

http://codereview.chromium.org/10804026/diff/15001/chrome/browser/ui/views/se...
File chrome/browser/ui/views/select_file_dialog_extension_unittest.cc (right):

http://codereview.chromium.org/10804026/diff/15001/chrome/browser/ui/views/se...
chrome/browser/ui/views/select_file_dialog_extension_unittest.cc:56:
ui::SelectedFileInfo file_info = ui::SelectedFileInfo(FilePath(), FilePath());
Why is this necessary? It seems like the default ctor does the right thing?

http://codereview.chromium.org/10804026/diff/15001/ui/base/dialogs/selected_f...
File ui/base/dialogs/selected_file_info.h (right):

http://codereview.chromium.org/10804026/diff/15001/ui/base/dialogs/selected_f...
ui/base/dialogs/selected_file_info.h:18: FilePath file_path;
Maybe add a comment here? What does file_path look like?

What's the difference between file_path and real_path?

[jam, 2012-07-24 20:30:56]

content changes lgtm (didn't look at entire change, just those three files)

[tbarzic, 2012-07-25 02:02:54]

http://codereview.chromium.org/10804026/diff/15001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10804026/diff/15001/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:1767: FilePath path = file_info.real_path;
On 2012/07/24 20:13:05, achuith.bhandarkar wrote:
> nit: const FilePath& would save a copy.

Done.

http://codereview.chromium.org/10804026/diff/15001/chrome/browser/ui/views/se...
File chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc
(right):

http://codereview.chromium.org/10804026/diff/15001/chrome/browser/ui/views/se...
chrome/browser/ui/views/select_file_dialog_extension_browsertest.cc:49: OVERRIDE
{
On 2012/07/24 20:13:05, achuith.bhandarkar wrote:
> nit: don't think you're supposed to break before OVERRIDE. Maybe break after (

Done.

http://codereview.chromium.org/10804026/diff/15001/chrome/browser/ui/views/se...
File chrome/browser/ui/views/select_file_dialog_extension_unittest.cc (right):

http://codereview.chromium.org/10804026/diff/15001/chrome/browser/ui/views/se...
chrome/browser/ui/views/select_file_dialog_extension_unittest.cc:56:
ui::SelectedFileInfo file_info = ui::SelectedFileInfo(FilePath(), FilePath());
On 2012/07/24 20:13:05, achuith.bhandarkar wrote:
> Why is this necessary? It seems like the default ctor does the right thing?

Done.

http://codereview.chromium.org/10804026/diff/15001/ui/base/dialogs/selected_f...
File ui/base/dialogs/selected_file_info.h (right):

http://codereview.chromium.org/10804026/diff/15001/ui/base/dialogs/selected_f...
ui/base/dialogs/selected_file_info.h:18: FilePath file_path;
On 2012/07/24 20:13:05, achuith.bhandarkar wrote:
> Maybe add a comment here? What does file_path look like?
> 
> What's the difference between file_path and real_path?

yeah, I meant to add a comment here..

[achuithb, 2012-07-25 18:11:23]

http://codereview.chromium.org/10804026/diff/22003/ui/base/dialogs/selected_f...
File ui/base/dialogs/selected_file_info.h (right):

http://codereview.chromium.org/10804026/diff/22003/ui/base/dialogs/selected_f...
ui/base/dialogs/selected_file_info.h:19: FilePath file_path;
What's the rationale for changing this name from path? It seems like you'd need
to touch a lot fewer files if you just called these path and real_path.

http://codereview.chromium.org/10804026/diff/22003/ui/base/dialogs/selected_f...
ui/base/dialogs/selected_file_info.h:26: FilePath real_path;
Is local_path a better name?

[tbarzic, 2012-07-25 18:42:59]

https://chromiumcodereview.appspot.com/10804026/diff/22003/ui/base/dialogs/se...
File ui/base/dialogs/selected_file_info.h (right):

https://chromiumcodereview.appspot.com/10804026/diff/22003/ui/base/dialogs/se...
ui/base/dialogs/selected_file_info.h:19: FilePath file_path;
On 2012/07/25 18:11:23, achuith.bhandarkar wrote:
> What's the rationale for changing this name from path? It seems like you'd
need
> to touch a lot fewer files if you just called these path and real_path. 

actually, |real_path| has taken over the role of |path|. If it was the other
way, I'd keep |path|, but in this case, I think it would be more confusing..

https://chromiumcodereview.appspot.com/10804026/diff/22003/ui/base/dialogs/se...
ui/base/dialogs/selected_file_info.h:26: FilePath real_path;
On 2012/07/25 18:11:23, achuith.bhandarkar wrote:
> Is local_path a better name? 

Done.

[achuithb, 2012-07-25 18:48:43]

lgtm

[asanka, 2012-07-25 19:00:07]

chrome/downloads/ LGTM

[sky, 2012-07-25 21:13:43]

LGTM

[commit-bot: I haz the power, 2012-07-26 01:08:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/10804026/19020

[commit-bot: I haz the power, 2012-07-26 02:31:12]

Change committed as 148492