Using ScopedTempDir as the download directory used by the DownloadViaPost test.

Using ScopedTempDir as the download directory used by the DownloadViaPost test.

BUG=653856
Committed: https://crrev.com/0afc6b3cfc1d61d2e8371b4c46cc72abd26b2a04
Cr-Commit-Position: refs/heads/master@{#424157}

[Łukasz Anforowicz, 2016-10-07 20:15:17]

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

[commit-bot: I haz the power, 2016-10-07 20:15:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2016-10-07 20:22:18]

lukasza@chromium.org changed reviewers:
+ asanka@chromium.org

[Łukasz Anforowicz, 2016-10-07 20:22:18]

asanka@, could you PTAL?  (thank you for suggesting what is going on in
https://crbug.com/653982#c2 and sorry for "rewarding" you with an incoming
review :-)

In the CL, I copy&pasted CreateAndSetDownloadsDirectory method from
chrome/browser/download/download_browsertest.cc.  I also see that there is
already a copy in
chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc.  I hope
this (this=copy&pasting) is okay - I haven't seen a good location for sharing
the test code.  OTOH, if you think this is worth it, maybe I should introduce a
chrome/browser/downloads/download_test_utils.h.

[asanka, 2016-10-07 20:24:29]

lgtm

[Łukasz Anforowicz, 2016-10-07 20:28:40]

lukasza@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Łukasz Anforowicz, 2016-10-07 20:28:41]

rdevlin.cronin@, could you do an OWNERS review for
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc?

[commit-bot: I haz the power, 2016-10-07 20:53:45]

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

[commit-bot: I haz the power, 2016-10-07 20:53:46]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-10-07 21:11:38]

lgtm

https://codereview.chromium.org/2407513002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/2407513002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:1018:
ASSERT_TRUE(CreateAndSetDownloadsDirectory(browser()));
I don't feel too strongly, but if this is only used here, should we inline it?

[Łukasz Anforowicz, 2016-10-07 23:32:03]

Thanks for the inlining suggestion - I think I like it better this way.  Do you
want to take another look?

https://codereview.chromium.org/2407513002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/2407513002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:1018:
ASSERT_TRUE(CreateAndSetDownloadsDirectory(browser()));
On 2016/10/07 21:11:38, Devlin wrote:
> I don't feel too strongly, but if this is only used here, should we inline it?

Hmmm... not sure.

Arguments for inlining:
- More localized code
- Extra |downloads_directory_| field is not needed outside of the testcase.

Arguments for keeping as-is:
- Easy to code-search for CreateAndSetDownloadsDirectory
- The inlined setup code is not really core part of the test,
  so maybe better to keep it away from the main test logic

In the latest patchset I've decided to inline, because I really like having the
ScopedTempDir as a local variable (rather than a field of the test fixture). 
I've mentioned CreateAndSetDownloadsDirectory in a comment to hopefully still
enable searching the code for similar test code.

https://codereview.chromium.org/2407513002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/2407513002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:1029:
"download-test3-attachment.gif")));
Inlining / having |downloads_directory| right here, also allows me add one more
simple test assert above.

[Łukasz Anforowicz, 2016-10-07 23:32:06]

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

[commit-bot: I haz the power, 2016-10-07 23:32:23]

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, 2016-10-08 00:09:51]

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

[commit-bot: I haz the power, 2016-10-08 00:09:52]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-10-10 14:38:50]

I'm fine with this as-is, but if you wanted to keep the
"CreateAndSetDownloadsDirectory", there's another option.

Either of these you decide to use lgtm.

https://codereview.chromium.org/2407513002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/2407513002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:1018:
ASSERT_TRUE(CreateAndSetDownloadsDirectory(browser()));
On 2016/10/07 23:32:02, Łukasz Anforowicz wrote:
> On 2016/10/07 21:11:38, Devlin wrote:
> > I don't feel too strongly, but if this is only used here, should we inline
it?
> 
> Hmmm... not sure.
> 
> Arguments for inlining:
> - More localized code
> - Extra |downloads_directory_| field is not needed outside of the testcase.
> 
> Arguments for keeping as-is:
> - Easy to code-search for CreateAndSetDownloadsDirectory
> - The inlined setup code is not really core part of the test,
>   so maybe better to keep it away from the main test logic
> 
> In the latest patchset I've decided to inline, because I really like having
the
> ScopedTempDir as a local variable (rather than a field of the test fixture). 
> I've mentioned CreateAndSetDownloadsDirectory in a comment to hopefully still
> enable searching the code for similar test code.

Another option: create a method in the anonymous namespace that takes a
PrefService and returns a ScopedTempDir.  That would allow for separation of
logic while retaining isolation to this test and having the dir as a local
variable.

[Devlin, 2016-10-10 14:38:51]

I'm fine with this as-is, but if you wanted to keep the
"CreateAndSetDownloadsDirectory", there's another option.

Either of these you decide to use lgtm.

[Łukasz Anforowicz, 2016-10-10 15:08:57]

https://codereview.chromium.org/2407513002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/2407513002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:1018:
ASSERT_TRUE(CreateAndSetDownloadsDirectory(browser()));
On 2016/10/10 14:38:50, Devlin wrote:
> On 2016/10/07 23:32:02, Łukasz Anforowicz wrote:
> > On 2016/10/07 21:11:38, Devlin wrote:
> > > I don't feel too strongly, but if this is only used here, should we inline
> it?
> > 
> > Hmmm... not sure.
> > 
> > Arguments for inlining:
> > - More localized code
> > - Extra |downloads_directory_| field is not needed outside of the testcase.
> > 
> > Arguments for keeping as-is:
> > - Easy to code-search for CreateAndSetDownloadsDirectory
> > - The inlined setup code is not really core part of the test,
> >   so maybe better to keep it away from the main test logic
> > 
> > In the latest patchset I've decided to inline, because I really like having
> the
> > ScopedTempDir as a local variable (rather than a field of the test fixture).

> > I've mentioned CreateAndSetDownloadsDirectory in a comment to hopefully
still
> > enable searching the code for similar test code.
> 
> Another option: create a method in the anonymous namespace that takes a
> PrefService and returns a ScopedTempDir.  That would allow for separation of
> logic while retaining isolation to this test and having the dir as a local
> variable.

Good idea - thanks.  Done.

[Łukasz Anforowicz, 2016-10-10 15:09:01]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-10-10 15:09:02]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2407513002/#ps40001
(title: "Making CreateAndSetDownloadsDirectory a standalone function.")

[commit-bot: I haz the power, 2016-10-10 15:09:16]

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, 2016-10-10 15:44:20]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-10 15:47:30]

Description was changed from

==========
Using ScopedTempDir as the download directory used by the DownloadViaPost test.

BUG=653856
==========

to

==========
Using ScopedTempDir as the download directory used by the DownloadViaPost test.

BUG=653856

Committed: https://crrev.com/0afc6b3cfc1d61d2e8371b4c46cc72abd26b2a04
Cr-Commit-Position: refs/heads/master@{#424157}
==========

[commit-bot: I haz the power, 2016-10-10 15:47:31]

Patchset 3 (id:??) landed as
https://crrev.com/0afc6b3cfc1d61d2e8371b4c46cc72abd26b2a04
Cr-Commit-Position: refs/heads/master@{#424157}