Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1265413002:
Introduce FetchFormDataConsumerHandle. (Closed)
|
Created:
5 years, 4 months ago by yhirano Modified:
5 years, 4 months ago Reviewers:
hiroshige CC:
blink-reviews Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionIntroduce FetchFormDataConsumerHandle.
In order to handle fetch() Request data more correctly, this CL introduces
FetchFormDataConsumerHandle which is made from FormData (including String
and ArrayBuffer) and provides a means to drain the data as a FormData.
This CL also modifies drainAsBlobDataHandle contract and implementation.
BUG=457484
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200701
Patch Set 1 : #
Total comments: 6
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : Add (const void*, size_t) creation function #
Total comments: 24
Patch Set 5 : #Patch Set 6 : #
Total comments: 29
Patch Set 7 : #
Total comments: 6
Patch Set 8 : #
Total comments: 2
Patch Set 9 : #Messages
Total messages: 23 (5 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
yhirano@chromium.org changed reviewers: + hiroshige@chromium.org
https://codereview.chromium.org/1265413002/diff/60001/Source/modules/fetch/Fe... File Source/modules/fetch/FetchDataConsumerHandle.h (right): https://codereview.chromium.org/1265413002/diff/60001/Source/modules/fetch/Fe... Source/modules/fetch/FetchDataConsumerHandle.h:48: // - The reader is running on a suitable thread nit: append ", and" at the end of line? https://codereview.chromium.org/1265413002/diff/60001/Source/modules/fetch/Fe... File Source/modules/fetch/FetchFormDataConsumerHandle.cpp (right): https://codereview.chromium.org/1265413002/diff/60001/Source/modules/fetch/Fe... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:37: if (m_handleForComplexFormData) { Switching by |m_handleForComplexFormData| seems complex and somehow prone to error. Can we split this mixed logic of FetchFormDataConsumerHandle::Context into two separate logics, i.e. - One for flattenable FormData only, and - One for complex FormData. A possible implementation would be: - Add |FetchBlobDataConsumerHandle::ReaderContext::m_formDataForDrain|, - Make |FetchFormDataConsumerHandle| only for flatten FormData, and - Return FetchBlobDataConsumerHandle in FetchFormDataConsumerHandle::create() if the formdata is complex, and return FetchFormDataConsumerHandle otherwise. pros: - We can merge branches by |m_handleForComplexFormData| into single if-statement in create(). - We can merge logic for clearing |m_formData| for complex FormData case (e.g. L51--54) into the similar logic in FetchBlobDataConsumerHandle. - Having |FetchBlobDataConsumerHandle::ReaderContext::m_formDataForDrain| in addition to |m_blobDataHandleForDrain| adds little complexity to FetchBlobDataConsumerHandle (just we set and clear |m_formDataForDrain| when we set and clear |m_blobDataHandleForDrain|). cons: - Having FormData support is a little conflicting with the name of FetchBlobDataConsumerHandle. https://codereview.chromium.org/1265413002/diff/60001/Source/modules/fetch/Fe... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:147: case FormDataElement::data: { nit: can we remove "{" here and "}" in L150?
https://codereview.chromium.org/1265413002/diff/60001/Source/modules/fetch/Fe... File Source/modules/fetch/FetchDataConsumerHandle.h (right): https://codereview.chromium.org/1265413002/diff/60001/Source/modules/fetch/Fe... Source/modules/fetch/FetchDataConsumerHandle.h:48: // - The reader is running on a suitable thread On 2015/08/10 13:27:03, hiroshige wrote: > nit: append ", and" at the end of line? Done. https://codereview.chromium.org/1265413002/diff/60001/Source/modules/fetch/Fe... File Source/modules/fetch/FetchFormDataConsumerHandle.cpp (right): https://codereview.chromium.org/1265413002/diff/60001/Source/modules/fetch/Fe... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:37: if (m_handleForComplexFormData) { On 2015/08/10 13:27:03, hiroshige wrote: > Switching by |m_handleForComplexFormData| seems complex and somehow prone to > error. > Can we split this mixed logic of FetchFormDataConsumerHandle::Context into two > separate logics, i.e. > - One for flattenable FormData only, and > - One for complex FormData. > > A possible implementation would be: > - Add |FetchBlobDataConsumerHandle::ReaderContext::m_formDataForDrain|, > - Make |FetchFormDataConsumerHandle| only for flatten FormData, and > - Return FetchBlobDataConsumerHandle in FetchFormDataConsumerHandle::create() if > the formdata is complex, and return FetchFormDataConsumerHandle otherwise. > > pros: > - We can merge branches by |m_handleForComplexFormData| into single if-statement > in create(). > - We can merge logic for clearing |m_formData| for complex FormData case (e.g. > L51--54) into the similar logic in FetchBlobDataConsumerHandle. > - Having |FetchBlobDataConsumerHandle::ReaderContext::m_formDataForDrain| in > addition to |m_blobDataHandleForDrain| adds little complexity to > FetchBlobDataConsumerHandle (just we set and clear |m_formDataForDrain| when we > set and clear |m_blobDataHandleForDrain|). > > cons: > - Having FormData support is a little conflicting with the name of > FetchBlobDataConsumerHandle. I split Context into SimpleContext and ComplexContext. I don't like conflating FetchBlobDataConsumerHandle with FetchFormDataConsumerHandle though. https://codereview.chromium.org/1265413002/diff/60001/Source/modules/fetch/Fe... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:147: case FormDataElement::data: { On 2015/08/10 13:27:03, hiroshige wrote: > nit: can we remove "{" here and "}" in L150? Done.
Probably there is a minor thread-safe issue (that I expect doesn't affect most of the code). (I'll test locally and comment later) https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... File Source/modules/fetch/FetchDataConsumerHandle.h (right): https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchDataConsumerHandle.h:39: // - Subsequent calls to read() / beginRead() return |Done|. Please add "and subsequent calls to drainAsFormData() return nullptr.". https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchDataConsumerHandle.h:48: // - The reader is running on a suitable thread, and What is "a suitable thread"? Isn't this a general threading restriction of the Reader thread? https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchDataConsumerHandle.h:50: // After that read() / beginRead() will return |Done|. Please add "and drainAsBlobDataHandle() will return nullptr". https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... File Source/modules/fetch/FetchFormDataConsumerHandle.cpp (right): https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:44: MutexLocker locker(m); BTW if we need memory barrier here, should we also add similar code to existing code? Can we detect if we are missing memory barriers here by tests? https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:48: PassRefPtr<FormData> drainFormData() IIUC: 1. If flatten() is called before drainFormData(), m_formData is cleared so subsequent drainFormData() returns nullptr; 2. If drainFormData() is called before flatten(), m_flattenFormData is kept empty so subsequent read()/beginRead() returns Done. right? If so, the interaction between |m_formData| and |m_flattenFormData| is a little confusing, stating in invariants of them in comments might be helpful. https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:63: How about adding "RELEASE_ASSERT(m_flattenFormData.size() >= m_flattenFormDataOffset)"? https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:79: return WebDataConsumerHandle::Done; ditto. https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:86: { Please add: RELEASE_ASSERT(m_flattenFormData.size() >= m_flattenFormDataOffset); if (read > m_flattenFormData.size() - m_flattenFormDataOffset) return UnexpectedError; https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... File Source/modules/fetch/FetchFormDataConsumerHandle.h (right): https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.h:34: static PassOwnPtr<FetchDataConsumerHandle> create(ExecutionContext* executionContext, PassRefPtr<FormData> body) { return adoptPtr(new FetchFormDataConsumerHandle(executionContext, body)); } optional: moving the bodies of the functions above to .cpp will make the header easier to read. (They are simple create() but the lines are long...) https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.h:39: static PassOwnPtr<FetchDataConsumerHandle> createForTest(ExecutionContext* executionContext, nit optional: how about wrapping this line before "ExecutionContext"? https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.h:46: static bool isSimple(const FormData*); Can we move this to .cpp file? (I think we don't have to expose isSimple())
https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... File Source/modules/fetch/FetchFormDataConsumerHandle.cpp (right): https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:224: blobData->appendFileSystemURL(element.m_fileSystemURL, element.m_fileStart, element.m_fileLength, element.m_expectedFileModificationTime); This is not thread-safe, because appendFileSystemURL() increases refs to |element.m_fileSystemURL|, which is inside |m_formData|, so this makes |m_formData| not a pure deep copy. (Similar thing applies to appendFile()) Working on |body| instead of |m_formData| in this constructor might resolve the problem. I added FormData::isSafeToSendToAnotherThread() in https://codereview.chromium.org/1276203004. Replacing FormData::hasOneRef() with FormData::isSafeToSendToAnotherThread() can check more on thread-safety.
https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... File Source/modules/fetch/FetchDataConsumerHandle.h (right): https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchDataConsumerHandle.h:39: // - Subsequent calls to read() / beginRead() return |Done|. On 2015/08/11 09:35:01, hiroshige wrote: > Please add "and subsequent calls to drainAsFormData() return nullptr.". I rewrote comments. https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchDataConsumerHandle.h:48: // - The reader is running on a suitable thread, and On 2015/08/11 09:35:01, hiroshige wrote: > What is "a suitable thread"? Isn't this a general threading restriction of the > Reader thread? Sorry the restriction was gone. https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchDataConsumerHandle.h:50: // After that read() / beginRead() will return |Done|. On 2015/08/11 09:35:01, hiroshige wrote: > Please add "and drainAsBlobDataHandle() will return nullptr". I rewrote comments. https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... File Source/modules/fetch/FetchFormDataConsumerHandle.cpp (right): https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:44: MutexLocker locker(m); On 2015/08/11 09:35:01, hiroshige wrote: > BTW if we need memory barrier here, should we also add similar code to existing > code? Can we detect if we are missing memory barriers here by tests? If a user obtains and releases a reader on one thread, and obtains a reader after that on another thread without a memory barrier, it could confuse the implementation. On the other hand, I'm sure such a usage is not reliable and always buggy without an external memory barrier. What do you think? https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:48: PassRefPtr<FormData> drainFormData() On 2015/08/11 09:35:01, hiroshige wrote: > IIUC: > 1. If flatten() is called before drainFormData(), m_formData is cleared so > subsequent drainFormData() returns nullptr; > 2. If drainFormData() is called before flatten(), m_flattenFormData is kept > empty so subsequent read()/beginRead() returns Done. > right? > If so, the interaction between |m_formData| and |m_flattenFormData| is a little > confusing, stating in invariants of them in comments might be helpful. Done. https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:63: On 2015/08/11 09:35:01, hiroshige wrote: > How about adding "RELEASE_ASSERT(m_flattenFormData.size() >= > m_flattenFormDataOffset)"? Done. https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:79: return WebDataConsumerHandle::Done; On 2015/08/11 09:35:01, hiroshige wrote: > ditto. Done. https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:86: { On 2015/08/11 09:35:01, hiroshige wrote: > Please add: > RELEASE_ASSERT(m_flattenFormData.size() >= > m_flattenFormDataOffset); > if (read > m_flattenFormData.size() - m_flattenFormDataOffset) > return UnexpectedError; Done. https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:224: blobData->appendFileSystemURL(element.m_fileSystemURL, element.m_fileStart, element.m_fileLength, element.m_expectedFileModificationTime); On 2015/08/11 10:19:04, hiroshige wrote: > This is not thread-safe, because appendFileSystemURL() increases refs to > |element.m_fileSystemURL|, which is inside |m_formData|, so this makes > |m_formData| not a pure deep copy. (Similar thing applies to appendFile()) > Working on |body| instead of |m_formData| in this constructor might resolve the > problem. > > I added FormData::isSafeToSendToAnotherThread() in > https://codereview.chromium.org/1276203004. > Replacing FormData::hasOneRef() with FormData::isSafeToSendToAnotherThread() can > check more on thread-safety. Nice catch! done. https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... File Source/modules/fetch/FetchFormDataConsumerHandle.h (right): https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.h:34: static PassOwnPtr<FetchDataConsumerHandle> create(ExecutionContext* executionContext, PassRefPtr<FormData> body) { return adoptPtr(new FetchFormDataConsumerHandle(executionContext, body)); } On 2015/08/11 09:35:01, hiroshige wrote: > optional: moving the bodies of the functions above to .cpp will make the header > easier to read. (They are simple create() but the lines are long...) Done. https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.h:39: static PassOwnPtr<FetchDataConsumerHandle> createForTest(ExecutionContext* executionContext, On 2015/08/11 09:35:01, hiroshige wrote: > nit optional: how about wrapping this line before "ExecutionContext"? Done. https://codereview.chromium.org/1265413002/diff/120001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.h:46: static bool isSimple(const FormData*); On 2015/08/11 09:35:01, hiroshige wrote: > Can we move this to .cpp file? (I think we don't have to expose isSimple()) Done.
https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp (right): https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp:369: EXPECT_TRUE(formData->hasOneRef()); isSafeToSendToAnotherThread() would be better. https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp:378: } Can we add tests for drainAs.*() called after 0-byte read() / non-0-byte read() / beginRead()? https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... File Source/modules/fetch/FetchFormDataConsumerHandle.cpp (right): https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:131: IIUC drainAsBlobDataHandle() here is omitted (thus it returns null for SimpleContext), just because it is currently not implemented/needed and we can implement it if needed (i.e. not because we intentionally disabled for some reasons). If so, leaving a comment would be helpful. https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:217: if (Ok == m_reader->beginRead(&p, FlagNone, &available)) { IIUC this is to make drainAsBlobDataHandle() to return nullptr after drainAsFormData(). How about implementing drainAsBlobDataHandle() as: RefPtr<BlobDataHandle> handle = m_reader->drainAsBlobDataHandle(); if (handle) { if (m_context->drainFormData()) return handle.release(); } return nullptr; and remove L213-L220? By doing so, - we don't start ThreadableLoader by calling beginRead() here unnecessarily, and - drainAs.*() is controlled by primarily m_context->drainFormData(), and I think this is clearer (i.e. I'd like to avoid to depend on side effects of beginRead() here). https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... File Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp (right): https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:244: EXPECT_TRUE(formData->hasOneRef()); isSafeToSendToAnotherThread() would be better. https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:259: EXPECT_TRUE(formData->hasOneRef()); ditto. https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:274: EXPECT_TRUE(outputFormData->hasOneRef()); ditto. https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:287: EXPECT_TRUE(outputFormData->hasOneRef()); ditto. https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:300: EXPECT_TRUE(formData->hasOneRef()); ditto. https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:342: EXPECT_TRUE(formData->hasOneRef()); ditto.
https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... File Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp (right): https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp:369: EXPECT_TRUE(formData->hasOneRef()); On 2015/08/12 06:44:48, hiroshige wrote: > isSafeToSendToAnotherThread() would be better. Done. https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp:378: } On 2015/08/12 06:44:48, hiroshige wrote: > Can we add tests for drainAs.*() called after 0-byte read() / non-0-byte read() > / beginRead()? Done (only for AsBlobDataConsumerHandle with no args but I think it's enough...). https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... File Source/modules/fetch/FetchFormDataConsumerHandle.cpp (right): https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:131: On 2015/08/12 06:44:48, hiroshige wrote: > IIUC drainAsBlobDataHandle() here is omitted (thus it returns null for > SimpleContext), just because it is currently not implemented/needed and we can > implement it if needed (i.e. not because we intentionally disabled for some > reasons). > If so, leaving a comment would be helpful. Hm, yes, but I don't think it is worth noting, is it? If we have a critical reason against implementing, it would be worth noting. https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:217: if (Ok == m_reader->beginRead(&p, FlagNone, &available)) { On 2015/08/12 06:44:48, hiroshige wrote: > IIUC this is to make drainAsBlobDataHandle() to return nullptr after > drainAsFormData(). > > How about implementing drainAsBlobDataHandle() as: > RefPtr<BlobDataHandle> handle = m_reader->drainAsBlobDataHandle(); > if (handle) { > if (m_context->drainFormData()) > return handle.release(); > } > return nullptr; > and remove L213-L220? > By doing so, > - we don't start ThreadableLoader by calling beginRead() here unnecessarily, and > - drainAs.*() is controlled by primarily m_context->drainFormData(), and I think > this is clearer (i.e. I'd like to avoid to depend on side effects of beginRead() > here). That depends on how strong we believe that the underlying handle returns a valid handle. For example, your code fails when the underlying handle has a blob with invalid size. Your point about ThreadableLoader is true but about clearness I'm not sure. By the way, we can introduce state in ReaderImpl and dispose the underlying handle and reader. What do you think about it? https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... File Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp (right): https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:244: EXPECT_TRUE(formData->hasOneRef()); On 2015/08/12 06:44:48, hiroshige wrote: > isSafeToSendToAnotherThread() would be better. Done. https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:259: EXPECT_TRUE(formData->hasOneRef()); On 2015/08/12 06:44:48, hiroshige wrote: > ditto. Done. https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:274: EXPECT_TRUE(outputFormData->hasOneRef()); On 2015/08/12 06:44:48, hiroshige wrote: > ditto. Done. https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:287: EXPECT_TRUE(outputFormData->hasOneRef()); On 2015/08/12 06:44:48, hiroshige wrote: > ditto. Done. https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:300: EXPECT_TRUE(formData->hasOneRef()); On 2015/08/12 06:44:48, hiroshige wrote: > ditto. Done. https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:342: EXPECT_TRUE(formData->hasOneRef()); On 2015/08/12 06:44:48, hiroshige wrote: > ditto. Done.
https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... File Source/modules/fetch/FetchFormDataConsumerHandle.cpp (right): https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:131: On 2015/08/12 07:43:42, yhirano wrote: > On 2015/08/12 06:44:48, hiroshige wrote: > > IIUC drainAsBlobDataHandle() here is omitted (thus it returns null for > > SimpleContext), just because it is currently not implemented/needed and we can > > implement it if needed (i.e. not because we intentionally disabled for some > > reasons). > > If so, leaving a comment would be helpful. > > Hm, yes, but I don't think it is worth noting, is it? > If we have a critical reason against implementing, it would be worth noting. optional: I think turning something implicit into explicit is always helpful for external readers. https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:217: if (Ok == m_reader->beginRead(&p, FlagNone, &available)) { > By the way, we can introduce state in ReaderImpl and dispose the underlying > handle and reader. What do you think about it? Sounds good.
https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... File Source/modules/fetch/FetchFormDataConsumerHandle.cpp (right): https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:131: On 2015/08/13 06:44:31, hiroshige wrote: > On 2015/08/12 07:43:42, yhirano wrote: > > On 2015/08/12 06:44:48, hiroshige wrote: > > > IIUC drainAsBlobDataHandle() here is omitted (thus it returns null for > > > SimpleContext), just because it is currently not implemented/needed and we > can > > > implement it if needed (i.e. not because we intentionally disabled for some > > > reasons). > > > If so, leaving a comment would be helpful. > > > > Hm, yes, but I don't think it is worth noting, is it? > > If we have a critical reason against implementing, it would be worth noting. > > optional: I think turning something implicit into explicit is always helpful for > external readers. And the DrainAsBlobDataHandle unit test asserts drainAsBlobDataHandle() to fail, so it would be a signal that we explicitly disallow drainAsBlobDataHandle() for FormData. https://codereview.chromium.org/1265413002/diff/180001/Source/modules/fetch/F... File Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp (right): https://codereview.chromium.org/1265413002/diff/180001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:97: void verifyFormData(FormData* data) nit optional: a name like verifyComplexFormData() would be better to make the name parallel to complexFormData(). https://codereview.chromium.org/1265413002/diff/180001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:144: TEST_F(FetchFormDataConsumerHandleTest, ReadFromStringNonLatain) nit: s/Latain/Latin/ https://codereview.chromium.org/1265413002/diff/180001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:326: reader->endRead(available); Should we call reader->endRead(0) and EXPECT_FALSE(reader->drainAsFormData()); again here, to test draubAsFormData() after beginRead()/endRead() (i.e. not only during two-phase read)?
https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... File Source/modules/fetch/FetchFormDataConsumerHandle.cpp (right): https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:131: On 2015/08/13 07:03:50, hiroshige (slow) wrote: > On 2015/08/13 06:44:31, hiroshige wrote: > > On 2015/08/12 07:43:42, yhirano wrote: > > > On 2015/08/12 06:44:48, hiroshige wrote: > > > > IIUC drainAsBlobDataHandle() here is omitted (thus it returns null for > > > > SimpleContext), just because it is currently not implemented/needed and we > > can > > > > implement it if needed (i.e. not because we intentionally disabled for > some > > > > reasons). > > > > If so, leaving a comment would be helpful. > > > > > > Hm, yes, but I don't think it is worth noting, is it? > > > If we have a critical reason against implementing, it would be worth noting. > > > > optional: I think turning something implicit into explicit is always helpful > for > > external readers. > > And the DrainAsBlobDataHandle unit test asserts drainAsBlobDataHandle() to fail, > so it would be a signal that we explicitly disallow drainAsBlobDataHandle() for > FormData. If stating the reason for doing the default action is needed, I think there should not be the default action, i.e. draining functions should be pure virtual.
https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... File Source/modules/fetch/FetchFormDataConsumerHandle.cpp (right): https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:131: On 2015/08/13 15:17:56, yhirano wrote: > On 2015/08/13 07:03:50, hiroshige (slow) wrote: > > On 2015/08/13 06:44:31, hiroshige wrote: > > > On 2015/08/12 07:43:42, yhirano wrote: > > > > On 2015/08/12 06:44:48, hiroshige wrote: > > > > > IIUC drainAsBlobDataHandle() here is omitted (thus it returns null for > > > > > SimpleContext), just because it is currently not implemented/needed and > we > > > can > > > > > implement it if needed (i.e. not because we intentionally disabled for > > some > > > > > reasons). > > > > > If so, leaving a comment would be helpful. > > > > > > > > Hm, yes, but I don't think it is worth noting, is it? > > > > If we have a critical reason against implementing, it would be worth > noting. > > > > > > optional: I think turning something implicit into explicit is always helpful > > for > > > external readers. > > > > And the DrainAsBlobDataHandle unit test asserts drainAsBlobDataHandle() to > fail, > > so it would be a signal that we explicitly disallow drainAsBlobDataHandle() > for > > FormData. > > If stating the reason for doing the default action is needed, I think there > should not be the default action, i.e. draining functions should be pure > virtual. Hmm. Anyway it is not a strong objection so please go ahead.
https://codereview.chromium.org/1265413002/diff/180001/Source/modules/fetch/F... File Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp (right): https://codereview.chromium.org/1265413002/diff/180001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:97: void verifyFormData(FormData* data) On 2015/08/13 07:03:51, hiroshige (ooo zombie) wrote: > nit optional: a name like verifyComplexFormData() would be better to make the > name parallel to complexFormData(). Done. https://codereview.chromium.org/1265413002/diff/180001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:144: TEST_F(FetchFormDataConsumerHandleTest, ReadFromStringNonLatain) On 2015/08/13 07:03:51, hiroshige (ooo zombie) wrote: > nit: s/Latain/Latin/ Done. https://codereview.chromium.org/1265413002/diff/180001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:326: reader->endRead(available); On 2015/08/13 07:03:50, hiroshige (ooo zombie) wrote: > Should we call reader->endRead(0) and EXPECT_FALSE(reader->drainAsFormData()); > again here, to test draubAsFormData() after beginRead()/endRead() (i.e. not only > during two-phase read)? Done.
https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... File Source/modules/fetch/FetchFormDataConsumerHandle.cpp (right): https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:204: RefPtr<BlobDataHandle> handle = m_reader->drainAsBlobDataHandle(); Please pass |policy| to m_reader->drainAsBlobDataHandle(). https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:217: if (Ok == m_reader->beginRead(&p, FlagNone, &available)) { On 2015/08/13 06:44:31, hiroshige (ooo zombie) wrote: > > By the way, we can introduce state in ReaderImpl and dispose the underlying > > handle and reader. What do you think about it? > Sounds good. Another way to avoid starting ThreadableLoader is calling m_reader->drainAsBlobDataHandle(AllowBlobWithInvalidSize) here. I make the issue related this comment thread (including my previous comments) as optional. (Leaving a comment stating this code block is to make drainAsBlobDataHandle() to return nullptr would be helpful) https://codereview.chromium.org/1265413002/diff/200001/Source/modules/fetch/F... File Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp (right): https://codereview.chromium.org/1265413002/diff/200001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:326: reader->endRead(available); Should we call endRead(0) rather than endRead(available) to test beginRead()/endRead(0) is not 0-byte read and thus invalidates FormData draining?
https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... File Source/modules/fetch/FetchFormDataConsumerHandle.cpp (right): https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:204: RefPtr<BlobDataHandle> handle = m_reader->drainAsBlobDataHandle(); On 2015/08/17 10:08:17, hiroshige (ooo zombie) wrote: > Please pass |policy| to m_reader->drainAsBlobDataHandle(). Done. https://codereview.chromium.org/1265413002/diff/160001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandle.cpp:217: if (Ok == m_reader->beginRead(&p, FlagNone, &available)) { On 2015/08/17 10:08:17, hiroshige (ooo zombie) wrote: > On 2015/08/13 06:44:31, hiroshige (ooo zombie) wrote: > > > By the way, we can introduce state in ReaderImpl and dispose the underlying > > > handle and reader. What do you think about it? > > Sounds good. > Another way to avoid starting ThreadableLoader is calling > m_reader->drainAsBlobDataHandle(AllowBlobWithInvalidSize) here. > > I make the issue related this comment thread (including my previous comments) as > optional. > (Leaving a comment stating this code block is to make drainAsBlobDataHandle() to > return nullptr would be helpful) Done. https://codereview.chromium.org/1265413002/diff/200001/Source/modules/fetch/F... File Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp (right): https://codereview.chromium.org/1265413002/diff/200001/Source/modules/fetch/F... Source/modules/fetch/FetchFormDataConsumerHandleTest.cpp:326: reader->endRead(available); On 2015/08/17 10:08:18, hiroshige (ooo zombie) wrote: > Should we call endRead(0) rather than endRead(available) to test > beginRead()/endRead(0) is not 0-byte read and thus invalidates FormData > draining? Done.
lgtm.
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1265413002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1265413002/220001
Message was sent while issue was closed.
Committed patchset #9 (id:220001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200701 |
