Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
706993003:
C++ readability review (Closed)
|
Created:
6 years, 1 month ago by Chirantan Ekbote Modified:
5 years, 8 months ago CC:
chromium-reviews, Sameer Nanda, Lei Zhang, blundell Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionC++ readability review
BUG=b/18275087
Committed: https://crrev.com/05e4d854dca737cfb43cd967cd72d024f6b1625b
Cr-Commit-Position: refs/heads/master@{#324004}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebase #
Total comments: 47
Patch Set 3 : refactor #Patch Set 4 : clean up comments #
Total comments: 6
Patch Set 5 : address comments #Patch Set 6 : reorder #
Total comments: 1
Patch Set 7 : Move tests into top-level function #Patch Set 8 : Fix gn build #
Total comments: 19
Patch Set 9 : address comments #
Total comments: 2
Patch Set 10 : Address comments #
Messages
Total messages: 29 (5 generated)
chirantan@chromium.org changed reviewers: + derat@chromium.org
https://codereview.chromium.org/706993003/diff/1/components/timers/alarm_timer.h File components/timers/alarm_timer.h (right): https://codereview.chromium.org/706993003/diff/1/components/timers/alarm_time... components/timers/alarm_timer.h:105: what are the extra blank lines here and in the other files for? i think that pretty much all chromium code that i've seen has exactly one blank line between the end of the namespace and the closing header guard.
https://codereview.chromium.org/706993003/diff/1/components/timers/alarm_timer.h File components/timers/alarm_timer.h (right): https://codereview.chromium.org/706993003/diff/1/components/timers/alarm_time... components/timers/alarm_timer.h:105: On 2014/11/06 20:43:15, Daniel Erat wrote: > what are the extra blank lines here and in the other files for? i think that > pretty much all chromium code that i've seen has exactly one blank line between > the end of the namespace and the closing header guard. I had to make a whitespace change so that git would include each file in the commit. They'll be removed before this CL is submitted.
chirantan@chromium.org changed reviewers: + gromer@google.com
Hi Geoffrey, please take a look.
Thanks for participating in the readability process! I'm only going to comment on each kind of issue in one place, so please look for other places in your code where the same comment applies, and fix it everywhere. I'm not very familiar with the Chrome codebase, so I hope you can bear with me when there are things I don't understand about how it works. https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... File components/timers/alarm_timer.cc (right): https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.cc:48: base::Timer::set_is_running(false); Why not just "set_is_running(false)"? It's necessary when you call Stop() above, but here there's no risk of ambiguity. https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.cc:103: Stop(); Could you avoid all the AddDestructionObserver()/RemoveDestructionObserver() business by making this something like if (is_running()) { Stop(); } https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... File components/timers/alarm_timer.h (right): https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:28: public base::MessageLoop::DestructionObserver { https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... "Multiple inheritance is allowed only when all superclasses, with the possible exception of the first one, are pure interfaces. In order to ensure that they remain pure interfaces, they must end with the Interface suffix." One way of fixing this might be to create a helper class which inherits from DestructionObserver, and forwards the calls to the AlarmTimer. https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:34: class Delegate : public base::RefCountedThreadSafe<Delegate> { Why not use shared_ptr for thread-safe refcounting? https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:34: class Delegate : public base::RefCountedThreadSafe<Delegate> { Why is this in the public section? It seems like an implementation detail. https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:34: class Delegate : public base::RefCountedThreadSafe<Delegate> { RtcAlarm is the only implementation of Delegate that I can find. If that's the case, why factor out the abstract base class? Could AlarmTimer just use RtcAlarm directly? https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:38: virtual bool Init(base::WeakPtr<AlarmTimer> timer) = 0; Init() methods are bad news; see http://go/totw:42 for more. Why not just use a constructor for this? https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:52: friend class base::RefCountedThreadSafe<Delegate>; Why is this needed? https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:55: AlarmTimer(bool retain_user_task, bool is_repeating); https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... "Every function declaration should have comments immediately preceding it that describe what the function does and how to use it." https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:55: AlarmTimer(bool retain_user_task, bool is_repeating); bool parameters are a readability hazard, especially when you have more than one. The issue is that a callsite like AlarmTimer my_timer(true, false); gives the user almost no clues about the meanings of those arguments. Even if you know that one controls retention of the user task, and the other controls repetition, you may not know which is which, and you may not know e.g. whether "true" means that the timer is single-use or that it's repeating. See also the "Function Argument Comments" section of https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:64: bool can_wake_from_suspend() { return can_wake_from_suspend_; } https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... "Use const whenever it makes sense." This method looks like it should be const. https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:86: scoped_ptr<base::PendingTask> pending_task_; Why not unique_ptr? https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... File components/timers/alarm_timer_unittest.cc (right): https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer_unittest.cc:18: // regular Timer so it should pass the same tests as the Timer class. GUnit has mechanisms for type-parameterized tests, so you can run the same test code on different types. Could you use that to avoid duplicating all this code from the base::Timer tests? https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer_unittest.cc:423: void RunTest_OneShotTimerConcurrentResetAndTimerFired( Test code tends to work better in the top-level test function than in subroutines. This function only has one caller, so could you just replace that function call with the function implementation? https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer_unittest.cc:437: // being reset at the same time. The previously queued task should be Not quite at the same time, right? The test code seems to assume that the reset is handled before the timer firing. https://codereview.chromium.org/706993003/diff/20001/components/timers/rtc_al... File components/timers/rtc_alarm.cc (right): https://codereview.chromium.org/706993003/diff/20001/components/timers/rtc_al... components/timers/rtc_alarm.cc:30: base::Thread& operator*() const { return *thread_.get(); } https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... "Define overloaded operators only if their meaning is obvious, unsurprising, and consistent with the corresponding built-in operators." It's surprising to me that a "helper" has * and -> operators, and it's not obvious to me what its meaning should be. Could you provide a get() method instead? https://codereview.chromium.org/706993003/diff/20001/components/timers/rtc_al... components/timers/rtc_alarm.cc:98: // Make sure that we are running on a MessageLoopForIO. This seems misleading- "Make sure X" to me implies that it's some sort of error if X is not true, but IIUC it's perfectly fine if "we" are running on another event loop; the code just needs to make sure ResetImpl runs on a MessageLoopForIO. Maybe "Run ResetImpl on a MessageLoopForIO"? https://codereview.chromium.org/706993003/diff/20001/components/timers/rtc_al... components/timers/rtc_alarm.cc:109: ResetImpl(delay, origin_event_id_); If this were inside an else-block, it would be much clearer that it and the above if-block are two different ways of accomplishing the same thing. https://codereview.chromium.org/706993003/diff/20001/components/timers/rtc_al... components/timers/rtc_alarm.cc:179: parent_->OnTimerFired(); Isn't there a race condition where parent_ could be destroyed between these two lines? https://codereview.chromium.org/706993003/diff/20001/components/timers/rtc_al... File components/timers/rtc_alarm.h (right): https://codereview.chromium.org/706993003/diff/20001/components/timers/rtc_al... components/timers/rtc_alarm.h:76: // These two variables are used for coordinating between the thread that Comments that say what something is "used for" tend to be unhelpfully vague, especially for the opening sentence, because they don't tell me specifically how to use them. Instead, try to talk about what the variable _is_, or what it _means_. For example, "The ID of the most recent Reset() call handled by origin_message_loop_" (I have no idea if that's correct, but it's the kind of comment I'm looking for). Relatedly, "event" is a very vague name. Especially in such subtle code, you should choose variable names that give readers specific cues about the meaning of the variable. For example, if I'm right that the events here are really Reset() calls, you could call them origin_reset_event_id_ and io_reset_event_id. Finally, the term "sequence number" is often used with this kind of scheme; consider saying that instead of "event ID".
https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... File components/timers/alarm_timer.h (right): https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:34: class Delegate : public base::RefCountedThreadSafe<Delegate> { On 2014/12/05 20:13:40, gromer wrote: > Why not use shared_ptr for thread-safe refcounting? We don't use shared_ptrs or unique_ptrs in Chromium. (yet)
Thanks for taking the time to do this review and sorry it took so long for me to reply. I'll have a new patch set addressing your comments up this week. https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... File components/timers/alarm_timer.cc (right): https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.cc:48: base::Timer::set_is_running(false); On 2014/12/05 20:13:40, gromer wrote: > Why not just "set_is_running(false)"? It's necessary when you call Stop() above, > but here there's no risk of ambiguity. Yes it's not really necessary but I find that it improves readability for me personally because it makes it clear that set_is_running() is a method provided by the parent class. https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.cc:103: Stop(); On 2014/12/05 20:13:40, gromer wrote: > Could you avoid all the AddDestructionObserver()/RemoveDestructionObserver() > business by making this something like > > if (is_running()) { > Stop(); > } I'm not sure what you mean by this. I would still need to call RemoveDestructionObserver() somehow in the destructor of this class because otherwise the MessageLoop would hold a pointer to a freed object. https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... File components/timers/alarm_timer.h (right): https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:28: public base::MessageLoop::DestructionObserver { On 2014/12/05 20:13:41, gromer wrote: > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > "Multiple inheritance is allowed only when all superclasses, with the possible > exception of the first one, are pure interfaces. In order to ensure that they > remain pure interfaces, they must end with the Interface suffix." > > One way of fixing this might be to create a helper class which inherits from > DestructionObserver, and forwards the calls to the AlarmTimer. MessageLoop::DestructionObserver _is_ a pure interface so this should be ok right? https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:34: class Delegate : public base::RefCountedThreadSafe<Delegate> { On 2014/12/05 20:13:40, gromer wrote: > Why is this in the public section? It seems like an implementation detail. The Delegate provides all the platform-specific functionality to make this feature work. It's in the public section so that mac and windows delegates can be written later if needed. But you're right, this is currently just an implementation detail and can be moved back into the public section later if we decide that we actually need this functionality on platforms other than Chrome OS. https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:34: class Delegate : public base::RefCountedThreadSafe<Delegate> { On 2014/12/05 20:13:40, gromer wrote: > RtcAlarm is the only implementation of Delegate that I can find. If that's the > case, why factor out the abstract base class? Could AlarmTimer just use RtcAlarm > directly? As I mentioned above, it's abstracted out to make it easier to add delegates for Windows and Mac. It's unlikely that this will ever happen though, so I'll make the RtcAlarm a private nested class. https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:38: virtual bool Init(base::WeakPtr<AlarmTimer> timer) = 0; On 2014/12/05 20:13:40, gromer wrote: > Init() methods are bad news; see http://go/totw:42 for more. Why not just use a > constructor for this? So this Init function exists to break a dependency cycle. The Delegate needs a base::WeakPtr<AlarmTimer> because it might end up outliving the AlarmTimer and should not try to call the OnTimerFired() function after the AlarmTimer has been destroyed. WeakPtrs are created using a base::WeakPtrFactory<AlarmTimer>. This needs to be the final member of the AlarmTimer class so that it gets destroyed before any other member, invalidating any existing WeakPtrs that those members might have to the AlarmTimer. Since the WeakPtrFactory has to be the final member, the Delegate appears before it in the initialization list and the WeakPtrFactory cannot be used at that time to create a WeakPtr that can be passed to the constructor for the Delegate. Using raw pointers is out because then we would still need some way to invalidate it in the AlarmTimer destructor to prevent a use-after-free. I suppose the simplest way to get rid of the Init function would be to construct the Delegate in the body of the AlarmTimer constructor instead of the initialization list. That seems straightforward enough so I think I'll do that. https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:52: friend class base::RefCountedThreadSafe<Delegate>; On 2014/12/05 20:13:41, gromer wrote: > Why is this needed? It's recommended by the docstring for base::RefCountedThreadSafe: // If you're using the default trait, then you should add compile time // asserts that no one else is deleting your object. i.e. // private: // friend class base::RefCountedThreadSafe<MyFoo>; // ~MyFoo(); https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:55: AlarmTimer(bool retain_user_task, bool is_repeating); On 2014/12/05 20:13:41, gromer wrote: > bool parameters are a readability hazard, especially when you have more than > one. The issue is that a callsite like > > AlarmTimer my_timer(true, false); > > gives the user almost no clues about the meanings of those arguments. Even if > you know that one controls retention of the user task, and the other controls > repetition, you may not know which is which, and you may not know e.g. whether > "true" means that the timer is single-use or that it's repeating. > I agree wholeheartedly. I was already bit by this once when refactoring some code to use this timer. This class is meant to be a drop in replacement for base::Timer, which is why I made these constructors mirror those of base::Timer. It's probably not necessary for the constructors to match though so I will clean this up. > See also the "Function Argument Comments" section of > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:64: bool can_wake_from_suspend() { return can_wake_from_suspend_; } On 2014/12/05 20:13:40, gromer wrote: > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > "Use const whenever it makes sense." > > This method looks like it should be const. Done. This was cleaned up in a separate CL. https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:86: scoped_ptr<base::PendingTask> pending_task_; On 2014/12/05 20:13:40, gromer wrote: > Why not unique_ptr? As Lei mentioned, chromium doesn't use unique_ptr yet. However, chromium's scoped_ptr provides move semantics so it is essentially equivalent to a unique_ptr. https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... File components/timers/alarm_timer_unittest.cc (right): https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer_unittest.cc:18: // regular Timer so it should pass the same tests as the Timer class. On 2014/12/05 20:13:41, gromer wrote: > GUnit has mechanisms for type-parameterized tests, so you can run the same test > code on different types. Could you use that to avoid duplicating all this code > from the base::Timer tests? Yes, there is a bug open for me to fix this: crbug.com/426647. https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer_unittest.cc:423: void RunTest_OneShotTimerConcurrentResetAndTimerFired( On 2014/12/05 20:13:41, gromer wrote: > Test code tends to work better in the top-level test function than in > subroutines. This function only has one caller, so could you just replace that > function call with the function implementation? The issue is that the same test needs to run on multiple types of MessageLoops, which is why this function takes a base::MessageLoop::Type as a parameter. Moving it into the top-level function would create unnecessary duplication of the code. I suppose this could be turned into a value-parameterized test but I'm not sure how that would work in combination with also being a type-parameterized test. https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer_unittest.cc:437: // being reset at the same time. The previously queued task should be On 2014/12/05 20:13:41, gromer wrote: > Not quite at the same time, right? The test code seems to assume that the reset > is handled before the timer firing. When the timer fires, it queues a task in the MessageLoop that started it. That task will not run until the current function returns. The situation being tested is as follows: - The timer is started on a MessageLoop that is not a MessageLoopForIO. So the timer sets up the system-level timer on a separate thread that does have a MessageLoopForIO. - The requested time elapses and the MessageLoopForIO posts a task back to the MessageLoop that started the timer to run the user task. - At the same time, the original MessageLoop is in the middle of a function that resets the timer. In this case we don't actually want the timer to fire and while we can't recreate the situation we can recreate the end result, which is that a task was posted on the original message loop and the timer was reset, cancelling that task. https://codereview.chromium.org/706993003/diff/20001/components/timers/rtc_al... File components/timers/rtc_alarm.cc (right): https://codereview.chromium.org/706993003/diff/20001/components/timers/rtc_al... components/timers/rtc_alarm.cc:30: base::Thread& operator*() const { return *thread_.get(); } On 2014/12/05 20:13:41, gromer wrote: > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > "Define overloaded operators only if their meaning is obvious, unsurprising, and > consistent with the corresponding built-in operators." > > It's surprising to me that a "helper" has * and -> operators, and it's not > obvious to me what its meaning should be. Could you provide a get() method > instead? This class exists because I needed a thread-safe way to ensure that the dedicated IO thread for the timer is only started once and base::LazyInstance provided a convenient guarantee that the constructor of the parameter class would only be called once. Otherwise I wanted this class to be as transparent as possible, which is why I overloaded these operators. While I admit that using g_io_thread.Get()->SomeFunc() hides some details about what type is actually being returned by the call to g_io_thread.Get(), it's not clear to me that g_io_thread.Get().Get().SomeFunc() is any better. https://codereview.chromium.org/706993003/diff/20001/components/timers/rtc_al... components/timers/rtc_alarm.cc:98: // Make sure that we are running on a MessageLoopForIO. On 2014/12/05 20:13:41, gromer wrote: > This seems misleading- "Make sure X" to me implies that it's some sort of error > if X is not true, but IIUC it's perfectly fine if "we" are running on another > event loop; the code just needs to make sure ResetImpl runs on a > MessageLoopForIO. Maybe "Run ResetImpl on a MessageLoopForIO"? Acknowledged. https://codereview.chromium.org/706993003/diff/20001/components/timers/rtc_al... components/timers/rtc_alarm.cc:109: ResetImpl(delay, origin_event_id_); On 2014/12/05 20:13:41, gromer wrote: > If this were inside an else-block, it would be much clearer that it and the > above if-block are two different ways of accomplishing the same thing. Acknowledged. https://codereview.chromium.org/706993003/diff/20001/components/timers/rtc_al... components/timers/rtc_alarm.cc:179: parent_->OnTimerFired(); On 2014/12/05 20:13:41, gromer wrote: > Isn't there a race condition where parent_ could be destroyed between these two > lines? No. WeakPtrs can only be dereferenced and invalidated on the SequencedTaskRunner on which they were created. A SequencedTaskRunner is essentially a FIFO of tasks. So parent_ could be invalidated before OnTimerFired is called and it could be invalidated after OnTimerFired returns but it could not be invalidated in the middle of OnTimerFired. https://codereview.chromium.org/706993003/diff/20001/components/timers/rtc_al... File components/timers/rtc_alarm.h (right): https://codereview.chromium.org/706993003/diff/20001/components/timers/rtc_al... components/timers/rtc_alarm.h:76: // These two variables are used for coordinating between the thread that On 2014/12/05 20:13:41, gromer wrote: > Comments that say what something is "used for" tend to be unhelpfully vague, > especially for the opening sentence, because they don't tell me specifically how > to use them. Instead, try to talk about what the variable _is_, or what it > _means_. For example, "The ID of the most recent Reset() call handled by > origin_message_loop_" (I have no idea if that's correct, but it's the kind of > comment I'm looking for). > > Relatedly, "event" is a very vague name. Especially in such subtle code, you > should choose variable names that give readers specific cues about the meaning > of the variable. For example, if I'm right that the events here are really > Reset() calls, you could call them origin_reset_event_id_ and io_reset_event_id. > > Finally, the term "sequence number" is often used with this kind of scheme; > consider saying that instead of "event ID". Ack. I'll try and clean up the explanation.
On Tue, Dec 30, 2014 at 4:58 PM, <chirantan@chromium.org> wrote: > Thanks for taking the time to do this review and sorry it took so long for > me to > reply. I'll have a new patch set addressing your comments up this week. > I haven't seen a followup mail about this. Did I miss something? > > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/alarm_timer.cc > File components/timers/alarm_timer.cc (right): > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/alarm_timer.cc#newcode48 > components/timers/alarm_timer.cc:48: base::Timer::set_is_running(false); > On 2014/12/05 20:13:40, gromer wrote: > >> Why not just "set_is_running(false)"? It's necessary when you call >> > Stop() above, > >> but here there's no risk of ambiguity. >> > > Yes it's not really necessary but I find that it improves readability > for me personally because it makes it clear that set_is_running() is a > method provided by the parent class. > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/alarm_timer.cc#newcode103 > components/timers/alarm_timer.cc:103: Stop(); > On 2014/12/05 20:13:40, gromer wrote: > >> Could you avoid all the >> > AddDestructionObserver()/RemoveDestructionObserver() > >> business by making this something like >> > > if (is_running()) { >> Stop(); >> } >> > > I'm not sure what you mean by this. I would still need to call > RemoveDestructionObserver() somehow in the destructor of this class > because otherwise the MessageLoop would hold a pointer to a freed > object. > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/alarm_timer.h > File components/timers/alarm_timer.h (right): > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/alarm_timer.h#newcode28 > components/timers/alarm_timer.h:28: public > base::MessageLoop::DestructionObserver { > On 2014/12/05 20:13:41, gromer wrote: > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/ > styleguide.shtml?cl=head#Multiple_Inheritance > > "Multiple inheritance is allowed only when all superclasses, with the >> > possible > >> exception of the first one, are pure interfaces. In order to ensure >> > that they > >> remain pure interfaces, they must end with the Interface suffix." >> > > One way of fixing this might be to create a helper class which >> > inherits from > >> DestructionObserver, and forwards the calls to the AlarmTimer. >> > > MessageLoop::DestructionObserver _is_ a pure interface so this should be > ok right? > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/alarm_timer.h#newcode34 > components/timers/alarm_timer.h:34: class Delegate : public > base::RefCountedThreadSafe<Delegate> { > On 2014/12/05 20:13:40, gromer wrote: > >> Why is this in the public section? It seems like an implementation >> > detail. > > The Delegate provides all the platform-specific functionality to make > this feature work. It's in the public section so that mac and windows > delegates can be written later if needed. But you're right, this is > currently just an implementation detail and can be moved back into the > public section later if we decide that we actually need this > functionality on platforms other than Chrome OS. > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/alarm_timer.h#newcode34 > components/timers/alarm_timer.h:34: class Delegate : public > base::RefCountedThreadSafe<Delegate> { > On 2014/12/05 20:13:40, gromer wrote: > >> RtcAlarm is the only implementation of Delegate that I can find. If >> > that's the > >> case, why factor out the abstract base class? Could AlarmTimer just >> > use RtcAlarm > >> directly? >> > > As I mentioned above, it's abstracted out to make it easier to add > delegates for Windows and Mac. It's unlikely that this will ever happen > though, so I'll make the RtcAlarm a private nested class. > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/alarm_timer.h#newcode38 > components/timers/alarm_timer.h:38: virtual bool > Init(base::WeakPtr<AlarmTimer> timer) = 0; > On 2014/12/05 20:13:40, gromer wrote: > >> Init() methods are bad news; see http://go/totw:42 for more. Why not >> > just use a > >> constructor for this? >> > > So this Init function exists to break a dependency cycle. The Delegate > needs a base::WeakPtr<AlarmTimer> because it might end up outliving the > AlarmTimer and should not try to call the OnTimerFired() function after > the AlarmTimer has been destroyed. > > WeakPtrs are created using a base::WeakPtrFactory<AlarmTimer>. This > needs to be the final member of the AlarmTimer class so that it gets > destroyed before any other member, invalidating any existing WeakPtrs > that those members might have to the AlarmTimer. > > Since the WeakPtrFactory has to be the final member, the Delegate > appears before it in the initialization list and the WeakPtrFactory > cannot be used at that time to create a WeakPtr that can be passed to > the constructor for the Delegate. > > Using raw pointers is out because then we would still need some way to > invalidate it in the AlarmTimer destructor to prevent a use-after-free. > I suppose the simplest way to get rid of the Init function would be to > construct the Delegate in the body of the AlarmTimer constructor instead > of the initialization list. That seems straightforward enough so I > think I'll do that. > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/alarm_timer.h#newcode52 > components/timers/alarm_timer.h:52: friend class > base::RefCountedThreadSafe<Delegate>; > On 2014/12/05 20:13:41, gromer wrote: > >> Why is this needed? >> > > It's recommended by the docstring for base::RefCountedThreadSafe: > > // If you're using the default trait, then you should add compile time > // asserts that no one else is deleting your object. i.e. > // private: > // friend class base::RefCountedThreadSafe<MyFoo>; > // ~MyFoo(); > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/alarm_timer.h#newcode55 > components/timers/alarm_timer.h:55: AlarmTimer(bool retain_user_task, > bool is_repeating); > On 2014/12/05 20:13:41, gromer wrote: > >> bool parameters are a readability hazard, especially when you have >> > more than > >> one. The issue is that a callsite like >> > > AlarmTimer my_timer(true, false); >> > > gives the user almost no clues about the meanings of those arguments. >> > Even if > >> you know that one controls retention of the user task, and the other >> > controls > >> repetition, you may not know which is which, and you may not know e.g. >> > whether > >> "true" means that the timer is single-use or that it's repeating. >> > > > I agree wholeheartedly. I was already bit by this once when refactoring > some code to use this timer. This class is meant to be a drop in > replacement for base::Timer, which is why I made these constructors > mirror those of base::Timer. It's probably not necessary for the > constructors to match though so I will clean this up. > > > See also the "Function Argument Comments" section of >> > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/ > styleguide.shtml?cl=head#Implementation_Comments > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/alarm_timer.h#newcode64 > components/timers/alarm_timer.h:64: bool can_wake_from_suspend() { > return can_wake_from_suspend_; } > On 2014/12/05 20:13:40, gromer wrote: > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/ > styleguide.shtml?cl=head#Use_of_const > > "Use const whenever it makes sense." >> > > This method looks like it should be const. >> > > Done. This was cleaned up in a separate CL. > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/alarm_timer.h#newcode86 > components/timers/alarm_timer.h:86: scoped_ptr<base::PendingTask> > pending_task_; > On 2014/12/05 20:13:40, gromer wrote: > >> Why not unique_ptr? >> > > As Lei mentioned, chromium doesn't use unique_ptr yet. However, > chromium's scoped_ptr provides move semantics so it is essentially > equivalent to a unique_ptr. > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/alarm_timer_unittest.cc > File components/timers/alarm_timer_unittest.cc (right): > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/alarm_timer_unittest.cc#newcode18 > components/timers/alarm_timer_unittest.cc:18: // regular Timer so it > should pass the same tests as the Timer class. > On 2014/12/05 20:13:41, gromer wrote: > >> GUnit has mechanisms for type-parameterized tests, so you can run the >> > same test > >> code on different types. Could you use that to avoid duplicating all >> > this code > >> from the base::Timer tests? >> > > Yes, there is a bug open for me to fix this: crbug.com/426647. > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/alarm_timer_unittest.cc#newcode423 > components/timers/alarm_timer_unittest.cc:423: void > RunTest_OneShotTimerConcurrentResetAndTimerFired( > On 2014/12/05 20:13:41, gromer wrote: > >> Test code tends to work better in the top-level test function than in >> subroutines. This function only has one caller, so could you just >> > replace that > >> function call with the function implementation? >> > > The issue is that the same test needs to run on multiple types of > MessageLoops, which is why this function takes a base::MessageLoop::Type > as a parameter. Moving it into the top-level function would create > unnecessary duplication of the code. I suppose this could be turned > into a value-parameterized test but I'm not sure how that would work in > combination with also being a type-parameterized test. > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/alarm_timer_unittest.cc#newcode437 > components/timers/alarm_timer_unittest.cc:437: // being reset at the > same time. The previously queued task should be > On 2014/12/05 20:13:41, gromer wrote: > >> Not quite at the same time, right? The test code seems to assume that >> > the reset > >> is handled before the timer firing. >> > > When the timer fires, it queues a task in the MessageLoop that started > it. That task will not run until the current function returns. The > situation being tested is as follows: > > - The timer is started on a MessageLoop that is not a MessageLoopForIO. > So the timer sets up the system-level timer on a separate thread that > does have a MessageLoopForIO. > - The requested time elapses and the MessageLoopForIO posts a task back > to the MessageLoop that started the timer to run the user task. > - At the same time, the original MessageLoop is in the middle of a > function that resets the timer. > > In this case we don't actually want the timer to fire and while we can't > recreate the situation we can recreate the end result, which is that a > task was posted on the original message loop and the timer was reset, > cancelling that task. > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/rtc_alarm.cc > File components/timers/rtc_alarm.cc (right): > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/rtc_alarm.cc#newcode30 > components/timers/rtc_alarm.cc:30: base::Thread& operator*() const { > return *thread_.get(); } > On 2014/12/05 20:13:41, gromer wrote: > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/ > styleguide.shtml?cl=head#Operator_Overloading > > "Define overloaded operators only if their meaning is obvious, >> > unsurprising, and > >> consistent with the corresponding built-in operators." >> > > It's surprising to me that a "helper" has * and -> operators, and it's >> > not > >> obvious to me what its meaning should be. Could you provide a get() >> > method > >> instead? >> > > This class exists because I needed a thread-safe way to ensure that the > dedicated IO thread for the timer is only started once and > base::LazyInstance provided a convenient guarantee that the constructor > of the parameter class would only be called once. > > Otherwise I wanted this class to be as transparent as possible, which is > why I overloaded these operators. While I admit that using > g_io_thread.Get()->SomeFunc() hides some details about what type is > actually being returned by the call to g_io_thread.Get(), it's not clear > to me that g_io_thread.Get().Get().SomeFunc() is any better. > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/rtc_alarm.cc#newcode98 > components/timers/rtc_alarm.cc:98: // Make sure that we are running on a > MessageLoopForIO. > On 2014/12/05 20:13:41, gromer wrote: > >> This seems misleading- "Make sure X" to me implies that it's some sort >> > of error > >> if X is not true, but IIUC it's perfectly fine if "we" are running on >> > another > >> event loop; the code just needs to make sure ResetImpl runs on a >> MessageLoopForIO. Maybe "Run ResetImpl on a MessageLoopForIO"? >> > > Acknowledged. > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/rtc_alarm.cc#newcode109 > components/timers/rtc_alarm.cc:109: ResetImpl(delay, origin_event_id_); > On 2014/12/05 20:13:41, gromer wrote: > >> If this were inside an else-block, it would be much clearer that it >> > and the > >> above if-block are two different ways of accomplishing the same thing. >> > > Acknowledged. > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/rtc_alarm.cc#newcode179 > components/timers/rtc_alarm.cc:179: parent_->OnTimerFired(); > On 2014/12/05 20:13:41, gromer wrote: > >> Isn't there a race condition where parent_ could be destroyed between >> > these two > >> lines? >> > > No. WeakPtrs can only be dereferenced and invalidated on the > SequencedTaskRunner on which they were created. A SequencedTaskRunner > is essentially a FIFO of tasks. So parent_ could be invalidated before > OnTimerFired is called and it could be invalidated after OnTimerFired > returns but it could not be invalidated in the middle of OnTimerFired. > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/rtc_alarm.h > File components/timers/rtc_alarm.h (right): > > https://codereview.chromium.org/706993003/diff/20001/ > components/timers/rtc_alarm.h#newcode76 > components/timers/rtc_alarm.h:76: // These two variables are used for > coordinating between the thread that > On 2014/12/05 20:13:41, gromer wrote: > >> Comments that say what something is "used for" tend to be unhelpfully >> > vague, > >> especially for the opening sentence, because they don't tell me >> > specifically how > >> to use them. Instead, try to talk about what the variable _is_, or >> > what it > >> _means_. For example, "The ID of the most recent Reset() call handled >> > by > >> origin_message_loop_" (I have no idea if that's correct, but it's the >> > kind of > >> comment I'm looking for). >> > > Relatedly, "event" is a very vague name. Especially in such subtle >> > code, you > >> should choose variable names that give readers specific cues about the >> > meaning > >> of the variable. For example, if I'm right that the events here are >> > really > >> Reset() calls, you could call them origin_reset_event_id_ and >> > io_reset_event_id. > > Finally, the term "sequence number" is often used with this kind of >> > scheme; > >> consider saying that instead of "event ID". >> > > Ack. I'll try and clean up the explanation. > > https://codereview.chromium.org/706993003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
chirantan@chromium.org changed reviewers: + zea@chromium.org
Sorry, I got wrapped up in the upcoming launch of my current project and this kind of got pushed to the back of my todo list. Please take another look.
https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... File components/timers/alarm_timer.cc (right): https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.cc:103: Stop(); On 2014/12/31 00:58:29, Chirantan Ekbote wrote: > On 2014/12/05 20:13:40, gromer wrote: > > Could you avoid all the AddDestructionObserver()/RemoveDestructionObserver() > > business by making this something like > > > > if (is_running()) { > > Stop(); > > } > > I'm not sure what you mean by this. I would still need to call > RemoveDestructionObserver() somehow in the destructor of this class because > otherwise the MessageLoop would hold a pointer to a freed object. Yeah, I'm not sure what I meant by this either. https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... File components/timers/alarm_timer.h (right): https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:28: public base::MessageLoop::DestructionObserver { On 2014/12/31 00:58:29, Chirantan Ekbote wrote: > On 2014/12/05 20:13:41, gromer wrote: > > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > > > "Multiple inheritance is allowed only when all superclasses, with the possible > > exception of the first one, are pure interfaces. In order to ensure that they > > remain pure interfaces, they must end with the Interface suffix." > > > > One way of fixing this might be to create a helper class which inherits from > > DestructionObserver, and forwards the calls to the AlarmTimer. > > MessageLoop::DestructionObserver _is_ a pure interface so this should be ok > right? "In order to ensure that they remain pure interfaces, they must end with the Interface suffix." https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... File components/timers/alarm_timer_unittest.cc (right): https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer_unittest.cc:423: void RunTest_OneShotTimerConcurrentResetAndTimerFired( On 2014/12/31 00:58:29, Chirantan Ekbote wrote: > On 2014/12/05 20:13:41, gromer wrote: > > Test code tends to work better in the top-level test function than in > > subroutines. This function only has one caller, so could you just replace that > > function call with the function implementation? > > The issue is that the same test needs to run on multiple types of MessageLoops, > which is why this function takes a base::MessageLoop::Type as a parameter. > Moving it into the top-level function would create unnecessary duplication of > the code. I suppose this could be turned into a value-parameterized test but > I'm not sure how that would work in combination with also being a > type-parameterized test. I don't understand- this function only has one caller, so where would the code duplication come from? https://codereview.chromium.org/706993003/diff/20001/components/timers/rtc_al... File components/timers/rtc_alarm.cc (right): https://codereview.chromium.org/706993003/diff/20001/components/timers/rtc_al... components/timers/rtc_alarm.cc:30: base::Thread& operator*() const { return *thread_.get(); } On 2014/12/31 00:58:29, Chirantan Ekbote wrote: > On 2014/12/05 20:13:41, gromer wrote: > > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > > > "Define overloaded operators only if their meaning is obvious, unsurprising, > and > > consistent with the corresponding built-in operators." > > > > It's surprising to me that a "helper" has * and -> operators, and it's not > > obvious to me what its meaning should be. Could you provide a get() method > > instead? > > This class exists because I needed a thread-safe way to ensure that the > dedicated IO thread for the timer is only started once and base::LazyInstance > provided a convenient guarantee that the constructor of the parameter class > would only be called once. > > Otherwise I wanted this class to be as transparent as possible, which is why I > overloaded these operators. While I admit that using > g_io_thread.Get()->SomeFunc() hides some details about what type is actually > being returned by the call to g_io_thread.Get(), it's not clear to me that > g_io_thread.Get().Get().SomeFunc() is any better. You seem to have implemented exactly the fix I was going to suggest, namely make this class inherit from Thread. I don't often recommend inheritance in place of composition, but here it seems like the right choice. https://codereview.chromium.org/706993003/diff/60001/components/timers/alarm_... File components/timers/alarm_timer.cc (right): https://codereview.chromium.org/706993003/diff/60001/components/timers/alarm_... components/timers/alarm_timer.cc:64: void Reset(base::TimeDelta delay) { https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... "Define functions inline only when they are small, say, 10 lines or less." It's not entirely clear whether this style rule is intended to cover member functions that are defined inside the class (which are technically "inline" even though they don't use the keyword), but putting such large function definitions inside the class body also hurts readability, since it's harder to get a high-level overview of the class. So I recommend moving these large method definitions out of the class body. https://codereview.chromium.org/706993003/diff/60001/components/timers/alarm_... components/timers/alarm_timer.cc:210: // The sequence number of the last Reset() call handled by the origin thread I have to read a long way into this sentence to figure out that it's talking about both members. Say "numbers" rather than "number", and consider moving "respectively" to the beginning of the sentence. https://codereview.chromium.org/706993003/diff/60001/components/timers/alarm_... File components/timers/alarm_timer.h (right): https://codereview.chromium.org/706993003/diff/60001/components/timers/alarm_... components/timers/alarm_timer.h:34: // Must be called by the delegate to indicate that the timer has fired and If only the delegate needs to call it, could it be moved to the private section?
ptal https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... File components/timers/alarm_timer.h (right): https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer.h:28: public base::MessageLoop::DestructionObserver { On 2015/02/10 23:47:10, gromer wrote: > On 2014/12/31 00:58:29, Chirantan Ekbote wrote: > > On 2014/12/05 20:13:41, gromer wrote: > > > > > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > > > > > "Multiple inheritance is allowed only when all superclasses, with the > possible > > > exception of the first one, are pure interfaces. In order to ensure that > they > > > remain pure interfaces, they must end with the Interface suffix." > > > > > > One way of fixing this might be to create a helper class which inherits from > > > DestructionObserver, and forwards the calls to the AlarmTimer. > > > > MessageLoop::DestructionObserver _is_ a pure interface so this should be ok > > right? > > "In order to ensure that they remain pure interfaces, they must end with the > Interface suffix." I had no say when this interface was created and it appears that a lot of code in chrome ignores this particular rule of the style guide. Regardless, I've moved this into a separate class. https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... File components/timers/alarm_timer_unittest.cc (right): https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer_unittest.cc:423: void RunTest_OneShotTimerConcurrentResetAndTimerFired( On 2015/02/10 23:47:10, gromer wrote: > On 2014/12/31 00:58:29, Chirantan Ekbote wrote: > > On 2014/12/05 20:13:41, gromer wrote: > > > Test code tends to work better in the top-level test function than in > > > subroutines. This function only has one caller, so could you just replace > that > > > function call with the function implementation? > > > > The issue is that the same test needs to run on multiple types of > MessageLoops, > > which is why this function takes a base::MessageLoop::Type as a parameter. > > Moving it into the top-level function would create unnecessary duplication of > > the code. I suppose this could be turned into a value-parameterized test but > > I'm not sure how that would work in combination with also being a > > type-parameterized test. > > I don't understand- this function only has one caller, so where would the code > duplication come from? That caller calls it multiple times with a different type of message loop each time. If we were to move each separate invocation into a top-level function, the only thing that would change in the code is |message_loop_type|. I already have some ideas on how I can turn this whole thing into a value-parameterized test so this shouldn't be an issue for much longer. https://codereview.chromium.org/706993003/diff/60001/components/timers/alarm_... File components/timers/alarm_timer.cc (right): https://codereview.chromium.org/706993003/diff/60001/components/timers/alarm_... components/timers/alarm_timer.cc:64: void Reset(base::TimeDelta delay) { On 2015/02/10 23:47:10, gromer wrote: > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > "Define functions inline only when they are small, say, 10 lines or less." > > It's not entirely clear whether this style rule is intended to cover member > functions that are defined inside the class (which are technically "inline" even > though they don't use the keyword), but putting such large function definitions > inside the class body also hurts readability, since it's harder to get a > high-level overview of the class. So I recommend moving these large method > definitions out of the class body. Done. https://codereview.chromium.org/706993003/diff/60001/components/timers/alarm_... components/timers/alarm_timer.cc:210: // The sequence number of the last Reset() call handled by the origin thread On 2015/02/10 23:47:10, gromer wrote: > I have to read a long way into this sentence to figure out that it's talking > about both members. Say "numbers" rather than "number", and consider moving > "respectively" to the beginning of the sentence. Done. https://codereview.chromium.org/706993003/diff/60001/components/timers/alarm_... File components/timers/alarm_timer.h (right): https://codereview.chromium.org/706993003/diff/60001/components/timers/alarm_... components/timers/alarm_timer.h:34: // Must be called by the delegate to indicate that the timer has fired and On 2015/02/10 23:47:10, gromer wrote: > If only the delegate needs to call it, could it be moved to the private section? Done.
https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... File components/timers/alarm_timer_unittest.cc (right): https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer_unittest.cc:423: void RunTest_OneShotTimerConcurrentResetAndTimerFired( On 2015/02/12 01:49:58, Chirantan Ekbote wrote: > On 2015/02/10 23:47:10, gromer wrote: > > On 2014/12/31 00:58:29, Chirantan Ekbote wrote: > > > On 2014/12/05 20:13:41, gromer wrote: > > > > Test code tends to work better in the top-level test function than in > > > > subroutines. This function only has one caller, so could you just replace > > that > > > > function call with the function implementation? > > > > > > The issue is that the same test needs to run on multiple types of > > MessageLoops, > > > which is why this function takes a base::MessageLoop::Type as a parameter. > > > Moving it into the top-level function would create unnecessary duplication > of > > > the code. I suppose this could be turned into a value-parameterized test > but > > > I'm not sure how that would work in combination with also being a > > > type-parameterized test. > > > > I don't understand- this function only has one caller, so where would the code > > duplication come from? > > That caller calls it multiple times with a different type of message loop each > time. If we were to move each separate invocation into a top-level function, > the only thing that would change in the code is |message_loop_type|. It's called multiple times, but inside a loop; there's still only one place in the _source code_ where it's called, so there would be no code duplication. I'm just saying put the body of this function inside the loop, instead of the function call. > > I already have some ideas on how I can turn this whole thing into a > value-parameterized test so this shouldn't be an issue for much longer. https://codereview.chromium.org/706993003/diff/100001/components/timers/alarm... File components/timers/alarm_timer.cc (right): https://codereview.chromium.org/706993003/diff/100001/components/timers/alarm... components/timers/alarm_timer.cc:100: bool CanWakeFromSuspend(); FWIW, I think this would be a reasonable method to keep inline. Either way is fine, though.
PTAL. Sorry this took a little long. I thought this would be a good opportunity to consolidate the unit tests with the ones in base/timer/ before uploading a new patch set but it turns out that base::Timer is quite broken now. See http://crbug.com/461626 if you want the depressing details. https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... File components/timers/alarm_timer_unittest.cc (right): https://codereview.chromium.org/706993003/diff/20001/components/timers/alarm_... components/timers/alarm_timer_unittest.cc:423: void RunTest_OneShotTimerConcurrentResetAndTimerFired( On 2015/02/17 22:02:08, gromer wrote: > On 2015/02/12 01:49:58, Chirantan Ekbote wrote: > > On 2015/02/10 23:47:10, gromer wrote: > > > On 2014/12/31 00:58:29, Chirantan Ekbote wrote: > > > > On 2014/12/05 20:13:41, gromer wrote: > > > > > Test code tends to work better in the top-level test function than in > > > > > subroutines. This function only has one caller, so could you just > replace > > > that > > > > > function call with the function implementation? > > > > > > > > The issue is that the same test needs to run on multiple types of > > > MessageLoops, > > > > which is why this function takes a base::MessageLoop::Type as a parameter. > > > > > Moving it into the top-level function would create unnecessary duplication > > of > > > > the code. I suppose this could be turned into a value-parameterized test > > but > > > > I'm not sure how that would work in combination with also being a > > > > type-parameterized test. > > > > > > I don't understand- this function only has one caller, so where would the > code > > > duplication come from? > > > > That caller calls it multiple times with a different type of message loop each > > time. If we were to move each separate invocation into a top-level function, > > the only thing that would change in the code is |message_loop_type|. > > It's called multiple times, but inside a loop; there's still only one place in > the _source code_ where it's called, so there would be no code duplication. I'm > just saying put the body of this function inside the loop, instead of the > function call. > I see. Sorry, I misunderstood what you were saying. This is done now.
lgtm OK, this looks good. Congratulations on achieving C++ readability! Please use your new power wisely. The next step is to get someone on your team to review this CL (preferably the original reviewer of this code), and then check it in.
On 2015/03/06 20:37:36, gromer wrote: > lgtm > > OK, this looks good. Congratulations on achieving C++ readability! Please use > your new power wisely. > > The next step is to get someone on your team to review this CL (preferably the > original reviewer of this code), and then check it in. Awesome! Thanks for your time and patience :-) Dan, could you please review components/timers? Nicolas, could you please review componests/gcm_driver?
https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... File components/timers/alarm_timer.cc (right): https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... components/timers/alarm_timer.cc:7: #include <sys/timerfd.h> trying to remember the background here. this is a linux-only header. can you rename this to alarm_timer_linux.cc so it's automatically excluded on other platforms? https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... components/timers/alarm_timer.cc:153: : alarm_fd_(timerfd_create(CLOCK_REALTIME_ALARM, 0)), move this into the c'tor body so you can PLOG(ERROR) if it fails? https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... components/timers/alarm_timer.cc:218: timerfd_settime(alarm_fd_, 0, &blank_time, NULL); check for -1 return value and PLOG(ERROR) https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... components/timers/alarm_timer.cc:226: base::ReadFromFD(alarm_fd_, val, sizeof(uint64_t)); check return value https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... File components/timers/alarm_timer.h (right): https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... components/timers/alarm_timer.h:105: RepeatingAlarmTimer() : AlarmTimer(true, true) {} i think that you need to define these c'tors (and a d'tor) in the .cc file instead of inlining them here: http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in.... https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... File components/timers/alarm_timer_unittest.cc (right): https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... components/timers/alarm_timer_unittest.cc:424: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(20)); code like this makes me uneasy. it's both potentially-flaky and slower than it needs to be. can you instead increase the delay to something like 10 seconds but also stop the message loop as soon as the timer runs? https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... components/timers/alarm_timer_unittest.cc:455: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(35)); same here
ptal https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... File components/timers/alarm_timer.cc (right): https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... components/timers/alarm_timer.cc:7: #include <sys/timerfd.h> On 2015/03/09 16:09:47, Daniel Erat wrote: > trying to remember the background here. this is a linux-only header. can you > rename this to alarm_timer_linux.cc so it's automatically excluded on other > platforms? The timers build target is only available on chrome os builds so it shouldn't be included on any other platforms. Do you feel its necessary to change the name as well? https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... components/timers/alarm_timer.cc:153: : alarm_fd_(timerfd_create(CLOCK_REALTIME_ALARM, 0)), On 2015/03/09 16:09:47, Daniel Erat wrote: > move this into the c'tor body so you can PLOG(ERROR) if it fails? This timer only exists on linux kernel versions 3.11 and higher. It's expected that this call will fail on older kernels and is used as a runtime check to determine whether the platform supports the AlarmTimer. I don't think logging an error here will provide a meaningful signal. https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... components/timers/alarm_timer.cc:218: timerfd_settime(alarm_fd_, 0, &blank_time, NULL); On 2015/03/09 16:09:47, Daniel Erat wrote: > check for -1 return value and PLOG(ERROR) Done. https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... components/timers/alarm_timer.cc:226: base::ReadFromFD(alarm_fd_, val, sizeof(uint64_t)); On 2015/03/09 16:09:47, Daniel Erat wrote: > check return value Done. https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... File components/timers/alarm_timer.h (right): https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... components/timers/alarm_timer.h:105: RepeatingAlarmTimer() : AlarmTimer(true, true) {} On 2015/03/09 16:09:47, Daniel Erat wrote: > i think that you need to define these c'tors (and a d'tor) in the .cc file > instead of inlining them here: > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in.... Done. https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... File components/timers/alarm_timer_unittest.cc (right): https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... components/timers/alarm_timer_unittest.cc:424: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(20)); On 2015/03/09 16:09:47, Daniel Erat wrote: > code like this makes me uneasy. it's both potentially-flaky and slower than it > needs to be. can you instead increase the delay to something like 10 seconds but > also stop the message loop as soon as the timer runs? In this test, and the one below, we don't actually want the timer to fire. Here is the situation it is trying to test: base::MessageLoopForUI base::MessageLoopForIO T1 | | T2 | | T3 | | T4 AlarmTimer::Reset Delegate::OnFileCanReadWithoutBlocking T5 | Delegate::Reset T6 AlarmTimer::OnTimerFired | While it's hard to reproduce this situation, we know what the state will be after T4: AlarmTimer::OnTimerFired will be queued in the base::MessageLoopForUI even though the timer has just been reset. That's what the Sleep() call is waiting for. The call to RunLoop::Run() in line 437 waits for the timer to fire after the reset and stops the message loop once it does.
gcm LGTM
generally lgtm with a few comments. if there are more questions i might not be able to take a look until next thursday. https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... File components/timers/alarm_timer.cc (right): https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... components/timers/alarm_timer.cc:7: #include <sys/timerfd.h> On 2015/03/13 20:43:43, Chirantan Ekbote wrote: > On 2015/03/09 16:09:47, Daniel Erat wrote: > > trying to remember the background here. this is a linux-only header. can you > > rename this to alarm_timer_linux.cc so it's automatically excluded on other > > platforms? > > The timers build target is only available on chrome os builds so it shouldn't be > included on any other platforms. Do you feel its necessary to change the name > as well? i think it'd probably be a bit better, as a _chromeos suffix would make it obvious to people that it's not built on other platforms even before they open the file or look at build rules. https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... components/timers/alarm_timer.cc:153: : alarm_fd_(timerfd_create(CLOCK_REALTIME_ALARM, 0)), On 2015/03/13 20:43:43, Chirantan Ekbote wrote: > On 2015/03/09 16:09:47, Daniel Erat wrote: > > move this into the c'tor body so you can PLOG(ERROR) if it fails? > > This timer only exists on linux kernel versions 3.11 and higher. It's expected > that this call will fail on older kernels and is used as a runtime check to > determine whether the platform supports the AlarmTimer. I don't think logging > an error here will provide a meaningful signal. sounds fine, then. add a short comment here explaining that? https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... File components/timers/alarm_timer_unittest.cc (right): https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... components/timers/alarm_timer_unittest.cc:424: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(20)); On 2015/03/13 20:43:44, Chirantan Ekbote wrote: > On 2015/03/09 16:09:47, Daniel Erat wrote: > > code like this makes me uneasy. it's both potentially-flaky and slower than it > > needs to be. can you instead increase the delay to something like 10 seconds > but > > also stop the message loop as soon as the timer runs? > > In this test, and the one below, we don't actually want the timer to fire. Here > is the situation it is trying to test: > > base::MessageLoopForUI base::MessageLoopForIO > T1 | | > T2 | | > T3 | | > T4 AlarmTimer::Reset Delegate::OnFileCanReadWithoutBlocking > T5 | Delegate::Reset > T6 AlarmTimer::OnTimerFired | > > While it's hard to reproduce this situation, we know what the state will be > after T4: AlarmTimer::OnTimerFired will be queued in the base::MessageLoopForUI > even though the timer has just been reset. That's what the Sleep() call is > waiting for. The call to RunLoop::Run() in line 437 waits for the timer to fire > after the reset and stops the message loop once it does. thanks for the explanation. could you instead add a testing-only hook to the class to register a callback that gets invoked when the timer's task is queued? then you could stop the event loop there, and you'd know that the test is both a) waiting long enough for the task to be queued, and b) not waiting any longer than it needs to. https://codereview.chromium.org/706993003/diff/160001/components/timers/alarm... File components/timers/alarm_timer.cc (right): https://codereview.chromium.org/706993003/diff/160001/components/timers/alarm... components/timers/alarm_timer.cc:228: DCHECK(result) << "Unable to read from timer file descriptor."; PLOG(DFATAL) is probably better here (FATAL in debug builds, ERROR otherwise)
https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... File components/timers/alarm_timer.cc (right): https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... components/timers/alarm_timer.cc:7: #include <sys/timerfd.h> On 2015/03/14 14:02:33, Daniel Erat wrote: > On 2015/03/13 20:43:43, Chirantan Ekbote wrote: > > On 2015/03/09 16:09:47, Daniel Erat wrote: > > > trying to remember the background here. this is a linux-only header. can you > > > rename this to alarm_timer_linux.cc so it's automatically excluded on other > > > platforms? > > > > The timers build target is only available on chrome os builds so it shouldn't > be > > included on any other platforms. Do you feel its necessary to change the name > > as well? > > i think it'd probably be a bit better, as a _chromeos suffix would make it > obvious to people that it's not built on other platforms even before they open > the file or look at build rules. Done. https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... components/timers/alarm_timer.cc:153: : alarm_fd_(timerfd_create(CLOCK_REALTIME_ALARM, 0)), On 2015/03/14 14:02:33, Daniel Erat (OOO to Thursday) wrote: > On 2015/03/13 20:43:43, Chirantan Ekbote wrote: > > On 2015/03/09 16:09:47, Daniel Erat wrote: > > > move this into the c'tor body so you can PLOG(ERROR) if it fails? > > > > This timer only exists on linux kernel versions 3.11 and higher. It's > expected > > that this call will fail on older kernels and is used as a runtime check to > > determine whether the platform supports the AlarmTimer. I don't think logging > > an error here will provide a meaningful signal. > > sounds fine, then. add a short comment here explaining that? Done. I also added a DPLOG statement. https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... File components/timers/alarm_timer_unittest.cc (right): https://codereview.chromium.org/706993003/diff/140001/components/timers/alarm... components/timers/alarm_timer_unittest.cc:424: base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(20)); On 2015/03/14 14:02:33, Daniel Erat wrote: > On 2015/03/13 20:43:44, Chirantan Ekbote wrote: > > On 2015/03/09 16:09:47, Daniel Erat wrote: > > > code like this makes me uneasy. it's both potentially-flaky and slower than > it > > > needs to be. can you instead increase the delay to something like 10 seconds > > but > > > also stop the message loop as soon as the timer runs? > > > > In this test, and the one below, we don't actually want the timer to fire. > Here > > is the situation it is trying to test: > > > > base::MessageLoopForUI base::MessageLoopForIO > > T1 | | > > T2 | | > > T3 | | > > T4 AlarmTimer::Reset Delegate::OnFileCanReadWithoutBlocking > > T5 | Delegate::Reset > > T6 AlarmTimer::OnTimerFired | > > > > While it's hard to reproduce this situation, we know what the state will be > > after T4: AlarmTimer::OnTimerFired will be queued in the > base::MessageLoopForUI > > even though the timer has just been reset. That's what the Sleep() call is > > waiting for. The call to RunLoop::Run() in line 437 waits for the timer to > fire > > after the reset and stops the message loop once it does. > > thanks for the explanation. could you instead add a testing-only hook to the > class to register a callback that gets invoked when the timer's task is queued? > then you could stop the event loop there, and you'd know that the test is both > a) waiting long enough for the task to be queued, and b) not waiting any longer > than it needs to. Done. This actually took a little long because my new test uncovered a bug in MessagePumpLibevent so I had to fix that first before I could upload a new patch set. https://codereview.chromium.org/706993003/diff/160001/components/timers/alarm... File components/timers/alarm_timer.cc (right): https://codereview.chromium.org/706993003/diff/160001/components/timers/alarm... components/timers/alarm_timer.cc:228: DCHECK(result) << "Unable to read from timer file descriptor."; On 2015/03/14 14:02:33, Daniel Erat (OOO to Thursday) wrote: > PLOG(DFATAL) is probably better here (FATAL in debug builds, ERROR otherwise) Done.
The CQ bit was checked by chirantan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gromer@google.com, derat@chromium.org, zea@chromium.org Link to the patchset: https://codereview.chromium.org/706993003/#ps180001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/706993003/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/05e4d854dca737cfb43cd967cd72d024f6b1625b Cr-Commit-Position: refs/heads/master@{#324004}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/1058693003/ by falken@chromium.org. The reason for reverting is: It looks like this broke tests the following tests on Linux Chromium OS: AlarmTimerTest.OneShotTimerConcurrentResetAndTimerFired AlarmTimerTest.RepeatingTimerConcurrentResetAndTimerFired Example output: AlarmTimerTest.OneShotTimerConcurrentResetAndTimerFired (run #1): [ RUN ] AlarmTimerTest.OneShotTimerConcurrentResetAndTimerFired [11334:11334:0406/201742:404219967:INFO:alarm_timer_chromeos.cc(166)] CLOCK_REALTIME_ALARM not supported on this system: Invalid argument [11334:11334:0406/201742:404230154:FATAL:timer.cc(116)] Check failed: !user_task_.is_null(). AlarmTimerTest.RepeatingTimerConcurrentResetAndTimerFired (run #1): [ RUN ] AlarmTimerTest.RepeatingTimerConcurrentResetAndTimerFired [11382:11382:0406/201748:411022458:INFO:alarm_timer_chromeos.cc(166)] CLOCK_REALTIME_ALARM not supported on this system: Invalid argument ../../components/timers/alarm_timer_unittest.cc:469: Failure Value of: did_run Actual: true Expected: false Example build: http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
