Don't save last used locations in open and save dialogs when in incognito mode.

Don't save last used locations in open and save dialogs when in incognito mode.

For "Save page as..." we simply don't store the save location in incognito mode.

For "Open File"... and file upload controls, it's a bit more complicated: because NSOpenPanel and NSSavePanel default to using the last chosen directory from NavigationServices (which we can't influence),
we provide a preference value prefs::kLastChosenDirectory as fallback there, which is not persisted in incognito mode.

BUG=45064
TEST=See bug for manual test.

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

[Bernhard Bauer, 2010-06-02 15:49:32]

Pam, could you review this?

Nico, do you think there's an easier/more waterproof way to avoid remembering
the last used directory on Mac? Saving/resetting NSUserDefaults didn't work,
probably because NSSavePanel and NSOpenPanel use singletons that cache the last
used directory.

[Pam (message me for reviews), 2010-06-07 15:52:56]

http://codereview.chromium.org/2450004/diff/8001/9001
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/2450004/diff/8001/9001#newcode1582
chrome/browser/browser.cc:1582: prefs::kLastSelectedDirectory);
Should this be using profile_->last_selected_directory(), or am I missing
something?

http://codereview.chromium.org/2450004/diff/8001/9002
File chrome/browser/download/save_package.cc (right):

http://codereview.chromium.org/2450004/diff/8001/9002#newcode1272
chrome/browser/download/save_package.cc:1272: (save_file_path.GetValue() !=
param->dir.ToWStringHack()))
nit: please remove the redundant parentheses around this second condition

http://codereview.chromium.org/2450004/diff/8001/9003
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/2450004/diff/8001/9003#newcode590
chrome/browser/profile.cc:590: } else {
No need for "else" after a return.

http://codereview.chromium.org/2450004/diff/8001/9006
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/2450004/diff/8001/9006#newcode657
chrome/common/pref_names.cc:657: extern const wchar_t kLastSelectedDirectory[] =
L"last_selected_directory";
Should this have some "domain" name too? upload.last_selected_directory or
something like that?

[(unused - use chromium), 2010-06-07 15:58:12]

Sorry, missed this somehow. Can you ping me about this early next week? (You
don't need to wait with committing this, I just want to check if there's a
better way).

On Jun 2, 2010 8:49 AM, <bauerb@chromium.org> wrote:
Reviewers: Pam,

Message:
Pam, could you review this?

Nico, do you think there's an easier/more waterproof way to avoid
remembering
the last used directory on Mac? Saving/resetting NSUserDefaults didn't work,
probably because NSSavePanel and NSOpenPanel use singletons that cache the
last
used directory.

Description:
Don't save last used locations in open and save dialogs when in incognito
mode.

For "Save page as..." we simply don't store the save location in incognito
mode.

For "Open File"... and file upload controls, it's a bit more complicated:
because NSOpenPanel and NSSavePanel default to using the last chosen
directory
from NavigationServices (which we can't influence),
we provide a preference value prefs::kLastChosenDirectory as fallback there,
which is not persisted in incognito mode.

BUG=45064
TEST=See bug for manual test.

Please review this at http://codereview.chromium.org/2450004/show

SVN Base: git://codf21.jail.google.com/chromium.git

Affected files:
 M chrome/browser/browser.cc
 M chrome/browser/download/save_package.cc
 M chrome/browser/profile.h
 M chrome/browser/profile.cc
 M chrome/browser/tab_contents/tab_contents.cc
 M chrome/common/pref_names.h
 M chrome/common/pref_names.cc
 M chrome/test/testing_profile.h

[Bernhard Bauer, 2010-06-08 10:13:25]

On 2010/06/07 15:52:56, Pam wrote:
> http://codereview.chromium.org/2450004/diff/8001/9001
> File chrome/browser/browser.cc (right):
> 
> http://codereview.chromium.org/2450004/diff/8001/9001#newcode1582
> chrome/browser/browser.cc:1582: prefs::kLastSelectedDirectory);
> Should this be using profile_->last_selected_directory(), or am I missing
> something?

You're right. done.

> http://codereview.chromium.org/2450004/diff/8001/9002
> File chrome/browser/download/save_package.cc (right):
> 
> http://codereview.chromium.org/2450004/diff/8001/9002#newcode1272
> chrome/browser/download/save_package.cc:1272: (save_file_path.GetValue() !=
> param->dir.ToWStringHack()))
> nit: please remove the redundant parentheses around this second condition

done.

> http://codereview.chromium.org/2450004/diff/8001/9003
> File chrome/browser/profile.cc (right):
> 
> http://codereview.chromium.org/2450004/diff/8001/9003#newcode590
> chrome/browser/profile.cc:590: } else {
> No need for "else" after a return.

done.

> http://codereview.chromium.org/2450004/diff/8001/9006
> File chrome/common/pref_names.cc (right):
> 
> http://codereview.chromium.org/2450004/diff/8001/9006#newcode657
> chrome/common/pref_names.cc:657: extern const wchar_t kLastSelectedDirectory[]
=
> L"last_selected_directory";
> Should this have some "domain" name too? upload.last_selected_directory or
> something like that?

The thing is, it's not specific to uploads, it's a fallback for when there is no
starting directory for SelectFile. I just couldn't find a good domain name for
it.

[Pam (message me for reviews), 2010-06-08 10:22:11]

> The thing is, it's not specific to uploads, it's a fallback for when there is
no
> starting directory for SelectFile. I just couldn't find a good domain name for
> it.

Following the savefile.default_directory pattern, maybe
selectfile.last_directory? Even kLastSelectedDirectory is hard to understand
without knowing that it applies to SelectFile. Consider making it more specific;
e.g. kSelectFileLastDirectory (which also matches kSafeFileDefaultDirectory).

[Bernhard Bauer, 2010-06-08 10:28:33]

On 2010/06/08 10:22:11, Pam wrote:
> > The thing is, it's not specific to uploads, it's a fallback for when there
is
> no
> > starting directory for SelectFile. I just couldn't find a good domain name
for
> > it.
> 
> Following the savefile.default_directory pattern, maybe
> selectfile.last_directory? Even kLastSelectedDirectory is hard to understand
> without knowing that it applies to SelectFile. Consider making it more
specific;
> e.g. kSelectFileLastDirectory (which also matches kSafeFileDefaultDirectory).

Sounds good. done.

[Pam (message me for reviews), 2010-06-08 10:58:18]

LGTM!

- Pam