Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2269953004:
Implment BytesConsumer::tee (Closed)
DescriptionImplment BytesConsumer::tee
This CL implements BytesConsumer::tee function which will replace
DataConsumerTee::create. This CL also introduces
BytesConsumerTestUtil::ReplayingBytesConsumer which is equivalent to
DataConsumerHandleTestUtil::ReplayingHandle.
BUG=610195
Committed: https://crrev.com/0cc0e2b584b5da0884a7b9ef244a1d94ec5e6600
Cr-Commit-Position: refs/heads/master@{#418217}
Patch Set 1 : fix #Patch Set 2 : fix #Patch Set 3 : rebase #Patch Set 4 : fix #
Total comments: 10
Patch Set 5 : fix #Patch Set 6 : fix #Patch Set 7 : fix #Patch Set 8 : fix #
Total comments: 12
Patch Set 9 : fix #Patch Set 10 : fix #Patch Set 11 : fix #
Total comments: 64
Patch Set 12 : fix #Patch Set 13 : fix #Patch Set 14 : fix #Dependent Patchsets: Messages
Total messages: 81 (62 generated)
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Implment BytesConsumer::tee wip BUG=610195 ========== to ========== Implment BytesConsumer::tee This CL implements BytesConsumer::tee function which will replace DataConsumerTee::create. This CL also introduces BytesConsumerTestUtil::ReplayingBytesConsumer whic is equivalent to DataConsumerHandleTestUtil::ReplayingHandle. BUG=610195 ==========
yhirano@chromium.org changed reviewers: + hiroshige@chromium.org
Description was changed from ========== Implment BytesConsumer::tee This CL implements BytesConsumer::tee function which will replace DataConsumerTee::create. This CL also introduces BytesConsumerTestUtil::ReplayingBytesConsumer whic is equivalent to DataConsumerHandleTestUtil::ReplayingHandle. BUG=610195 ========== to ========== Implment BytesConsumer::tee This CL implements BytesConsumer::tee function which will replace DataConsumerTee::create. This CL also introduces BytesConsumerTestUtil::ReplayingBytesConsumer whic is equivalent to DataConsumerHandleTestUtil::ReplayingHandle. BUG=610195 ==========
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== Implment BytesConsumer::tee This CL implements BytesConsumer::tee function which will replace DataConsumerTee::create. This CL also introduces BytesConsumerTestUtil::ReplayingBytesConsumer whic is equivalent to DataConsumerHandleTestUtil::ReplayingHandle. BUG=610195 ========== to ========== Implment BytesConsumer::tee This CL implements BytesConsumer::tee function which will replace DataConsumerTee::create. This CL also introduces BytesConsumerTestUtil::ReplayingBytesConsumer which is equivalent to DataConsumerHandleTestUtil::ReplayingHandle. BUG=610195 ==========
https://codereview.chromium.org/2269953004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp (right): https://codereview.chromium.org/2269953004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp:137: if (m_chunks.isEmpty()) { How about turning this if statement into |if (!m_chunks.isEmpty())| and reversing the order of Lines 138--147 and Lines 149--153, in order to make the order consistent with getPublicState()? https://codereview.chromium.org/2269953004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp:142: clearClientIfClosedOrErrored(); Is clearClient() sufficient? https://codereview.chromium.org/2269953004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp:145: clearClientIfClosedOrErrored(); ditto. https://codereview.chromium.org/2269953004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp:189: if (state == PublicState::Closed || state == PublicState::Errored) When |m_isCancelled| is false and |m_chunks.isEmpty()| is true (i.e. the source became Closed/Errored and cancel() is called before Tee::onStateChange() is called), then cancel() does nothing and leave |m_isCancelled| and |m_client| as-is. - Leaving |m_client| non-null causes the client to be notified after cancel() in notify() that is called from onStateChange(). - I noticed BytesConsumer.h doesn't say anything about Client's lifetime, e.g. whether BytesConsumer holds a strong reference to Client when setClient() is called, or whether the strong reference is cleared when the BytesConsumer becomes Closed or Errored.
https://codereview.chromium.org/2269953004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp (right): https://codereview.chromium.org/2269953004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp:158: DCHECK(!m_chunks.isEmpty()); This DCHECK means clearChunks() and thus onStateChange() cannot be called during two-phase read, but this restriction is not guaranteed in the BytesConsumer.h's comments.
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I changed my mind after the offline discussion. We don't allow Done with non-zero size. https://codereview.chromium.org/2269953004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp (right): https://codereview.chromium.org/2269953004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp:137: if (m_chunks.isEmpty()) { On 2016/08/30 07:42:47, hiroshige wrote: > How about turning this if statement into |if (!m_chunks.isEmpty())| and > reversing the order of Lines 138--147 and Lines 149--153, in order to make the > order consistent with getPublicState()? Done. https://codereview.chromium.org/2269953004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp:142: clearClientIfClosedOrErrored(); On 2016/08/30 07:42:47, hiroshige wrote: > Is clearClient() sufficient? Done. https://codereview.chromium.org/2269953004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp:145: clearClientIfClosedOrErrored(); On 2016/08/30 07:42:47, hiroshige wrote: > ditto. Done. https://codereview.chromium.org/2269953004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp:158: DCHECK(!m_chunks.isEmpty()); On 2016/08/30 07:47:38, hiroshige wrote: > This DCHECK means clearChunks() and thus onStateChange() cannot be called during > two-phase read, but this restriction is not guaranteed in the BytesConsumer.h's > comments. Done. https://codereview.chromium.org/2269953004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp:189: if (state == PublicState::Closed || state == PublicState::Errored) On 2016/08/30 07:42:47, hiroshige wrote: > When |m_isCancelled| is false and |m_chunks.isEmpty()| is true (i.e. the source > became Closed/Errored and cancel() is called before Tee::onStateChange() is > called), then cancel() does nothing and leave |m_isCancelled| and |m_client| > as-is. > - Leaving |m_client| non-null causes the client to be notified after cancel() in > notify() that is called from onStateChange(). > - I noticed BytesConsumer.h doesn't say anything about Client's lifetime, e.g. > whether BytesConsumer holds a strong reference to Client when setClient() is > called, or whether the strong reference is cleared when the BytesConsumer > becomes Closed or Errored. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hiroshige: ping
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2269953004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp (right): https://codereview.chromium.org/2269953004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp:144: Bytes* bytes = m_chunks[0]; We use "bytes" and "chunks" for the same thing. Can we unify the names? https://codereview.chromium.org/2269953004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp:168: DCHECK(m_chunkInUse); optional: Adding DCHECK(m_chunkInUse == m_chunks[0]) if m_chunk is not empty might be helpful for catching unexpected errors. https://codereview.chromium.org/2269953004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumer.h (right): https://codereview.chromium.org/2269953004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:97: // even if endRead is not called. Just to check: does this mean it is valid to garbage collect without calling endRead()? https://codereview.chromium.org/2269953004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:132: // errored (after the state change itself it notified). nit: /it notified/is notified/? https://codereview.chromium.org/2269953004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp (right): https://codereview.chromium.org/2269953004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:170: if (m_isInTwoPhaseRead) { Should we do this check before m_reader->read()? we can't call read() during two-phase read.
https://codereview.chromium.org/2269953004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumer.h (right): https://codereview.chromium.org/2269953004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:104: virtual Result endRead(size_t readSize) = 0; Can we add WARN_UNUSED_RESULT or something like?
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2269953004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp (right): https://codereview.chromium.org/2269953004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp:144: Bytes* bytes = m_chunks[0]; On 2016/09/07 09:17:47, hiroshige wrote: > We use "bytes" and "chunks" for the same thing. Can we unify the names? Done. https://codereview.chromium.org/2269953004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp:168: DCHECK(m_chunkInUse); On 2016/09/07 09:17:47, hiroshige wrote: > optional: Adding DCHECK(m_chunkInUse == m_chunks[0]) if m_chunk is not empty > might be helpful for catching unexpected errors. Done. https://codereview.chromium.org/2269953004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumer.h (right): https://codereview.chromium.org/2269953004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:97: // even if endRead is not called. On 2016/09/07 09:17:47, hiroshige wrote: > Just to check: does this mean it is valid to garbage collect without calling > endRead()? Yes, do you think it problematic? https://codereview.chromium.org/2269953004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:104: virtual Result endRead(size_t readSize) = 0; On 2016/09/07 09:26:50, hiroshige wrote: > Can we add WARN_UNUSED_RESULT or something like? I would do that separately. https://codereview.chromium.org/2269953004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:132: // errored (after the state change itself it notified). On 2016/09/07 09:17:47, hiroshige wrote: > nit: /it notified/is notified/? Done. https://codereview.chromium.org/2269953004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp (right): https://codereview.chromium.org/2269953004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:170: if (m_isInTwoPhaseRead) { On 2016/09/07 09:17:47, hiroshige wrote: > Should we do this check before m_reader->read()? we can't call read() during > two-phase read. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp (right): https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp:70: DCHECK_EQ(Result::Ok, result); nit: how about using switch() and |case Result::Ok|? https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp:116: m_buffer.reserveCapacity(size); reserveInitialCapacity()? (I'm not a WTF::Vector specialist though) https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp:212: m_chunks.clear(); How about calling clearChunks() to make |m_offset == 0 when m_chunks is empty| an invariant? https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp (right): https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:108: EXPECT_TRUE(result1.second.isEmpty()); Add EXPECT_EQ(Done, dest1->getPublicState()); https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:110: EXPECT_TRUE(result2.second.isEmpty()); Add EXPECT_EQ(Done, dest2->getPublicState()); https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:111: EXPECT_FALSE(src->isCancelled()); How about adding here or somewhere: dest1->cancel(); dest2->cancel(); EXPECT_EQ(Done, dest1->getPublicState()); // not changing EXPECT_EQ(Done, dest2->getPublicState()); // not changing EXPECT_FALSE(src->isCancelled()); to confirm cancel does nothing after Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:129: Add EXPECT_EQ(ReadableOrWaiting, dest1->getPublicState()); Add EXPECT_EQ(ReadableOrWaiting, dest2->getPublicState()); https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:134: EXPECT_EQ("hello, world", toString(result1.second)); Add EXPECT_EQ(Done, dest1->getPublicState()); https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:136: EXPECT_EQ("hello, world", toString(result2.second)); Add EXPECT_EQ(Done, dest2->getPublicState()); https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:155: Add EXPECT_EQ(ReadableOrWaiting, dest1->getPublicState()); Add EXPECT_EQ(ReadableOrWaiting, dest2->getPublicState()); https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:160: EXPECT_EQ("hello, world", toString(result1.second)); Add EXPECT_EQ(Done, dest1->getPublicState()); https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:162: EXPECT_EQ("hello, world", toString(result2.second)); Add EXPECT_EQ(Done, dest2->getPublicState()); https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:177: Add EXPECT_EQ(Errored, dest1->getPublicState()); Add EXPECT_EQ(Errored, dest2->getPublicState()); https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:182: EXPECT_EQ(Result::Error, result2.first); Add EXPECT_TRUE(result1.second.isEmpty()); EXPECT_TRUE(result2.second.isEmpty()); If the public state before run() is Errored, then the result must be empty. (If the public state before run() was not Error, the result doesn't have to be empty though) https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:183: EXPECT_FALSE(src->isCancelled()); How about adding dest1->cancel(); dest2->cancel(); EXPECT_EQ(Errored, dest1->getPublicState()); // not changing to Done EXPECT_EQ(Errored, dest2->getPublicState()); // not changing to Done EXPECT_FALSE(src->isCancelled()); to confirm cancel does nothing after errored. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:197: EXPECT_FALSE(src->isCancelled()); EXPECT_EQ(ReadableOrWaiting, dest1->getPublicState()); https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:198: dest1->cancel(); EXPECT_EQ(Closed, dest1->getPublicState()); EXPECT_EQ(ReadableOrWaiting, dest2->getPublicState()); https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:200: dest2->cancel(); EXPECT_EQ(Closed, dest1->getPublicState()); EXPECT_EQ(Closed, dest2->getPublicState()); https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:203: Please add tests where: - |dest1| is cancelled and |dest2| is not cancelled and becomes |Done| - |dest1| is cancelled and |dest2| is not cancelled and becomes |Errored| to confirm nothing wrong happens when only one branch of tee is cancelled? e.g. dest1->cancel(); EXPECT_EQ(Done, dest1->getPublicState()); EXPECT_EQ(ReadableOrWaiting, dest2->getPublicState()); (...Reader(dest2)...)->run(); EXPECT_EQ(Done, dest1->getPublicState()); // check that dest1's status doesn't change EXPECT_EQ(Done/Errored, dest2->getPublicState()); // check read result for dest2 EXPECT_FALSE(src->isCancelled()); https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:253: while (true) { Why do we need this while loop? Doesn't dest1->beginRead() return Ok for the first invocation? To avoid this test depending on the current behavior of tee too much? https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:261: ASSERT_EQ(BytesConsumer::PublicState::Errored, dest1->getPublicState()); https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:266: EXPECT_EQ(Result::Ok, dest1->endRead(0)); How about testing that we can read the data (i.e. EXPECT_EQ(buffer[0], 'a') and endRead(1)) even when the state becomes Errored? https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:279: In this test, |src->getPublicState()| is Closed here. Do we need another test where |src->getPublicState()| becomes Closed during two-phase read? https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:288: testing::runPendingTasks(); Add EXPECT_EQ('a', buffer[0]); (this might be helpful in catching use-after-free if tee failed to retain |*buffer| alive when src becomes Done) https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumerTestUtil.cpp (right): https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTestUtil.cpp:54: error(Error(String::fromUTF8(command.body().data(), command.body().size()))); |command| (=m_commands[0]) is accessed after |m_commands.removeFirst()|, which looks unsafe. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTestUtil.cpp:159: char buffer[3]; Does the number |3| have meaning? (IIUC this is just a buffering size and any positive integer works) https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTestUtil.cpp:181: testing::runPendingTasks(); How about always executing runPendingTasks(), to confirm something wrong does not occur after |m_result| becomes Done/Error? https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTestUtil.cpp:199: size_t read = std::max(static_cast<size_t>(3), available); Does the number |3| have meaning? Must the |3| here be the same as |3| in BytesConsumerTestUtil::Reader::onStateChange()? (IIUC this is just a buffering size and any positive integer works) https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTestUtil.cpp:220: testing::runPendingTasks(); How about always executing runPendingTasks(), to confirm something wrong does not occur after |m_result| becomes Done/Error?
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp (right): https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp:70: DCHECK_EQ(Result::Ok, result); On 2016/09/08 08:25:54, hiroshige wrote: > nit: how about using switch() and |case Result::Ok|? Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp:116: m_buffer.reserveCapacity(size); On 2016/09/08 08:25:54, hiroshige wrote: > reserveInitialCapacity()? (I'm not a WTF::Vector specialist though) Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.cpp:212: m_chunks.clear(); On 2016/09/08 08:25:54, hiroshige wrote: > How about calling clearChunks() to make |m_offset == 0 when m_chunks is empty| > an invariant? Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp (right): https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:108: EXPECT_TRUE(result1.second.isEmpty()); On 2016/09/08 08:25:55, hiroshige wrote: > Add EXPECT_EQ(Done, dest1->getPublicState()); Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:110: EXPECT_TRUE(result2.second.isEmpty()); On 2016/09/08 08:25:55, hiroshige wrote: > Add EXPECT_EQ(Done, dest2->getPublicState()); Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:111: EXPECT_FALSE(src->isCancelled()); On 2016/09/08 08:25:54, hiroshige wrote: > How about adding here or somewhere: > dest1->cancel(); > dest2->cancel(); > EXPECT_EQ(Done, dest1->getPublicState()); // not changing > EXPECT_EQ(Done, dest2->getPublicState()); // not changing > EXPECT_FALSE(src->isCancelled()); > to confirm cancel does nothing after Done. Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:129: On 2016/09/08 08:25:54, hiroshige wrote: > Add EXPECT_EQ(ReadableOrWaiting, dest1->getPublicState()); > Add EXPECT_EQ(ReadableOrWaiting, dest2->getPublicState()); Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:134: EXPECT_EQ("hello, world", toString(result1.second)); On 2016/09/08 08:25:55, hiroshige wrote: > Add EXPECT_EQ(Done, dest1->getPublicState()); Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:136: EXPECT_EQ("hello, world", toString(result2.second)); On 2016/09/08 08:25:54, hiroshige wrote: > Add EXPECT_EQ(Done, dest2->getPublicState()); Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:155: On 2016/09/08 08:25:54, hiroshige wrote: > Add EXPECT_EQ(ReadableOrWaiting, dest1->getPublicState()); > Add EXPECT_EQ(ReadableOrWaiting, dest2->getPublicState()); Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:160: EXPECT_EQ("hello, world", toString(result1.second)); On 2016/09/08 08:25:54, hiroshige wrote: > Add EXPECT_EQ(Done, dest1->getPublicState()); Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:162: EXPECT_EQ("hello, world", toString(result2.second)); On 2016/09/08 08:25:54, hiroshige wrote: > Add EXPECT_EQ(Done, dest2->getPublicState()); Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:177: On 2016/09/08 08:25:55, hiroshige wrote: > Add EXPECT_EQ(Errored, dest1->getPublicState()); > Add EXPECT_EQ(Errored, dest2->getPublicState()); Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:182: EXPECT_EQ(Result::Error, result2.first); On 2016/09/08 08:25:54, hiroshige wrote: > Add > EXPECT_TRUE(result1.second.isEmpty()); > EXPECT_TRUE(result2.second.isEmpty()); > > If the public state before run() is Errored, then the result must be empty. > (If the public state before run() was not Error, the result doesn't have to be > empty though) Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:183: EXPECT_FALSE(src->isCancelled()); On 2016/09/08 08:25:55, hiroshige wrote: > How about adding > dest1->cancel(); > dest2->cancel(); > EXPECT_EQ(Errored, dest1->getPublicState()); // not changing to Done > EXPECT_EQ(Errored, dest2->getPublicState()); // not changing to Done > EXPECT_FALSE(src->isCancelled()); > to confirm cancel does nothing after errored. Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:197: EXPECT_FALSE(src->isCancelled()); On 2016/09/08 08:25:54, hiroshige wrote: > EXPECT_EQ(ReadableOrWaiting, dest1->getPublicState()); Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:198: dest1->cancel(); On 2016/09/08 08:25:54, hiroshige wrote: > EXPECT_EQ(Closed, dest1->getPublicState()); > EXPECT_EQ(ReadableOrWaiting, dest2->getPublicState()); Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:200: dest2->cancel(); On 2016/09/08 08:25:55, hiroshige wrote: > EXPECT_EQ(Closed, dest1->getPublicState()); > EXPECT_EQ(Closed, dest2->getPublicState()); Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:203: On 2016/09/08 08:25:55, hiroshige wrote: > Please add tests where: > - |dest1| is cancelled and |dest2| is not cancelled and becomes |Done| > - |dest1| is cancelled and |dest2| is not cancelled and becomes |Errored| > to confirm nothing wrong happens when only one branch of tee is cancelled? > e.g. > dest1->cancel(); > EXPECT_EQ(Done, dest1->getPublicState()); > EXPECT_EQ(ReadableOrWaiting, dest2->getPublicState()); > (...Reader(dest2)...)->run(); > EXPECT_EQ(Done, dest1->getPublicState()); // check that dest1's status doesn't > change > EXPECT_EQ(Done/Errored, dest2->getPublicState()); > // check read result for dest2 > EXPECT_FALSE(src->isCancelled()); Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:253: while (true) { On 2016/09/08 08:25:54, hiroshige wrote: > Why do we need this while loop? Doesn't dest1->beginRead() return Ok for the > first invocation? To avoid this test depending on the current behavior of tee > too much? Hmm ok, deleted https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:261: On 2016/09/08 08:25:54, hiroshige wrote: > ASSERT_EQ(BytesConsumer::PublicState::Errored, dest1->getPublicState()); It's ReadableOrWaiting here. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:266: EXPECT_EQ(Result::Ok, dest1->endRead(0)); On 2016/09/08 08:25:54, hiroshige wrote: > How about testing that we can read the data (i.e. EXPECT_EQ(buffer[0], 'a') and > endRead(1)) even when the state becomes Errored? Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:279: On 2016/09/08 08:25:54, hiroshige wrote: > In this test, |src->getPublicState()| is Closed here. > Do we need another test where |src->getPublicState()| becomes Closed during > two-phase read? I think we don't need the former if we have the latter. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:288: testing::runPendingTasks(); On 2016/09/08 08:25:54, hiroshige wrote: > Add EXPECT_EQ('a', buffer[0]); > (this might be helpful in catching use-after-free if tee failed to retain > |*buffer| alive when src becomes Done) Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumerTestUtil.cpp (right): https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTestUtil.cpp:54: error(Error(String::fromUTF8(command.body().data(), command.body().size()))); On 2016/09/08 08:25:55, hiroshige wrote: > |command| (=m_commands[0]) is accessed after |m_commands.removeFirst()|, which > looks unsafe. Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTestUtil.cpp:159: char buffer[3]; On 2016/09/08 08:25:55, hiroshige wrote: > Does the number |3| have meaning? > (IIUC this is just a buffering size and any positive integer works) I want the number to be small (to split a string into multiple chunks) and odd (I guess odd numbers can detect heap-overflow bugs than even numbers). https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTestUtil.cpp:181: testing::runPendingTasks(); On 2016/09/08 08:25:55, hiroshige wrote: > How about always executing runPendingTasks(), to confirm something wrong does > not occur after |m_result| becomes Done/Error? Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTestUtil.cpp:199: size_t read = std::max(static_cast<size_t>(3), available); On 2016/09/08 08:25:55, hiroshige wrote: > Does the number |3| have meaning? Must the |3| here be the same as |3| in > BytesConsumerTestUtil::Reader::onStateChange()? > (IIUC this is just a buffering size and any positive integer works) No, but the reason is the same as the read one. Actually, we don't need any number in this case. Using |available| works perfectly. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTestUtil.cpp:220: testing::runPendingTasks(); On 2016/09/08 08:25:55, hiroshige wrote: > How about always executing runPendingTasks(), to confirm something wrong does > not occur after |m_result| becomes Done/Error? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp (right): https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:261: On 2016/09/08 09:41:34, yhirano wrote: > On 2016/09/08 08:25:54, hiroshige wrote: > > ASSERT_EQ(BytesConsumer::PublicState::Errored, dest1->getPublicState()); > > It's ReadableOrWaiting here. Acknowledged. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTest.cpp:279: On 2016/09/08 09:41:35, yhirano wrote: > On 2016/09/08 08:25:54, hiroshige wrote: > > In this test, |src->getPublicState()| is Closed here. > > Do we need another test where |src->getPublicState()| becomes Closed during > > two-phase read? > > I think we don't need the former if we have the latter. Acknowledged. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumerTestUtil.cpp (right): https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTestUtil.cpp:159: char buffer[3]; On 2016/09/08 09:41:35, yhirano wrote: > On 2016/09/08 08:25:55, hiroshige wrote: > > Does the number |3| have meaning? > > (IIUC this is just a buffering size and any positive integer works) > > I want the number to be small (to split a string into multiple chunks) and odd > (I guess odd numbers can detect heap-overflow bugs than even numbers). Understood. Adding a short comment might be helpful. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTestUtil.cpp:199: size_t read = std::max(static_cast<size_t>(3), available); On 2016/09/08 09:41:35, yhirano wrote: > On 2016/09/08 08:25:55, hiroshige wrote: > > Does the number |3| have meaning? Must the |3| here be the same as |3| in > > BytesConsumerTestUtil::Reader::onStateChange()? > > (IIUC this is just a buffering size and any positive integer works) > > No, but the reason is the same as the read one. Actually, we don't need any > number in this case. Using |available| works perfectly. ditto.
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumerTestUtil.cpp (right): https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTestUtil.cpp:159: char buffer[3]; On 2016/09/09 06:37:30, hiroshige wrote: > On 2016/09/08 09:41:35, yhirano wrote: > > On 2016/09/08 08:25:55, hiroshige wrote: > > > Does the number |3| have meaning? > > > (IIUC this is just a buffering size and any positive integer works) > > > > I want the number to be small (to split a string into multiple chunks) and odd > > (I guess odd numbers can detect heap-overflow bugs than even numbers). > > Understood. Adding a short comment might be helpful. Done. https://codereview.chromium.org/2269953004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerTestUtil.cpp:199: size_t read = std::max(static_cast<size_t>(3), available); On 2016/09/09 06:37:30, hiroshige wrote: > On 2016/09/08 09:41:35, yhirano wrote: > > On 2016/09/08 08:25:55, hiroshige wrote: > > > Does the number |3| have meaning? Must the |3| here be the same as |3| in > > > BytesConsumerTestUtil::Reader::onStateChange()? > > > (IIUC this is just a buffering size and any positive integer works) > > > > No, but the reason is the same as the read one. Actually, we don't need any > > number in this case. Using |available| works perfectly. > > ditto. Done.
yhirano@chromium.org changed reviewers: + haraken@chromium.org
+haraken for modules/BUILD.gn
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
haraken@, can you take a look at modules/BUILD.gn?
On 2016/09/13 07:31:02, yhirano (slow) wrote: > haraken@, can you take a look at modules/BUILD.gn? LGTM
On 2016/09/13 07:31:02, yhirano (slow) wrote: > haraken@, can you take a look at modules/BUILD.gn? LGTM
The CQ bit was checked by yhirano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org Link to the patchset: https://codereview.chromium.org/2269953004/#ps280001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Implment BytesConsumer::tee This CL implements BytesConsumer::tee function which will replace DataConsumerTee::create. This CL also introduces BytesConsumerTestUtil::ReplayingBytesConsumer which is equivalent to DataConsumerHandleTestUtil::ReplayingHandle. BUG=610195 ========== to ========== Implment BytesConsumer::tee This CL implements BytesConsumer::tee function which will replace DataConsumerTee::create. This CL also introduces BytesConsumerTestUtil::ReplayingBytesConsumer which is equivalent to DataConsumerHandleTestUtil::ReplayingHandle. BUG=610195 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Implment BytesConsumer::tee This CL implements BytesConsumer::tee function which will replace DataConsumerTee::create. This CL also introduces BytesConsumerTestUtil::ReplayingBytesConsumer which is equivalent to DataConsumerHandleTestUtil::ReplayingHandle. BUG=610195 ========== to ========== Implment BytesConsumer::tee This CL implements BytesConsumer::tee function which will replace DataConsumerTee::create. This CL also introduces BytesConsumerTestUtil::ReplayingBytesConsumer which is equivalent to DataConsumerHandleTestUtil::ReplayingHandle. BUG=610195 Committed: https://crrev.com/0cc0e2b584b5da0884a7b9ef244a1d94ec5e6600 Cr-Commit-Position: refs/heads/master@{#418217} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/0cc0e2b584b5da0884a7b9ef244a1d94ec5e6600 Cr-Commit-Position: refs/heads/master@{#418217} |
