Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14757022:
Add a non-blocking "OneShotEvent" class (Closed)
|
Created:
7 years, 7 months ago by Jeffrey Yasskin Modified:
7 years, 7 months ago CC:
chromium-reviews, Aaron Boodman, chromium-apps-reviews_chromium.org, Devlin Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a non-blocking "OneShotEvent" class
to simplify code that needs to run after something has happened, and
start using it for the ExtensionService's READY notification.
This change doesn't, in itself replace any uses of NOTIFICATION_EXTENSIONS_READY, but it paves the way for doing so.
BUG=240968
R=atwilson@chromium.org, kalman@chromium.org, mpcomplete@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200918
Patch Set 1 #Patch Set 2 : Fix tests; add a test #
Total comments: 18
Patch Set 3 : dcronin's comments; ExtensionSystem::ready; and WeakPtr support #
Total comments: 21
Patch Set 4 : Rename AsyncEvent to Latch, and move 'ready' fully into ExtensionSystem #Patch Set 5 : Remove unnecessary changes, and add a comment yoz requested #Patch Set 6 : Rename Latch to OneShotEvent #
Total comments: 19
Patch Set 7 : kalman's comments #Patch Set 8 : s/ThenRun/Post/ and sync #Patch Set 9 : Replace ExtensionServiceReady() with passing &ready_ into the ExtensionService constructor #
Total comments: 2
Patch Set 10 : Add a test for the behavior within a Post()ed callback #Messages
Total messages: 29 (0 generated)
Neat. :) Saw your post in #crx; decided to take a look for my own edification (not a reviewer - just offering my input). https://codereview.chromium.org/14757022/diff/5001/chrome/browser/extensions/... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/14757022/diff/5001/chrome/browser/extensions/... chrome/browser/extensions/extension_service.h:145: extensions::AsyncEvent ready_; I might suggest renaming ready_... Maybe on_ready_[task[s]] or something similar? https://codereview.chromium.org/14757022/diff/5001/chrome/common/extensions/e... File chrome/common/extensions/extension_set.h (right): https://codereview.chromium.org/14757022/diff/5001/chrome/common/extensions/e... chrome/common/extensions/extension_set.h:47: typedef scoped_refptr<const extensions::Extension> value_type; Never used. https://codereview.chromium.org/14757022/diff/5001/extensions/common/async_ev... File extensions/common/async_event.cc (right): https://codereview.chromium.org/14757022/diff/5001/extensions/common/async_ev... extensions/common/async_event.cc:9: #include "base/logging.h" logging is included in both the .h and .cc file. https://codereview.chromium.org/14757022/diff/5001/extensions/common/async_ev... extensions/common/async_event.cc:67: } // namespace extensions https://codereview.chromium.org/14757022/diff/5001/extensions/common/async_ev... File extensions/common/async_event.h (right): https://codereview.chromium.org/14757022/diff/5001/extensions/common/async_ev... extensions/common/async_event.h:36: class AsyncEvent { Just my opinion, but I think that AsyncEvent is a bit of a misnomer, given that (a)synchronicity is usually an indicator of threading, and this class is not thread-safe. I might suggest something like "TriggeredEvent" instead. https://codereview.chromium.org/14757022/diff/5001/extensions/common/async_ev... extensions/common/async_event.h:67: const scoped_refptr<base::TaskRunner>& runner = 0) const; Style forbids default arguments here: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments. https://codereview.chromium.org/14757022/diff/5001/extensions/common/async_ev... extensions/common/async_event.h:88: }; s/;/ \/\/ namespace extensions https://codereview.chromium.org/14757022/diff/5001/extensions/common/async_ev... extensions/common/async_event.h:90: #endif // EXTENSIONS_COMMON_ASYNC_EVENT_H_ nit: remove extra space after //.
Thanks for the review and the attention to detail! I haven't renamed anything yet, but I'll definitely incorporate your naming thoughts into whatever other comments I get. I'll post again when I have a sample use uploaded, which is when I think this'll be ready for more people to take a look. https://codereview.chromium.org/14757022/diff/5001/chrome/browser/extensions/... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/14757022/diff/5001/chrome/browser/extensions/... chrome/browser/extensions/extension_service.h:145: extensions::AsyncEvent ready_; On 2013/05/15 00:21:16, D Cronin wrote: > I might suggest renaming ready_... Maybe on_ready_[task[s]] or something > similar? What's your reasoning behind that? We have a bunch of functions named OnWhatever(), but that's because they run On Whatever. This object just represents "Whatever" itself. "task" isn't quite right either. To some extent this is actually a "startup_extensions_loaded" event, but I kept the original name since the rest of the change is big enough. https://codereview.chromium.org/14757022/diff/5001/chrome/common/extensions/e... File chrome/common/extensions/extension_set.h (right): https://codereview.chromium.org/14757022/diff/5001/chrome/common/extensions/e... chrome/common/extensions/extension_set.h:47: typedef scoped_refptr<const extensions::Extension> value_type; On 2013/05/15 00:21:16, D Cronin wrote: > Never used. Whoops. I took out the gmock assertion that would have used it. Thanks. https://codereview.chromium.org/14757022/diff/5001/extensions/common/async_ev... File extensions/common/async_event.cc (right): https://codereview.chromium.org/14757022/diff/5001/extensions/common/async_ev... extensions/common/async_event.cc:9: #include "base/logging.h" On 2013/05/15 00:21:16, D Cronin wrote: > logging is included in both the .h and .cc file. That's not a big problem, but sure, removed it from here. https://codereview.chromium.org/14757022/diff/5001/extensions/common/async_ev... extensions/common/async_event.cc:67: } On 2013/05/15 00:21:16, D Cronin wrote: > // namespace extensions Done. https://codereview.chromium.org/14757022/diff/5001/extensions/common/async_ev... File extensions/common/async_event.h (right): https://codereview.chromium.org/14757022/diff/5001/extensions/common/async_ev... extensions/common/async_event.h:36: class AsyncEvent { On 2013/05/15 00:21:16, D Cronin wrote: > Just my opinion, but I think that AsyncEvent is a bit of a misnomer, given that > (a)synchronicity is usually an indicator of threading, and this class is not > thread-safe. I might suggest something like "TriggeredEvent" instead. Yeah, this name isn't great. I'd like to use just "Event", but that's already used by both Windows (for a blocking, thread-safe type) and the UI system (for data describing a piece of user input). "Notification" could work. "Latch" (http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/CountDownLatch....) would do well if we decide this should be able to count N calls instead of just 1. I don't like "Triggered" because that focuses on the part of this class that _isn't_ distinct from the Windows Event. I don't think the non-thread-safety of this class should affect the naming much, since most objects in Chrome are designed to be accessed from a single thread, and interact with other threads by sending messages to their TaskRunners. It would be easy to make this class thread-safe, if that's what people want. https://codereview.chromium.org/14757022/diff/5001/extensions/common/async_ev... extensions/common/async_event.h:42: bool has_happened() const { Maybe done, released, or notified? https://codereview.chromium.org/14757022/diff/5001/extensions/common/async_ev... extensions/common/async_event.h:65: void RunAfter(const tracked_objects::Location& from_here, event.RunAfter(task) isn't great either. Maybe event.ThenRun(task). https://codereview.chromium.org/14757022/diff/5001/extensions/common/async_ev... extensions/common/async_event.h:67: const scoped_refptr<base::TaskRunner>& runner = 0) const; On 2013/05/15 00:21:16, D Cronin wrote: > Style forbids default arguments here: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments. Sure, switched to 2 overloads instead and removed the special case for NULL, which simplified the implementation. https://codereview.chromium.org/14757022/diff/5001/extensions/common/async_ev... extensions/common/async_event.h:88: }; On 2013/05/15 00:21:16, D Cronin wrote: > s/;/ \/\/ namespace extensions Done. https://codereview.chromium.org/14757022/diff/5001/extensions/common/async_ev... extensions/common/async_event.h:90: #endif // EXTENSIONS_COMMON_ASYNC_EVENT_H_ On 2013/05/15 00:21:16, D Cronin wrote: > nit: remove extra space after //. Done.
Ok, see https://codereview.chromium.org/14895016/ for a use that demonstrates that this does in fact save code. That use pointed out that I needed to add the ready() event on ExtensionSystem instead of ExtensionService, because the ExtensionService doesn't always exist yet when clients want to queue a task. Right now, the ExtensionSystem watches ExtensionService::ready() in order to set its own ready(), but I could pass a completion callback into ExtensionService instead and remove ExtensionService::ready() entirely. Please review and complain about names when you get a chance. :)
https://codereview.chromium.org/14757022/diff/19001/chrome/browser/extensions... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/14757022/diff/19001/chrome/browser/extensions... chrome/browser/extensions/extension_service.h:137: const extensions::AsyncEvent& ready() { return ready_; } I can see why you did this, but as an interface, this class shouldn't have an implemented methods. https://codereview.chromium.org/14757022/diff/19001/chrome/browser/extensions... File chrome/browser/extensions/extension_system.cc (right): https://codereview.chromium.org/14757022/diff/19001/chrome/browser/extensions... chrome/browser/extensions/extension_system.cc:155: FROM_HERE, base::Bind(&AsyncEvent::MarkHappened, ready_.AsWeakPtr())); I think it will get confusing to have 2 "ready" signals for extensions. All of extensions should become ready at the same time. I don't have a strong opinion which class should hold the |ready| object, but only one of them should. https://codereview.chromium.org/14757022/diff/19001/extensions/common/async_e... File extensions/common/async_event.cc (right): https://codereview.chromium.org/14757022/diff/19001/extensions/common/async_e... extensions/common/async_event.cc:25: scoped_refptr<TaskRunner> runner; Is it kosher to hang on to a TaskRunner reference for this? https://codereview.chromium.org/14757022/diff/19001/extensions/common/async_e... File extensions/common/async_event.h (right): https://codereview.chromium.org/14757022/diff/19001/extensions/common/async_e... extensions/common/async_event.h:32: // WaitableEventWatchers, but using it is simpler. Maybe there's a way to unify these concepts somehow. Or at the least, use similar terminology (e.g. "Signal" instead of "MarkHappened", etc)? https://codereview.chromium.org/14757022/diff/19001/extensions/common/async_e... extensions/common/async_event.h:35: class AsyncEvent : public base::SupportsWeakPtr<AsyncEvent> { Can you change the class name? And I don't like what you were going to change it to, either, so please change it again after that. https://codereview.chromium.org/14757022/diff/19001/extensions/common/async_e... extensions/common/async_event.h:67: void RunAfter(const tracked_objects::Location& from_here, Is from_here really useful? Seems that most callers will only have a single entry point. I would just set FROM_HERE to the AsyncEvent code. https://codereview.chromium.org/14757022/diff/19001/extensions/common/async_e... extensions/common/async_event.h:85: // Optimization note: We could reduce the size of this class to a There won't be enough of these objects for that to be worth it, right?
https://codereview.chromium.org/14757022/diff/19001/chrome/browser/extensions... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/14757022/diff/19001/chrome/browser/extensions... chrome/browser/extensions/extension_service.h:136: bool is_ready() const { return ready_.has_happened(); } Arguably, the point of AsyncEvent is to not have to write different code paths for ready/unready, so is it necessary to have an "is_ready" at all?
https://codereview.chromium.org/14757022/diff/19001/chrome/browser/extensions... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/14757022/diff/19001/chrome/browser/extensions... chrome/browser/extensions/extension_service.h:136: bool is_ready() const { return ready_.has_happened(); } On 2013/05/15 22:38:41, Yoyo Zhou wrote: > Arguably, the point of AsyncEvent is to not have to write different code paths > for ready/unready, so is it necessary to have an "is_ready" at all? I was actually leaning the other way - that maybe the fact that |ready| is an AsyncEvent is an implementation detail, and we should really only expose IsReady and RunWhenReady APIs.
https://codereview.chromium.org/14757022/diff/19001/chrome/browser/extensions... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/14757022/diff/19001/chrome/browser/extensions... chrome/browser/extensions/extension_service.h:136: bool is_ready() const { return ready_.has_happened(); } On 2013/05/15 22:38:41, Yoyo Zhou wrote: > Arguably, the point of AsyncEvent is to not have to write different code paths > for ready/unready, so is it necessary to have an "is_ready" at all? We need is_ready for backward-compatibility with existing ExtensionService code, and I suspect it'll keep being useful for other migrations. You're right that any use of it in client code is likely unwise. I'll add a comment. https://codereview.chromium.org/14757022/diff/19001/chrome/browser/extensions... chrome/browser/extensions/extension_service.h:137: const extensions::AsyncEvent& ready() { return ready_; } On 2013/05/15 21:08:54, Matt Perry wrote: > I can see why you did this, but as an interface, this class shouldn't have an > implemented methods. Done. https://codereview.chromium.org/14757022/diff/19001/chrome/browser/extensions... File chrome/browser/extensions/extension_system.cc (right): https://codereview.chromium.org/14757022/diff/19001/chrome/browser/extensions... chrome/browser/extensions/extension_system.cc:155: FROM_HERE, base::Bind(&AsyncEvent::MarkHappened, ready_.AsWeakPtr())); On 2013/05/15 21:08:54, Matt Perry wrote: > I think it will get confusing to have 2 "ready" signals for extensions. All of > extensions should become ready at the same time. I don't have a strong opinion > which class should hold the |ready| object, but only one of them should. Ok, I've moved them into ExtensionSystem. They need to be there instead of ExtensionService because many users want to schedule something to happen after 'ready' when the ExtensionService might not have been created yet. https://codereview.chromium.org/14757022/diff/19001/extensions/common/async_e... File extensions/common/async_event.cc (right): https://codereview.chromium.org/14757022/diff/19001/extensions/common/async_e... extensions/common/async_event.cc:25: scoped_refptr<TaskRunner> runner; On 2013/05/15 21:08:54, Matt Perry wrote: > Is it kosher to hang on to a TaskRunner reference for this? A MessageLoopProxy is definitely kosher. The other TaskRunners are all refcounted, I assume so they can be used this way. I'd use a WeakPtr, but I don't have that option. https://codereview.chromium.org/14757022/diff/19001/extensions/common/async_e... File extensions/common/async_event.h (right): https://codereview.chromium.org/14757022/diff/19001/extensions/common/async_e... extensions/common/async_event.h:32: // WaitableEventWatchers, but using it is simpler. On 2013/05/15 21:08:54, Matt Perry wrote: > Maybe there's a way to unify these concepts somehow. Or at the least, use > similar terminology (e.g. "Signal" instead of "MarkHappened", etc)? Signal is easy enough. With "Latch" as the class, I think "is_open" makes more sense than "is_signaled"? On the other hand, many functions returning a Latch read better as ready().is_signaled() than ready().is_open(), so maybe I've picked another bad class name. https://codereview.chromium.org/14757022/diff/19001/extensions/common/async_e... extensions/common/async_event.h:35: class AsyncEvent : public base::SupportsWeakPtr<AsyncEvent> { On 2013/05/15 21:08:54, Matt Perry wrote: > Can you change the class name? And I don't like what you were going to change it > to, either, so please change it again after that. Changed. We'll see if you want something else. ;) https://codereview.chromium.org/14757022/diff/19001/extensions/common/async_e... extensions/common/async_event.h:67: void RunAfter(const tracked_objects::Location& from_here, On 2013/05/15 21:08:54, Matt Perry wrote: > Is from_here really useful? Seems that most callers will only have a single > entry point. I would just set FROM_HERE to the AsyncEvent code. I think you're saying that any function passed to RunAfter is likely to be used in only one place, so people trying to debug code can just look for all references to that function instead of needing to grab the from_here information. Does that apply more to this function than to general TaskRunner tasks? I've definitely used the Location to debug tasks on TaskRunners. https://codereview.chromium.org/14757022/diff/19001/extensions/common/async_e... extensions/common/async_event.h:85: // Optimization note: We could reduce the size of this class to a On 2013/05/15 21:08:54, Matt Perry wrote: > There won't be enough of these objects for that to be worth it, right? There might be, but there aren't yet, which is why I haven't done it.
https://codereview.chromium.org/14757022/diff/19001/extensions/common/async_e... File extensions/common/async_event.h (right): https://codereview.chromium.org/14757022/diff/19001/extensions/common/async_e... extensions/common/async_event.h:32: // WaitableEventWatchers, but using it is simpler. On 2013/05/15 22:46:50, Jeffrey Yasskin wrote: > On 2013/05/15 21:08:54, Matt Perry wrote: > > Maybe there's a way to unify these concepts somehow. Or at the least, use > > similar terminology (e.g. "Signal" instead of "MarkHappened", etc)? > > Signal is easy enough. With "Latch" as the class, I think "is_open" makes more > sense than "is_signaled"? On the other hand, many functions returning a Latch > read better as ready().is_signaled() than ready().is_open(), so maybe I've > picked another bad class name. As a consumer of the class, I think I'd find it hard to remember whether the "ready" state was "open" or "closed". I certainly didn't know until I looked at the code. https://codereview.chromium.org/14757022/diff/19001/extensions/common/async_e... extensions/common/async_event.h:67: void RunAfter(const tracked_objects::Location& from_here, On 2013/05/15 22:46:50, Jeffrey Yasskin wrote: > On 2013/05/15 21:08:54, Matt Perry wrote: > > Is from_here really useful? Seems that most callers will only have a single > > entry point. I would just set FROM_HERE to the AsyncEvent code. > > I think you're saying that any function passed to RunAfter is likely to be used > in only one place, so people trying to debug code can just look for all > references to that function instead of needing to grab the from_here > information. > > Does that apply more to this function than to general TaskRunner tasks? I've > definitely used the Location to debug tasks on TaskRunners. Yes, I think it's less likely to be useful for this class, because the event that triggered the callback is more interesting than where the callback was registered.
https://codereview.chromium.org/14757022/diff/19001/extensions/common/async_e... File extensions/common/async_event.h (right): https://codereview.chromium.org/14757022/diff/19001/extensions/common/async_e... extensions/common/async_event.h:32: // WaitableEventWatchers, but using it is simpler. On 2013/05/15 22:55:40, Matt Perry wrote: > On 2013/05/15 22:46:50, Jeffrey Yasskin wrote: > > On 2013/05/15 21:08:54, Matt Perry wrote: > > > Maybe there's a way to unify these concepts somehow. Or at the least, use > > > similar terminology (e.g. "Signal" instead of "MarkHappened", etc)? > > > > Signal is easy enough. With "Latch" as the class, I think "is_open" makes more > > sense than "is_signaled"? On the other hand, many functions returning a Latch > > read better as ready().is_signaled() than ready().is_open(), so maybe I've > > picked another bad class name. > > As a consumer of the class, I think I'd find it hard to remember whether the > "ready" state was "open" or "closed". I certainly didn't know until I looked at > the code. Renamed to is_signaled(), and renamed the class a second time to match. :) https://codereview.chromium.org/14757022/diff/19001/extensions/common/async_e... extensions/common/async_event.h:67: void RunAfter(const tracked_objects::Location& from_here, On 2013/05/15 22:55:40, Matt Perry wrote: > On 2013/05/15 22:46:50, Jeffrey Yasskin wrote: > > On 2013/05/15 21:08:54, Matt Perry wrote: > > > Is from_here really useful? Seems that most callers will only have a single > > > entry point. I would just set FROM_HERE to the AsyncEvent code. > > > > I think you're saying that any function passed to RunAfter is likely to be > used > > in only one place, so people trying to debug code can just look for all > > references to that function instead of needing to grab the from_here > > information. > > > > Does that apply more to this function than to general TaskRunner tasks? I've > > definitely used the Location to debug tasks on TaskRunners. > > Yes, I think it's less likely to be useful for this class, because the event > that triggered the callback is more interesting than where the callback was > registered. I managed to convince Matt that the from_here was worthwhile by pointing out that since the tasks are enqueued on a TaskRunner instead of being run from MarkHappened, you lose the call stack describing the triggering event.
Andrew, would you review the line in background_application_list_model_unittest?
lgtm https://codereview.chromium.org/14757022/diff/39001/chrome/browser/extensions... File chrome/browser/extensions/test_extension_system.cc (right): https://codereview.chromium.org/14757022/diff/39001/chrome/browser/extensions... chrome/browser/extensions/test_extension_system.cc:188: void TestExtensionSystem::MakeReady() { ready_.Signal(); } Might this cause problems since Signal can only be called once?
https://codereview.chromium.org/14757022/diff/39001/chrome/browser/extensions... File chrome/browser/extensions/test_extension_system.cc (right): https://codereview.chromium.org/14757022/diff/39001/chrome/browser/extensions... chrome/browser/extensions/test_extension_system.cc:188: void TestExtensionSystem::MakeReady() { ready_.Signal(); } On 2013/05/16 00:55:50, Matt Perry wrote: > Might this cause problems since Signal can only be called once? Tests could cause a DCHECK by calling MakeReady() twice, but as long as some trybots run tests in debug mode, we'll catch that. On the other hand, no tests are actually using this yet; maybe we don't need it.
browser/background LGTM
just adding a few thoughts, I know it's already been lg'd. https://codereview.chromium.org/14757022/diff/39001/chrome/browser/extensions... File chrome/browser/extensions/extension_system.h (right): https://codereview.chromium.org/14757022/diff/39001/chrome/browser/extensions... chrome/browser/extensions/extension_system.h:145: // Whether the extension system has completed its startup tasks. The comment (and in a way, method name) implies that this method returns a bool when it doesn't. It's not clear from this file how OneShotEvent state --> readiness. https://codereview.chromium.org/14757022/diff/39001/chrome/browser/extensions... chrome/browser/extensions/extension_system.h:152: virtual void ExtensionServiceReady() = 0; I can see why ExtensionService owns this event, but calling back into the ExtensionSystem from ExtensionService for this seems like a violation of demeter. Can ExtensionSystem pass the event into ExtensionService instead? https://codereview.chromium.org/14757022/diff/39001/extensions/common/one_sho... File extensions/common/one_shot_event.cc (right): https://codereview.chromium.org/14757022/diff/39001/extensions/common/one_sho... extensions/common/one_shot_event.cc:22: CHECK(runner != NULL); // Detect mistakes with a decent stack frame. just CHECK(runner)? https://codereview.chromium.org/14757022/diff/39001/extensions/common/one_sho... extensions/common/one_shot_event.cc:33: const base::Closure& task) const { indentation +1 looks like there are a couple of other places in this file like this, presumably because you renamed. https://codereview.chromium.org/14757022/diff/39001/extensions/common/one_sho... extensions/common/one_shot_event.cc:42: if (is_signaled()) { I actually wonder if this may at some point cause confusion - it's just as legitimate to call CHECK(!is_signalled) here and I think that this may often be what is safer for callers. WDYT about having two separate methods here? One version that CHECKs, the other that does what you're doing (... and never a version that runs the callback synchronously, obviously). Naming... well, it's not worth thinking much about until you think about it, but maybe like AddTask vs AddOrRunTask/AddOrPostTask (or some variation using the ThenRun theme; ThenRunEventually?). https://codereview.chromium.org/14757022/diff/39001/extensions/common/one_sho... File extensions/common/one_shot_event.h (right): https://codereview.chromium.org/14757022/diff/39001/extensions/common/one_sho... extensions/common/one_shot_event.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. I believe we leave out the (c) these days.
https://codereview.chromium.org/14757022/diff/39001/chrome/browser/extensions... File chrome/browser/extensions/extension_system.h (right): https://codereview.chromium.org/14757022/diff/39001/chrome/browser/extensions... chrome/browser/extensions/extension_system.h:145: // Whether the extension system has completed its startup tasks. On 2013/05/16 15:51:08, kalman wrote: > The comment (and in a way, method name) implies that this method returns a bool > when it doesn't. It's not clear from this file how OneShotEvent state --> > readiness. Good point; re-commented. https://codereview.chromium.org/14757022/diff/39001/chrome/browser/extensions... chrome/browser/extensions/extension_system.h:152: virtual void ExtensionServiceReady() = 0; On 2013/05/16 15:51:08, kalman wrote: > I can see why ExtensionService owns this event, but calling back into the > ExtensionSystem from ExtensionService for this seems like a violation of > demeter. Can ExtensionSystem pass the event into ExtensionService instead? I agree that it's icky, and I tried your suggestion, but it looks like it would cause fragile tests. Several tests call ExtensionService::Init(), which I'd have to change to ExtensionService::Init(OneShotEvent*). However, then the tests would be passing in an Event that doesn't affect ExtensionService::is_ready() or ExtensionSystem::ready(). I could store the Event* inside the ExtensionService to make is_ready() consistent, but then we'd still have the state Matt was worried about (https://codereview.chromium.org/14757022/diff/19001/chrome/browser/extensions...) where there are two different ready-states for different parts of the extensions system. https://codereview.chromium.org/14757022/diff/39001/extensions/common/one_sho... File extensions/common/one_shot_event.cc (right): https://codereview.chromium.org/14757022/diff/39001/extensions/common/one_sho... extensions/common/one_shot_event.cc:22: CHECK(runner != NULL); // Detect mistakes with a decent stack frame. On 2013/05/16 15:51:08, kalman wrote: > just CHECK(runner)? I have no idea. Sure. https://codereview.chromium.org/14757022/diff/39001/extensions/common/one_sho... extensions/common/one_shot_event.cc:33: const base::Closure& task) const { On 2013/05/16 15:51:08, kalman wrote: > indentation +1 > looks like there are a couple of other places in this file like this, presumably > because you renamed. Oops, done. https://codereview.chromium.org/14757022/diff/39001/extensions/common/one_sho... extensions/common/one_shot_event.cc:42: if (is_signaled()) { On 2013/05/16 15:51:08, kalman wrote: > I actually wonder if this may at some point cause confusion - it's just as > legitimate to call CHECK(!is_signalled) here and I think that this may often be > what is safer for callers. > > WDYT about having two separate methods here? One version that CHECKs, the other > that does what you're doing (... and never a version that runs the callback > synchronously, obviously). Naming... well, it's not worth thinking much about > until you think about it, but maybe like AddTask vs AddOrRunTask/AddOrPostTask > (or some variation using the ThenRun theme; ThenRunEventually?). We talked about this offline, and agreed on the following: * We want to encourage uses to avoid caring whether the event has already happened, so we want the version of this function that has if(is_signaled()) to have a shorter name. * There are some valid uses (e.g. icon loading) that need to run inline when the event is already signaled, and possibly run different code if they're running inline. A separate method to support them is likely to be useful. * We don't want to add that separate method until we have a use case or two. * The name of this method should anticipate that the second method may eventually exist. Other names for this method include: AddListener(), Post(), AfterSignaled(). RunAfter(), CallAfter(), etc. aren't great because event.RunAfter(callback) reads like the event is going to run after the callback, which is backwards. I'm going to let the names sit until this afternoon, and then poll the team. https://codereview.chromium.org/14757022/diff/39001/extensions/common/one_sho... File extensions/common/one_shot_event.h (right): https://codereview.chromium.org/14757022/diff/39001/extensions/common/one_sho... extensions/common/one_shot_event.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/05/16 15:51:08, kalman wrote: > I believe we leave out the (c) these days. If only I'd obeyed the style guide's rule to avoid copying headers from other files.
https://codereview.chromium.org/14757022/diff/39001/extensions/common/one_sho... File extensions/common/one_shot_event.cc (right): https://codereview.chromium.org/14757022/diff/39001/extensions/common/one_sho... extensions/common/one_shot_event.cc:42: if (is_signaled()) { On 2013/05/16 19:05:36, Jeffrey Yasskin wrote: > On 2013/05/16 15:51:08, kalman wrote: > > I actually wonder if this may at some point cause confusion - it's just as > > legitimate to call CHECK(!is_signalled) here and I think that this may often > be > > what is safer for callers. > > > > WDYT about having two separate methods here? One version that CHECKs, the > other > > that does what you're doing (... and never a version that runs the callback > > synchronously, obviously). Naming... well, it's not worth thinking much about > > until you think about it, but maybe like AddTask vs AddOrRunTask/AddOrPostTask > > (or some variation using the ThenRun theme; ThenRunEventually?). > > We talked about this offline, and agreed on the following: > * We want to encourage uses to avoid caring whether the event has already > happened, so we want the version of this function that has if(is_signaled()) to > have a shorter name. > * There are some valid uses (e.g. icon loading) that need to run inline when the > event is already signaled, and possibly run different code if they're running > inline. A separate method to support them is likely to be useful. > * We don't want to add that separate method until we have a use case or two. > * The name of this method should anticipate that the second method may > eventually exist. > > Other names for this method include: AddListener(), Post(), AfterSignaled(). > RunAfter(), CallAfter(), etc. aren't great because event.RunAfter(callback) > reads like the event is going to run after the callback, which is backwards. > > I'm going to let the names sit until this afternoon, and then poll the team. RunWhenSignaled?
My current favourite name is Post(), it's the only one so far which is both concise and clearly communicates to the caller that the Callback will definitely be called unless the event is destroyed.
https://codereview.chromium.org/14757022/diff/39001/extensions/common/one_sho... File extensions/common/one_shot_event.cc (right): https://codereview.chromium.org/14757022/diff/39001/extensions/common/one_sho... extensions/common/one_shot_event.cc:42: if (is_signaled()) { On 2013/05/16 19:12:04, Matt Perry wrote: > On 2013/05/16 19:05:36, Jeffrey Yasskin wrote: > > On 2013/05/16 15:51:08, kalman wrote: > > > I actually wonder if this may at some point cause confusion - it's just as > > > legitimate to call CHECK(!is_signalled) here and I think that this may often > > be > > > what is safer for callers. > > > > > > WDYT about having two separate methods here? One version that CHECKs, the > > other > > > that does what you're doing (... and never a version that runs the callback > > > synchronously, obviously). Naming... well, it's not worth thinking much > about > > > until you think about it, but maybe like AddTask vs > AddOrRunTask/AddOrPostTask > > > (or some variation using the ThenRun theme; ThenRunEventually?). > > > > We talked about this offline, and agreed on the following: > > * We want to encourage uses to avoid caring whether the event has already > > happened, so we want the version of this function that has if(is_signaled()) > to > > have a shorter name. > > * There are some valid uses (e.g. icon loading) that need to run inline when > the > > event is already signaled, and possibly run different code if they're running > > inline. A separate method to support them is likely to be useful. > > * We don't want to add that separate method until we have a use case or two. > > * The name of this method should anticipate that the second method may > > eventually exist. > > > > Other names for this method include: AddListener(), Post(), AfterSignaled(). > > RunAfter(), CallAfter(), etc. aren't great because event.RunAfter(callback) > > reads like the event is going to run after the callback, which is backwards. > > > > I'm going to let the names sit until this afternoon, and then poll the team. > > RunWhenSignaled? I dislike that one because it implies the task will _only_ be run at the point the Event is signaled, when actually, if the Event has already been signaled by the call to this function, the task will be run at the call to this function, not when the Signal() happens.
https://codereview.chromium.org/14757022/diff/39001/extensions/common/one_sho... File extensions/common/one_shot_event.cc (right): https://codereview.chromium.org/14757022/diff/39001/extensions/common/one_sho... extensions/common/one_shot_event.cc:42: if (is_signaled()) { On 2013/05/16 19:14:42, Jeffrey Yasskin wrote: > On 2013/05/16 19:12:04, Matt Perry wrote: > > On 2013/05/16 19:05:36, Jeffrey Yasskin wrote: > > > On 2013/05/16 15:51:08, kalman wrote: > > > > I actually wonder if this may at some point cause confusion - it's just as > > > > legitimate to call CHECK(!is_signalled) here and I think that this may > often > > > be > > > > what is safer for callers. > > > > > > > > WDYT about having two separate methods here? One version that CHECKs, the > > > other > > > > that does what you're doing (... and never a version that runs the > callback > > > > synchronously, obviously). Naming... well, it's not worth thinking much > > about > > > > until you think about it, but maybe like AddTask vs > > AddOrRunTask/AddOrPostTask > > > > (or some variation using the ThenRun theme; ThenRunEventually?). > > > > > > We talked about this offline, and agreed on the following: > > > * We want to encourage uses to avoid caring whether the event has already > > > happened, so we want the version of this function that has if(is_signaled()) > > to > > > have a shorter name. > > > * There are some valid uses (e.g. icon loading) that need to run inline when > > the > > > event is already signaled, and possibly run different code if they're > running > > > inline. A separate method to support them is likely to be useful. > > > * We don't want to add that separate method until we have a use case or two. > > > * The name of this method should anticipate that the second method may > > > eventually exist. > > > > > > Other names for this method include: AddListener(), Post(), AfterSignaled(). > > > RunAfter(), CallAfter(), etc. aren't great because event.RunAfter(callback) > > > reads like the event is going to run after the callback, which is backwards. > > > > > > I'm going to let the names sit until this afternoon, and then poll the team. > > > > RunWhenSignaled? > > I dislike that one because it implies the task will _only_ be run at the point > the Event is signaled, when actually, if the Event has already been signaled by > the call to this function, the task will be run at the call to this function, > not when the Signal() happens. A quick vote in the extensions room concluded that we'll be going with Post().
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jyasskin@chromium.org/14757022/61001
https://codereview.chromium.org/14757022/diff/39001/chrome/browser/extensions... File chrome/browser/extensions/extension_system.h (right): https://codereview.chromium.org/14757022/diff/39001/chrome/browser/extensions... chrome/browser/extensions/extension_system.h:152: virtual void ExtensionServiceReady() = 0; On 2013/05/16 19:05:36, Jeffrey Yasskin wrote: > On 2013/05/16 15:51:08, kalman wrote: > > I can see why ExtensionService owns this event, but calling back into the > > ExtensionSystem from ExtensionService for this seems like a violation of > > demeter. Can ExtensionSystem pass the event into ExtensionService instead? > > I agree that it's icky, and I tried your suggestion, but it looks like it would > cause fragile tests. Several tests call ExtensionService::Init(), which I'd have > to change to ExtensionService::Init(OneShotEvent*). However, then the tests > would be passing in an Event that doesn't affect ExtensionService::is_ready() or > ExtensionSystem::ready(). I could store the Event* inside the ExtensionService > to make is_ready() consistent, but then we'd still have the state Matt was > worried about > (https://codereview.chromium.org/14757022/diff/19001/chrome/browser/extensions...) > where there are two different ready-states for different parts of the extensions > system. A constructor parameter to ExtensionService won't work?
https://codereview.chromium.org/14757022/diff/39001/chrome/browser/extensions... File chrome/browser/extensions/extension_system.h (right): https://codereview.chromium.org/14757022/diff/39001/chrome/browser/extensions... chrome/browser/extensions/extension_system.h:152: virtual void ExtensionServiceReady() = 0; On 2013/05/16 21:38:17, kalman wrote: > On 2013/05/16 19:05:36, Jeffrey Yasskin wrote: > > On 2013/05/16 15:51:08, kalman wrote: > > > I can see why ExtensionService owns this event, but calling back into the > > > ExtensionSystem from ExtensionService for this seems like a violation of > > > demeter. Can ExtensionSystem pass the event into ExtensionService instead? > > > > I agree that it's icky, and I tried your suggestion, but it looks like it > would > > cause fragile tests. Several tests call ExtensionService::Init(), which I'd > have > > to change to ExtensionService::Init(OneShotEvent*). However, then the tests > > would be passing in an Event that doesn't affect ExtensionService::is_ready() > or > > ExtensionSystem::ready(). I could store the Event* inside the ExtensionService > > to make is_ready() consistent, but then we'd still have the state Matt was > > worried about > > > (https://codereview.chromium.org/14757022/diff/19001/chrome/browser/extensions...) > > where there are two different ready-states for different parts of the > extensions > > system. > > A constructor parameter to ExtensionService won't work? That seems to work and be cleaner that this callback. Thanks!
nice, thanks for that. lgtm but see my comment about testing, it's an important corner case. https://codereview.chromium.org/14757022/diff/88001/extensions/common/one_sho... File extensions/common/one_shot_event_unittest.cc (right): https://codereview.chromium.org/14757022/diff/88001/extensions/common/one_sho... extensions/common/one_shot_event_unittest.cc:16: void Increment(int* i) { ++*i; } There are a couple more scenarios it would be nice to test: - that is_signalled returns true from the callbacks - that adding a another callback from a callback calls that another-callback (would be good to test this with a couple chained together, and transitively, but might be overkill)
https://codereview.chromium.org/14757022/diff/88001/extensions/common/one_sho... File extensions/common/one_shot_event_unittest.cc (right): https://codereview.chromium.org/14757022/diff/88001/extensions/common/one_sho... extensions/common/one_shot_event_unittest.cc:16: void Increment(int* i) { ++*i; } On 2013/05/16 22:26:11, kalman wrote: > There are a couple more scenarios it would be nice to test: > - that is_signalled returns true from the callbacks > - that adding a another callback from a callback calls that another-callback > (would be good to test this with a couple chained together, and transitively, > but might be overkill) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jyasskin@chromium.org/14757022/75002
Retried try job too often on win_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jyasskin@chromium.org/14757022/75002
Retried try job too often on win_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Message was sent while issue was closed.
Committed patchset #10 manually as r200918 (presubmit successful). |
