Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(15)

Issue 2275553005: //base: Make ScopedTempDir::path() a GetPath() with a DCHECK (Closed)

Created:
4 years, 6 months ago by vabr (Chromium)
Modified:
4 years, 6 months ago
Reviewers:
danakj
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

//base: Make ScopedTempDir::path() a GetPath() with a DCHECK This CL changes the inline ScopedTempDir::path() to an out-of-line GetPath() and enhances it with a DCHECK against the directory not having been created yet. FAQ Q: Why GetPath? A: Because of the added DCHECK in the path accessor, which needs #include "base/logging.h", there were two alternatives: (A) Keep the inline path() and #include "base/logging.h" in the header. (B) Change the inline path() to an out-of-line GetPath and implement it in the .cc file. (A) would affect compile-time, (B) affects run-time of the tests. I made no measuring, but obtaining the path is usually a one-off step in a test, so an unlikely hotspot. Therefore the trade-off seems much better for (B). Q: Why deprecating path() instead of deleting it? A: Due to the big amount of callsites (almost 600 files in the whole codebase), this change is rolled out in phases. This CL changes //base. After all of the codebase is converted, the deprecated path() will be deleted. R=danakj@chromium.org BUG=640599 Committed: https://crrev.com/411f4fc60df3a256dd2d15ab39c540228ed66e7f Cr-Commit-Position: refs/heads/master@{#417241}

Patch Set 1 #

Patch Set 2 : Converting //base #

Patch Set 3 : Add a hint to the DCHECK #

Total comments: 4

Patch Set 4 : Comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+258 lines, -245 lines) Patch
M base/debug/activity_tracker_unittest.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M base/files/dir_reader_posix_unittest.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M base/files/file_locking_unittest.cc View 1 2 chunks +4 lines, -4 lines 0 comments Download
M base/files/file_path_watcher_unittest.cc View 1 16 chunks +24 lines, -23 lines 0 comments Download
M base/files/file_proxy_unittest.cc View 1 2 3 16 chunks +29 lines, -34 lines 0 comments Download
M base/files/file_unittest.cc View 1 14 chunks +17 lines, -14 lines 0 comments Download
M base/files/file_util_proxy_unittest.cc View 1 2 3 4 chunks +10 lines, -16 lines 0 comments Download
M base/files/file_util_unittest.cc View 1 72 chunks +123 lines, -115 lines 0 comments Download
M base/files/important_file_writer_unittest.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M base/files/scoped_temp_dir.h View 1 1 chunk +6 lines, -0 lines 0 comments Download
M base/files/scoped_temp_dir.cc View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
M base/files/scoped_temp_dir_unittest.cc View 1 3 chunks +4 lines, -4 lines 0 comments Download
M base/json/json_value_serializer_unittest.cc View 1 4 chunks +5 lines, -4 lines 0 comments Download
M base/mac/mac_util_unittest.mm View 1 4 chunks +4 lines, -4 lines 0 comments Download
M base/metrics/persistent_histogram_allocator_unittest.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M base/metrics/persistent_memory_allocator_unittest.cc View 1 4 chunks +4 lines, -4 lines 0 comments Download
M base/path_service_unittest.cc View 1 4 chunks +6 lines, -6 lines 0 comments Download
M base/process/process_metrics_unittest.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M base/test/launcher/test_launcher_ios.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M base/test/launcher/unit_test_launcher.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M base/test/scoped_path_override.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M base/win/event_trace_consumer_unittest.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M base/win/event_trace_controller_unittest.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M base/win/shortcut_unittest.cc View 1 4 chunks +7 lines, -7 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 31 (18 generated)
vabr (Chromium)
Hello danakj@! Could you please review this? My goal is to provide a warning against ...
4 years, 6 months ago (2016-08-24 13:02:12 UTC) #4
vabr (Chromium)
On 2016/08/24 13:27:16, commit-bot: I haz the power wrote: > Dry run: Try jobs failed ...
4 years, 6 months ago (2016-08-24 13:33:36 UTC) #7
danakj
On 2016/08/24 13:33:36, vabr (OOO until 29 Aug) wrote: > On 2016/08/24 13:27:16, commit-bot: I ...
4 years, 6 months ago (2016-08-25 23:47:52 UTC) #8
danakj
On 2016/08/25 23:47:52, danakj wrote: > On 2016/08/24 13:33:36, vabr (OOO until 29 Aug) wrote: ...
4 years, 6 months ago (2016-08-25 23:48:43 UTC) #9
vabr (Chromium)
Thanks for the good comments! I do agree that unlike IsValid(), what I want to ...
4 years, 6 months ago (2016-09-06 19:27:20 UTC) #11
danakj
On Tue, Sep 6, 2016 at 12:27 PM, <vabr@chromium.org> wrote: > Thanks for the good ...
4 years, 6 months ago (2016-09-06 20:28:16 UTC) #12
vabr (Chromium)
Hi danakj@, This CL now morphed into the first step: introducing GetPath and converting //base ...
4 years, 6 months ago (2016-09-07 09:47:39 UTC) #21
vabr (Chromium)
Also, I plan to send a PSA to chromium-dev telling people to use GetPath() and ...
4 years, 6 months ago (2016-09-07 10:18:08 UTC) #22
danakj
LGTM https://codereview.chromium.org/2275553005/diff/50001/base/files/file_proxy_unittest.cc File base/files/file_proxy_unittest.cc (right): https://codereview.chromium.org/2275553005/diff/50001/base/files/file_proxy_unittest.cc#newcode91 base/files/file_proxy_unittest.cc:91: const FilePath& test_dir_path() const { return dir_.GetPath(); } ...
4 years, 6 months ago (2016-09-07 20:47:18 UTC) #23
vabr (Chromium)
Thanks! I removed one test_dir_path() because it was not used (see below), and also ran ...
4 years, 6 months ago (2016-09-08 07:45:36 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2275553005/70001
4 years, 6 months ago (2016-09-08 07:50:00 UTC) #27
commit-bot: I haz the power
Committed patchset #4 (id:70001)
4 years, 6 months ago (2016-09-08 09:26:46 UTC) #29
commit-bot: I haz the power
4 years, 6 months ago (2016-09-08 09:28:37 UTC) #31
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/411f4fc60df3a256dd2d15ab39c540228ed66e7f
Cr-Commit-Position: refs/heads/master@{#417241}

Powered by Google App Engine
This is Rietveld 408576698