Change linux default hidden file save directory to XDG_DATA_HOME

Change linux default hidden file save directory to XDG_DATA_HOME

Versions of GTK3 up to 3.14.7 had a bug that would cause a segfault
when a GtkFileChooser was open and a hidden file in the directory was
deleted [1].

Chrome can trigger this bug in the following scenario:
1. Open 2 windows
2. Open a save dialog in each window
3. Close one of the dialogs

Chrome saves a file in two stages: it writes the contents to a hidden
file, and then renames it to the actual file.  This is so the file can
be downloading as the user is choosing a file to save to.

By default, the hidden file is placed into whichever directory the
save dialog opens with.  In the case of 2 save dialogs open at once,
if the user saves the file (renames the hidden file) or cancels the
download (deletes the hidden file), the GTK bug would be triggered.

This CL changes the default directory for the hidden files to
XDG_DATA_HOME in an effort to make triggering the GTK bug far less
likely.

[1] https://mail.gnome.org/archives/commits-list/2014-December/msg02447.html

BUG=728501
R=thestig@chromium.org
TBR=qinmin@chromium.org

Review-Url: https://codereview.chromium.org/2924433002
Cr-Commit-Position: refs/heads/master@{#479213}
Committed: https://chromium.googlesource.com/chromium/src/+/1799a1223112af65e44eba61dc4feb885389bfe2

[Tom Anderson, 2017-06-03 01:49:14]

The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run

[Tom Anderson, 2017-06-03 01:49:21]

thestig ptal

[commit-bot: I haz the power, 2017-06-03 01:49:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Anderson, 2017-06-03 01:55:42]

Hm.. I just realized.  What if /tmp is memory-mounted and the user is trying to
download a file that doesn't fit in memory (but does fit on disk)?  This CL
would probably break that case.  Also if /tmp is on a separate filesystem (or is
memory-mounted), this would require a copy.

[Tom Anderson, 2017-06-03 01:58:31]

On 2017/06/03 01:55:42, Tom Anderson wrote:
> Hm.. I just realized.  What if /tmp is memory-mounted and the user is trying
to
> download a file that doesn't fit in memory (but does fit on disk)?  This CL
> would probably break that case.  Also if /tmp is on a separate filesystem (or
is
> memory-mounted), this would require a copy.

Ah, this is what /var/tmp is for

[commit-bot: I haz the power, 2017-06-03 03:39:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-03 03:39:38]

Dry run: This issue passed the CQ dry run.

[Tom Anderson, 2017-06-05 18:24:15]

The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 18:24:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Anderson, 2017-06-05 18:26:13]

Description was changed from

==========
Change linux default temporary download save directory to /tmp

Versions of GTK3 up to 3.14.7 had a bug that would cause a segfault
when a GtkFileChooser was open and a hidden file in the directory was
deleted [1].

Chrome can trigger this bug in the following scenario:
1. Open 2 windows
2. Open a save dialog in each window
3. Close one of the dialogs

Chrome saves a file in two stages: it writes the contents to a hidden
file, and then renames it to the actual file.  This is so the file can
be writing as the user is choosing a file to save to.

By default, the hidden file is placed into whichever directory the
save dialog opens with.  In the case of 2 save dialogs open at once,
if the user saves the file (renames the hidden file) or cancels the
download (deletes the hidden file), the GTK bug would be triggered.

This CL changes the default directory for the hidden files to /tmp in
an effort to make triggering the GTK bug far less likely.

[1] https://mail.gnome.org/archives/commits-list/2014-December/msg02447.html

BUG=728501
R=thestig@chromium.org
==========

to

==========
Change linux default temporary download save directory to /var/tmp

Versions of GTK3 up to 3.14.7 had a bug that would cause a segfault
when a GtkFileChooser was open and a hidden file in the directory was
deleted [1].

Chrome can trigger this bug in the following scenario:
1. Open 2 windows
2. Open a save dialog in each window
3. Close one of the dialogs

Chrome saves a file in two stages: it writes the contents to a hidden
file, and then renames it to the actual file.  This is so the file can
be writing as the user is choosing a file to save to.

By default, the hidden file is placed into whichever directory the
save dialog opens with.  In the case of 2 save dialogs open at once,
if the user saves the file (renames the hidden file) or cancels the
download (deletes the hidden file), the GTK bug would be triggered.

This CL changes the default directory for the hidden files to /var/tmp
in an effort to make triggering the GTK bug far less likely.

[1] https://mail.gnome.org/archives/commits-list/2014-December/msg02447.html

BUG=728501
R=thestig@chromium.org
==========

[Tom Anderson, 2017-06-05 18:26:58]

Description was changed from

==========
Change linux default temporary download save directory to /var/tmp

Versions of GTK3 up to 3.14.7 had a bug that would cause a segfault
when a GtkFileChooser was open and a hidden file in the directory was
deleted [1].

Chrome can trigger this bug in the following scenario:
1. Open 2 windows
2. Open a save dialog in each window
3. Close one of the dialogs

Chrome saves a file in two stages: it writes the contents to a hidden
file, and then renames it to the actual file.  This is so the file can
be writing as the user is choosing a file to save to.

By default, the hidden file is placed into whichever directory the
save dialog opens with.  In the case of 2 save dialogs open at once,
if the user saves the file (renames the hidden file) or cancels the
download (deletes the hidden file), the GTK bug would be triggered.

This CL changes the default directory for the hidden files to /var/tmp
in an effort to make triggering the GTK bug far less likely.

[1] https://mail.gnome.org/archives/commits-list/2014-December/msg02447.html

BUG=728501
R=thestig@chromium.org
==========

to

==========
Change linux default hidden file save directory to /var/tmp

Versions of GTK3 up to 3.14.7 had a bug that would cause a segfault
when a GtkFileChooser was open and a hidden file in the directory was
deleted [1].

Chrome can trigger this bug in the following scenario:
1. Open 2 windows
2. Open a save dialog in each window
3. Close one of the dialogs

Chrome saves a file in two stages: it writes the contents to a hidden
file, and then renames it to the actual file.  This is so the file can
be writing as the user is choosing a file to save to.

By default, the hidden file is placed into whichever directory the
save dialog opens with.  In the case of 2 save dialogs open at once,
if the user saves the file (renames the hidden file) or cancels the
download (deletes the hidden file), the GTK bug would be triggered.

This CL changes the default directory for the hidden files to /var/tmp
in an effort to make triggering the GTK bug far less likely.

[1] https://mail.gnome.org/archives/commits-list/2014-December/msg02447.html

BUG=728501
R=thestig@chromium.org
==========

[commit-bot: I haz the power, 2017-06-05 21:28:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-05 21:28:47]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Tom Anderson, 2017-06-05 22:17:13]

The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 22:17:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 00:28:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-06 00:28:47]

Dry run: This issue passed the CQ dry run.

[Tom Anderson, 2017-06-06 00:50:04]

Description was changed from

==========
Change linux default hidden file save directory to /var/tmp

Versions of GTK3 up to 3.14.7 had a bug that would cause a segfault
when a GtkFileChooser was open and a hidden file in the directory was
deleted [1].

Chrome can trigger this bug in the following scenario:
1. Open 2 windows
2. Open a save dialog in each window
3. Close one of the dialogs

Chrome saves a file in two stages: it writes the contents to a hidden
file, and then renames it to the actual file.  This is so the file can
be writing as the user is choosing a file to save to.

By default, the hidden file is placed into whichever directory the
save dialog opens with.  In the case of 2 save dialogs open at once,
if the user saves the file (renames the hidden file) or cancels the
download (deletes the hidden file), the GTK bug would be triggered.

This CL changes the default directory for the hidden files to /var/tmp
in an effort to make triggering the GTK bug far less likely.

[1] https://mail.gnome.org/archives/commits-list/2014-December/msg02447.html

BUG=728501
R=thestig@chromium.org
==========

to

==========
Change linux default hidden file save directory to /var/tmp

Versions of GTK3 up to 3.14.7 had a bug that would cause a segfault
when a GtkFileChooser was open and a hidden file in the directory was
deleted [1].

Chrome can trigger this bug in the following scenario:
1. Open 2 windows
2. Open a save dialog in each window
3. Close one of the dialogs

Chrome saves a file in two stages: it writes the contents to a hidden
file, and then renames it to the actual file.  This is so the file can
be downloading as the user is choosing a file to save to.

By default, the hidden file is placed into whichever directory the
save dialog opens with.  In the case of 2 save dialogs open at once,
if the user saves the file (renames the hidden file) or cancels the
download (deletes the hidden file), the GTK bug would be triggered.

This CL changes the default directory for the hidden files to /var/tmp
in an effort to make triggering the GTK bug far less likely.

[1] https://mail.gnome.org/archives/commits-list/2014-December/msg02447.html

BUG=728501
R=thestig@chromium.org
==========

[Lei Zhang, 2017-06-06 21:18:50]

I'd ask the download folks about this. From my understanding, we ideally want
the hidden file to be on the same partition as the final destination. That way,
when the download finishes, all it takes to move the file to its final
destination is a rename(). Defaulting to /var/tmp may end up causing a lot of
extra copying in some cases. e.g. /home and / are on different partitions.

[Tom Anderson, 2017-06-06 21:27:39]

thomasanderson@chromium.org changed reviewers:
+ dtrainor@chromium.org

[Tom Anderson, 2017-06-06 21:27:39]

On 2017/06/06 21:18:50, Lei Zhang wrote:
> I'd ask the download folks about this. From my understanding, we ideally want
> the hidden file to be on the same partition as the final destination. That
way,
> when the download finishes, all it takes to move the file to its final
> destination is a rename(). Defaulting to /var/tmp may end up causing a lot of
> extra copying in some cases. e.g. /home and / are on different partitions.

Yeah that is worrying.  Maybe put it in ~/.cache?  Or somewhere else?

+dtrainor as a reviewer

[Tom Anderson, 2017-06-06 21:28:23]

thomasanderson@chromium.org changed reviewers:
+ qinmin@chromium.org

[Tom Anderson, 2017-06-06 21:28:23]

also +qinmin

[qinmin, 2017-06-06 22:10:14]

https://codereview.chromium.org/2924433002/diff/20001/chrome/browser/download...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/2924433002/diff/20001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:462:
*download_save_dir = base::FilePath(kVarTmp);
Shouldn't this be done in download_prefs_?
what if the *download_save_dir is defaulted to something else, either by test or
by user

[Tom Anderson, 2017-06-07 01:26:46]

Description was changed from

==========
Change linux default hidden file save directory to /var/tmp

Versions of GTK3 up to 3.14.7 had a bug that would cause a segfault
when a GtkFileChooser was open and a hidden file in the directory was
deleted [1].

Chrome can trigger this bug in the following scenario:
1. Open 2 windows
2. Open a save dialog in each window
3. Close one of the dialogs

Chrome saves a file in two stages: it writes the contents to a hidden
file, and then renames it to the actual file.  This is so the file can
be downloading as the user is choosing a file to save to.

By default, the hidden file is placed into whichever directory the
save dialog opens with.  In the case of 2 save dialogs open at once,
if the user saves the file (renames the hidden file) or cancels the
download (deletes the hidden file), the GTK bug would be triggered.

This CL changes the default directory for the hidden files to /var/tmp
in an effort to make triggering the GTK bug far less likely.

[1] https://mail.gnome.org/archives/commits-list/2014-December/msg02447.html

BUG=728501
R=thestig@chromium.org
==========

to

==========
Change linux default hidden file save directory to XDG_CONFIG_HOME

Versions of GTK3 up to 3.14.7 had a bug that would cause a segfault
when a GtkFileChooser was open and a hidden file in the directory was
deleted [1].

Chrome can trigger this bug in the following scenario:
1. Open 2 windows
2. Open a save dialog in each window
3. Close one of the dialogs

Chrome saves a file in two stages: it writes the contents to a hidden
file, and then renames it to the actual file.  This is so the file can
be downloading as the user is choosing a file to save to.

By default, the hidden file is placed into whichever directory the
save dialog opens with.  In the case of 2 save dialogs open at once,
if the user saves the file (renames the hidden file) or cancels the
download (deletes the hidden file), the GTK bug would be triggered.

This CL changes the default directory for the hidden files to
XDG_CONFIG_HOME in an effort to make triggering the GTK bug far less
likely.

[1] https://mail.gnome.org/archives/commits-list/2014-December/msg02447.html

BUG=728501
R=thestig@chromium.org
==========

[Tom Anderson, 2017-06-07 01:29:51]

I think XDG_DATA_HOME (~/.local/share) is the right path to use here.  It's
usually on the same partition as the default download directory (~/Downloads).

https://codereview.chromium.org/2924433002/diff/20001/chrome/browser/download...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/2924433002/diff/20001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:462:
*download_save_dir = base::FilePath(kVarTmp);
On 2017/06/06 22:10:14, qinmin wrote:
> Shouldn't this be done in download_prefs_?
> what if the *download_save_dir is defaulted to something else, either by test
or
> by user 

Done.

[Tom Anderson, 2017-06-07 01:30:04]

Description was changed from

==========
Change linux default hidden file save directory to XDG_CONFIG_HOME

Versions of GTK3 up to 3.14.7 had a bug that would cause a segfault
when a GtkFileChooser was open and a hidden file in the directory was
deleted [1].

Chrome can trigger this bug in the following scenario:
1. Open 2 windows
2. Open a save dialog in each window
3. Close one of the dialogs

Chrome saves a file in two stages: it writes the contents to a hidden
file, and then renames it to the actual file.  This is so the file can
be downloading as the user is choosing a file to save to.

By default, the hidden file is placed into whichever directory the
save dialog opens with.  In the case of 2 save dialogs open at once,
if the user saves the file (renames the hidden file) or cancels the
download (deletes the hidden file), the GTK bug would be triggered.

This CL changes the default directory for the hidden files to
XDG_CONFIG_HOME in an effort to make triggering the GTK bug far less
likely.

[1] https://mail.gnome.org/archives/commits-list/2014-December/msg02447.html

BUG=728501
R=thestig@chromium.org
==========

to

==========
Change linux default hidden file save directory to XDG_DATA_HOME

Versions of GTK3 up to 3.14.7 had a bug that would cause a segfault
when a GtkFileChooser was open and a hidden file in the directory was
deleted [1].

Chrome can trigger this bug in the following scenario:
1. Open 2 windows
2. Open a save dialog in each window
3. Close one of the dialogs

Chrome saves a file in two stages: it writes the contents to a hidden
file, and then renames it to the actual file.  This is so the file can
be downloading as the user is choosing a file to save to.

By default, the hidden file is placed into whichever directory the
save dialog opens with.  In the case of 2 save dialogs open at once,
if the user saves the file (renames the hidden file) or cancels the
download (deletes the hidden file), the GTK bug would be triggered.

This CL changes the default directory for the hidden files to
XDG_DATA_HOME in an effort to make triggering the GTK bug far less
likely.

[1] https://mail.gnome.org/archives/commits-list/2014-December/msg02447.html

BUG=728501
R=thestig@chromium.org
==========

[Tom Anderson, 2017-06-07 03:22:19]

The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 03:22:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 04:28:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-07 04:28:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[David Trainor- moved to gerrit, 2017-06-07 06:11:34]

Moving myself to CC since I'm still out for the next day.  Thanks Min!

[Lei Zhang, 2017-06-07 06:48:33]

lgtm with approval from the downloads folks.

https://codereview.chromium.org/2924433002/diff/40001/chrome/browser/shell_in...
File chrome/browser/shell_integration_linux.cc (left):

https://codereview.chromium.org/2924433002/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_linux.cc:565:
base::ThreadRestrictions::AssertIOAllowed();
Comment still says "called on the FILE thread" in the header, BTW. Update?

I don't see the code hitting the disk on Linux. Apparently I touched this at one
point:

https://codereview.chromium.org/827013004/diff/20001/chrome/browser/shell_int...

And the previous code had effectively this check, but clearly did not need it.

[qinmin, 2017-06-07 14:21:18]

https://codereview.chromium.org/2924433002/diff/40001/chrome/browser/download...
File chrome/browser/download/download_prefs.cc (right):

https://codereview.chromium.org/2924433002/diff/40001/chrome/browser/download...
chrome/browser/download/download_prefs.cc:263: // TODO(thomasanderson): Remove
this when all Linux distros with
This logic would override the |download_path_| set by
DownloadPrefs::SetDownloadPath().
Is this intended? If a test want the download_path_ to be /test/, will that test
fail?

[Tom Anderson, 2017-06-07 18:38:38]

https://codereview.chromium.org/2924433002/diff/40001/chrome/browser/download...
File chrome/browser/download/download_prefs.cc (right):

https://codereview.chromium.org/2924433002/diff/40001/chrome/browser/download...
chrome/browser/download/download_prefs.cc:263: // TODO(thomasanderson): Remove
this when all Linux distros with
On 2017/06/07 14:21:18, qinmin wrote:
> This logic would override the |download_path_| set by
> DownloadPrefs::SetDownloadPath().
> Is this intended? If a test want the download_path_ to be /test/, will that
test
> fail?

oops.. done.
Hopefully this will fix the test failures

https://codereview.chromium.org/2924433002/diff/40001/chrome/browser/shell_in...
File chrome/browser/shell_integration_linux.cc (left):

https://codereview.chromium.org/2924433002/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_linux.cc:565:
base::ThreadRestrictions::AssertIOAllowed();
On 2017/06/07 06:48:33, Lei Zhang wrote:
> Comment still says "called on the FILE thread" in the header, BTW. Update?
> 
> I don't see the code hitting the disk on Linux. Apparently I touched this at
one
> point:
> 
>
https://codereview.chromium.org/827013004/diff/20001/chrome/browser/shell_int...
> 
> And the previous code had effectively this check, but clearly did not need it.

Done.

[Tom Anderson, 2017-06-07 18:38:45]

The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 18:39:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Anderson, 2017-06-07 18:50:00]

The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 18:50:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 19:07:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-07 19:07:47]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Tom Anderson, 2017-06-07 19:38:29]

Patchset #4 (id:60001) has been deleted

[Tom Anderson, 2017-06-07 20:38:58]

qinmin: I don't know where to put the new linux-specific override.

I'm not sure download_prefs is the right place because test code sets the
download path using prefs::kDownloadDefaultDirectory.  Overriding it like in PS4
would cause tests to fail.  But overriding the underlying property doesn't seem
right either.

Maybe the right place is in download_manager_impl:
https://cs.chromium.org/chromium/src/content/browser/download/download_manage...

Or possibly chrome_download_manager_delgate like it was in PS1 (and there were
no test failures).

[qinmin, 2017-06-07 21:13:20]

On 2017/06/07 20:38:58, Tom Anderson wrote:
> qinmin: I don't know where to put the new linux-specific override.
> 
> I'm not sure download_prefs is the right place because test code sets the
> download path using prefs::kDownloadDefaultDirectory.  Overriding it like in
PS4
> would cause tests to fail.  But overriding the underlying property doesn't
seem
> right either.
> 
> Maybe the right place is in download_manager_impl:
>
https://cs.chromium.org/chromium/src/content/browser/download/download_manage...
> 
> Or possibly chrome_download_manager_delgate like it was in PS1 (and there were
> no test failures).

what about checking prefs::kDownloadDefaultDirectory before initializing
download_path_? If it is using default value (or not being set), you can
override it. And SetDownloadPath() can also override the value() later.

[Tom Anderson, 2017-06-08 00:05:04]

On 2017/06/07 21:13:20, qinmin wrote:
> On 2017/06/07 20:38:58, Tom Anderson wrote:
> > qinmin: I don't know where to put the new linux-specific override.
> > 
> > I'm not sure download_prefs is the right place because test code sets the
> > download path using prefs::kDownloadDefaultDirectory.  Overriding it like in
> PS4
> > would cause tests to fail.  But overriding the underlying property doesn't
> seem
> > right either.
> > 
> > Maybe the right place is in download_manager_impl:
> >
>
https://cs.chromium.org/chromium/src/content/browser/download/download_manage...
> > 
> > Or possibly chrome_download_manager_delgate like it was in PS1 (and there
were
> > no test failures).
> 
> what about checking prefs::kDownloadDefaultDirectory before initializing
> download_path_? If it is using default value (or not being set), you can
> override it. And SetDownloadPath() can also override the value() later.

But then Chrome would still crash if the user set the default download directory
to something else.

I'm relatively convinced that the override should go in download_manager_impl. 
The only thing we want to change is the location of the hidden file.  (Although,
when I do make the change in download_prefs, save dialogs still open to
~/Downloads, and files saved by extensions (without using the save dialog) also
save to ~/Downloads, so maybe I'm misunderstanding the scope of download_path_)

[qinmin, 2017-06-08 07:17:00]

On 2017/06/08 00:05:04, Tom Anderson wrote:
> On 2017/06/07 21:13:20, qinmin wrote:
> > On 2017/06/07 20:38:58, Tom Anderson wrote:
> > > qinmin: I don't know where to put the new linux-specific override.
> > > 
> > > I'm not sure download_prefs is the right place because test code sets the
> > > download path using prefs::kDownloadDefaultDirectory.  Overriding it like
in
> > PS4
> > > would cause tests to fail.  But overriding the underlying property doesn't
> > seem
> > > right either.
> > > 
> > > Maybe the right place is in download_manager_impl:
> > >
> >
>
https://cs.chromium.org/chromium/src/content/browser/download/download_manage...
> > > 
> > > Or possibly chrome_download_manager_delgate like it was in PS1 (and there
> were
> > > no test failures).
> > 
> > what about checking prefs::kDownloadDefaultDirectory before initializing
> > download_path_? If it is using default value (or not being set), you can
> > override it. And SetDownloadPath() can also override the value() later.
> 
> But then Chrome would still crash if the user set the default download
directory
> to something else.
> 
> I'm relatively convinced that the override should go in download_manager_impl.

> The only thing we want to change is the location of the hidden file. 
(Although,
> when I do make the change in download_prefs, save dialogs still open to
> ~/Downloads, and files saved by extensions (without using the save dialog)
also
> save to ~/Downloads, so maybe I'm misunderstanding the scope of
download_path_)

ok, DownloadManagerImpl makes sense to me. Add a method
GetTemporaryDownloadDirectory() to differentiate the default directory and
temporary directory.

[Tom Anderson, 2017-06-08 18:40:42]

On 2017/06/08 07:17:00, qinmin wrote:
> On 2017/06/08 00:05:04, Tom Anderson wrote:
> > I'm relatively convinced that the override should go in
download_manager_impl.
> 
> > The only thing we want to change is the location of the hidden file. 
> (Although,
> > when I do make the change in download_prefs, save dialogs still open to
> > ~/Downloads, and files saved by extensions (without using the save dialog)
> also
> > save to ~/Downloads, so maybe I'm misunderstanding the scope of
> download_path_)
> 
> ok, DownloadManagerImpl makes sense to me. Add a method
> GetTemporaryDownloadDirectory() to differentiate the default directory and
> temporary directory.

Done.

Lei FYI: the latest patchset moves the override into //content (but
shell_integration_linux is in //browser).  This means I had to duplicate the
logic of GetDataWriteLocation() (which is just one line, but still..).  I think
a better long-term solution is to move shell_integration_linux into //base. 
wdyt?

[Tom Anderson, 2017-06-08 18:40:47]

The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 18:41:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-08 19:13:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-08 19:13:53]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Tom Anderson, 2017-06-08 20:10:32]

Patchset #4 (id:80001) has been deleted

[Tom Anderson, 2017-06-08 20:26:00]

The CQ bit was checked by thomasanderson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 20:26:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2017-06-08 21:00:32]

On 2017/06/08 18:40:42, Tom Anderson wrote:
> Lei FYI: the latest patchset moves the override into //content (but
> shell_integration_linux is in //browser).  This means I had to duplicate the
> logic of GetDataWriteLocation() (which is just one line, but still..).  I
think
> a better long-term solution is to move shell_integration_linux into //base. 
> wdyt?

Well, shell_integration_linux is part of a whole family of shell_integration*
files. Not sure if they are all useful in base. There may be specific bits that
you would want to move down to base or to contents.

[commit-bot: I haz the power, 2017-06-08 21:30:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-08 21:30:44]

Dry run: This issue passed the CQ dry run.

[Tom Anderson, 2017-06-13 18:13:43]

Pinging qinmin and/or dtrainor

On 2017/06/08 21:00:32, Lei Zhang wrote:
> On 2017/06/08 18:40:42, Tom Anderson wrote:
> > Lei FYI: the latest patchset moves the override into //content (but
> > shell_integration_linux is in //browser).  This means I had to duplicate the
> > logic of GetDataWriteLocation() (which is just one line, but still..).  I
> think
> > a better long-term solution is to move shell_integration_linux into //base. 
> > wdyt?
> 
> Well, shell_integration_linux is part of a whole family of shell_integration*
> files. Not sure if they are all useful in base. There may be specific bits
that
> you would want to move down to base or to contents.

Ack.

[Min Qin, 2017-06-13 18:19:38]

qinmin@google.com changed reviewers:
+ qinmin@google.com

[Min Qin, 2017-06-13 18:19:38]

lgtm

[Tom Anderson, 2017-06-13 20:28:37]

The CQ bit was checked by thomasanderson@chromium.org

[Tom Anderson, 2017-06-13 20:28:37]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2924433002/#ps120001
(title: "Fix DownloadManagerTest.StartDownload")

[commit-bot: I haz the power, 2017-06-13 20:29:20]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 20:41:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-13 20:41:43]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Tom Anderson, 2017-06-13 23:53:35]

Description was changed from

==========
Change linux default hidden file save directory to XDG_DATA_HOME

Versions of GTK3 up to 3.14.7 had a bug that would cause a segfault
when a GtkFileChooser was open and a hidden file in the directory was
deleted [1].

Chrome can trigger this bug in the following scenario:
1. Open 2 windows
2. Open a save dialog in each window
3. Close one of the dialogs

Chrome saves a file in two stages: it writes the contents to a hidden
file, and then renames it to the actual file.  This is so the file can
be downloading as the user is choosing a file to save to.

By default, the hidden file is placed into whichever directory the
save dialog opens with.  In the case of 2 save dialogs open at once,
if the user saves the file (renames the hidden file) or cancels the
download (deletes the hidden file), the GTK bug would be triggered.

This CL changes the default directory for the hidden files to
XDG_DATA_HOME in an effort to make triggering the GTK bug far less
likely.

[1] https://mail.gnome.org/archives/commits-list/2014-December/msg02447.html

BUG=728501
R=thestig@chromium.org
==========

to

==========
Change linux default hidden file save directory to XDG_DATA_HOME

Versions of GTK3 up to 3.14.7 had a bug that would cause a segfault
when a GtkFileChooser was open and a hidden file in the directory was
deleted [1].

Chrome can trigger this bug in the following scenario:
1. Open 2 windows
2. Open a save dialog in each window
3. Close one of the dialogs

Chrome saves a file in two stages: it writes the contents to a hidden
file, and then renames it to the actual file.  This is so the file can
be downloading as the user is choosing a file to save to.

By default, the hidden file is placed into whichever directory the
save dialog opens with.  In the case of 2 save dialogs open at once,
if the user saves the file (renames the hidden file) or cancels the
download (deletes the hidden file), the GTK bug would be triggered.

This CL changes the default directory for the hidden files to
XDG_DATA_HOME in an effort to make triggering the GTK bug far less
likely.

[1] https://mail.gnome.org/archives/commits-list/2014-December/msg02447.html

BUG=728501
R=thestig@chromium.org
TBR=qinmin@chromium.org
==========

[Tom Anderson, 2017-06-13 23:53:42]

The CQ bit was checked by thomasanderson@chromium.org

[commit-bot: I haz the power, 2017-06-13 23:54:17]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Anderson, 2017-06-13 23:56:47]

On 2017/06/13 20:41:43, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

tbr'ing qinmin@ because the l-g-t-m was from his google account

[commit-bot: I haz the power, 2017-06-14 00:02:05]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497398022165990,
"parent_rev": "fdf52023f531d3bb73459e021a243d5aae076de7", "commit_rev":
"1799a1223112af65e44eba61dc4feb885389bfe2"}

[commit-bot: I haz the power, 2017-06-14 00:02:14]

Description was changed from

==========
Change linux default hidden file save directory to XDG_DATA_HOME

Versions of GTK3 up to 3.14.7 had a bug that would cause a segfault
when a GtkFileChooser was open and a hidden file in the directory was
deleted [1].

Chrome can trigger this bug in the following scenario:
1. Open 2 windows
2. Open a save dialog in each window
3. Close one of the dialogs

Chrome saves a file in two stages: it writes the contents to a hidden
file, and then renames it to the actual file.  This is so the file can
be downloading as the user is choosing a file to save to.

By default, the hidden file is placed into whichever directory the
save dialog opens with.  In the case of 2 save dialogs open at once,
if the user saves the file (renames the hidden file) or cancels the
download (deletes the hidden file), the GTK bug would be triggered.

This CL changes the default directory for the hidden files to
XDG_DATA_HOME in an effort to make triggering the GTK bug far less
likely.

[1] https://mail.gnome.org/archives/commits-list/2014-December/msg02447.html

BUG=728501
R=thestig@chromium.org
TBR=qinmin@chromium.org
==========

to

==========
Change linux default hidden file save directory to XDG_DATA_HOME

Versions of GTK3 up to 3.14.7 had a bug that would cause a segfault
when a GtkFileChooser was open and a hidden file in the directory was
deleted [1].

Chrome can trigger this bug in the following scenario:
1. Open 2 windows
2. Open a save dialog in each window
3. Close one of the dialogs

Chrome saves a file in two stages: it writes the contents to a hidden
file, and then renames it to the actual file.  This is so the file can
be downloading as the user is choosing a file to save to.

By default, the hidden file is placed into whichever directory the
save dialog opens with.  In the case of 2 save dialogs open at once,
if the user saves the file (renames the hidden file) or cancels the
download (deletes the hidden file), the GTK bug would be triggered.

This CL changes the default directory for the hidden files to
XDG_DATA_HOME in an effort to make triggering the GTK bug far less
likely.

[1] https://mail.gnome.org/archives/commits-list/2014-December/msg02447.html

BUG=728501
R=thestig@chromium.org
TBR=qinmin@chromium.org

Review-Url: https://codereview.chromium.org/2924433002
Cr-Commit-Position: refs/heads/master@{#479213}
Committed:
https://chromium.googlesource.com/chromium/src/+/1799a1223112af65e44eba61dc4f...
==========

[commit-bot: I haz the power, 2017-06-14 00:02:16]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/1799a1223112af65e44eba61dc4f...