Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
93243003:
Add CDM FileIO tests. (Closed)
|
Created:
7 years ago by xhwang Modified:
7 years ago Reviewers:
ddorwin CC:
chromium-reviews, yusukes+watch_chromium.org, raymes+watch_chromium.org, feature-media-reviews_chromium.org, teravest+watch_chromium.org, yzshen+watch_chromium.org, nfullagar1, piman+watch_chromium.org, noelallen1, binji, ihf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd CDM FileIO tests.
- Add CdmFileIOTest, which tests CdmFileIO in ClearKeyCdm.
- Update EncryptedMediaTest to check the result of CdmFileIOTest.
BUG=324134
TEST=Tests added pass on Linux.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242027
Patch Set 1 #
Total comments: 78
Patch Set 2 : rebase only #Patch Set 3 : more rebase cleanup #Patch Set 4 : comments addressed #
Total comments: 33
Patch Set 5 : comments addressed; adds more tests; fixes impl #
Total comments: 59
Patch Set 6 : comments addressed #
Total comments: 10
Patch Set 7 : rebase only #Patch Set 8 : rebase only #Patch Set 9 : comments addressed #Patch Set 10 : rebase on 102503002 #Patch Set 11 : rebase only #Patch Set 12 : rebase only #Patch Set 13 : rebase only #Patch Set 14 : not to use base::MessageLoopProxy::current() #
Messages
Total messages: 31 (0 generated)
There are still a lot to be done. But I'd like you to take a initial look to make sure I am on the right track. The TODO's left are in the CL description. PTAL.
Thanks. https://codereview.chromium.org/93243003/diff/1/chrome/test/data/media/encryp... File chrome/test/data/media/encrypted_media_utils.js (right): https://codereview.chromium.org/93243003/diff/1/chrome/test/data/media/encryp... chrome/test/data/media/encrypted_media_utils.js:26: for (var i = 0; i < prefix.length; ++i) { is there a reason we can't use substr? https://codereview.chromium.org/93243003/diff/1/chrome/test/data/media/encryp... chrome/test/data/media/encrypted_media_utils.js:139: // Only External Clear Key sends a HEARTBEAT message. update comment https://codereview.chromium.org/93243003/diff/1/chrome/test/data/media/encryp... chrome/test/data/media/encrypted_media_utils.js:145: if (e.message.length != CDM_FILE_IO_TEST_RESULT_HEADER.length + 1) Explain that the result is appended and one character? https://codereview.chromium.org/93243003/diff/1/chrome/test/data/media/encryp... chrome/test/data/media/encrypted_media_utils.js:149: return String.fromCharCode(e.message[result_index]) == 1; '1' for clarity? https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_adapter.cc File media/cdm/ppapi/cdm_adapter.cc (right): https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_adapter.c... media/cdm/ppapi/cdm_adapter.cc:873: // The CDM owns this object and must call CdmFileIO::Close() to release it. s/this/the returned/ And move to a function-level comment. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... File media/cdm/ppapi/cdm_file_io_impl.cc (right): https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:21: int32_t result = func_call; \ PP_DCHECK(result != PP_OK)? https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:24: error_message << #func_call << " failed with result: " << result; \ All these strings will get included in prod binaries. I don't think we can do that. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:46: PP_Instance pp_instance) fits? https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:47: : state_(FILE_CLOSED), Kind of odd to start out in closed, esp. since the API doesn't support open->close->open. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:82: file_name_ += file_name_str; Can we merge with above line? https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:91: // | | not finished This is a bit confusing. I guess because of the difference in layout with "finished". Maybe put this inside the pipes on line 92 then remove this line? https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:127: PP_DCHECK(state_ == FILE_OPENED); duplicate with 122? https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:153: CHECK_PP_OK_COMPLETIONPENDING(isolated_file_system.Open(cb), OPEN_ERROR); Is it okay that isolated_file_system will go out of scope during the asynchronous call? https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:157: pp::FileSystem file_system) { fits https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:158: DVLOG(3) << __FUNCTION__; PP_DCHECK(IsMainThread());? Need to check callbacks per comment, right? https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:211: PP_DCHECK(state_ == READING_FILE); PP_DCHECK(IsMainThread());? https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:214: if (bytes_read < PP_OK) { |bytes_read| is a |result|? https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:219: PP_DCHECK(!buffer_.empty()); This could be at 212, right? Might be unnecessary given the next line, though. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:225: buffer_.erase(buffer_.begin(), buffer_.begin() + bytes_read); This seems unnecessarily inefficient. Can we just clear or something? Won't we need to resize before reading into it again? https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:228: // Still have data to read and we have not received end-of-file yet. With all the complexity, we should probably include >kReadSize file accesses in the test. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:229: if (!buffer_.empty() && bytes_read > 0) { Why wouldn't buffer_ be empty here? Shouldn't it be? (I suppose a read could be partial, but in most cases where we need multiple reads, we would have erased the entire buffer.) https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:260: PP_DCHECK(state_ == WRITING_FILE); PP_DCHECK(IsMainThread());? https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:269: buffer_.erase(buffer_.begin(), buffer_.begin() + bytes_written); Same issues as read. Wouldn't it be easier to just use the file_offset_ as the index to 255 (and subtract from the size)? https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:273: WriteFile(); Just to be clear, this probably won't happen, right? https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:300: void CdmFileIOImpl::NotifyClientOfError(int32_t /* result */, DCHECK(result == PP_OK)? https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:304: switch (error_type) { I'm not wild about this conversion, but I can't think of a better way. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... File media/cdm/ppapi/cdm_file_io_impl.h (right): https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.h:52: void OnFileSystemOpen(int32_t result, pp::FileSystem file_system); Open_ed_? ditto at 54. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.h:64: // happen in normal cases, we are not optimizing these cases. nit: s/these/such/ sounds better IMO https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.h:85: // A temporary buffer to hold data to write or the data that has been read. If this holds read data, why do we have read_buffer_? https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.h:89: std::vector<char> buffer_; It seems we should name this such that it's clear it is related to the actual I/O calls. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.h:96: std::vector<char> read_buffer_; cumulative_read_buffer_? Or something similar to indicate we're collecting from multiple reads to get the whole file. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_t... File media/cdm/ppapi/cdm_file_io_test.cc (right): https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_t... media/cdm/ppapi/cdm_file_io_test.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. Ideally, we'd have more tests that close the file, re-open it, have multiple files open at the same time, etc. We can start with this, though. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_t... media/cdm/ppapi/cdm_file_io_test.cc:15: LOG(ERROR) << log; \ Since this comes from a CDM, we should probably add more details. Something like: "CDM File IO Test failure: " << log https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_t... media/cdm/ppapi/cdm_file_io_test.cc:22: const uint8_t kTestData[] = { 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, We need really big test data to test the multiple reads code. (I'm not sure we can test the multiple writes code.) https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_t... media/cdm/ppapi/cdm_file_io_test.cc:32: LOG(ERROR) << __FUNCTION__; debugging leftovers? https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_t... media/cdm/ppapi/cdm_file_io_test.cc:45: OnTestComplete(success); Here we don't get details about what failed. Should we use the macro then always call this with true? https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_t... File media/cdm/ppapi/cdm_file_io_test.h (right): https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_t... media/cdm/ppapi/cdm_file_io_test.h:14: class CdmFileIOTest : public cdm::CdmFileIOClient { Please explain how this is intended to be do. It's not a normal "test". I assume it's supposed to be included in a CDM for testing. How does it work? Especially for detecting failures other than via logging. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/clear_key_cdm.cc File media/cdm/ppapi/clear_key_cdm.cc (right): https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/clear_key_cdm... media/cdm/ppapi/clear_key_cdm.cc:623: cdm::CdmFileIO* cdm_file_io = host_->GetCdmFileIO(cdm_file_io_test_.get()); Could/should we encapsulate this in the test file too? Why does the main CDM need to know these details? I guest that could add Host versioning issues to that file.
rebase only
more rebase cleanup
comments addressed
Comments resolved. I'll start looking at the TODO's in the CL description. I hope to get this CL landed before it get too big though, so I'll probably first update this CL to a committable state (i.e. fix logs and update the CL to CDM3); then add more tests and do cleanups in later CLs. Let me know WDYT. https://codereview.chromium.org/93243003/diff/1/chrome/test/data/media/encryp... File chrome/test/data/media/encrypted_media_utils.js (right): https://codereview.chromium.org/93243003/diff/1/chrome/test/data/media/encryp... chrome/test/data/media/encrypted_media_utils.js:26: for (var i = 0; i < prefix.length; ++i) { On 2013/12/04 05:27:08, ddorwin wrote: > is there a reason we can't use substr? msg is an uint8 array, not a string :( https://codereview.chromium.org/93243003/diff/1/chrome/test/data/media/encryp... chrome/test/data/media/encrypted_media_utils.js:139: // Only External Clear Key sends a HEARTBEAT message. On 2013/12/04 05:27:08, ddorwin wrote: > update comment Done. https://codereview.chromium.org/93243003/diff/1/chrome/test/data/media/encryp... chrome/test/data/media/encrypted_media_utils.js:145: if (e.message.length != CDM_FILE_IO_TEST_RESULT_HEADER.length + 1) On 2013/12/04 05:27:08, ddorwin wrote: > Explain that the result is appended and one character? Done. https://codereview.chromium.org/93243003/diff/1/chrome/test/data/media/encryp... chrome/test/data/media/encrypted_media_utils.js:149: return String.fromCharCode(e.message[result_index]) == 1; On 2013/12/04 05:27:08, ddorwin wrote: > '1' for clarity? Well, e.message[result_index] == '1' doesn't work... So this is the most clear code I can think of now. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_adapter.cc File media/cdm/ppapi/cdm_adapter.cc (right): https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_adapter.c... media/cdm/ppapi/cdm_adapter.cc:873: // The CDM owns this object and must call CdmFileIO::Close() to release it. On 2013/12/04 05:27:08, ddorwin wrote: > s/this/the returned/ > And move to a function-level comment. Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... File media/cdm/ppapi/cdm_file_io_impl.cc (right): https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:21: int32_t result = func_call; \ On 2013/12/04 05:27:08, ddorwin wrote: > PP_DCHECK(result != PP_OK)? Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:24: error_message << #func_call << " failed with result: " << result; \ On 2013/12/04 05:27:08, ddorwin wrote: > All these strings will get included in prod binaries. I don't think we can do > that. Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:46: PP_Instance pp_instance) On 2013/12/04 05:27:08, ddorwin wrote: > fits? Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:47: : state_(FILE_CLOSED), On 2013/12/04 05:27:08, ddorwin wrote: > Kind of odd to start out in closed, esp. since the API doesn't support > open->close->open. Done. Added FILE_UNOPENED. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:82: file_name_ += file_name_str; On 2013/12/04 05:27:08, ddorwin wrote: > Can we merge with above line? Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:91: // | | not finished On 2013/12/04 05:27:08, ddorwin wrote: > This is a bit confusing. I guess because of the difference in layout with > "finished". Maybe put this inside the pipes on line 92 then remove this line? Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:127: PP_DCHECK(state_ == FILE_OPENED); On 2013/12/04 05:27:08, ddorwin wrote: > duplicate with 122? Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:153: CHECK_PP_OK_COMPLETIONPENDING(isolated_file_system.Open(cb), OPEN_ERROR); On 2013/12/04 05:27:08, ddorwin wrote: > Is it okay that isolated_file_system will go out of scope during the > asynchronous call? Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:157: pp::FileSystem file_system) { On 2013/12/04 05:27:08, ddorwin wrote: > fits doesn't fit now https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:158: DVLOG(3) << __FUNCTION__; On 2013/12/04 05:27:08, ddorwin wrote: > PP_DCHECK(IsMainThread());? Need to check callbacks per comment, right? Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:211: PP_DCHECK(state_ == READING_FILE); On 2013/12/04 05:27:08, ddorwin wrote: > PP_DCHECK(IsMainThread());? Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:214: if (bytes_read < PP_OK) { On 2013/12/04 05:27:08, ddorwin wrote: > |bytes_read| is a |result|? yes https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:219: PP_DCHECK(!buffer_.empty()); On 2013/12/04 05:27:08, ddorwin wrote: > This could be at 212, right? Might be unnecessary given the next line, though. Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:225: buffer_.erase(buffer_.begin(), buffer_.begin() + bytes_read); On 2013/12/04 05:27:08, ddorwin wrote: > This seems unnecessarily inefficient. Can we just clear or something? > Won't we need to resize before reading into it again? Done. Reuse the read buffer and this is not needed anymore. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:228: // Still have data to read and we have not received end-of-file yet. On 2013/12/04 05:27:08, ddorwin wrote: > With all the complexity, we should probably include >kReadSize file accesses in > the test. I'll add more tests later. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:229: if (!buffer_.empty() && bytes_read > 0) { On 2013/12/04 05:27:08, ddorwin wrote: > Why wouldn't buffer_ be empty here? Shouldn't it be? (I suppose a read could be > partial, but in most cases where we need multiple reads, we would have erased > the entire buffer.) For example, we ask to read 100 bytes, the only 50 bytes were read. In this case, buffer.size() is 50. That being said, we need to read until EOF. So this condition needs to be removed. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:260: PP_DCHECK(state_ == WRITING_FILE); On 2013/12/04 05:27:08, ddorwin wrote: > PP_DCHECK(IsMainThread());? Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:269: buffer_.erase(buffer_.begin(), buffer_.begin() + bytes_written); On 2013/12/04 05:27:08, ddorwin wrote: > Same issues as read. > Wouldn't it be easier to just use the file_offset_ as the index to 255 (and > subtract from the size)? Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:273: WriteFile(); On 2013/12/04 05:27:08, ddorwin wrote: > Just to be clear, this probably won't happen, right? This happens when only part of the data were written. From FileIO's perspective, this could happen. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:300: void CdmFileIOImpl::NotifyClientOfError(int32_t /* result */, On 2013/12/04 05:27:08, ddorwin wrote: > DCHECK(result == PP_OK)? Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:304: switch (error_type) { On 2013/12/04 05:27:08, ddorwin wrote: > I'm not wild about this conversion, but I can't think of a better way. Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... File media/cdm/ppapi/cdm_file_io_impl.h (right): https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.h:52: void OnFileSystemOpen(int32_t result, pp::FileSystem file_system); On 2013/12/04 05:27:08, ddorwin wrote: > Open_ed_? > ditto at 54. Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.h:64: // happen in normal cases, we are not optimizing these cases. On 2013/12/04 05:27:08, ddorwin wrote: > nit: s/these/such/ sounds better IMO Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.h:85: // A temporary buffer to hold data to write or the data that has been read. On 2013/12/04 05:27:08, ddorwin wrote: > If this holds read data, why do we have read_buffer_? This holds "partial" read/write data. Renamed to io_buffer_ and updated comment. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.h:89: std::vector<char> buffer_; On 2013/12/04 05:27:08, ddorwin wrote: > It seems we should name this such that it's clear it is related to the actual > I/O calls. Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.h:96: std::vector<char> read_buffer_; On 2013/12/04 05:27:08, ddorwin wrote: > cumulative_read_buffer_? Or something similar to indicate we're collecting from > multiple reads to get the whole file. Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_t... File media/cdm/ppapi/cdm_file_io_test.cc (right): https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_t... media/cdm/ppapi/cdm_file_io_test.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2013/12/04 05:27:08, ddorwin wrote: > Ideally, we'd have more tests that close the file, re-open it, have multiple > files open at the same time, etc. We can start with this, though. Yes, that's in the plan. Added TODO. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_t... media/cdm/ppapi/cdm_file_io_test.cc:15: LOG(ERROR) << log; \ On 2013/12/04 05:27:08, ddorwin wrote: > Since this comes from a CDM, we should probably add more details. Something > like: > "CDM File IO Test failure: " << log Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_t... media/cdm/ppapi/cdm_file_io_test.cc:22: const uint8_t kTestData[] = { 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, On 2013/12/04 05:27:08, ddorwin wrote: > We need really big test data to test the multiple reads code. (I'm not sure we > can test the multiple writes code.) Added TODO. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_t... media/cdm/ppapi/cdm_file_io_test.cc:32: LOG(ERROR) << __FUNCTION__; On 2013/12/04 05:27:08, ddorwin wrote: > debugging leftovers? Removed. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_t... media/cdm/ppapi/cdm_file_io_test.cc:45: OnTestComplete(success); On 2013/12/04 05:27:08, ddorwin wrote: > Here we don't get details about what failed. Should we use the macro then always > call this with true? Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_t... File media/cdm/ppapi/cdm_file_io_test.h (right): https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_t... media/cdm/ppapi/cdm_file_io_test.h:14: class CdmFileIOTest : public cdm::CdmFileIOClient { On 2013/12/04 05:27:08, ddorwin wrote: > Please explain how this is intended to be do. It's not a normal "test". I assume > it's supposed to be included in a CDM for testing. > How does it work? Especially for detecting failures other than via logging. Done. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/clear_key_cdm.cc File media/cdm/ppapi/clear_key_cdm.cc (right): https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/clear_key_cdm... media/cdm/ppapi/clear_key_cdm.cc:623: cdm::CdmFileIO* cdm_file_io = host_->GetCdmFileIO(cdm_file_io_test_.get()); On 2013/12/04 05:27:08, ddorwin wrote: > Could/should we encapsulate this in the test file too? Why does the main CDM > need to know these details? I guest that could add Host versioning issues to > that file. Done.
https://codereview.chromium.org/93243003/diff/1/chrome/test/data/media/encryp... File chrome/test/data/media/encrypted_media_utils.js (right): https://codereview.chromium.org/93243003/diff/1/chrome/test/data/media/encryp... chrome/test/data/media/encrypted_media_utils.js:149: return String.fromCharCode(e.message[result_index]) == 1; On 2013/12/10 01:24:25, xhwang wrote: > On 2013/12/04 05:27:08, ddorwin wrote: > > '1' for clarity? > > Well, e.message[result_index] == '1' doesn't work... So this is the most clear > code I can think of now. but can we s/1/'1'/? There's an implicit conversion one way or the other now, right? And the value appended is a char/string, not an integer, right? So that would be consistent. Anyway, it doesn't really matter. https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... File media/cdm/ppapi/cdm_file_io_impl.cc (right): https://codereview.chromium.org/93243003/diff/1/media/cdm/ppapi/cdm_file_io_i... media/cdm/ppapi/cdm_file_io_impl.cc:176: // TODO(xhwang): This is not safe when we have multiple instances trying to What did you do that made this safe in the new patch set? https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_adapt... File media/cdm/ppapi/cdm_adapter.h (right): https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_adapt... media/cdm/ppapi/cdm_adapter.h:105: virtual cdm::FileIO* CreateFileIO(cdm::FileIOClient* client) OVERRIDE; This needs to move down to Host_3. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... File media/cdm/ppapi/cdm_file_io_impl.cc (right): https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.cc:24: std::ostringstream error_message; \ can we pipe directly to the DVLOG and eliminate the local variable? https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.cc:67: DVLOG(3) << "Open() called in an invalid state."; CDM_DLOG? https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.cc:306: cumulative_read_buffer_.clear(); reset offset too? https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.cc:313: DCHECK(result == PP_OK); PP_ ? https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... File media/cdm/ppapi/cdm_file_io_impl.h (right): https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.h:5: #ifndef MEDIA_CDM_PPAPI_CDM_FILE_IO_IMPL_H_ Should we drop "Cdm" from the class names "cdm_" from the file names? Let's discuss. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.h:86: // been read. The size of |buffer_| is always "bytes to write" or "bytes to buffer_ is no longer a member https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... File media/cdm/ppapi/cdm_file_io_test.cc (right): https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:12: #define RETURN_IF_FAILURE(success, log) \ It's really "return if false" now or "log failure and return if false". https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:12: #define RETURN_IF_FAILURE(success, log) \ Not in this CL, but bonus points if you can figure out how to allow MACRO(bool) << log :). Might not be possible. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:15: DVLOG(1) << "CDM File IO Test failure: " << log; \ DLOG https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:23: // TODO(xhwang): Add tests that test multiple reads (needs large data to read). Just a thought for future tests: Is there a threshold that would allow us to ensure that write gets broken into multiple parts? https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:57: cdm::FileIO* cdm_file_io = cdm_host_->CreateFileIO(this); We could separate this class from CK by passing in a CreateFileIo callback. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:67: cdm_file_io_->Close(); Delete the file to clean up after the test. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... File media/cdm/ppapi/cdm_file_io_test.h (right): https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:11: #include "media/cdm/ppapi/clear_key_cdm_common.h" The comment below says "a CDM", but this include and its types limit it to a specific one. That's okay for a test, but it would be nice if it wasn't tied to CK. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:33: void Run( CompletionCB completion_cb); extra space after ( https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/clear_key... File media/cdm/ppapi/clear_key_cdm.cc (right): https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/clear_key... media/cdm/ppapi/clear_key_cdm.cc:638: cdm_file_io_test_.reset(new CdmFileIOTest(host_)); Doh. I'm the one that asked you to encapsulate FileIO creation. Hmm. Possible solution: Template a simple CdmFileIOTest class that just has a constructor and Run(). It will also have a private CreateFileIo() function that it passes to the constructor of the real test class (currently CdmFileIOTest), which it keeps as a member (pimpl?). That way, the test is not tied to CK, CK doesn't know about FileIO, and we don't have a giant template class that is hard to debug. WDYT? (I'm okay doing this in another CL.)
comments addressed; adds more tests; fixes impl
Comments addressed. Also I fixed some implementation error and added tests. In this CL, the test result order is note deterministic. I need to look into the order of test results (callbacks) a little bit more. PTAL again. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_adapt... File media/cdm/ppapi/cdm_adapter.h (right): https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_adapt... media/cdm/ppapi/cdm_adapter.h:105: virtual cdm::FileIO* CreateFileIO(cdm::FileIOClient* client) OVERRIDE; On 2013/12/11 21:16:17, ddorwin wrote: > This needs to move down to Host_3. ClearKeyCdm is still using Host_2. I'll do this later before I land it. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... File media/cdm/ppapi/cdm_file_io_impl.cc (right): https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.cc:24: std::ostringstream error_message; \ On 2013/12/11 21:16:17, ddorwin wrote: > can we pipe directly to the DVLOG and eliminate the local variable? Done. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.cc:67: DVLOG(3) << "Open() called in an invalid state."; On 2013/12/11 21:16:17, ddorwin wrote: > CDM_DLOG? Done. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.cc:306: cumulative_read_buffer_.clear(); On 2013/12/11 21:16:17, ddorwin wrote: > reset offset too? Done. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.cc:313: DCHECK(result == PP_OK); On 2013/12/11 21:16:17, ddorwin wrote: > PP_ ? Done. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... File media/cdm/ppapi/cdm_file_io_impl.h (right): https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.h:5: #ifndef MEDIA_CDM_PPAPI_CDM_FILE_IO_IMPL_H_ On 2013/12/11 21:16:17, ddorwin wrote: > Should we drop "Cdm" from the class names "cdm_" from the file names? Let's > discuss. Let's talk about naming. For easier review, I'll keep names as-is for now. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.h:86: // been read. The size of |buffer_| is always "bytes to write" or "bytes to On 2013/12/11 21:16:17, ddorwin wrote: > buffer_ is no longer a member Done. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... File media/cdm/ppapi/cdm_file_io_test.cc (right): https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:12: #define RETURN_IF_FAILURE(success, log) \ On 2013/12/11 21:16:17, ddorwin wrote: > It's really "return if false" now or "log failure and return if false". This Macro is Removed. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:12: #define RETURN_IF_FAILURE(success, log) \ On 2013/12/11 21:16:17, ddorwin wrote: > Not in this CL, but bonus points if you can figure out how to allow MACRO(bool) > << log :). Might not be possible. This Macro is Removed. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:15: DVLOG(1) << "CDM File IO Test failure: " << log; \ On 2013/12/11 21:16:17, ddorwin wrote: > DLOG This Macro is Removed. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:23: // TODO(xhwang): Add tests that test multiple reads (needs large data to read). On 2013/12/11 21:16:17, ddorwin wrote: > Just a thought for future tests: > Is there a threshold that would allow us to ensure that write gets broken into > multiple parts? There WAS no limit. I filed a bug and now (as of this afternoon) we have a limit: 32M. I feel it's too big for a test to be checked in. But I'll test it after rebase locally. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:57: cdm::FileIO* cdm_file_io = cdm_host_->CreateFileIO(this); On 2013/12/11 21:16:17, ddorwin wrote: > We could separate this class from CK by passing in a CreateFileIo callback. Done. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:67: cdm_file_io_->Close(); On 2013/12/11 21:16:17, ddorwin wrote: > Delete the file to clean up after the test. There's no "delete" in FileIO. The closest thing is Write(NULL, 0) which sets the file length to 0. I guess leaving this as-is is fine because browser_tests creates a temp user-data-dir for each tests (and cleans it up I suppose). https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... File media/cdm/ppapi/cdm_file_io_test.h (right): https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:11: #include "media/cdm/ppapi/clear_key_cdm_common.h" On 2013/12/11 21:16:17, ddorwin wrote: > The comment below says "a CDM", but this include and its types limit it to a > specific one. That's okay for a test, but it would be nice if it wasn't tied to > CK. Done. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:33: void Run( CompletionCB completion_cb); On 2013/12/11 21:16:17, ddorwin wrote: > extra space after ( Done. https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/clear_key... File media/cdm/ppapi/clear_key_cdm.cc (right): https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/clear_key... media/cdm/ppapi/clear_key_cdm.cc:638: cdm_file_io_test_.reset(new CdmFileIOTest(host_)); On 2013/12/11 21:16:17, ddorwin wrote: > Doh. I'm the one that asked you to encapsulate FileIO creation. Hmm. > > Possible solution: > Template a simple CdmFileIOTest class that just has a constructor and Run(). It > will also have a private CreateFileIo() function that it passes to the > constructor of the real test class (currently CdmFileIOTest), which it keeps as > a member (pimpl?). > That way, the test is not tied to CK, CK doesn't know about FileIO, and we don't > have a giant template class that is hard to debug. WDYT? (I'm okay doing this in > another CL.) Now I pass in a callback for CreateFileIO. But I don't understand the "CK doesn't know about FIleIO" part. We need to talk.
https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... File media/cdm/ppapi/cdm_file_io_test.cc (right): https://codereview.chromium.org/93243003/diff/60001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:23: // TODO(xhwang): Add tests that test multiple reads (needs large data to read). On 2013/12/13 02:51:47, xhwang wrote: > On 2013/12/11 21:16:17, ddorwin wrote: > > Just a thought for future tests: > > Is there a threshold that would allow us to ensure that write gets broken into > > multiple parts? > > There WAS no limit. I filed a bug and now (as of this afternoon) we have a > limit: 32M. I feel it's too big for a test to be checked in. But I'll test it > after rebase locally. FYI: I tested writing and reading ~33M data and I saw two writes :) The current implementation is working well.
I still need to review the tests .cc file. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_adapt... File media/cdm/ppapi/cdm_adapter.h (right): https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_adapt... media/cdm/ppapi/cdm_adapter.h:105: virtual cdm::FileIO* CreateFileIO(cdm::FileIOClient* client) OVERRIDE; Reminder to remove. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... File media/cdm/ppapi/cdm_file_io_impl.cc (right): https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.cc:101: bool in_use = state_ == READING_FILE || state_ == WRITING_FILE; You share the logging this way, but it might be simpler to check for these before 99 so you don't need the check at 102. Maybe more code, but clearer. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.cc:102: OnError(in_use ? READ_IN_USE : READ_ERROR); READ_WHILE_IN_USE? https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.cc:141: SetLength(data_size); You might explain why you need to do this. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.cc:337: if (error_type == READ_ERROR || error_type == WRITE_ERROR) { Is there any harm in always resetting these values? https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... File media/cdm/ppapi/cdm_file_io_test.h (right): https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:20: class FileIOTest : public cdm::FileIOClient { Should you briefly describe how this worksat a high level (an introduction to the steps at 22-24)? https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:41: // Adds a test step into this test case. nits: s/into/to/? "Test Case" in gtest means the class. Do you mean to refer to the instance? https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:43: StepType type, Status status, const uint8* data, uint32 data_size); It's not clear what |status| is. expected_status? https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:43: StepType type, Status status, const uint8* data, uint32 data_size); Does this function take ownership of |data|? https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:45: // Runs this test case and return the test result through |completion_cb|. return_s_ https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:48: // cdm::FileIOClient implementation. Can these be private? https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:57: TestStep(StepType type, Status status, const uint8* data, uint32 data_size) Does this object take ownership of |data|? https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:87: cdm::FileIO* old_file_io_; What is old? https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:88: CompletionCB completion_cb_; nit: Group CBs together? https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:109: std::list<FileIOTest> tests_; remaining_tests_? https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:111: size_t num_tests_; // Total number of tests. total_num_tests? or something that indicates it's different from tests_. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/clear_key... File media/cdm/ppapi/clear_key_cdm.h (right): https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/clear_key... media/cdm/ppapi/clear_key_cdm.h:162: const bool test_file_io_; should_ ? is_file_io_test_? https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/clear_key... media/cdm/ppapi/clear_key_cdm.h:189: scoped_ptr<FileIOTestRunner> cdm_file_io_test_; rename?
https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... File media/cdm/ppapi/cdm_file_io_test.cc (right): https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:13: #define FILE_IO_DVLOG(level) DVLOG(level) << "File IO Test: " I think you'll need to replace DVLOG and probably drop the level. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:35: test_case.AddTestStep(FileIOTest::type, cdm::FileIOClient::status, \ Does this really need to be a macro? Can you just shorten things with "using"? https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:42: #define CREATE_FILE_IO \ Could these all be functions? (If they were functions, they would all be Add...) I guess the actual usage looks better this way. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:85: FILE_IO_DVLOG(1) << "Not Finished (probably due to timeout). " Does a timeout really get here vs. killing the process? https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:87: << (num_tests_ - num_passed_tests_) << " failed in " failed or not run, right? Do we need to do the math or can we shorten this log? https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:106: EXPECT_FILE_READ(kError, NULL, 0) This seems to contradict Note above - you will get a specific order here. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:155: START_TEST_CASE("ReadEmptyFile") Are empty and non-existent the same thing? https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:328: return test_step.type == RESULT_OPEN || test_step.type == RESULT_READ || Use a switch statement instead? (Including all cases) https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:347: return; This being first seems to assume this was called from OnResult(). Otherwise, we'd stop testing? (i.e if the first step is a RESULT_) This also seems to handle the back-to-back RESULT_ case. Is that right? It's also used to wait for the result after having executed a step during the first (or more) iteration of code below. Some of this probably warrants comments. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:355: if (old_file_io_) Please explain. Does this retire the oldest of 3 FileIOs? https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:367: // Use test name as the test file name. It might be good to mention this in a higher-level comment. At least where ACTION_OPEN is defined.
comments addressed
PTAL again. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_adapt... File media/cdm/ppapi/cdm_adapter.h (right): https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_adapt... media/cdm/ppapi/cdm_adapter.h:105: virtual cdm::FileIO* CreateFileIO(cdm::FileIOClient* client) OVERRIDE; On 2013/12/14 20:44:04, ddorwin wrote: > Reminder to remove. Done. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... File media/cdm/ppapi/cdm_file_io_impl.cc (right): https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.cc:101: bool in_use = state_ == READING_FILE || state_ == WRITING_FILE; On 2013/12/14 20:44:04, ddorwin wrote: > You share the logging this way, but it might be simpler to check for these > before 99 so you don't need the check at 102. Maybe more code, but clearer. Done. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.cc:102: OnError(in_use ? READ_IN_USE : READ_ERROR); On 2013/12/14 20:44:04, ddorwin wrote: > READ_WHILE_IN_USE? Done. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.cc:141: SetLength(data_size); On 2013/12/14 20:44:04, ddorwin wrote: > You might explain why you need to do this. Done. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.cc:337: if (error_type == READ_ERROR || error_type == WRITE_ERROR) { On 2013/12/14 20:44:04, ddorwin wrote: > Is there any harm in always resetting these values? Yes, for *_WHILE_IN_USE, we don't want to interrupt the existing read/write operation, which uses these values. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... File media/cdm/ppapi/cdm_file_io_test.cc (right): https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:13: #define FILE_IO_DVLOG(level) DVLOG(level) << "File IO Test: " On 2013/12/16 18:16:52, ddorwin wrote: > I think you'll need to replace DVLOG and probably drop the level. This is in ClearKeyCdm code, where it's okay to use base/logging. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:35: test_case.AddTestStep(FileIOTest::type, cdm::FileIOClient::status, \ On 2013/12/16 18:16:52, ddorwin wrote: > Does this really need to be a macro? Can you just shorten things with "using"? Using can only be used for class type. So I can't use "using" for FileIOTest::ACTION_CREATE. I can typedef it, but that results in more code. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:42: #define CREATE_FILE_IO \ On 2013/12/16 18:16:52, ddorwin wrote: > Could these all be functions? (If they were functions, they would all be Add...) > I guess the actual usage looks better this way. To use function, I have to use real types. So the caller has to pass in full types like FileIOTest::ACTION_CREATE and cdm::FileIOClient::kSuccess, which will be long. I like the current way of adding tests, which is concise and clear. WDYT? https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:85: FILE_IO_DVLOG(1) << "Not Finished (probably due to timeout). " On 2013/12/16 18:16:52, ddorwin wrote: > Does a timeout really get here vs. killing the process? It doesn't crash the process. Instead, it tears down everything. So, Yes, it does get here:) https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:87: << (num_tests_ - num_passed_tests_) << " failed in " On 2013/12/16 18:16:52, ddorwin wrote: > failed or not run, right? > Do we need to do the math or can we shorten this log? Done. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:106: EXPECT_FILE_READ(kError, NULL, 0) On 2013/12/16 18:16:52, ddorwin wrote: > This seems to contradict Note above - you will get a specific order here. Done. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:155: START_TEST_CASE("ReadEmptyFile") On 2013/12/16 18:16:52, ddorwin wrote: > Are empty and non-existent the same thing? The result is the same since we specify PP_FILEOPENFLAG_CREATE. If we want to make them different. We have to drop PP_FILEOPENFLAG_CREATE when we try to read (but keep it when we try to write). The caller has to specify flags when calling open(), which will make the implementation more complicated. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:328: return test_step.type == RESULT_OPEN || test_step.type == RESULT_READ || On 2013/12/16 18:16:52, ddorwin wrote: > Use a switch statement instead? (Including all cases) Done. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:347: return; On 2013/12/16 18:16:52, ddorwin wrote: > This being first seems to assume this was called from OnResult(). Otherwise, > we'd stop testing? (i.e if the first step is a RESULT_) > This also seems to handle the back-to-back RESULT_ case. Is that right? > It's also used to wait for the result after having executed a step during the > first (or more) iteration of code below. > > Some of this probably warrants comments. Added comments. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:355: if (old_file_io_) On 2013/12/16 18:16:52, ddorwin wrote: > Please explain. Does this retire the oldest of 3 FileIOs? Done. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:367: // Use test name as the test file name. On 2013/12/16 18:16:52, ddorwin wrote: > It might be good to mention this in a higher-level comment. At least where > ACTION_OPEN is defined. Done. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... File media/cdm/ppapi/cdm_file_io_test.h (right): https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:20: class FileIOTest : public cdm::FileIOClient { On 2013/12/14 20:44:04, ddorwin wrote: > Should you briefly describe how this worksat a high level (an introduction to > the steps at 22-24)? Done. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:41: // Adds a test step into this test case. On 2013/12/14 20:44:04, ddorwin wrote: > nits: > s/into/to/? > "Test Case" in gtest means the class. Do you mean to refer to the instance? Done. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:43: StepType type, Status status, const uint8* data, uint32 data_size); On 2013/12/14 20:44:04, ddorwin wrote: > Does this function take ownership of |data|? Done. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:43: StepType type, Status status, const uint8* data, uint32 data_size); On 2013/12/14 20:44:04, ddorwin wrote: > It's not clear what |status| is. expected_status? Yeah, |expected_status| makes sense. But it's longer. Also, we have |data| which can be data for action, or |expected_data| for result. In that case, we cannot simply use "expected". Added comments to make things clearer. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:45: // Runs this test case and return the test result through |completion_cb|. On 2013/12/14 20:44:04, ddorwin wrote: > return_s_ Done. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:48: // cdm::FileIOClient implementation. On 2013/12/14 20:44:04, ddorwin wrote: > Can these be private? Done. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:57: TestStep(StepType type, Status status, const uint8* data, uint32 data_size) On 2013/12/14 20:44:04, ddorwin wrote: > Does this object take ownership of |data|? Done. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:87: cdm::FileIO* old_file_io_; On 2013/12/14 20:44:04, ddorwin wrote: > What is old? Added comments. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:88: CompletionCB completion_cb_; On 2013/12/14 20:44:04, ddorwin wrote: > nit: Group CBs together? Done. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:109: std::list<FileIOTest> tests_; On 2013/12/14 20:44:04, ddorwin wrote: > remaining_tests_? Done. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:111: size_t num_tests_; // Total number of tests. On 2013/12/14 20:44:04, ddorwin wrote: > total_num_tests? or something that indicates it's different from tests_. Done. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/clear_key... File media/cdm/ppapi/clear_key_cdm.h (right): https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/clear_key... media/cdm/ppapi/clear_key_cdm.h:162: const bool test_file_io_; On 2013/12/14 20:44:04, ddorwin wrote: > should_ ? > is_file_io_test_? Done. https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/clear_key... media/cdm/ppapi/clear_key_cdm.h:189: scoped_ptr<FileIOTestRunner> cdm_file_io_test_; On 2013/12/14 20:44:04, ddorwin wrote: > rename? Done.
rebase only
LGTM with comments https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... File media/cdm/ppapi/cdm_file_io_test.cc (right): https://codereview.chromium.org/93243003/diff/80001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.cc:155: START_TEST_CASE("ReadEmptyFile") On 2013/12/16 23:04:29, xhwang wrote: > On 2013/12/16 18:16:52, ddorwin wrote: > > Are empty and non-existent the same thing? > > The result is the same since we specify PP_FILEOPENFLAG_CREATE. > > If we want to make them different. We have to drop PP_FILEOPENFLAG_CREATE when > we try to read (but keep it when we try to write). The caller has to specify > flags when calling open(), which will make the implementation more complicated. Okay, maybe we should note in the interface that opening a non-existent file will appear as a 0-byte file. https://codereview.chromium.org/93243003/diff/90001/media/cdm/ppapi/cdm_file_... File media/cdm/ppapi/cdm_file_io_impl.cc (right): https://codereview.chromium.org/93243003/diff/90001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.cc:349: // For *_WHILE_IN_USE errors, do not reset these values. Otherwise, the nit: The comment and check don't match. This would probably be better if it checked *_WHILE_IN_USE and skipped the reset. https://codereview.chromium.org/93243003/diff/90001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.cc:351: if (error_type == READ_ERROR || error_type == WRITE_ERROR) { On an OPEN_ERROR,these are already at the reset values? https://codereview.chromium.org/93243003/diff/90001/media/cdm/ppapi/cdm_file_... File media/cdm/ppapi/cdm_file_io_test.h (right): https://codereview.chromium.org/93243003/diff/90001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:20: // A customizable test class that tests cdm::FileIO implementation. Nice! Thanks. :) https://codereview.chromium.org/93243003/diff/90001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:125: // accessing the same file. Would things be easier if it was a vector? Even if we only need two, would the code be simpler? https://codereview.chromium.org/93243003/diff/90001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:149: size_t num_tests_; // Total number of tests. On 2013/12/16 23:04:29, xhwang wrote: > On 2013/12/14 20:44:04, ddorwin wrote: > > total_num_tests? or something that indicates it's different from tests_. > > Done. This wasn't renamed.
Comments only on cdm_file_io_impl.cc https://codereview.chromium.org/93243003/diff/90001/media/cdm/ppapi/cdm_file_... File media/cdm/ppapi/cdm_file_io_impl.cc (right): https://codereview.chromium.org/93243003/diff/90001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.cc:349: // For *_WHILE_IN_USE errors, do not reset these values. Otherwise, the On 2013/12/17 00:17:04, ddorwin wrote: > nit: The comment and check don't match. This would probably be better if it > checked *_WHILE_IN_USE and skipped the reset. Hmm, this appears to be the cleanest I can get now... https://codereview.chromium.org/93243003/diff/90001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_impl.cc:351: if (error_type == READ_ERROR || error_type == WRITE_ERROR) { On 2013/12/17 00:17:04, ddorwin wrote: > On an OPEN_ERROR,these are already at the reset values? OPEN_ERROR is only reported during opening a file. In which case all these values should not be set yet.
rebase only
comments addressed
Now I use a stack to store opened FileIO objects. Thanks for the suggestion! https://codereview.chromium.org/93243003/diff/90001/media/cdm/ppapi/cdm_file_... File media/cdm/ppapi/cdm_file_io_test.h (right): https://codereview.chromium.org/93243003/diff/90001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:20: // A customizable test class that tests cdm::FileIO implementation. On 2013/12/17 00:17:04, ddorwin wrote: > Nice! Thanks. :) Done. https://codereview.chromium.org/93243003/diff/90001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:125: // accessing the same file. On 2013/12/17 00:17:04, ddorwin wrote: > Would things be easier if it was a vector? Even if we only need two, would the > code be simpler? Done. https://codereview.chromium.org/93243003/diff/90001/media/cdm/ppapi/cdm_file_... media/cdm/ppapi/cdm_file_io_test.h:149: size_t num_tests_; // Total number of tests. On 2013/12/17 00:17:04, ddorwin wrote: > On 2013/12/16 23:04:29, xhwang wrote: > > On 2013/12/14 20:44:04, ddorwin wrote: > > > total_num_tests? or something that indicates it's different from tests_. > > > > Done. > > This wasn't renamed. Done.
lgtm
rebase only
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/93243003/190001
Retried try job too often on linux_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
rebase only
rebase only
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/93243003/230001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
not to use base::MessageLoopProxy::current()
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/93243003/250001
Message was sent while issue was closed.
Change committed as 242027 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
