Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(759)

Issue 2318033002: c/browser, c/common, components M-N: Change ScopedTempDir::path() to GetPath() (Closed)

Created:
4 years, 3 months ago by vabr (Chromium)
Modified:
4 years, 3 months ago
CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org, feature-media-reviews_chromium.org, gcasto+watchlist_chromium.org, grt+watch_chromium.org, kinuko+fileapi, mcasas+watch+vc_chromium.org, Matt Giuca, miu+watch_chromium.org, nhiroki, phoglund+watch_chromium.org, posciak+watch_chromium.org, rginda+watch_chromium.org, tapted, tfarina, Lei Zhang, tnakamura+watch_chromium.org, tommycli, tzik, vabr+watchlistpasswordmanager_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

c/browser, c/common, components M-N: Change ScopedTempDir::path() to GetPath() path() is being deprecated, GetPath() has better checking against wrong use. For more context, see https://codereview.chromium.org/2275553005/. This CL does two non-trivial changes in addition to the mechanical renaming and reformatting: * In components/nacl/browser/pnacl_translation_cache_unittest.cc, and * in chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc the CL modifies the test code to keep track whether the ScopedTempDir was initialised and can vend the path, or whether it was not, and cannot be asked for the path. In both cases, before this CL, ScopedTempDir needed to trust the test code to not to store any files under the uninitialised path. After the CL, ScopedTempDir can assume that whenever it vends the path, files may be written under it, and the contract of not touching the uninitialised path stays limited to the test and the tested code. BUG=640599 R=mseaborn@chromium.org, reillyg@chromium.org TBR=rsesek@chromium.org, blundell@chromium.org, sergeyu@chromium.org Committed: https://crrev.com/6c5aae893b0dea189b18412f3795dba30d12f721 Cr-Commit-Position: refs/heads/master@{#418798}

Patch Set 1 #

Patch Set 2 : Add components #

Patch Set 3 : Remove O-P #

Patch Set 4 : Just rebased #

Patch Set 5 : Just rebased #

Patch Set 6 : Rebase and fix PnaclTranslationCacheTest #

Total comments: 3

Patch Set 7 : Fix ItunesFileUtilTest #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+124 lines, -125 lines) Patch
M chrome/browser/media/webrtc/tab_desktop_media_list_unittest.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/media/webrtc/webrtc_audio_quality_browsertest.cc View 1 2 3 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/browser/media/webrtc/webrtc_log_uploader_unittest.cc View 1 2 3 4 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/media/webrtc/webrtc_log_util_unittest.cc View 1 2 3 3 chunks +7 lines, -6 lines 0 comments Download
M chrome/browser/media/webrtc/webrtc_rtp_dump_handler_unittest.cc View 1 2 3 2 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/media/webrtc/webrtc_rtp_dump_writer_unittest.cc View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/media/webrtc/webrtc_video_quality_browsertest.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/media_galleries/fileapi/iapps_finder_impl_win_browsertest.cc View 1 chunk +2 lines, -6 lines 0 comments Download
M chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc View 1 2 3 1 chunk +2 lines, -4 lines 2 comments Download
M chrome/browser/media_galleries/fileapi/itunes_file_util_unittest.cc View 1 2 3 4 5 6 4 chunks +11 lines, -11 lines 3 comments Download
M chrome/browser/media_galleries/fileapi/media_file_validator_browsertest.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/media_galleries/fileapi/native_media_file_util_unittest.cc View 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/media_galleries/fileapi/picasa_data_provider_browsertest.cc View 1 2 3 2 chunks +5 lines, -5 lines 0 comments Download
M chrome/browser/media_galleries/fileapi/picasa_file_util_unittest.cc View 6 chunks +10 lines, -10 lines 0 comments Download
M chrome/browser/media_galleries/gallery_watch_manager_unittest.cc View 10 chunks +21 lines, -21 lines 0 comments Download
M chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm View 1 chunk +4 lines, -4 lines 0 comments Download
M chrome/browser/media_galleries/media_file_system_registry_unittest.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/media_galleries/media_galleries_test_util.cc View 3 chunks +7 lines, -7 lines 0 comments Download
M chrome/common/mac/app_mode_chrome_locator_browsertest.mm View 1 chunk +2 lines, -4 lines 0 comments Download
M components/metrics/file_metrics_provider_unittest.cc View 1 4 chunks +24 lines, -18 lines 0 comments Download
M components/metrics/serialization/serialization_utils_unittest.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M components/nacl/browser/nacl_file_host_unittest.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M components/nacl/browser/pnacl_host_unittest.cc View 1 3 chunks +3 lines, -3 lines 0 comments Download
M components/nacl/browser/pnacl_translation_cache_unittest.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M components/net_log/net_log_file_writer_unittest.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M components/ntp_snippets/ntp_snippets_database_unittest.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M components/ntp_snippets/ntp_snippets_service_unittest.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download

Dependent Patchsets:

Messages

Total messages: 35 (23 generated)
vabr (Chromium)
Hello reviewers! @mseaborn: Please review components/nacl/browser/*. The only non-mechanical change is highlighted below. @blundell: Please ...
4 years, 3 months ago (2016-09-14 13:32:42 UTC) #16
blundell
//components - nacl lgtm
4 years, 3 months ago (2016-09-14 14:01:03 UTC) #17
vabr (Chromium)
Adding two more reviewers: reillyg@: Please review chrome/browser/media_galleries. I highlighted the only non-trivial change by ...
4 years, 3 months ago (2016-09-14 16:22:17 UTC) #22
Robert Sesek
app_mode_chrome_locator_browsertest.mm LGTM
4 years, 3 months ago (2016-09-14 16:31:02 UTC) #23
Reilly Grant (use Gerrit)
lgtm https://codereview.chromium.org/2318033002/diff/120001/chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc File chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc (right): https://codereview.chromium.org/2318033002/diff/120001/chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc#newcode120 chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc:120: return library_dir_.GetPath().AppendASCII("library.xml"); I'm assuming this is correct because ...
4 years, 3 months ago (2016-09-14 18:06:20 UTC) #24
Mark Seaborn
https://codereview.chromium.org/2318033002/diff/100001/components/nacl/browser/pnacl_translation_cache_unittest.cc File components/nacl/browser/pnacl_translation_cache_unittest.cc (right): https://codereview.chromium.org/2318033002/diff/100001/components/nacl/browser/pnacl_translation_cache_unittest.cc#newcode63 components/nacl/browser/pnacl_translation_cache_unittest.cc:63: in_mem ? base::FilePath() : temp_dir_.GetPath(), On 2016/09/14 13:32:41, vabr ...
4 years, 3 months ago (2016-09-14 18:27:26 UTC) #25
vabr (Chromium)
Thanks for reviews! Mark and Reilly, please find your commends addressed below. Cheers, Vaclav https://codereview.chromium.org/2318033002/diff/100001/components/nacl/browser/pnacl_translation_cache_unittest.cc ...
4 years, 3 months ago (2016-09-14 20:14:39 UTC) #27
Reilly Grant (use Gerrit)
Thank you for the explanations. lgtm still stands.
4 years, 3 months ago (2016-09-14 20:25:35 UTC) #28
vabr (Chromium)
Thank you! TBR-ing sergeyu@ for chrome/browser/media/*, which only has trivial changes, and landing. Cheers, Vaclav
4 years, 3 months ago (2016-09-15 07:25:46 UTC) #29
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/2318033002/120001
4 years, 3 months ago (2016-09-15 07:26:07 UTC) #31
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 3 months ago (2016-09-15 07:31:31 UTC) #33
commit-bot: I haz the power
4 years, 3 months ago (2016-09-15 07:33:53 UTC) #35
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/6c5aae893b0dea189b18412f3795dba30d12f721
Cr-Commit-Position: refs/heads/master@{#418798}

Powered by Google App Engine
This is Rietveld 408576698