Replacing base::DIR_TEMP with ScopedTempDir when appropriate.

Replacing base::DIR_TEMP with ScopedTempDir when appropriate.

BUG=73854
TEST=unit_tests


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

[Mike West, 2011-04-02 23:42:32]

Hey Dominic, one question related to this bug: as you mentioned in the
description,`base::DIR_TEMP` shows up 39 times in the codebase. A quick skim of
the results shows that more than half of the occurrences look like tests that
could be trivially replaced with `ScopedTempDir`.  I'm happy to take that on,
but I'm not sure how to approach it with regard to workflow. Would you expect a
separate CL for each file, reviewed by whoever feels responsible for the test in
question, or would you prefer all the bits and pieces to be wrapped up into this
CL? Let me know what's preferred, and I'll make it happen.  :)

[Paweł Hajdan Jr., 2011-04-04 08:08:54]

Drive-by with testing comments. By the way, feel free to just make changes to
all files in one CL and send it to me.

http://codereview.chromium.org/6793008/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_manager_unittest.cc:34: test_dir_ =
temp_dir_.path();
How about removing the member variable and using temp_dir_.path() directly in
tests?

http://codereview.chromium.org/6793008/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_manager_unittest.cc:45:
ASSERT_TRUE(temp_dir_.Delete());
This shouldn't be necessary. That's the entire point of the scoper.

http://codereview.chromium.org/6793008/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_manager_unittest.cc:57: ScopedTempDir temp_dir_;
Please make ScopedTempDir the first member variable declared here (before
message_loop_). Some tasks posted to it might want to use the directory.

[Mike West, 2011-04-04 08:39:33]

Great, I appreciate the feedback. I'll start going through some of the other
tests that could make use of `ScopedTempDir` once I get the following questions
about mechanics cleared up in this case:

http://codereview.chromium.org/6793008/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_manager_unittest.cc:34: test_dir_ =
temp_dir_.path();
On 2011/04/04 08:08:54, Paweł Hajdan Jr. wrote:
> How about removing the member variable and using temp_dir_.path() directly in
> tests?

Two reasons:

1. This was the minimal changeset, and I'd rather change the least amount code
possible for targeted cleanups (especially since I'm not familiar with most of
the tests I'll be fiddling with :) ).

2. Pulling this out into a temp variable made some sort of aesthetic sense to
me.

That said, I'm happy to change the implementation here if there's a good reason
to do so. Should be a trivial search/replace.

http://codereview.chromium.org/6793008/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_manager_unittest.cc:45:
ASSERT_TRUE(temp_dir_.Delete());
On 2011/04/04 08:08:54, Paweł Hajdan Jr. wrote:
> This shouldn't be necessary. That's the entire point of the scoper.

Maybe I'm misunderstanding the mechanics here, but I don't believe the object
actually goes out of scope between tests if it's stored as a member variable. If
I don't call `Delete` explicitly in `TearDown`, my expectation is that
`CreateUniqueTempDir` would throw an error in `SetUp` (based on my reading of
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/base/scoped_t...
), as the variable still exists across tests.

Does the whole `ProfileManagerTest` object get thrown away and recreated for
each test?

http://codereview.chromium.org/6793008/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_manager_unittest.cc:57: ScopedTempDir temp_dir_;
On 2011/04/04 08:08:54, Paweł Hajdan Jr. wrote:
> Please make ScopedTempDir the first member variable declared here (before
> message_loop_). Some tasks posted to it might want to use the directory.

Done.

[Paweł Hajdan Jr., 2011-04-04 09:05:31]

http://codereview.chromium.org/6793008/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_manager_unittest.cc:34: test_dir_ =
temp_dir_.path();
On 2011/04/04 08:39:33, Mike West wrote:
> 1. This was the minimal changeset, and I'd rather change the least amount code
> possible for targeted cleanups (especially since I'm not familiar with most of
> the tests I'll be fiddling with :) ).

Makes sense, but on the other hand it may be worth it to make the cleanup more
complete.

> 2. Pulling this out into a temp variable made some sort of aesthetic sense to
> me.

I'm not sure how that could be beneficial. I see is as a duplication of
information, and a potential for those two variables to go out of sync (someone
might have an idea to change test_dir_, and so on).

> That said, I'm happy to change the implementation here if there's a good
reason
> to do so. Should be a trivial search/replace.

I think the right thing to do is to make the change.

http://codereview.chromium.org/6793008/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_manager_unittest.cc:45:
ASSERT_TRUE(temp_dir_.Delete());
On 2011/04/04 08:39:33, Mike West wrote:
> Does the whole `ProfileManagerTest` object get thrown away and recreated for
> each test?

Yes, it does. Feel free to check with a debugger and/or read GTest docs.
Unfortunately many people are confused by this behavior.

[Mike West, 2011-04-04 09:16:07]

Thanks. I've run out of free time for the moment, but I plan to go through the
other 39 instances of base::DIR_TEMP on Thursday, once my schedule is back to
something like normal. You should have a CL for review by the end of the week.

http://codereview.chromium.org/6793008/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_manager_unittest.cc:34: test_dir_ =
temp_dir_.path();
On 2011/04/04 09:05:31, Paweł Hajdan Jr. wrote:

> I think the right thing to do is to make the change.

Fair enough, you've convinced me. :) I'll change it here, and drop it in the
other tests I'll touch later on.

http://codereview.chromium.org/6793008/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_manager_unittest.cc:45:
ASSERT_TRUE(temp_dir_.Delete());
On 2011/04/04 09:05:31, Paweł Hajdan Jr. wrote:

> Yes, it does. Feel free to check with a debugger and/or read GTest docs.
> Unfortunately many people are confused by this behavior.

Me, for instance. :)

[Mike West, 2011-04-08 15:25:34]

So, I've made it through about 17 of the ~40 `base::DIR_TEMP` instances.  Some
are slightly more complex than others to remove, but I'm making progress.

Paweł, it would be great if you could take a look at this batch now.  That would
save time in the long run, and ensure that there's less rework at the end if I'm
doing something systematically incorrect.  :)

[Paweł Hajdan Jr., 2011-04-08 15:58:17]

Great, some comments just to remove even more unneeded code. Please make sure to
scan the entire CL for comments that don't start with a capital letter or don't
end with a dot (only near to the code being changed).

http://codereview.chromium.org/6793008/diff/6001/app/sql/connection_unittest.cc
File app/sql/connection_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/app/sql/connection_unittest....
app/sql/connection_unittest.cc:24: // If this fails something is going on with
cleanup and later tests may
This can also be removed now.

http://codereview.chromium.org/6793008/diff/6001/app/sql/statement_unittest.cc
File app/sql/statement_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/app/sql/statement_unittest.c...
app/sql/statement_unittest.cc:61: ASSERT_TRUE(file_util::Delete(
Can be removed too.

http://codereview.chromium.org/6793008/diff/6001/app/sql/transaction_unittest.cc
File app/sql/transaction_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/app/sql/transaction_unittest...
app/sql/transaction_unittest.cc:28: // If this fails something is going on with
cleanup and later tests may
Can be removed.

http://codereview.chromium.org/6793008/diff/6001/base/i18n/file_util_icu_unit...
File base/i18n/file_util_icu_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/base/i18n/file_util_icu_unit...
base/i18n/file_util_icu_unittest.cc:14: class FileUtilICUTest : public
PlatformTest {
Please switch it to a typedef then, or at least don't override SetUp and
TearDown which just call the parent implementation.

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_html_writer_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_html_writer_unittest.cc:46: virtual void
SetUp() {
Oh, could you also make it call the parent's SetUp method?

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_html_writer_unittest.cc:49:
file_util::Delete(path_, true);
Shouldn't be needed.

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_html_writer_unittest.cc:52: virtual void
TearDown() {
Can you remove it entirely then?

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/history/text_...
File chrome/browser/history/text_database_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/history/text_...
chrome/browser/history/text_database_unittest.cc:95:
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
Make sure to call PlatformTest::SetUp(), that call shouldn't be removed!

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/history/text_...
chrome/browser/history/text_database_unittest.cc:99: for (size_t i = 0; i <
opened_files_.size(); i++)
Can we get rid of this TearDown then? And maybe opened_files_ too.

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/importer/fire...
File chrome/browser/importer/firefox_profile_lock_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/importer/fire...
chrome/browser/importer/firefox_profile_lock_unittest.cc:18: public:
nit: Can this "public:" bit be removed?

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/importer/fire...
chrome/browser/importer/firefox_profile_lock_unittest.cc:24: virtual void
TearDown() {
An empty TearDown can just be removed.

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager_unittest.cc:44:
ASSERT_TRUE(temp_dir_.Delete());
Shouldn't be needed.

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager_unittest.cc:51: // the path to temporary
directory used to contain the test operations
nit: Start with a capital letter, end with a dot.

http://codereview.chromium.org/6793008/diff/6001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_unpacker_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/chrome/common/extensions/ext...
chrome/common/extensions/extension_unpacker_unittest.cc:41: virtual void
TearDown() {
An empty TearDown can be removed.

http://codereview.chromium.org/6793008/diff/6001/chrome/common/json_pref_stor...
File chrome/common/json_pref_store_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/chrome/common/json_pref_stor...
chrome/common/json_pref_store_unittest.cc:34: virtual void TearDown() {
Empty TearDown can be removed.

http://codereview.chromium.org/6793008/diff/6001/chrome/installer/util/delete...
File chrome/installer/util/delete_tree_work_item_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/chrome/installer/util/delete...
chrome/installer/util/delete_tree_work_item_unittest.cc:25: // Name a
subdirectory of the user temp directory.
nit: This comment seems weird now.

http://codereview.chromium.org/6793008/diff/6001/chrome/installer/util/delete...
chrome/installer/util/delete_tree_work_item_unittest.cc:29: virtual void
TearDown() {
Empty TearDown can be removed.

[Mike West, 2011-04-11 11:07:06]

Thanks for the detailed review. :P I'll fiddle around with this a bit more today
while I'm on calls and try to get through another 10 files or so... might get it
done this week!

[Mike West, 2011-04-11 14:47:04]

http://codereview.chromium.org/6793008/diff/6001/app/sql/connection_unittest.cc
File app/sql/connection_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/app/sql/connection_unittest....
app/sql/connection_unittest.cc:24: // If this fails something is going on with
cleanup and later tests may
On 2011/04/08 15:58:17, Paweł Hajdan Jr. wrote:
> This can also be removed now.

Done.

http://codereview.chromium.org/6793008/diff/6001/app/sql/statement_unittest.cc
File app/sql/statement_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/app/sql/statement_unittest.c...
app/sql/statement_unittest.cc:61: ASSERT_TRUE(file_util::Delete(
On 2011/04/08 15:58:17, Paweł Hajdan Jr. wrote:
> Can be removed too.

Done.

http://codereview.chromium.org/6793008/diff/6001/app/sql/transaction_unittest.cc
File app/sql/transaction_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/app/sql/transaction_unittest...
app/sql/transaction_unittest.cc:28: // If this fails something is going on with
cleanup and later tests may
On 2011/04/08 15:58:17, Paweł Hajdan Jr. wrote:
> Can be removed.

Done.

http://codereview.chromium.org/6793008/diff/6001/base/i18n/file_util_icu_unit...
File base/i18n/file_util_icu_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/base/i18n/file_util_icu_unit...
base/i18n/file_util_icu_unittest.cc:14: class FileUtilICUTest : public
PlatformTest {
On 2011/04/08 15:58:17, Paweł Hajdan Jr. wrote:
> Please switch it to a typedef then, or at least don't override SetUp and
> TearDown which just call the parent implementation.

Fair enough. I've dropped the overrides, but left the class definition. A
typedef doesn't feel right here...

If you feel strongly otherwise, I'm happy to change it.

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_html_writer_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_html_writer_unittest.cc:46: virtual void
SetUp() {
On 2011/04/08 15:58:17, Paweł Hajdan Jr. wrote:
> Oh, could you also make it call the parent's SetUp method?

Done.

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_html_writer_unittest.cc:49:
file_util::Delete(path_, true);
On 2011/04/08 15:58:17, Paweł Hajdan Jr. wrote:
> Shouldn't be needed.

Done.

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_html_writer_unittest.cc:52: virtual void
TearDown() {
On 2011/04/08 15:58:17, Paweł Hajdan Jr. wrote:
> Can you remove it entirely then?

Done.

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/history/text_...
File chrome/browser/history/text_database_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/history/text_...
chrome/browser/history/text_database_unittest.cc:95:
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
On 2011/04/08 15:58:17, Paweł Hajdan Jr. wrote:
> Make sure to call PlatformTest::SetUp(), that call shouldn't be removed!

Yup, I hit `d` too many times.  Good catch!  :)

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/history/text_...
chrome/browser/history/text_database_unittest.cc:99: for (size_t i = 0; i <
opened_files_.size(); i++)
On 2011/04/08 15:58:17, Paweł Hajdan Jr. wrote:
> Can we get rid of this TearDown then? And maybe opened_files_ too.

Done.

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/importer/fire...
File chrome/browser/importer/firefox_profile_lock_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/importer/fire...
chrome/browser/importer/firefox_profile_lock_unittest.cc:18: public:
On 2011/04/08 15:58:17, Paweł Hajdan Jr. wrote:
> nit: Can this "public:" bit be removed?

Done.

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/importer/fire...
chrome/browser/importer/firefox_profile_lock_unittest.cc:24: virtual void
TearDown() {
On 2011/04/08 15:58:17, Paweł Hajdan Jr. wrote:
> An empty TearDown can just be removed.

Done.

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager_unittest.cc:44:
ASSERT_TRUE(temp_dir_.Delete());
On 2011/04/08 15:58:17, Paweł Hajdan Jr. wrote:
> Shouldn't be needed.

Done.

http://codereview.chromium.org/6793008/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_manager_unittest.cc:51: // the path to temporary
directory used to contain the test operations
On 2011/04/08 15:58:17, Paweł Hajdan Jr. wrote:
> nit: Start with a capital letter, end with a dot.

Done.

http://codereview.chromium.org/6793008/diff/6001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_unpacker_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/chrome/common/extensions/ext...
chrome/common/extensions/extension_unpacker_unittest.cc:41: virtual void
TearDown() {
On 2011/04/08 15:58:17, Paweł Hajdan Jr. wrote:
> An empty TearDown can be removed.

Done.

http://codereview.chromium.org/6793008/diff/6001/chrome/common/json_pref_stor...
File chrome/common/json_pref_store_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/chrome/common/json_pref_stor...
chrome/common/json_pref_store_unittest.cc:34: virtual void TearDown() {
On 2011/04/08 15:58:17, Paweł Hajdan Jr. wrote:
> Empty TearDown can be removed.

Done.

http://codereview.chromium.org/6793008/diff/6001/chrome/installer/util/delete...
File chrome/installer/util/delete_tree_work_item_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/6001/chrome/installer/util/delete...
chrome/installer/util/delete_tree_work_item_unittest.cc:25: // Name a
subdirectory of the user temp directory.
On 2011/04/08 15:58:17, Paweł Hajdan Jr. wrote:
> nit: This comment seems weird now.

Done.

http://codereview.chromium.org/6793008/diff/6001/chrome/installer/util/delete...
chrome/installer/util/delete_tree_work_item_unittest.cc:29: virtual void
TearDown() {
On 2011/04/08 15:58:17, Paweł Hajdan Jr. wrote:
> Empty TearDown can be removed.

Done.

[Paweł Hajdan Jr., 2011-04-11 18:16:53]

http://codereview.chromium.org/6793008/diff/7055/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/6793008/diff/7055/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_browsertest.cc:38:
DCHECK(temp_dir_.CreateUniqueTempDir());
Oops, two things are wrong about that:

1) DCHECK only runs in Debug mode. The temporary directory will not be created
in Release mode.

2) We avoid DCHECK in tests because they abort the entire process. Instead, you
should be able to just use EXPECT_TRUE. Also, it's probably better to put it in
something SetUp-like rather than the ctor.

http://codereview.chromium.org/6793008/diff/7055/chrome/browser/extensions/sa...
File chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/7055/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc:81: // write
the CRX to the temp directory, and create a subdirectory into
nit: "create write" (see the previous line). Please make sure to make the entire
sentence correct.

http://codereview.chromium.org/6793008/diff/7055/chrome/browser/ui/tests/brow...
File chrome/browser/ui/tests/browser_uitest.cc (right):

http://codereview.chromium.org/6793008/diff/7055/chrome/browser/ui/tests/brow...
chrome/browser/ui/tests/browser_uitest.cc:287:
file_util::CreateDirectory(tmp_profile);
Could you check the return value of that? Just ASSERT_TRUE.

http://codereview.chromium.org/6793008/diff/7055/chrome/browser/ui/tests/brow...
chrome/browser/ui/tests/browser_uitest.cc:297: void TearDown() {
You can remove the entire TearDown now.

http://codereview.chromium.org/6793008/diff/7055/net/disk_cache/disk_cache_te...
File net/disk_cache/disk_cache_test_util.cc (right):

http://codereview.chromium.org/6793008/diff/7055/net/disk_cache/disk_cache_te...
net/disk_cache/disk_cache_test_util.cc:26: if (!temp_dir_.IsValid())
Oh, let's leave this one as-is. I know Ricardo Vargas wants this to be at a
predictable location. Alternatively, please add him as a reviewer.

[Mike West, 2011-04-12 07:04:04]

Some fixes, and a question about the EXPECT_TRUE/SetUp issue you noted.

http://codereview.chromium.org/6793008/diff/7055/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/6793008/diff/7055/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_browsertest.cc:38:
DCHECK(temp_dir_.CreateUniqueTempDir());
Right, I kinda knew that DCHECK was wrong, but I didn't know about EXPECT_TRUE. 
Thanks for the tip.

With regard to putting the check in the ctor, I agree that it's a bit rough, but
I don't know this class well enough to know what gets called in what order.
Putting it in the constructor ensured that it got set up once and only once
before any of the methods that end up calling `PackExtension`.

Adding a `SetUp` method is certainly viable, but then I'd just be calling that
from the constructor. I'm not sure that's any better.

http://codereview.chromium.org/6793008/diff/7055/chrome/browser/extensions/sa...
File chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/7055/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc:81: // write
the CRX to the temp directory, and create a subdirectory into
On 2011/04/11 18:16:53, Paweł Hajdan Jr. wrote:
> nit: "create write" (see the previous line). Please make sure to make the
entire
> sentence correct.

Good eye.  Thanks!

http://codereview.chromium.org/6793008/diff/7055/chrome/browser/ui/tests/brow...
File chrome/browser/ui/tests/browser_uitest.cc (right):

http://codereview.chromium.org/6793008/diff/7055/chrome/browser/ui/tests/brow...
chrome/browser/ui/tests/browser_uitest.cc:287:
file_util::CreateDirectory(tmp_profile);
On 2011/04/11 18:16:53, Paweł Hajdan Jr. wrote:
> Could you check the return value of that? Just ASSERT_TRUE.

Done.

http://codereview.chromium.org/6793008/diff/7055/chrome/browser/ui/tests/brow...
chrome/browser/ui/tests/browser_uitest.cc:297: void TearDown() {
On 2011/04/11 18:16:53, Paweł Hajdan Jr. wrote:
> You can remove the entire TearDown now.

Done.

http://codereview.chromium.org/6793008/diff/7055/net/disk_cache/disk_cache_te...
File net/disk_cache/disk_cache_test_util.cc (right):

http://codereview.chromium.org/6793008/diff/7055/net/disk_cache/disk_cache_te...
net/disk_cache/disk_cache_test_util.cc:26: if (!temp_dir_.IsValid())
On 2011/04/11 18:16:53, Paweł Hajdan Jr. wrote:
> Oh, let's leave this one as-is. I know Ricardo Vargas wants this to be at a
> predictable location. Alternatively, please add him as a reviewer.

Great, this is exactly why I'm thrilled you're reviewing the changes.  :)

[Paweł Hajdan Jr., 2011-04-12 08:32:02]

LGTM with comments, thank you!

http://codereview.chromium.org/6793008/diff/7055/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/6793008/diff/7055/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_browsertest.cc:38:
DCHECK(temp_dir_.CreateUniqueTempDir());
On 2011/04/12 07:04:04, Mike West wrote:
> With regard to putting the check in the ctor, I agree that it's a bit rough,
but
> I don't know this class well enough to know what gets called in what order.
> Putting it in the constructor ensured that it got set up once and only once
> before any of the methods that end up calling `PackExtension`.
> 
> Adding a `SetUp` method is certainly viable, but then I'd just be calling that
> from the constructor. I'm not sure that's any better.

EXPECT_TRUE in ctor is fine for me. BrowserTest has a tricky initialization,
lots of SetUp-like methods.

http://codereview.chromium.org/6793008/diff/11036/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service_uitest.cc (right):

http://codereview.chromium.org/6793008/diff/11036/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_uitest.cc:28:
file_util::CreateDirectory(tmp_profile);
Ah, check return value of this (ASSERT_TRUE).

http://codereview.chromium.org/6793008/diff/11036/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/bloom_filter_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/11036/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/bloom_filter_unittest.cc:96:
file_util::Delete(filter_path, false);
This line shouldn't be needed.

http://codereview.chromium.org/6793008/diff/11036/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/bloom_filter_unittest.cc:97:
ASSERT_FALSE(file_util::PathExists(filter_path));
This too.

http://codereview.chromium.org/6793008/diff/11036/net/disk_cache/disk_cache_t...
File net/disk_cache/disk_cache_test_util.cc (right):

http://codereview.chromium.org/6793008/diff/11036/net/disk_cache/disk_cache_t...
net/disk_cache/disk_cache_test_util.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
nit: It's probably better to remove this file from the CL if there are no other
changes to it. Up to you.

[Mike West, 2011-04-12 12:00:04]

Ok, I think everything you've noted is taken care of. Anything else catch your
eye? :) If not, I'll ask our friendly commit-bot to take care of this once the
trybots finish their run.

http://codereview.chromium.org/6793008/diff/11036/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service_uitest.cc (right):

http://codereview.chromium.org/6793008/diff/11036/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_uitest.cc:28:
file_util::CreateDirectory(tmp_profile);
On 2011/04/12 08:32:02, Paweł Hajdan Jr. wrote:
> Ah, check return value of this (ASSERT_TRUE).

Done.

http://codereview.chromium.org/6793008/diff/11036/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/bloom_filter_unittest.cc (right):

http://codereview.chromium.org/6793008/diff/11036/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/bloom_filter_unittest.cc:96:
file_util::Delete(filter_path, false);
On 2011/04/12 08:32:02, Paweł Hajdan Jr. wrote:
> This line shouldn't be needed.

Done.

http://codereview.chromium.org/6793008/diff/11036/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/bloom_filter_unittest.cc:97:
ASSERT_FALSE(file_util::PathExists(filter_path));
On 2011/04/12 08:32:02, Paweł Hajdan Jr. wrote:
> This too.

Done.

http://codereview.chromium.org/6793008/diff/11036/net/disk_cache/disk_cache_t...
File net/disk_cache/disk_cache_test_util.cc (right):

http://codereview.chromium.org/6793008/diff/11036/net/disk_cache/disk_cache_t...
net/disk_cache/disk_cache_test_util.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2011/04/12 08:32:02, Paweł Hajdan Jr. wrote:
> nit: It's probably better to remove this file from the CL if there are no
other
> changes to it. Up to you.

Yup. I reverted the actual changes to the file, but missed the other commit
where I fiddled with the licenses. :) Fixed now.

[Paweł Hajdan Jr., 2011-04-12 12:17:10]

LGTM (you didn't need to wait for that; when fixes are trivial and in this case
they are, reviewers often say something like "LGTM with comments", which means
you can commit without another review after addressing those comments).

[commit-bot: I haz the power, 2011-04-12 13:40:59]

Change committed as 81245