Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
292443006:
New ZipReader::ExtractCurrentEntryToString API. (Closed)
|
Created:
6 years, 7 months ago by João Eiras Modified:
6 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src Visibility:
Public. |
DescriptionNew ZipReader::ExtractCurrentEntryToString API.
New API which extract a zip entry into a memory buffer.
BUG=359428
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278766
Patch Set 1 #
Total comments: 8
Patch Set 2 : review comments #
Total comments: 4
Patch Set 3 : #Patch Set 4 : rebase #
Messages
Total messages: 17 (0 generated)
Hi again. Another chunk. For reference, original review https://codereview.chromium.org/179963002/
sorry for the belated response! https://codereview.chromium.org/292443006/diff/1/third_party/zlib/google/zip_... File third_party/zlib/google/zip_reader.cc (right): https://codereview.chromium.org/292443006/diff/1/third_party/zlib/google/zip_... third_party/zlib/google/zip_reader.cc:355: output->clear(); i think we should always call output->clear() even if it's not a directory because |output| may contain something. https://codereview.chromium.org/292443006/diff/1/third_party/zlib/google/zip_... third_party/zlib/google/zip_reader.cc:369: max_read_bytes, current_entry_info()->original_size()) + 1); i think we don't need +1 https://codereview.chromium.org/292443006/diff/1/third_party/zlib/google/zip_... third_party/zlib/google/zip_reader.cc:374: // This just sets size() to be equal to capacity() so the chars which are read line should be < 80 chars https://codereview.chromium.org/292443006/diff/1/third_party/zlib/google/zip_... third_party/zlib/google/zip_reader.cc:376: contents.resize(contents.capacity()); hmm, this looks complex. I understand that you are trying to avoid copying bytes, but I think the cost for copying bytes is small enough compared to the expensive computation required for uncompression. I'd suggest to simply append() data, like: char buffer[kZipBufSize]; while (true) { bytes_read = unzReadCurrentFile(zip_file_, buf, kZipBufiSize]; if (bytes_read == 0) { break; } else if (bytes_read < 0) { success = false; break; } else { if (contents.size() + bytes_read > max_read_bytes) { success = false; break; } contents.append(buffer, kZipBufSize); } } This way, you don't have to worry about resizing and overrunning |contents|.
https://codereview.chromium.org/292443006/diff/1/third_party/zlib/google/zip_... File third_party/zlib/google/zip_reader.cc (right): https://codereview.chromium.org/292443006/diff/1/third_party/zlib/google/zip_... third_party/zlib/google/zip_reader.cc:355: output->clear(); On 2014/05/23 23:45:47, satorux1 wrote: > i think we should always call output->clear() even if it's not a directory > because |output| may contain something. It's my personal style to avoid changing output parameters in case the function fails. Ahead, the loop uses a new string (std::string contents) and in case of success swaps with the output arg. Are you fine with that ? https://codereview.chromium.org/292443006/diff/1/third_party/zlib/google/zip_... third_party/zlib/google/zip_reader.cc:369: max_read_bytes, current_entry_info()->original_size()) + 1); On 2014/05/23 23:45:47, satorux1 wrote: > i think we don't need +1 Was there for unzReadCurrentFile() to tell if it hit the eof on the first call. https://codereview.chromium.org/292443006/diff/1/third_party/zlib/google/zip_... third_party/zlib/google/zip_reader.cc:376: contents.resize(contents.capacity()); On 2014/05/23 23:45:47, satorux1 wrote: > hmm, this looks complex. I understand that you are trying to avoid copying > bytes, but I think the cost for copying bytes is small enough compared to the > expensive computation required for uncompression. > > I'd suggest to simply append() data, like: > > char buffer[kZipBufSize]; > while (true) { > bytes_read = unzReadCurrentFile(zip_file_, buf, kZipBufiSize]; > if (bytes_read == 0) { > break; > } else if (bytes_read < 0) { > success = false; > break; > } else { > if (contents.size() + bytes_read > max_read_bytes) { > success = false; > break; > } > contents.append(buffer, kZipBufSize); > } > } > > This way, you don't have to worry about resizing and overrunning |contents|. It does look a bit overly complicated, but then my concern was for the common case (the original size being properly set) the loop body only runs once and there's only one call to unzReadCurrentFile() which fills the buffer in one go. But I'll accept your change.
https://codereview.chromium.org/292443006/diff/1/third_party/zlib/google/zip_... File third_party/zlib/google/zip_reader.cc (right): https://codereview.chromium.org/292443006/diff/1/third_party/zlib/google/zip_... third_party/zlib/google/zip_reader.cc:355: output->clear(); On 2014/06/02 16:41:07, João Eiras wrote: > On 2014/05/23 23:45:47, satorux1 wrote: > > i think we should always call output->clear() even if it's not a directory > > because |output| may contain something. > > It's my personal style to avoid changing output parameters in case the function > fails. > > Ahead, the loop uses a new string (std::string contents) and in case of success > swaps with the output arg. > > Are you fine with that ? That's fine by me. https://codereview.chromium.org/292443006/diff/20001/third_party/zlib/google/... File third_party/zlib/google/zip_reader_unittest.cc (right): https://codereview.chromium.org/292443006/diff/20001/third_party/zlib/google/... third_party/zlib/google/zip_reader_unittest.cc:553: base::StringPrintf(FILE_PATH_LITERAL("%d.txt"), static_cast<int>(i))); This looks tricky. FILE_PATH_LITERAL() returns a wstring. StringPrintf can probably handle wstring on Windows, but I'm not sure. Instead, how about: base::FilePath file_name = base::FilePath::FromUTF8Unsafe( base::StringPrintf("%d.txt", static_cast<int>(i)));
https://codereview.chromium.org/292443006/diff/20001/third_party/zlib/google/... File third_party/zlib/google/zip_reader_unittest.cc (right): https://codereview.chromium.org/292443006/diff/20001/third_party/zlib/google/... third_party/zlib/google/zip_reader_unittest.cc:553: base::StringPrintf(FILE_PATH_LITERAL("%d.txt"), static_cast<int>(i))); On 2014/06/03 08:17:40, satorux1 wrote: > This looks tricky. FILE_PATH_LITERAL() returns a wstring. StringPrintf can > probably handle wstring on Windows, but I'm not sure. Instead, how about: > > > base::FilePath file_name = base::FilePath::FromUTF8Unsafe( > base::StringPrintf("%d.txt", static_cast<int>(i))); There is an overload for string and wstring. Works fine on windows.
https://codereview.chromium.org/292443006/diff/20001/third_party/zlib/google/... File third_party/zlib/google/zip_reader_unittest.cc (right): https://codereview.chromium.org/292443006/diff/20001/third_party/zlib/google/... third_party/zlib/google/zip_reader_unittest.cc:553: base::StringPrintf(FILE_PATH_LITERAL("%d.txt"), static_cast<int>(i))); On 2014/06/03 14:39:02, João Eiras wrote: > On 2014/06/03 08:17:40, satorux1 wrote: > > This looks tricky. FILE_PATH_LITERAL() returns a wstring. StringPrintf can > > probably handle wstring on Windows, but I'm not sure. Instead, how about: > > > > > > base::FilePath file_name = base::FilePath::FromUTF8Unsafe( > > base::StringPrintf("%d.txt", static_cast<int>(i))); > > There is an overload for string and wstring. Works fine on windows. i think relying on the overload is not obvious hence i found it tricky.
https://codereview.chromium.org/292443006/diff/20001/third_party/zlib/google/... File third_party/zlib/google/zip_reader_unittest.cc (right): https://codereview.chromium.org/292443006/diff/20001/third_party/zlib/google/... third_party/zlib/google/zip_reader_unittest.cc:553: base::StringPrintf(FILE_PATH_LITERAL("%d.txt"), static_cast<int>(i))); On 2014/06/03 21:30:15, satorux1 wrote: > On 2014/06/03 14:39:02, João Eiras wrote: > > On 2014/06/03 08:17:40, satorux1 wrote: > > > This looks tricky. FILE_PATH_LITERAL() returns a wstring. StringPrintf can > > > probably handle wstring on Windows, but I'm not sure. Instead, how about: > > > > > > > > > base::FilePath file_name = base::FilePath::FromUTF8Unsafe( > > > base::StringPrintf("%d.txt", static_cast<int>(i))); > > > > There is an overload for string and wstring. Works fine on windows. > > i think relying on the overload is not obvious hence i found it tricky. OK, changed.
LGTM
The CQ bit was checked by joaoe@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joaoe@opera.com/292443006/50001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by joaoe@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joaoe@opera.com/292443006/50001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
Message was sent while issue was closed.
Change committed as 278766 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
