Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
680643003:
Read contents of test file after getting its absolute path. (Closed)
DescriptionRemoves hard-coded 10K limit on file-size.
BUG=
Committed: https://crrev.com/8cd061582f4996cfe121232da9d957565cf6adfb
Cr-Commit-Position: refs/heads/master@{#301510}
Patch Set 1 #
Total comments: 8
Patch Set 2 : assert_ne instead of assert_false and compare. #
Total comments: 2
Patch Set 3 : Create absolute path, for use in call to ReadFileToString. #Messages
Total messages: 15 (4 generated)
Patchset #1 (id:1) has been deleted
anandc@chromium.org changed reviewers: + jamiewalch@chromium.org
Hi Jamie, Please take a look. Thanks.
https://codereview.chromium.org/680643003/diff/20001/chrome/test/remoting/rem... File chrome/test/remoting/remote_desktop_browsertest.cc (right): https://codereview.chromium.org/680643003/diff/20001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:829: scoped_ptr<char[]> file_contents(new char[accounts_file_size]); scoped_array feels like a better fit here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/cld/ba...
lambroslambrou@chromium.org changed reviewers: + lambroslambrou@chromium.org
https://codereview.chromium.org/680643003/diff/20001/chrome/test/remoting/rem... File chrome/test/remoting/remote_desktop_browsertest.cc (right): https://codereview.chromium.org/680643003/diff/20001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:830: ASSERT_FALSE(base::ReadFile(accounts_file_path, file_contents.get(), ASSERT_NE https://codereview.chromium.org/680643003/diff/20001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:834: std::string accounts_info(file_contents.get(), accounts_file_size); Why not use base::ReadFileToString() ?
Thanks Jamie and Lambros. PTAL at patch-set #2. https://codereview.chromium.org/680643003/diff/20001/chrome/test/remoting/rem... File chrome/test/remoting/remote_desktop_browsertest.cc (right): https://codereview.chromium.org/680643003/diff/20001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:829: scoped_ptr<char[]> file_contents(new char[accounts_file_size]); On 2014/10/25 00:05:14, Jamie wrote: > scoped_array feels like a better fit here: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/cld/ba... As discussed offline, scoped_ptr does the work of delete[], so it should do for our purposes. https://codereview.chromium.org/680643003/diff/20001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:830: ASSERT_FALSE(base::ReadFile(accounts_file_path, file_contents.get(), On 2014/10/25 00:13:09, Lambros wrote: > ASSERT_NE Done. https://codereview.chromium.org/680643003/diff/20001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:834: std::string accounts_info(file_contents.get(), accounts_file_size); On 2014/10/25 00:13:09, Lambros wrote: > Why not use base::ReadFileToString() ? Yes, we actually were using ReadFileToString earlier. See comment on line #824 of this patch. But ReadFileToString doesn't work when the path to the file is relative. See comment here: https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_ut... On the Chromoting waterfall, the input file path is going to be relative on the test slaves. So we have to use Read() instead.
lgtm
https://codereview.chromium.org/680643003/diff/20001/chrome/test/remoting/rem... File chrome/test/remoting/remote_desktop_browsertest.cc (right): https://codereview.chromium.org/680643003/diff/20001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:834: std::string accounts_info(file_contents.get(), accounts_file_size); On 2014/10/27 19:17:53, anandc wrote: > On 2014/10/25 00:13:09, Lambros wrote: > > Why not use base::ReadFileToString() ? > > Yes, we actually were using ReadFileToString earlier. See comment on line #824 > of this patch. > But ReadFileToString doesn't work when the path to the file is relative. > See comment here: > https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_ut... > On the Chromoting waterfall, the input file path is going to be relative on the > test slaves. So we have to use Read() instead. Sorry, I didn't see the comment. Maybe call base::MakeAbsoluteFilePath() to convert to an absolute path, and then pass that in to base::ReadFileToString() ? Re-implementing ReadFileToString() from scratch to circumvent a security limitation doesn't seem the right thing to do. https://codereview.chromium.org/680643003/diff/40001/chrome/test/remoting/rem... File chrome/test/remoting/remote_desktop_browsertest.cc (right): https://codereview.chromium.org/680643003/diff/40001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:828: base::GetFileSize(accounts_file_path, &accounts_file_size); Please ASSERT_TRUE() on base::GetFileSize(). If it fails, |accounts_file_size| will hold garbage.
Thanks Lambros. PTAL at PS #3. https://codereview.chromium.org/680643003/diff/20001/chrome/test/remoting/rem... File chrome/test/remoting/remote_desktop_browsertest.cc (right): https://codereview.chromium.org/680643003/diff/20001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:834: std::string accounts_info(file_contents.get(), accounts_file_size); On 2014/10/27 21:20:49, Lambros wrote: > On 2014/10/27 19:17:53, anandc wrote: > > On 2014/10/25 00:13:09, Lambros wrote: > > > Why not use base::ReadFileToString() ? > > > > Yes, we actually were using ReadFileToString earlier. See comment on line #824 > > of this patch. > > But ReadFileToString doesn't work when the path to the file is relative. > > See comment here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_ut... > > On the Chromoting waterfall, the input file path is going to be relative on > the > > test slaves. So we have to use Read() instead. > > Sorry, I didn't see the comment. Maybe call base::MakeAbsoluteFilePath() to > convert to an absolute path, and then pass that in to base::ReadFileToString() ? > > Re-implementing ReadFileToString() from scratch to circumvent a security > limitation doesn't seem the right thing to do. Done. https://codereview.chromium.org/680643003/diff/40001/chrome/test/remoting/rem... File chrome/test/remoting/remote_desktop_browsertest.cc (right): https://codereview.chromium.org/680643003/diff/40001/chrome/test/remoting/rem... chrome/test/remoting/remote_desktop_browsertest.cc:828: base::GetFileSize(accounts_file_path, &accounts_file_size); On 2014/10/27 21:20:50, Lambros wrote: > Please ASSERT_TRUE() on base::GetFileSize(). If it fails, |accounts_file_size| > will hold garbage. Acknowledged.
lgtm
The CQ bit was checked by anandc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/680643003/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8cd061582f4996cfe121232da9d957565cf6adfb Cr-Commit-Position: refs/heads/master@{#301510} |
