Chromium Code Reviews
|
|
DescriptionIntroduce DataConsumerHandleTee
Implement a function that "tee"s a WebDataConsumerHandle, i.e. creates
two handles that will generate the same data as the original handle would
generate.
BUG=480746
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197733
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 10
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Total comments: 4
Patch Set 7 : #
Total comments: 28
Patch Set 8 : #
Total comments: 6
Patch Set 9 : #
Total comments: 2
Messages
Total messages: 30 (11 generated)
Patchset #2 (id:20001) has been deleted
Patchset #3 (id:50001) has been deleted
Patchset #2 (id:10005) has been deleted
yhirano@chromium.org changed reviewers: + hiroshige@chromium.org
https://codereview.chromium.org/1195563002/diff/110001/Source/modules/fetch/D... File Source/modules/fetch/DataConsumerTee.cpp (right): https://codereview.chromium.org/1195563002/diff/110001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTee.cpp:37: // destination contexts. Destination readers read data from its assocaited nit: associated https://codereview.chromium.org/1195563002/diff/110001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTee.cpp:89: void initialize(SourceContext* context) optional: I prefer names like |sourceContext|, |destinationContext| etc. rather than just |context|, because there are three kinds of contexts in this file. https://codereview.chromium.org/1195563002/diff/110001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTee.cpp:113: void didDetach(Identifier identifier) How about calling m_root->stop() in the dtor? Then we can remove Identifier, m_mutex, m_isFirst|SecondActive, didDetach(). https://codereview.chromium.org/1195563002/diff/110001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTee.cpp:207: detach(); optional: I prefer to create a tracker object that is referenced by DestinationHandler and DestinationReader and references DestinationContext, and call DestinationContext::detach() on the tracker's dtor; liveness management using |m_isHandleActive| (as well as |m_isFirstActive| and |m_isSecondActive|) seems somehow prone to error. However, this increases the numbers of classes and objects... https://codereview.chromium.org/1195563002/diff/110001/Source/modules/fetch/D... File Source/modules/fetch/DataConsumerTee.h (right): https://codereview.chromium.org/1195563002/diff/110001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTee.h:20: static void create(ExecutionContext*, PassOwnPtr<WebDataConsumerHandle> in, OwnPtr<WebDataConsumerHandle>* out1, OwnPtr<WebDataConsumerHandle>* out2); nit: Should we name the argument as |src|, |dest1/2| etc. to make the declaration consistent with the definition and the comments in .cpp?
Patchset #5 (id:130001) has been deleted
https://codereview.chromium.org/1195563002/diff/110001/Source/modules/fetch/D... File Source/modules/fetch/DataConsumerTee.cpp (right): https://codereview.chromium.org/1195563002/diff/110001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTee.cpp:37: // destination contexts. Destination readers read data from its assocaited On 2015/06/19 06:40:35, hiroshige wrote: > nit: associated Done. https://codereview.chromium.org/1195563002/diff/110001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTee.cpp:89: void initialize(SourceContext* context) On 2015/06/19 06:40:35, hiroshige wrote: > optional: I prefer names like |sourceContext|, |destinationContext| etc. rather > than just |context|, because there are three kinds of contexts in this file. Done in this class. I think calling a DestinationContext as context in DestinationX classes is sane. https://codereview.chromium.org/1195563002/diff/110001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTee.cpp:113: void didDetach(Identifier identifier) On 2015/06/19 06:40:35, hiroshige wrote: > How about calling m_root->stop() in the dtor? > Then we can remove Identifier, m_mutex, m_isFirst|SecondActive, didDetach(). You're right, thanks! Done. https://codereview.chromium.org/1195563002/diff/110001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTee.cpp:207: detach(); On 2015/06/19 06:40:35, hiroshige wrote: > optional: I prefer to create a tracker object that is referenced by > DestinationHandler and DestinationReader and references DestinationContext, and > call DestinationContext::detach() on the tracker's dtor; liveness management > using |m_isHandleActive| (as well as |m_isFirstActive| and |m_isSecondActive|) > seems somehow prone to error. > However, this increases the numbers of classes and objects... Done. https://codereview.chromium.org/1195563002/diff/110001/Source/modules/fetch/D... File Source/modules/fetch/DataConsumerTee.h (right): https://codereview.chromium.org/1195563002/diff/110001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTee.h:20: static void create(ExecutionContext*, PassOwnPtr<WebDataConsumerHandle> in, OwnPtr<WebDataConsumerHandle>* out1, OwnPtr<WebDataConsumerHandle>* out2); On 2015/06/19 06:40:35, hiroshige wrote: > nit: Should we name the argument as |src|, |dest1/2| etc. to make the > declaration consistent with the definition and the comments in .cpp? Done.
https://codereview.chromium.org/1195563002/diff/150001/Source/modules/fetch/D... File Source/modules/fetch/DataConsumerTee.cpp (right): https://codereview.chromium.org/1195563002/diff/150001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTee.cpp:178: MutexLocker locker(m_mutex); Crashing here... #0 0x4ca2855 in WTF::MutexBase::lock() Source/wtf/ThreadingPthreads.cpp:136:5 #1 0x4d1e7dd in WTF::Locker<WTF::MutexBase>::Locker(WTF::MutexBase&) Source/wtf/Locker.h:38:50 #2 0x76aa799 in blink::(anonymous namespace)::DestinationContext::notify() Source/modules/fetch/DataConsumerTee.cpp:179:25 #3 0x76ac18a in WTF::FunctionWrapper<void (blink::(anonymous namespace)::DestinationContext::*)()>::operator()(blink::(anonymous namespace)::DestinationContext*)
https://codereview.chromium.org/1195563002/diff/150001/Source/modules/fetch/D... File Source/modules/fetch/DataConsumerTee.cpp (right): https://codereview.chromium.org/1195563002/diff/150001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTee.cpp:178: MutexLocker locker(m_mutex); On 2015/06/19 07:55:40, hiroshige wrote: > Crashing here... > > #0 0x4ca2855 in WTF::MutexBase::lock() Source/wtf/ThreadingPthreads.cpp:136:5 > #1 0x4d1e7dd in WTF::Locker<WTF::MutexBase>::Locker(WTF::MutexBase&) > Source/wtf/Locker.h:38:50 > #2 0x76aa799 in blink::(anonymous namespace)::DestinationContext::notify() > Source/modules/fetch/DataConsumerTee.cpp:179:25 > #3 0x76ac18a in WTF::FunctionWrapper<void (blink::(anonymous > namespace)::DestinationContext::*)()>::operator()(blink::(anonymous > namespace)::DestinationContext*) Fixed.
https://codereview.chromium.org/1195563002/diff/170001/Source/modules/fetch/D... File Source/modules/fetch/DataConsumerTee.cpp (right): https://codereview.chromium.org/1195563002/diff/170001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTee.cpp:194: // The following functions don't use lock. These functions are protected by |m_mutex| on the callers of the functions, right? So please mention it in the comment. https://codereview.chromium.org/1195563002/diff/170001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTee.cpp:214: ASSERT(m_offset + size <= top->size()); How about adding ASSERT(m_offset <= m_offset + size)? (e.g. |size| == (size_t)-1)
https://codereview.chromium.org/1195563002/diff/170001/Source/modules/fetch/D... File Source/modules/fetch/DataConsumerTee.cpp (right): https://codereview.chromium.org/1195563002/diff/170001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTee.cpp:194: // The following functions don't use lock. On 2015/06/23 06:19:49, hiroshige wrote: > These functions are protected by |m_mutex| on the callers of the functions, > right? So please mention it in the comment. Done. https://codereview.chromium.org/1195563002/diff/170001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTee.cpp:214: ASSERT(m_offset + size <= top->size()); On 2015/06/23 06:19:49, hiroshige wrote: > How about adding ASSERT(m_offset <= m_offset + size)? (e.g. |size| == > (size_t)-1) Done.
https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... File Source/modules/fetch/DataConsumerTeeTest.cpp (right): https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:62: class Handle final : public WebDataConsumerHandle { Please add a comment that says Handle is a WebDataConsumerHandle constructed from a sequence of Command by Handle:add(). https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:98: m_detachEvent->signal(); Can we move |m_detachEvent->signal()| to the dtor and remove |m_isHandleAttached|? (I think Context is destructed shortly after detachReader() if |!m_isHandleAttached|, because the reader with the last reference to |this| is destructed). https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:105: m_detachEvent->signal(); ditto, and can we remove detachHandle()? https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:119: bool fullyConsumed = (size + m_offset == top().body().size()); ">=" instead of "==" is a little more robust and consistent. https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:138: WebWaitableEvent* detachEvent() { return m_detachEvent.get(); } nit optional: detachEvent() seems "to detach an event". https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:179: MutexLocker locker(m_context->mutex()); I prefer taking lock in Context, because it is clearer for me who takes locks. https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:184: MutexLocker locker(m_context->mutex()); ditto. https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:194: memcpy(buffer, src, *readSize); We should take min(size, *readSize). https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:199: MutexLocker locker(m_context->mutex()); ditto, and can we move this implementation to Context and let ReaderImpl::beginRead() to call it? https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:322: m_waitableEvent = adoptPtr(Platform::current()->createWaitableEvent()); L322 sould be before L321, because didGetReadable() can be executed between L321 and L322? https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:345: // These should be accessed after the thread joines. nit: s/joines/joins/? https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:371: m_waitableEvent = adoptPtr(Platform::current()->createWaitableEvent()); ditto. https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:386: size_t readSize = std::max(size * 2 / 3, static_cast<size_t>(1)); BTW, can we assume beginRead()'s size > 0? https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:397: // These should be accessed after the thread joines. nit: joins.
https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... File Source/modules/fetch/DataConsumerTeeTest.cpp (right): https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:62: class Handle final : public WebDataConsumerHandle { On 2015/06/23 09:20:44, hiroshige wrote: > Please add a comment that says Handle is a WebDataConsumerHandle constructed > from a sequence of Command by Handle:add(). Done. https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:98: m_detachEvent->signal(); On 2015/06/23 09:20:44, hiroshige wrote: > Can we move |m_detachEvent->signal()| to the dtor and remove > |m_isHandleAttached|? (I think Context is destructed shortly after > detachReader() if |!m_isHandleAttached|, because the reader with the last > reference to |this| is destructed). That assumption does not hold in DetachBothDestinationsShouldStopSourceReader. https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:105: m_detachEvent->signal(); On 2015/06/23 09:20:44, hiroshige wrote: > ditto, and can we remove detachHandle()? Ditto https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:119: bool fullyConsumed = (size + m_offset == top().body().size()); On 2015/06/23 09:20:45, hiroshige wrote: > ">=" instead of "==" is a little more robust and consistent. Done. https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:138: WebWaitableEvent* detachEvent() { return m_detachEvent.get(); } On 2015/06/23 09:20:45, hiroshige wrote: > nit optional: detachEvent() seems "to detach an event". Done. https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:179: MutexLocker locker(m_context->mutex()); On 2015/06/23 09:20:44, hiroshige wrote: > I prefer taking lock in Context, because it is clearer for me who takes locks. Done. https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:184: MutexLocker locker(m_context->mutex()); On 2015/06/23 09:20:44, hiroshige wrote: > ditto. Done. https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:194: memcpy(buffer, src, *readSize); On 2015/06/23 09:20:44, hiroshige wrote: > We should take min(size, *readSize). Done. https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:199: MutexLocker locker(m_context->mutex()); On 2015/06/23 09:20:44, hiroshige wrote: > ditto, and can we move this implementation to Context and let > ReaderImpl::beginRead() to call it? Done. https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:322: m_waitableEvent = adoptPtr(Platform::current()->createWaitableEvent()); On 2015/06/23 09:20:44, hiroshige wrote: > L322 sould be before L321, because didGetReadable() can be executed between L321 > and L322? Done. https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:345: // These should be accessed after the thread joines. On 2015/06/23 09:20:44, hiroshige wrote: > nit: s/joines/joins/? Done. https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:371: m_waitableEvent = adoptPtr(Platform::current()->createWaitableEvent()); On 2015/06/23 09:20:44, hiroshige wrote: > ditto. Done. https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:386: size_t readSize = std::max(size * 2 / 3, static_cast<size_t>(1)); On 2015/06/23 09:20:45, hiroshige wrote: > BTW, can we assume beginRead()'s size > 0? No I think, calling read or beginRead with zero size may be useful to detect errors, although I didn't specified the behavior yet. https://codereview.chromium.org/1195563002/diff/190001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:397: // These should be accessed after the thread joines. On 2015/06/23 09:20:45, hiroshige wrote: > nit: joins. Done.
lgtm. I'm not familiar with Isolate and not sure about the isolate-related code in L292-305 in Source/modules/fetch/DataConsumerTeeTest.cpp though.
yhirano@chromium.org changed reviewers: + haraken@chromium.org
haraken, can you take a look at TestingThread defined in /DataConsumerTeeTest.cpp? It is a thread with v8::Isolate and ExecutionContext for unit tests.
https://codereview.chromium.org/1195563002/diff/210001/Source/modules/fetch/D... File Source/modules/fetch/DataConsumerTeeTest.cpp (right): https://codereview.chromium.org/1195563002/diff/210001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:292: m_isolate = v8::Isolate::New(v8::Isolate::CreateParams()); Maybe would it be better to create a ScriptState by calling ScriptState::create()? It will prepare a bunch of things to execute V8 things gracefully (using gin behind it). https://codereview.chromium.org/1195563002/diff/210001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:295: m_executionContext = adoptRefWillBeNoop(new NullExecutionContext()); Using NullExecutionContext is OK if you don't need a valid ExecutionContext in this test. https://codereview.chromium.org/1195563002/diff/210001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:661: Heap::collectGarbage(ThreadState::HeapPointersOnStack, ThreadState::GCWithSweep, Heap::ForcedGC); You can call Heap::collectAllGarbage(). ThreadState::HeapPointersOnStack can lead to a flaky result because of the conservative scanning. If you want to keep something from the stack, you can use a Persistent handle.
Thank you for the review. https://codereview.chromium.org/1195563002/diff/210001/Source/modules/fetch/D... File Source/modules/fetch/DataConsumerTeeTest.cpp (right): https://codereview.chromium.org/1195563002/diff/210001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:292: m_isolate = v8::Isolate::New(v8::Isolate::CreateParams()); On 2015/06/24 03:44:26, haraken wrote: > > Maybe would it be better to create a ScriptState by calling > ScriptState::create()? It will prepare a bunch of things to execute V8 things > gracefully (using gin behind it). Done. I added dependency to "gin/public" from "modules/fetch". https://codereview.chromium.org/1195563002/diff/210001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:295: m_executionContext = adoptRefWillBeNoop(new NullExecutionContext()); On 2015/06/24 03:44:26, haraken wrote: > > Using NullExecutionContext is OK if you don't need a valid ExecutionContext in > this test. Yeah, I only need to verify stop notification. https://codereview.chromium.org/1195563002/diff/210001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTeeTest.cpp:661: Heap::collectGarbage(ThreadState::HeapPointersOnStack, ThreadState::GCWithSweep, Heap::ForcedGC); On 2015/06/24 03:44:26, haraken wrote: > > You can call Heap::collectAllGarbage(). ThreadState::HeapPointersOnStack can > lead to a flaky result because of the conservative scanning. If you want to keep > something from the stack, you can use a Persistent handle. > Done.
The TestingThread 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/1195563002/#ps230001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1195563002/230001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/68061)
The CQ bit was checked by hiroshige@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1195563002/230001
Message was sent while issue was closed.
Committed patchset #9 (id:230001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197733
Message was sent while issue was closed.
jochen@chromium.org changed reviewers: + jochen@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1195563002/diff/230001/Source/modules/fetch/D... File Source/modules/fetch/DataConsumerTee.cpp (right): https://codereview.chromium.org/1195563002/diff/230001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTee.cpp:265: MutexLocker(context()->mutex()); this actually doesn't do what you think it does (missing variable name) https://codereview.chromium.org/1195563002/diff/230001/Source/modules/fetch/D... Source/modules/fetch/DataConsumerTee.cpp:276: MutexLocker(context()->mutex()); same here |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
