Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2177283005:
Add WTF::makeCancellable (Closed)
DescriptionAdd WTF::makeCancellable
Current Blink code has several tricky code for Function cancellation,
such as ImageLoader::Task, WorkerEventQueue::EventDispatcherTask,
CancellableTaskFactory, and 0-delay oneshot timers.
They essentially make code dups, and adds complexity to the code base.
This CL adds WTF::makeCancellable() to unify them.
Committed: https://crrev.com/e5b8b24c7412a07913e4fd0648d71624d09e6c55
Cr-Commit-Position: refs/heads/master@{#409457}
Patch Set 1 : update #Patch Set 2 : s/run/runUnlessCancelled/ #Patch Set 3 : update #Patch Set 4 : +test, +DCHECK #
Total comments: 4
Patch Set 5 : +example #
Total comments: 7
Patch Set 6 : s/unless/if/ #Patch Set 7 : s/FunctionCanceller/ScopedFunctionCanceller/, s/FunctionCancellerImplBase/FunctionCanceller/ #Patch Set 8 : comment fix #
Total comments: 6
Patch Set 9 : +self-assign fix, +test, +comment and examples #
Total comments: 10
Patch Set 10 : update #Patch Set 11 : +MakeCancellableResult #Patch Set 12 : update comment #Patch Set 13 : return ScopedFunctionCanceller. +operator bool #Patch Set 14 : update #Patch Set 15 : +comment #
Messages
Total messages: 102 (74 generated)
The CQ bit was checked by tzik@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 tzik@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 tzik@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...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by tzik@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 ========== Add WTF::makeCancellable BUG= ========== to ========== Add WTF::makeCancellable ==========
tzik@chromium.org changed reviewers: + hiroshige@chromium.org, yutak@chromium.org
Description was changed from ========== Add WTF::makeCancellable ========== to ========== Add WTF::makeCancellable To be discussed: - Should we clear the wrapped function immediately after cancel() call? - Should this class be thread-safe? ==========
tzik@chromium.org changed reviewers: + haraken@chromium.org
Description was changed from ========== Add WTF::makeCancellable To be discussed: - Should we clear the wrapped function immediately after cancel() call? - Should this class be thread-safe? ========== to ========== Add WTF::makeCancellable To be discussed: - Should we clear the wrapped function immediately after cancel() call? * WorkerEventQueue::EventDispatcherTask does. - Should this class be thread-safe? ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add WTF::makeCancellable To be discussed: - Should we clear the wrapped function immediately after cancel() call? * WorkerEventQueue::EventDispatcherTask does. - Should this class be thread-safe? ========== to ========== Add WTF::makeCancellable To be discussed: - Should we clear the wrapped function immediately after cancel() call? * WorkerEventQueue::EventDispatcherTask does. - Should this class be thread-safe? - Should we support repeating Function (i.e. can be called more than once)? ==========
Description was changed from ========== Add WTF::makeCancellable To be discussed: - Should we clear the wrapped function immediately after cancel() call? * WorkerEventQueue::EventDispatcherTask does. - Should this class be thread-safe? - Should we support repeating Function (i.e. can be called more than once)? ========== to ========== Add WTF::makeCancellable To be discussed: - Should we clear the wrapped function immediately after cancel() call? * WorkerEventQueue::EventDispatcherTask does. - Should this class be thread-safe? * Probably, no. - Should we support repeating Function (i.e. can be called more than once)? * There seems no use case. ==========
Description was changed from ========== Add WTF::makeCancellable To be discussed: - Should we clear the wrapped function immediately after cancel() call? * WorkerEventQueue::EventDispatcherTask does. - Should this class be thread-safe? * Probably, no. - Should we support repeating Function (i.e. can be called more than once)? * There seems no use case. ========== to ========== Add WTF::makeCancellable To be discussed: - Should we clear the wrapped function immediately after cancel() call? * WorkerEventQueue::EventDispatcherTask does. But this makes things complex. - Should this class be thread-safe? * Probably, no. - Should we support repeating Function (i.e. can be called more than once)? * There seems no use case. ==========
The CQ bit was checked by tzik@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 tzik@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 ========== Add WTF::makeCancellable To be discussed: - Should we clear the wrapped function immediately after cancel() call? * WorkerEventQueue::EventDispatcherTask does. But this makes things complex. - Should this class be thread-safe? * Probably, no. - Should we support repeating Function (i.e. can be called more than once)? * There seems no use case. ========== to ========== Add WTF::makeCancellable ==========
Description was changed from ========== Add WTF::makeCancellable ========== to ========== Add WTF::makeCancellable Current Blink code has several tricky code for Function cancellation, such as ImageLoader::Task, WorkerEventQueue::EventDispatcherTask, CancellableTaskFactory, and 0-delay oneshot timers. They essentially make code dups, and adds complexity to the code base. This CL adds WTF::makeCancellable() to unify them. ==========
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Makes sense overall. https://codereview.chromium.org/2177283005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/MakeCancellable.cpp (right): https://codereview.chromium.org/2177283005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.cpp:29: cancel(); This might be a bit surprising (task getting cancelled without explicit cancel() call). Do you have any concrete use case in mind? At least we should make an emphasis on this cancel-on- destruction semantics in the comments for makeCancellable(). https://codereview.chromium.org/2177283005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/MakeCancellable.h (right): https://codereview.chromium.org/2177283005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:41: void runUnlessCancelled(const FunctionCanceller&, Params... params) Is the first argument necessary?
https://codereview.chromium.org/2177283005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/MakeCancellable.cpp (right): https://codereview.chromium.org/2177283005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.cpp:29: cancel(); On 2016/07/27 09:08:02, Yuta Kitamura wrote: > This might be a bit surprising (task getting cancelled without > explicit cancel() call). Do you have any concrete use case > in mind? > > At least we should make an emphasis on this cancel-on- > destruction semantics in the comments for makeCancellable(). Added a comment and example there. Yes, this will replace several cancellation classes listed in the CL description. And all of them have the cancel-on-destruction semantics. https://codereview.chromium.org/2177283005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/MakeCancellable.h (right): https://codereview.chromium.org/2177283005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:41: void runUnlessCancelled(const FunctionCanceller&, Params... params) On 2016/07/27 09:08:02, Yuta Kitamura wrote: > Is the first argument necessary? Yes, it's needed to ensure the FunctionCancellerImpl is cleared when the wrapped Function is destroyed.
The CQ bit was checked by tzik@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/2177283005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/MakeCancellable.h (right): https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:20: class WTF_EXPORT FunctionCancellerImplBase : public RefCounted<FunctionCancellerImplBase> { Do we need WTF_EXPORT? It looks strange to export a class inside the internal namespace. https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:83: // unless the FunctionCanceller is alive and its cancel() is not called. unless => if ? https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:91: // std::tie(wrappedFunction, canceller) = makeCancellable(std::move(function)); Instead of returning a pair of WTF::Function and FunctionCanceller, would it be an option to return WTF::CancellableFunction? WTF::CancellableFunction should provide cancel(). I'm not sure if it's a good idea to decouple FunctionCanceller from the function itself because people need to be careful about the lifetime of FunctionCanceller and the function together. Otherwise, FunctionCanceller may be unintentionally destructed before the function, which may unintentionally prevent the function from running.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/MakeCancellable.h (right): https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:20: class WTF_EXPORT FunctionCancellerImplBase : public RefCounted<FunctionCancellerImplBase> { On 2016/07/27 11:18:04, haraken wrote: > > Do we need WTF_EXPORT? It looks strange to export a class inside the internal > namespace. Yes, we need it to expose the ctor and dtor to FunctionCancellerImpl and makeCancellable. Since they are a template function and a class template, ctor and dtor invocation needs to be visible from the user of these function where the template instantiation happens. https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:83: // unless the FunctionCanceller is alive and its cancel() is not called. On 2016/07/27 11:18:04, haraken wrote: > > unless => if ? Done. https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:91: // std::tie(wrappedFunction, canceller) = makeCancellable(std::move(function)); On 2016/07/27 11:18:04, haraken wrote: > > Instead of returning a pair of WTF::Function and FunctionCanceller, would it be > an option to return WTF::CancellableFunction? WTF::CancellableFunction should > provide cancel(). > > I'm not sure if it's a good idea to decouple FunctionCanceller from the function > itself because people need to be careful about the lifetime of FunctionCanceller > and the function together. Otherwise, FunctionCanceller may be unintentionally > destructed before the function, which may unintentionally prevent the function > from running. Hmm, since the Function will be posted to a message loop as a unique_ptr, and its ownership is passed, we'll have to lose one of the object. If we have CancellableFunction that the user keeps for cancellation, we need another mechanism to get a Function, which will be posted to a message loop. I think it'll not be simpler than my proposal.
https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/MakeCancellable.h (right): https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:91: // std::tie(wrappedFunction, canceller) = makeCancellable(std::move(function)); On 2016/07/27 13:56:53, tzik wrote: > On 2016/07/27 11:18:04, haraken wrote: > > > > Instead of returning a pair of WTF::Function and FunctionCanceller, would it > be > > an option to return WTF::CancellableFunction? WTF::CancellableFunction should > > provide cancel(). > > > > I'm not sure if it's a good idea to decouple FunctionCanceller from the > function > > itself because people need to be careful about the lifetime of > FunctionCanceller > > and the function together. Otherwise, FunctionCanceller may be unintentionally > > destructed before the function, which may unintentionally prevent the function > > from running. > > Hmm, since the Function will be posted to a message loop as a unique_ptr, and > its ownership is passed, we'll have to lose one of the object. > If we have CancellableFunction that the user keeps for cancellation, we need > another mechanism to get a Function, which will be posted to a message loop. > I think it'll not be simpler than my proposal. Do you mean that the following pattern is complicated? std::unique_ptr<Function<void()>> function = WTF::bind(&foo); CancellableFunction cancellable = makeCancellable(std::move(function)); ...; postTask(cancellable.function()); ...; cancellable.cancel();
On 2016/07/27 14:05:46, haraken wrote: > https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/wtf/MakeCancellable.h (right): > > https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/wtf/MakeCancellable.h:91: // > std::tie(wrappedFunction, canceller) = makeCancellable(std::move(function)); > On 2016/07/27 13:56:53, tzik wrote: > > On 2016/07/27 11:18:04, haraken wrote: > > > > > > Instead of returning a pair of WTF::Function and FunctionCanceller, would it > > be > > > an option to return WTF::CancellableFunction? WTF::CancellableFunction > should > > > provide cancel(). > > > > > > I'm not sure if it's a good idea to decouple FunctionCanceller from the > > function > > > itself because people need to be careful about the lifetime of > > FunctionCanceller > > > and the function together. Otherwise, FunctionCanceller may be > unintentionally > > > destructed before the function, which may unintentionally prevent the > function > > > from running. > > > > Hmm, since the Function will be posted to a message loop as a unique_ptr, and > > its ownership is passed, we'll have to lose one of the object. > > If we have CancellableFunction that the user keeps for cancellation, we need > > another mechanism to get a Function, which will be posted to a message loop. > > I think it'll not be simpler than my proposal. > > Do you mean that the following pattern is complicated? > > std::unique_ptr<Function<void()>> function = WTF::bind(&foo); > CancellableFunction cancellable = makeCancellable(std::move(function)); > ...; > postTask(cancellable.function()); > ...; > cancellable.cancel(); It looks not simpler than mine. Since the signature of function() does not prevent the user to call it more than once, it's not clear that when the original function is destroyed in this case. std::unique_ptr<std::Function<void()>> f = cancellable.function(); std::unique_ptr<std::Function<void()>> g = cancellable.function(); // Should this be valid? f = nullptr; // Should this destroy the original Function?
On 2016/07/27 14:32:34, tzik wrote: > On 2016/07/27 14:05:46, haraken wrote: > > > https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/wtf/MakeCancellable.h (right): > > > > > https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... > > third_party/WebKit/Source/wtf/MakeCancellable.h:91: // > > std::tie(wrappedFunction, canceller) = makeCancellable(std::move(function)); > > On 2016/07/27 13:56:53, tzik wrote: > > > On 2016/07/27 11:18:04, haraken wrote: > > > > > > > > Instead of returning a pair of WTF::Function and FunctionCanceller, would > it > > > be > > > > an option to return WTF::CancellableFunction? WTF::CancellableFunction > > should > > > > provide cancel(). > > > > > > > > I'm not sure if it's a good idea to decouple FunctionCanceller from the > > > function > > > > itself because people need to be careful about the lifetime of > > > FunctionCanceller > > > > and the function together. Otherwise, FunctionCanceller may be > > unintentionally > > > > destructed before the function, which may unintentionally prevent the > > function > > > > from running. > > > > > > Hmm, since the Function will be posted to a message loop as a unique_ptr, > and > > > its ownership is passed, we'll have to lose one of the object. > > > If we have CancellableFunction that the user keeps for cancellation, we need > > > another mechanism to get a Function, which will be posted to a message loop. > > > I think it'll not be simpler than my proposal. > > > > Do you mean that the following pattern is complicated? > > > > std::unique_ptr<Function<void()>> function = WTF::bind(&foo); > > CancellableFunction cancellable = makeCancellable(std::move(function)); > > ...; > > postTask(cancellable.function()); > > ...; > > cancellable.cancel(); > > It looks not simpler than mine. > Since the signature of function() does not prevent the user to call it more than > once, it's not clear that when the original function is destroyed in this case. > std::unique_ptr<std::Function<void()>> f = cancellable.function(); > std::unique_ptr<std::Function<void()>> g = cancellable.function(); // Should > this be valid? > f = nullptr; // Should this destroy the original Function? Thanks, I'm getting to understand what you want. Yeah, your approach looks better. My concern is that FunctionCanceller is destroyed earlier than developers expect and cancels functions that should run. Would it be an option to stop calling cancel() in FunctionCanceller's destructor (even if the existing use cases are expecting the behavior)?
The CQ bit was checked by tzik@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 tzik@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 tzik@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...
Patchset #8 (id:180001) has been deleted
On 2016/07/27 15:13:17, haraken wrote: > On 2016/07/27 14:32:34, tzik wrote: > > On 2016/07/27 14:05:46, haraken wrote: > > > > > > https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/wtf/MakeCancellable.h (right): > > > > > > > > > https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/wtf/MakeCancellable.h:91: // > > > std::tie(wrappedFunction, canceller) = makeCancellable(std::move(function)); > > > On 2016/07/27 13:56:53, tzik wrote: > > > > On 2016/07/27 11:18:04, haraken wrote: > > > > > > > > > > Instead of returning a pair of WTF::Function and FunctionCanceller, > would > > it > > > > be > > > > > an option to return WTF::CancellableFunction? WTF::CancellableFunction > > > should > > > > > provide cancel(). > > > > > > > > > > I'm not sure if it's a good idea to decouple FunctionCanceller from the > > > > function > > > > > itself because people need to be careful about the lifetime of > > > > FunctionCanceller > > > > > and the function together. Otherwise, FunctionCanceller may be > > > unintentionally > > > > > destructed before the function, which may unintentionally prevent the > > > function > > > > > from running. > > > > > > > > Hmm, since the Function will be posted to a message loop as a unique_ptr, > > and > > > > its ownership is passed, we'll have to lose one of the object. > > > > If we have CancellableFunction that the user keeps for cancellation, we > need > > > > another mechanism to get a Function, which will be posted to a message > loop. > > > > I think it'll not be simpler than my proposal. > > > > > > Do you mean that the following pattern is complicated? > > > > > > std::unique_ptr<Function<void()>> function = WTF::bind(&foo); > > > CancellableFunction cancellable = makeCancellable(std::move(function)); > > > ...; > > > postTask(cancellable.function()); > > > ...; > > > cancellable.cancel(); > > > > It looks not simpler than mine. > > Since the signature of function() does not prevent the user to call it more > than > > once, it's not clear that when the original function is destroyed in this > case. > > std::unique_ptr<std::Function<void()>> f = cancellable.function(); > > std::unique_ptr<std::Function<void()>> g = cancellable.function(); // Should > > this be valid? > > f = nullptr; // Should this destroy the original Function? > > Thanks, I'm getting to understand what you want. > > Yeah, your approach looks better. My concern is that FunctionCanceller is > destroyed earlier than developers expect and cancels functions that should run. > Would it be an option to stop calling cancel() in FunctionCanceller's destructor > (even if the existing use cases are expecting the behavior)? Ok, updated the CL to stop cancelling it by default, and added ScopedFunctionCanceller to make it opt-in.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2177283005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/MakeCancellable.h (right): https://codereview.chromium.org/2177283005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:103: std::unique_ptr<Function<void(Params...)>> wrappedFunction = bind(&Canceller::runUnlessCancelled, canceller, ScopedFunctionCanceller(canceller)); I'm still unsure about the merit of cancelling on the destruction of a wrapped function. I think my doubt boils down to the following question: is cancelling observable from the user? After the wrapped function is destructed, the user can no longer call the function, thus the user won't be able to observe the effect of cancel anyway. Is there anything I have missed?
https://codereview.chromium.org/2177283005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/MakeCancellable.h (right): https://codereview.chromium.org/2177283005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:103: std::unique_ptr<Function<void(Params...)>> wrappedFunction = bind(&Canceller::runUnlessCancelled, canceller, ScopedFunctionCanceller(canceller)); On 2016/07/29 00:57:42, Yuta Kitamura wrote: > I'm still unsure about the merit of cancelling on the > destruction of a wrapped function. > > I think my doubt boils down to the following question: > is cancelling observable from the user? After the > wrapped function is destructed, the user can no longer > call the function, thus the user won't be able to > observe the effect of cancel anyway. > > Is there anything I have missed? My concern is that, without the auto destruction, makeCancellable unnecessarily extends the lifetime of the inner function. That may even make a circular reference in an example below. And it's also for the behavior parity to WorkerEventQueue::EventDispatcherTask and others, which we are going to replace with makeCancellable. struct Foo : GarbageCollectedFinalized<Foo> { FunctionCanceller m_canceller; void bar(); }; Foo* foo = new Foo(); Closure f = bind(&Foo::bar, wrapPersistent(foo)); Closure g; std::tie(g, foo->m_canceller) = makeCancellable(std::move(f)); g = nullptr; On the example above, if we don't destroy |f| on the destruction of |g|, they form a circular strong reference of foo -> m_canceller -> m_function -> foo, and that causes a memory leak.
OK, LGTM w/ nits. https://codereview.chromium.org/2177283005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/MakeCancellable.h (right): https://codereview.chromium.org/2177283005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:28: class WTF_EXPORT ScopedFunctionCanceller { A brief comment about the usage of this class would be nice. The semantics of detach and cancel might not be too obvious from its name. When I first saw cancel(), I wasn't sure whether cancel() cancels the function or cancels the functionality of the canceller object. A small example would definitely help resolving such a misunderstanding. https://codereview.chromium.org/2177283005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:103: std::unique_ptr<Function<void(Params...)>> wrappedFunction = bind(&Canceller::runUnlessCancelled, canceller, ScopedFunctionCanceller(canceller)); On 2016/07/29 05:25:40, tzik wrote: > On 2016/07/29 00:57:42, Yuta Kitamura wrote: > > I'm still unsure about the merit of cancelling on the > > destruction of a wrapped function. > > > > I think my doubt boils down to the following question: > > is cancelling observable from the user? After the > > wrapped function is destructed, the user can no longer > > call the function, thus the user won't be able to > > observe the effect of cancel anyway. > > > > Is there anything I have missed? > > My concern is that, without the auto destruction, makeCancellable unnecessarily > extends the lifetime of the inner function. That may even make a circular > reference in an example below. And it's also for the behavior parity to > WorkerEventQueue::EventDispatcherTask and others, which we are going to replace > with makeCancellable. > > struct Foo : GarbageCollectedFinalized<Foo> { > FunctionCanceller m_canceller; > void bar(); > }; > > Foo* foo = new Foo(); > Closure f = bind(&Foo::bar, wrapPersistent(foo)); > Closure g; > std::tie(g, foo->m_canceller) = makeCancellable(std::move(f)); > g = nullptr; > > On the example above, if we don't destroy |f| on the destruction of |g|, they > form a circular strong reference of > foo -> m_canceller -> m_function -> foo, and that causes a memory leak. OK, that makes sense. Maybe you could add some more comments for future readers? The current comment is not clear about the intention of forced cancelling.
On 2016/07/29 05:25:41, tzik wrote: > https://codereview.chromium.org/2177283005/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/wtf/MakeCancellable.h (right): > > https://codereview.chromium.org/2177283005/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/wtf/MakeCancellable.h:103: > std::unique_ptr<Function<void(Params...)>> wrappedFunction = > bind(&Canceller::runUnlessCancelled, canceller, > ScopedFunctionCanceller(canceller)); > On 2016/07/29 00:57:42, Yuta Kitamura wrote: > > I'm still unsure about the merit of cancelling on the > > destruction of a wrapped function. > > > > I think my doubt boils down to the following question: > > is cancelling observable from the user? After the > > wrapped function is destructed, the user can no longer > > call the function, thus the user won't be able to > > observe the effect of cancel anyway. > > > > Is there anything I have missed? > > My concern is that, without the auto destruction, makeCancellable unnecessarily > extends the lifetime of the inner function. That may even make a circular > reference in an example below. And it's also for the behavior parity to > WorkerEventQueue::EventDispatcherTask and others, which we are going to replace > with makeCancellable. > > struct Foo : GarbageCollectedFinalized<Foo> { > FunctionCanceller m_canceller; > void bar(); > }; > > Foo* foo = new Foo(); > Closure f = bind(&Foo::bar, wrapPersistent(foo)); > Closure g; > std::tie(g, foo->m_canceller) = makeCancellable(std::move(f)); > g = nullptr; > > On the example above, if we don't destroy |f| on the destruction of |g|, they > form a circular strong reference of > foo -> m_canceller -> m_function -> foo, and that causes a memory leak. Hmm, but who guarantees that the canceller object is destroyed in a finite time period? With this programming model, (regardless of whether FunctionCanceller implicitly cancels the function or not, ) developers anyway need to be careful about the lifetime of both the function object and the canceller object. That said, I understand that the idea of cancellableFunction.function() isn't nice either, so I'm okay with this CL.
On 2016/07/29 08:06:36, haraken wrote: > On 2016/07/29 05:25:41, tzik wrote: > > > https://codereview.chromium.org/2177283005/diff/200001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/wtf/MakeCancellable.h (right): > > > > > https://codereview.chromium.org/2177283005/diff/200001/third_party/WebKit/Sou... > > third_party/WebKit/Source/wtf/MakeCancellable.h:103: > > std::unique_ptr<Function<void(Params...)>> wrappedFunction = > > bind(&Canceller::runUnlessCancelled, canceller, > > ScopedFunctionCanceller(canceller)); > > On 2016/07/29 00:57:42, Yuta Kitamura wrote: > > > I'm still unsure about the merit of cancelling on the > > > destruction of a wrapped function. > > > > > > I think my doubt boils down to the following question: > > > is cancelling observable from the user? After the > > > wrapped function is destructed, the user can no longer > > > call the function, thus the user won't be able to > > > observe the effect of cancel anyway. > > > > > > Is there anything I have missed? > > > > My concern is that, without the auto destruction, makeCancellable > unnecessarily > > extends the lifetime of the inner function. That may even make a circular > > reference in an example below. And it's also for the behavior parity to > > WorkerEventQueue::EventDispatcherTask and others, which we are going to > replace > > with makeCancellable. > > > > struct Foo : GarbageCollectedFinalized<Foo> { > > FunctionCanceller m_canceller; > > void bar(); > > }; > > > > Foo* foo = new Foo(); > > Closure f = bind(&Foo::bar, wrapPersistent(foo)); > > Closure g; > > std::tie(g, foo->m_canceller) = makeCancellable(std::move(f)); > > g = nullptr; > > > > On the example above, if we don't destroy |f| on the destruction of |g|, they > > form a circular strong reference of > > foo -> m_canceller -> m_function -> foo, and that causes a memory leak. > > Hmm, but who guarantees that the canceller object is destroyed in a finite time > period? > > With this programming model, (regardless of whether FunctionCanceller implicitly > cancels the function or not, ) developers anyway need to be careful about the > lifetime of both the function object and the canceller object. > > That said, I understand that the idea of cancellableFunction.function() isn't > nice either, so I'm okay with this CL. Maybe would something like this work? std::unique_ptr<Function<void()>> function = WTF::bind(&foo); CancellableFunction cancellableFunction = makeCancellable(std::move(function)); cancellableFunction.call(); // This calls the function. cancellableFunction.call(); // We can call the function multiple times. ... = cancellableFunction.function(); // This releases the ownership. cancellableFunction.call(); // Nothing happens.
The CQ bit was checked by tzik@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...
On 2016/07/29 08:06:36, haraken wrote: > On 2016/07/29 05:25:41, tzik wrote: > > > https://codereview.chromium.org/2177283005/diff/200001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/wtf/MakeCancellable.h (right): > > > > > https://codereview.chromium.org/2177283005/diff/200001/third_party/WebKit/Sou... > > third_party/WebKit/Source/wtf/MakeCancellable.h:103: > > std::unique_ptr<Function<void(Params...)>> wrappedFunction = > > bind(&Canceller::runUnlessCancelled, canceller, > > ScopedFunctionCanceller(canceller)); > > On 2016/07/29 00:57:42, Yuta Kitamura wrote: > > > I'm still unsure about the merit of cancelling on the > > > destruction of a wrapped function. > > > > > > I think my doubt boils down to the following question: > > > is cancelling observable from the user? After the > > > wrapped function is destructed, the user can no longer > > > call the function, thus the user won't be able to > > > observe the effect of cancel anyway. > > > > > > Is there anything I have missed? > > > > My concern is that, without the auto destruction, makeCancellable > unnecessarily > > extends the lifetime of the inner function. That may even make a circular > > reference in an example below. And it's also for the behavior parity to > > WorkerEventQueue::EventDispatcherTask and others, which we are going to > replace > > with makeCancellable. > > > > struct Foo : GarbageCollectedFinalized<Foo> { > > FunctionCanceller m_canceller; > > void bar(); > > }; > > > > Foo* foo = new Foo(); > > Closure f = bind(&Foo::bar, wrapPersistent(foo)); > > Closure g; > > std::tie(g, foo->m_canceller) = makeCancellable(std::move(f)); > > g = nullptr; > > > > On the example above, if we don't destroy |f| on the destruction of |g|, they > > form a circular strong reference of > > foo -> m_canceller -> m_function -> foo, and that causes a memory leak. > > Hmm, but who guarantees that the canceller object is destroyed in a finite time > period? > > With this programming model, (regardless of whether FunctionCanceller implicitly > cancels the function or not, ) developers anyway need to be careful about the > lifetime of both the function object and the canceller object. > > That said, I understand that the idea of cancellableFunction.function() isn't > nice either, so I'm okay with this CL. If we automatically cancel the invocation of |f| via the canceller (as this CL does), |g|-destruction resets the reference from |m_canceller| to |m_function|. So, there no longer exists the circular reference. Though developers need to care about the lifetime of function anyway, the lifetime of the canceller should not matter, since it does not extend the lifetime of other objects.
https://codereview.chromium.org/2177283005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/MakeCancellable.h (right): https://codereview.chromium.org/2177283005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:28: class WTF_EXPORT ScopedFunctionCanceller { On 2016/07/29 05:58:28, Yuta Kitamura wrote: > A brief comment about the usage of this class would > be nice. The semantics of detach and cancel might not > be too obvious from its name. > > When I first saw cancel(), I wasn't sure whether > cancel() cancels the function or cancels the > functionality of the canceller object. A small example > would definitely help resolving such a misunderstanding. Done. https://codereview.chromium.org/2177283005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:103: std::unique_ptr<Function<void(Params...)>> wrappedFunction = bind(&Canceller::runUnlessCancelled, canceller, ScopedFunctionCanceller(canceller)); On 2016/07/29 05:58:28, Yuta Kitamura wrote: > On 2016/07/29 05:25:40, tzik wrote: > > On 2016/07/29 00:57:42, Yuta Kitamura wrote: > > > I'm still unsure about the merit of cancelling on the > > > destruction of a wrapped function. > > > > > > I think my doubt boils down to the following question: > > > is cancelling observable from the user? After the > > > wrapped function is destructed, the user can no longer > > > call the function, thus the user won't be able to > > > observe the effect of cancel anyway. > > > > > > Is there anything I have missed? > > > > My concern is that, without the auto destruction, makeCancellable > unnecessarily > > extends the lifetime of the inner function. That may even make a circular > > reference in an example below. And it's also for the behavior parity to > > WorkerEventQueue::EventDispatcherTask and others, which we are going to > replace > > with makeCancellable. > > > > struct Foo : GarbageCollectedFinalized<Foo> { > > FunctionCanceller m_canceller; > > void bar(); > > }; > > > > Foo* foo = new Foo(); > > Closure f = bind(&Foo::bar, wrapPersistent(foo)); > > Closure g; > > std::tie(g, foo->m_canceller) = makeCancellable(std::move(f)); > > g = nullptr; > > > > On the example above, if we don't destroy |f| on the destruction of |g|, they > > form a circular strong reference of > > foo -> m_canceller -> m_function -> foo, and that causes a memory leak. > > OK, that makes sense. > > Maybe you could add some more comments for future > readers? The current comment is not clear about the > intention of forced cancelling. Done.
https://codereview.chromium.org/2177283005/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/MakeCancellable.cpp (right): https://codereview.chromium.org/2177283005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.cpp:7: #include "base/logging.h" Do you need this? https://codereview.chromium.org/2177283005/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/MakeCancellable.h (right): https://codereview.chromium.org/2177283005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:29: // function on its destruction. Are you assuming that normal developers use both FunctionCanceller and ScopedFunctionCanceller (which seems a bit too complex to me)? Or is FunctionCanceller the only customer of ScopedFunctionCanceller? https://codereview.chromium.org/2177283005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:35: // std::tie(g, canceller) = makeCancellable(std::move(f)); Nit: I'd prefer passing the canceller parameter as a reference. std::unique_ptr<Closure> g = makeCancellable(std::move(f), canceller); https://codereview.chromium.org/2177283005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:54: class WTF_EXPORT ScopedFunctionCanceller { Can this be DISALLOW_ALLOCATION?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by tzik@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by tzik@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/2177283005/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/MakeCancellable.cpp (right): https://codereview.chromium.org/2177283005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.cpp:7: #include "base/logging.h" On 2016/07/29 14:02:52, haraken wrote: > > Do you need this? Oh, no. It's no longer needed. https://codereview.chromium.org/2177283005/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/MakeCancellable.h (right): https://codereview.chromium.org/2177283005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:29: // function on its destruction. On 2016/07/29 14:02:52, haraken wrote: > > Are you assuming that normal developers use both FunctionCanceller and > ScopedFunctionCanceller (which seems a bit too complex to me)? Or is > FunctionCanceller the only customer of ScopedFunctionCanceller? > Since all existing implementations of custom cancellation use cancel-on-delete, I expect their replacement should be ScopedFunctionCanceller rather than raw FunctionCanceller. https://codereview.chromium.org/2177283005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:35: // std::tie(g, canceller) = makeCancellable(std::move(f)); On 2016/07/29 14:02:52, haraken wrote: > > Nit: I'd prefer passing the canceller parameter as a reference. > > std::unique_ptr<Closure> g = makeCancellable(std::move(f), canceller); Updated. I want to avoid using the output parameters at least on new code... Let me pass it to the cxx list. https://codereview.chromium.org/2177283005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:54: class WTF_EXPORT ScopedFunctionCanceller { On 2016/07/29 14:02:52, haraken wrote: > > Can this be DISALLOW_ALLOCATION? Done.
https://codereview.chromium.org/2177283005/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/MakeCancellable.h (right): https://codereview.chromium.org/2177283005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:29: // function on its destruction. On 2016/07/29 16:49:13, tzik wrote: > On 2016/07/29 14:02:52, haraken wrote: > > > > Are you assuming that normal developers use both FunctionCanceller and > > ScopedFunctionCanceller (which seems a bit too complex to me)? Or is > > FunctionCanceller the only customer of ScopedFunctionCanceller? > > > > Since all existing implementations of custom cancellation use cancel-on-delete, > I expect their replacement should be ScopedFunctionCanceller rather than raw > FunctionCanceller. I'm a bit confused. Then what's the point of splitting ScopedFunctionCanceller from FunctionCanceller? Can we just make FunctionCanceller call cancel() when it gets destructed? Sorry for being nit-picky on this. But I'd like to simplify the interface as much as possible since the semantics of makeCancellable is already a lot complicated (as we've been discussing in this CL).
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 tzik@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/2177283005/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/MakeCancellable.h (right): https://codereview.chromium.org/2177283005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/MakeCancellable.h:29: // function on its destruction. On 2016/07/29 19:18:39, haraken wrote: > On 2016/07/29 16:49:13, tzik wrote: > > On 2016/07/29 14:02:52, haraken wrote: > > > > > > Are you assuming that normal developers use both FunctionCanceller and > > > ScopedFunctionCanceller (which seems a bit too complex to me)? Or is > > > FunctionCanceller the only customer of ScopedFunctionCanceller? > > > > > > > Since all existing implementations of custom cancellation use > cancel-on-delete, > > I expect their replacement should be ScopedFunctionCanceller rather than raw > > FunctionCanceller. > > I'm a bit confused. Then what's the point of splitting ScopedFunctionCanceller > from FunctionCanceller? Can we just make FunctionCanceller call cancel() when it > gets destructed? That's to make the auto cancel opt-in per comments at https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... and https://codereview.chromium.org/2177283005/diff/100001/third_party/WebKit/Sou... Since FunctionCanceller outlives the outer Function, it's not useful to call cancel() in its destructor. > > Sorry for being nit-picky on this. But I'd like to simplify the interface as > much as possible since the semantics of makeCancellable is already a lot > complicated (as we've been discussing in this CL). > Is it that complicated for users? Do you have any concern?
On 2016/07/29 21:10:36, tzik wrote: > https://codereview.chromium.org/2177283005/diff/220001/third_party/WebKit/Sou... > File third_party/WebKit/Source/wtf/MakeCancellable.h (right): > > https://codereview.chromium.org/2177283005/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/wtf/MakeCancellable.h:29: // function on its > destruction. > On 2016/07/29 19:18:39, haraken wrote: > > On 2016/07/29 16:49:13, tzik wrote: > > > On 2016/07/29 14:02:52, haraken wrote: > > > > > > > > Are you assuming that normal developers use both FunctionCanceller and > > > > ScopedFunctionCanceller (which seems a bit too complex to me)? Or is > > > > FunctionCanceller the only customer of ScopedFunctionCanceller? > > > > > > > > > > Since all existing implementations of custom cancellation use > > cancel-on-delete, > > > I expect their replacement should be ScopedFunctionCanceller rather than raw > > > FunctionCanceller. > > > > I'm a bit confused. Then what's the point of splitting ScopedFunctionCanceller > > from FunctionCanceller? Can we just make FunctionCanceller call cancel() when > it > > gets destructed? > > That's to make the auto cancel opt-in per comments at > https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... > and > https://codereview.chromium.org/2177283005/diff/100001/third_party/WebKit/Sou... > > Since FunctionCanceller outlives the outer Function, it's not useful to call > cancel() in its destructor. > > > > > Sorry for being nit-picky on this. But I'd like to simplify the interface as > > much as possible since the semantics of makeCancellable is already a lot > > complicated (as we've been discussing in this CL). > > > > Is it that complicated for users? Do you have any concern? Now developers need to take care of four objects: Original function, cancellable function, FunctionCanceller, ScopedFunctionCanceller. Can we encapsulate the FunctionCanceller inside makeCancellable so that developers just need to take care of ScopedFunctionCanceller? > That's to make the auto cancel opt-in per comments at > https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... > and > https://codereview.chromium.org/2177283005/diff/100001/third_party/WebKit/Sou... Sorry for not being clear... I didn't mean to make it opt-in but was taking time to understand the semantics. As yutak@ was asking above, the semantics was not that intuitive for me as well.
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 tzik@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_...)
The CQ bit was checked by tzik@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 tzik@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.
On 2016/07/29 21:23:53, haraken wrote: > On 2016/07/29 21:10:36, tzik wrote: > > > https://codereview.chromium.org/2177283005/diff/220001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/wtf/MakeCancellable.h (right): > > > > > https://codereview.chromium.org/2177283005/diff/220001/third_party/WebKit/Sou... > > third_party/WebKit/Source/wtf/MakeCancellable.h:29: // function on its > > destruction. > > On 2016/07/29 19:18:39, haraken wrote: > > > On 2016/07/29 16:49:13, tzik wrote: > > > > On 2016/07/29 14:02:52, haraken wrote: > > > > > > > > > > Are you assuming that normal developers use both FunctionCanceller and > > > > > ScopedFunctionCanceller (which seems a bit too complex to me)? Or is > > > > > FunctionCanceller the only customer of ScopedFunctionCanceller? > > > > > > > > > > > > > Since all existing implementations of custom cancellation use > > > cancel-on-delete, > > > > I expect their replacement should be ScopedFunctionCanceller rather than > raw > > > > FunctionCanceller. > > > > > > I'm a bit confused. Then what's the point of splitting > ScopedFunctionCanceller > > > from FunctionCanceller? Can we just make FunctionCanceller call cancel() > when > > it > > > gets destructed? > > > > That's to make the auto cancel opt-in per comments at > > > https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... > > and > > > https://codereview.chromium.org/2177283005/diff/100001/third_party/WebKit/Sou... > > > > Since FunctionCanceller outlives the outer Function, it's not useful to call > > cancel() in its destructor. > > > > > > > > Sorry for being nit-picky on this. But I'd like to simplify the interface as > > > much as possible since the semantics of makeCancellable is already a lot > > > complicated (as we've been discussing in this CL). > > > > > > > Is it that complicated for users? Do you have any concern? > > Now developers need to take care of four objects: Original function, cancellable > function, FunctionCanceller, ScopedFunctionCanceller. > > Can we encapsulate the FunctionCanceller inside makeCancellable so that > developers just need to take care of ScopedFunctionCanceller? OK, I hided FunctionCanceller into internal namespace, and exposed ScopedFunctionCanceller. > > > That's to make the auto cancel opt-in per comments at > > > https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... > > and > > > https://codereview.chromium.org/2177283005/diff/100001/third_party/WebKit/Sou... > > Sorry for not being clear... I didn't mean to make it opt-in but was taking time > to understand the semantics. As yutak@ was asking above, the semantics was not > that intuitive for me as well.
On 2016/08/02 09:35:59, tzik wrote: > On 2016/07/29 21:23:53, haraken wrote: > > On 2016/07/29 21:10:36, tzik wrote: > > > > > > https://codereview.chromium.org/2177283005/diff/220001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/wtf/MakeCancellable.h (right): > > > > > > > > > https://codereview.chromium.org/2177283005/diff/220001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/wtf/MakeCancellable.h:29: // function on its > > > destruction. > > > On 2016/07/29 19:18:39, haraken wrote: > > > > On 2016/07/29 16:49:13, tzik wrote: > > > > > On 2016/07/29 14:02:52, haraken wrote: > > > > > > > > > > > > Are you assuming that normal developers use both FunctionCanceller and > > > > > > ScopedFunctionCanceller (which seems a bit too complex to me)? Or is > > > > > > FunctionCanceller the only customer of ScopedFunctionCanceller? > > > > > > > > > > > > > > > > Since all existing implementations of custom cancellation use > > > > cancel-on-delete, > > > > > I expect their replacement should be ScopedFunctionCanceller rather than > > raw > > > > > FunctionCanceller. > > > > > > > > I'm a bit confused. Then what's the point of splitting > > ScopedFunctionCanceller > > > > from FunctionCanceller? Can we just make FunctionCanceller call cancel() > > when > > > it > > > > gets destructed? > > > > > > That's to make the auto cancel opt-in per comments at > > > > > > https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... > > > and > > > > > > https://codereview.chromium.org/2177283005/diff/100001/third_party/WebKit/Sou... > > > > > > Since FunctionCanceller outlives the outer Function, it's not useful to call > > > cancel() in its destructor. > > > > > > > > > > > Sorry for being nit-picky on this. But I'd like to simplify the interface > as > > > > much as possible since the semantics of makeCancellable is already a lot > > > > complicated (as we've been discussing in this CL). > > > > > > > > > > Is it that complicated for users? Do you have any concern? > > > > Now developers need to take care of four objects: Original function, > cancellable > > function, FunctionCanceller, ScopedFunctionCanceller. > > > > Can we encapsulate the FunctionCanceller inside makeCancellable so that > > developers just need to take care of ScopedFunctionCanceller? > > OK, I hided FunctionCanceller into internal namespace, and exposed > ScopedFunctionCanceller. > > > > > > That's to make the auto cancel opt-in per comments at > > > > > > https://codereview.chromium.org/2177283005/diff/120001/third_party/WebKit/Sou... > > > and > > > > > > https://codereview.chromium.org/2177283005/diff/100001/third_party/WebKit/Sou... > > > > Sorry for not being clear... I didn't mean to make it opt-in but was taking > time > > to understand the semantics. As yutak@ was asking above, the semantics was not > > that intuitive for me as well. Thanks, LGTM
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yutak@chromium.org Link to the patchset: https://codereview.chromium.org/2177283005/#ps340001 (title: "+comment")
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 ========== Add WTF::makeCancellable Current Blink code has several tricky code for Function cancellation, such as ImageLoader::Task, WorkerEventQueue::EventDispatcherTask, CancellableTaskFactory, and 0-delay oneshot timers. They essentially make code dups, and adds complexity to the code base. This CL adds WTF::makeCancellable() to unify them. ========== to ========== Add WTF::makeCancellable Current Blink code has several tricky code for Function cancellation, such as ImageLoader::Task, WorkerEventQueue::EventDispatcherTask, CancellableTaskFactory, and 0-delay oneshot timers. They essentially make code dups, and adds complexity to the code base. This CL adds WTF::makeCancellable() to unify them. ==========
Message was sent while issue was closed.
Committed patchset #15 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Add WTF::makeCancellable Current Blink code has several tricky code for Function cancellation, such as ImageLoader::Task, WorkerEventQueue::EventDispatcherTask, CancellableTaskFactory, and 0-delay oneshot timers. They essentially make code dups, and adds complexity to the code base. This CL adds WTF::makeCancellable() to unify them. ========== to ========== Add WTF::makeCancellable Current Blink code has several tricky code for Function cancellation, such as ImageLoader::Task, WorkerEventQueue::EventDispatcherTask, CancellableTaskFactory, and 0-delay oneshot timers. They essentially make code dups, and adds complexity to the code base. This CL adds WTF::makeCancellable() to unify them. Committed: https://crrev.com/e5b8b24c7412a07913e4fd0648d71624d09e6c55 Cr-Commit-Position: refs/heads/master@{#409457} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/e5b8b24c7412a07913e4fd0648d71624d09e6c55 Cr-Commit-Position: refs/heads/master@{#409457}
Message was sent while issue was closed.
alexclarke@google.com changed reviewers: + alexclarke@google.com
Message was sent while issue was closed.
Can we reconsider? :) The problem is this adds yet another layer of abstraction to achieve cancellation, but Sami and I are considering making cancellation work natively in the scheduler.
Message was sent while issue was closed.
On 2016/08/10 09:36:28, alexclarke wrote: > Can we reconsider? :) The problem is this adds yet another layer of abstraction > to achieve cancellation, but Sami and I are considering making cancellation work > natively in the scheduler. Yes, we should. I just chatted with Sami offline about it. Currently we have multiple classes to create a cancellable task: CancellableTaskFactory, Timer, makeCancellable etc. We need to figure out how to unify the design and APIs before increasing the usage of makeCancellable. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
