Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
719983002:
Reporting of policy errors via host-offline-reason: part 3 (Closed)
|
Created:
6 years, 1 month ago by Łukasz Anforowicz Modified:
5 years, 11 months ago CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@hor-nohoststatussender Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReporting of policy errors via host-offline-reason: part 3
Introducing HostSignalingManager for extending host process's
lifetime while we wait for an ack to host-offline-reason heartbeat.
- HostSignalingManager is needed primarily to control process
lifetime by holding a ref-count to network_task_runner via a field
of HostSignalingManager - network_task_runner (via its
stop/join) callback) keeps a ref-count to the main thread
(ui_task_runner). And main thread / ui_task_runner will keep the
whole process running until its refcount drops to zero.
- Introducing HostSignalingManager also helps take multiple
fields that used to be held by HostProcess and embed/hide those
fields/objects inside HostSignalingManager
- Lifetime of HostSignalingManager is temporarily extended while
we are waiting for a timeout or ack to host-offline-reason
heartbeat.
BUG=410050
Committed: https://crrev.com/806efecb817d9a4fd9dfe460b5ce7d132ace615d
Cr-Commit-Position: refs/heads/master@{#311077}
Patch Set 1 #
Total comments: 24
Patch Set 2 : Addressed CR feedback + extracted ReportAckOrTimeout function. #Patch Set 3 : Switched to base::Unretained in mock_callback.h #Patch Set 4 : Minor tweaks after end-to-end tests and self-review. #
Total comments: 9
Patch Set 5 : Rebasing... #
Total comments: 23
Patch Set 6 : Addressed code review feedback from Lambros. #Patch Set 7 : Rebasing... #
Total comments: 7
Patch Set 8 : Better MockCallback documentation and use-after-death. #Patch Set 9 : git cl format #
Total comments: 15
Patch Set 10 : Addressed (some) code review feedback from Wez. #Patch Set 11 : Trying to see how things look without mock_callback.h #
Total comments: 35
Patch Set 12 : Addressed code review feedback from Lambros. #
Total comments: 47
Patch Set 13 : Addressed (some) code review feedback from Wez. #Patch Set 14 : Added a unit test for the case of a synchronous ack. #Patch Set 15 : Extracted kHostOfflineReasonTimeoutSeconds constant. #
Total comments: 18
Patch Set 16 : Removed ReportAckOrTimeout and ref-counting. Renamed MinimumHeartbeatSupporter to HostSignalingMan… #
Total comments: 37
Patch Set 17 : Addressed code review feedback from Lambros. #Patch Set 18 : Better callback currying. #
Total comments: 104
Patch Set 19 : Addressed code review feedback from Lambros. #Patch Set 20 : Addressed code review feedback from Wez. #
Total comments: 15
Patch Set 21 : Addressed most feedback from Wez. #
Total comments: 14
Patch Set 22 : Small formatting and commenting fixes. #Patch Set 23 : Rebasing... #
Total comments: 22
Patch Set 24 : Addressed code review feedback from Wez. #Patch Set 25 : Replaced callbacks with a listener interface in HostSignalingManager. #
Total comments: 11
Patch Set 26 : Addressed code review feedback from Wez. #Patch Set 27 : Rebasing... #Messages
Total messages: 57 (6 generated)
lukasza@chromium.org changed reviewers: + lambroslambrou@chromium.org
Lambros, could you please review the 3 changelists for reporting policy errors via host-offline-reason? This is part 3. Thanks, Lukasz
https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... File remoting/host/minimum_heartbeat_supporter.cc (right): https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2014, no '(c)' https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.cc:23: /* TODO(lukasza): code review please: I heard that when passing Remove TODO https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.cc:27: * Should I repent? */ I don't feel strongly. Passing by const reference is better I think. At the call-site, you would pass in *context instead of context.get() scoped_ptr::operator* will assert() if the object is null. https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.cc:44: net::NetworkChangeNotifier::Create()); nit: Fits on one line. https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.cc:73: oauth_credentials.Pass(), nit: Indentation should be +4, not +2. https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.cc:91: // TODO(lukasza): code review please: anything we can do here? Remove TODO. https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.cc:93: // are not used by frames higher on the callstack Are these fields exposed to things higher up in the call stack? Why is this class owning them? This goes back to my earlier question about who should own the things? Either: This object should be the owner, and should say to callers "you can look at my fields, but don't use them beyond my lifetime". Or: Something else should be the owner, and this class should respect lifetime requirements imposed by the owner. https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.cc:194: this->AddRef(); This is smelly! :) Is there some nicer way we can add a reference to |this|, in a way that we can't accidentally forget about it? https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... File remoting/host/minimum_heartbeat_supporter.h (right): https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2014, no '(c)' https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.h:35: /* Keeping an instance of MinimumHeartbeatSupporter alive nit: C++ style '//' https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.h:73: // gets signal strategy used for talking to the Chromoting bot nit: Capital letter + period. https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.h:111: scoped_ptr<XmppSignalStrategy> signal_strategy_; I'm not sure about this being the class that owns all this stuff? Things like SignalStrategy should be maybe passed in to the ctor (dependency injection)? On the other hand, if this is going to be an important class that owns the world, it should probably have a more generic name like "StatusSender" or something that implies: this class is *the* place for doing all status-related stuff, and is responsible for storing and creating the SignalStrategy and anything else needed for sending status messages?
Patchset #2 (id:20001) has been deleted
I addressed CR feedback + extracted ReportAckOrTimeout function. Could you please re-review? Thanks, Lukasz https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... File remoting/host/minimum_heartbeat_supporter.cc (right): https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/11/14 01:24:57, Lambros wrote: > 2014, no '(c)' Thanks. Done. https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.cc:23: /* TODO(lukasza): code review please: I heard that when passing On 2014/11/14 01:24:57, Lambros wrote: > Remove TODO Done. https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.cc:27: * Should I repent? */ On 2014/11/14 01:24:57, Lambros wrote: > I don't feel strongly. Passing by const reference is better I think. At the > call-site, you would pass in > *context > instead of > context.get() > > scoped_ptr::operator* will assert() if the object is null. Ah, ok. For some reason I thought that I would have to do (*(context_.get())), rather than just (*_context). I'll switch to a const reference. https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.cc:44: net::NetworkChangeNotifier::Create()); On 2014/11/14 01:24:57, Lambros wrote: > nit: Fits on one line. Done. https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.cc:73: oauth_credentials.Pass(), On 2014/11/14 01:24:57, Lambros wrote: > nit: Indentation should be +4, not +2. Done. https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.cc:91: // TODO(lukasza): code review please: anything we can do here? On 2014/11/14 01:24:57, Lambros wrote: > Remove TODO. Done. https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.cc:93: // are not used by frames higher on the callstack On 2014/11/14 01:24:57, Lambros wrote: > Are these fields exposed to things higher up in the call stack? Why is this > class owning them? This goes back to my earlier question about who should own > the things? > > Either: > This object should be the owner, and should say to callers "you can look at my > fields, but don't use them beyond my lifetime". > Or: > Something else should be the owner, and this class should respect lifetime > requirements imposed by the owner. MinimumHeartbeatSupporter object is the owner, but it owns itself and an instance method is on the callstack when the MinimumHeartbeatSupporter object drops the final refcount to itself. Let's discuss this in the comment you had about AddRef+ReleaseSoon pattern. https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.cc:194: this->AddRef(); On 2014/11/14 01:24:57, Lambros wrote: > This is smelly! :) Is there some nicer way we can add a reference to |this|, in > a way that we can't accidentally forget about it? Initially I planned to respond by adding a comment that explains what is going on: // When OnAck or OnTimeout callback is called, it is possible that // the callbacks are the only objects holding a reference to // MinimumHeartgeatSupporter and keeping it alive (i.e. the HostProcess // object might have been destroyed already). If we simply destroyed // the callbacks (by calling ack/timeout_callback_.reset()) below, // then it would destroy |this| as well, which would mean that memory // referenced by implementation of CancelableCallback.reset would no longer be valid. // So - we have to extend our lifetime a little bit more - until all // our frames unwind / until the callstack unwinds to the message pump. // FWIW, a comment in base/message_loop/message_loop.h says that // the pattern we use below is a "common pattern". But... after extracting this code into ack_or_timeout_reporter.cc I realized that I don't need to use CancelableCallback and when directly using WeakPtrFactory, the lifetime gimmicks are no longer necessary. https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... File remoting/host/minimum_heartbeat_supporter.h (right): https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/11/14 01:24:58, Lambros wrote: > 2014, no '(c)' Done. https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.h:35: /* Keeping an instance of MinimumHeartbeatSupporter alive On 2014/11/14 01:24:57, Lambros wrote: > nit: C++ style '//' Done. https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.h:73: // gets signal strategy used for talking to the Chromoting bot On 2014/11/14 01:24:57, Lambros wrote: > nit: Capital letter + period. Done. https://codereview.chromium.org/719983002/diff/1/remoting/host/minimum_heartb... remoting/host/minimum_heartbeat_supporter.h:111: scoped_ptr<XmppSignalStrategy> signal_strategy_; On 2014/11/14 01:24:57, Lambros wrote: > I'm not sure about this being the class that owns all this stuff? Things like > SignalStrategy should be maybe passed in to the ctor (dependency injection)? > > On the other hand, if this is going to be an important class that owns the > world, it should probably have a more generic name like "StatusSender" or > something that implies: this class is *the* place for doing all status-related > stuff, and is responsible for storing and creating the SignalStrategy and > anything else needed for sending status messages? RE: this class owning the fields This class needs to control the lifetime of the things above (like signal_strategy_, signaling_connector_, oauth_token_getter_, heartbeat_sender_) in order to be able to send the host-offline-reason heartbeat, even if the rest of the host is down. When I say "control", I primarily mean "keep those objects alive and be able to use them", but I also think it is good to encapsulate creation of those objects here (otherwise HostProcess becomes a kind of a god object). Maybe we can discuss the naming in person. I picked MinimumHeartbeatSupporter, because having an object of this class alive, is a minimal thing that will periodically keep sending heartbeats to the bot. RE: dependency injection Hmmm... good point. To make things unit-testable, I extracted a ReportAckOrTimeout function and put it into a separate ack_or_timeout_reporter.h/cc. After converting to Callbacks (instead of Listener pattern) I was also able to follow your advice on having a dumb constructor, with most of object creation logic in a separate, static method.
Switched to base::Unretained in mock_callback.h (as well as added unit tests in mock_callback_unittest.cc, as the implementation after the switch crossed my confidence threshold for checking in without unit tests). -Lukasz
Patchset #4 (id:80001) has been deleted
I've run part1 + part2 + part2b + part3 against updated Chromoting servers in "test" environment and made some tweaks (need to invalidate WeakPtrs on network task runner, before HostProcess gets destructed on another thread). Made some other minor tweaks after catching some missing periods and unordered gypi entries in a self-review. I think this is ready for a re-review please. Thanks, Lukasz
https://codereview.chromium.org/719983002/diff/100001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter.h (right): https://codereview.chromium.org/719983002/diff/100001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.h:39: function_that_acks, I'm having a little conceptual trouble with this. Why does |function_that_acks| have to be a function that takes a parameter? Why can't it be a fully-bound Closure itself? You have some function that, when called, promises to call some other function some time in the future. That other function might be related to the first function in many different ways. It might be the first and only parameter (which is what this code assumes). It might be the 29th parameter. It might be a member variable of some object that is passed as a parameter. And so on... In fact, why does |function_that_acks| need to be passed in at all? All we do is Run() it, then throw it away. The caller could just as well do that itself?
https://codereview.chromium.org/719983002/diff/100001/remoting/host/minimum_h... File remoting/host/minimum_heartbeat_supporter.cc (right): https://codereview.chromium.org/719983002/diff/100001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.cc:142: void MinimumHeartbeatSupporter::OnAckOrTimeout(AckOrTimeout ack_or_timeout) Do you plan on doing anything else here, other than logging? https://codereview.chromium.org/719983002/diff/100001/remoting/host/minimum_h... File remoting/host/minimum_heartbeat_supporter.h (right): https://codereview.chromium.org/719983002/diff/100001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:46: : public base::RefCounted<MinimumHeartbeatSupporter> { Do we need ref-counting here? Will this have multiple owners, or can single-ownership be transferred between owners? https://codereview.chromium.org/719983002/diff/100001/remoting/host/mock_call... File remoting/host/mock_callback.h (right): https://codereview.chromium.org/719983002/diff/100001/remoting/host/mock_call... remoting/host/mock_callback.h:68: // callbacks go through a long-lived MockCallbackTracker MockCallbackTracker outlives MockCallback? So why is MockCallbackTracker keeping a raw pointer to MockCallback?
https://codereview.chromium.org/719983002/diff/100001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter.h (right): https://codereview.chromium.org/719983002/diff/100001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.h:39: function_that_acks, On 2014/11/19 02:06:17, Lambros wrote: > I'm having a little conceptual trouble with this. Why does |function_that_acks| > have to be a function that takes a parameter? Why can't it be a fully-bound > Closure itself? I don't understand what you mean by "Why can't it be a fully-bound Closure itself". If you want to separate act of invoking |function_that_acks| from the act of registering an |ack_callback| then I think this won't work - i.e. |function_that_acks| might ack before ReportAckOrTimeout registers an |ack_callback|. > You have some function that, when called, promises to call some other function > some time in the future. That other function might be related to the first > function in many different ways. It might be the first and only parameter (which > is what this code assumes). It might be the 29th parameter. It might be a member > variable of some object that is passed as a parameter. And so on... > The complaints about |ack_callback| being 1st or 29th parameter or a member field or listener pattern are valid, but I think they don't belong here. Such complains apply to any environment that supports partially bound functions, regardless if it is Haskell, Chromium's Callback or something else. If Chromium's Callbacks don't easily support partially binding of middle arguments, then we should fix Chromium's Callbacks (when such partial binding is actually needed). Anyway - I am not sure why the 1st vs 29th parameter is brought up here, because in case of HeartbeatSender things work just fine, because |ack_callback| is the last parameter. We are extracting ReportAckOrTimeout from MinimumHeartbeatSender to make it testable, and not necessarily to make it fully generic, broadly applicable function (which it is, as long as Chromium's Callbacks machinery is easy to wrap parameters in a different order). > In fact, why does |function_that_acks| need to be passed in at all? All we do is > Run() it, then throw it away. The caller could just as well do that itself? We do more than run |function_that_acks| and throw it away :-) We run |function_that_acks| while at the same time providing an |ack_callback| controlled by internals of AckOrTimeoutReporter. https://codereview.chromium.org/719983002/diff/100001/remoting/host/minimum_h... File remoting/host/minimum_heartbeat_supporter.cc (right): https://codereview.chromium.org/719983002/diff/100001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.cc:142: void MinimumHeartbeatSupporter::OnAckOrTimeout(AckOrTimeout ack_or_timeout) On 2014/11/19 02:29:42, Lambros wrote: > Do you plan on doing anything else here, other than logging? No. I think logging is useful in its own right. By having an instance method as a callback we are also extending the lifetime of ref-counted MinimumHeartbeatSupporter until an ack or timeout comes - this postpones exiting the host process until an ack or timeout comes (by virtue of holding a ref-count to network_task_runner which indirectly keeps ui_task_runner alive, and ui_task_runner is the main thread / keeps the whole process alive). https://codereview.chromium.org/719983002/diff/100001/remoting/host/minimum_h... File remoting/host/minimum_heartbeat_supporter.h (right): https://codereview.chromium.org/719983002/diff/100001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:46: : public base::RefCounted<MinimumHeartbeatSupporter> { On 2014/11/19 02:29:42, Lambros wrote: > Do we need ref-counting here? Will this have multiple owners, or can > single-ownership be transferred between owners? There are 2 owners: 1) HostProcess 2) AckOrTimeout callback created in MinimumHeartbeatSender::SendHostOfflineReason https://codereview.chromium.org/719983002/diff/100001/remoting/host/mock_call... File remoting/host/mock_callback.h (right): https://codereview.chromium.org/719983002/diff/100001/remoting/host/mock_call... remoting/host/mock_callback.h:68: // callbacks go through a long-lived MockCallbackTracker On 2014/11/19 02:29:42, Lambros wrote: > MockCallbackTracker outlives MockCallback? So why is MockCallbackTracker keeping > a raw pointer to MockCallback? Well, you proposed changing WeakPtr to base::Unretained (which I really appreciate and wholeheartedly agree with). We are just moving a raw pointer from inside a callback to inside of MockCallbackTracker. Keeping a raw pointer inside MockCallbackTracker will let us reintroduce WeakPtr-like semantics later on (but with us in control of what happens when calling a callback associated with a dead MockCallback, rather with a default behavior of Callback that treats dead WeakPtrs as no-op callbacks). Do you think the changelist would be better and more self-contained if I reintroduced WeakPtr-like semantics today, rather than postponing it until later (i.e. until we are trying to move this to base/mock_callback.h and need to discuss desired behavior with broader Chromium audience). If yes, then please shout at me and I'll do it :-). Otherwise, if I misunderstood your conrerns, then let's talk more about this.
https://codereview.chromium.org/719983002/diff/100001/remoting/host/mock_call... File remoting/host/mock_callback.h (right): https://codereview.chromium.org/719983002/diff/100001/remoting/host/mock_call... remoting/host/mock_callback.h:68: // callbacks go through a long-lived MockCallbackTracker On 2014/11/19 21:44:24, lukasza wrote: > On 2014/11/19 02:29:42, Lambros wrote: > > MockCallbackTracker outlives MockCallback? So why is MockCallbackTracker > keeping > > a raw pointer to MockCallback? > > Well, you proposed changing WeakPtr to base::Unretained (which I really > appreciate and wholeheartedly agree with). We are just moving a raw pointer > from inside a callback to inside of MockCallbackTracker. Keeping a raw pointer > inside MockCallbackTracker will let us reintroduce WeakPtr-like semantics later > on (but with us in control of what happens when calling a callback associated > with a dead MockCallback, rather with a default behavior of Callback that treats > dead WeakPtrs as no-op callbacks). > > Do you think the changelist would be better and more self-contained if I > reintroduced WeakPtr-like semantics today, rather than postponing it until later > (i.e. until we are trying to move this to base/mock_callback.h and need to > discuss desired behavior with broader Chromium audience). If yes, then please > shout at me and I'll do it :-). Otherwise, if I misunderstood your conrerns, > then let's talk more about this. BTW: having MockCallbackTracker is useful even without reintroducing WeakPtr-like semantics - it is needed in the current changelist to enable HasRemainingCallbacks method of MockCallback.
Parts 1, 2 and 2b have landed. Could you please take another look at part3?
Here are some comments so far, but I'm still trying to get my head around the MockCallbackTracker stuff. ReportAckOrTimeout looks fine, thanks for explaining it! I'll go over it again for nits later. https://codereview.chromium.org/719983002/diff/120001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.h (right): https://codereview.chromium.org/719983002/diff/120001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:143: const scoped_refptr<RsaKeyPair> key_pair_; Why change this? https://codereview.chromium.org/719983002/diff/120001/remoting/host/minimum_h... File remoting/host/minimum_heartbeat_supporter.cc (right): https://codereview.chromium.org/719983002/diff/120001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.cc:107: // Order of destroying fields below is important. Rather than manually calling reset(), why not just order them correctly in the header file? C++ guarantees to destroy these objects in reverse order. And please add a comment explaining what the dependencies actually are? https://codereview.chromium.org/719983002/diff/120001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.cc:113: HOST_LOG << "MinimumHeartbeatSupporter is ready to die " If you do need to log this, you could maybe simplify it to "MinimumHeartbeatSupporter destroyed." https://codereview.chromium.org/719983002/diff/120001/remoting/host/minimum_h... File remoting/host/minimum_heartbeat_supporter.h (right): https://codereview.chromium.org/719983002/diff/120001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:48: static scoped_refptr<MinimumHeartbeatSupporter> Create( This feels like too many parameters to one method, which goes against Chromium style. Could we simplify this interface? For example, |host_id| and |key_pair| are used only to construct a HeartbeatSender. Could the caller create the HeartbeatSender and pass that in? https://codereview.chromium.org/719983002/diff/120001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:56: const scoped_refptr<RsaKeyPair> key_pair, It is strange to have 'const' with a pass-by-value parameter. Either lose the 'const', or pass by const-reference. https://codereview.chromium.org/719983002/diff/120001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:63: SignalStrategy* GetSignalStrategy(); nit: This is a simple getter, so name it Unix-style: signal_strategy() and inline the definition here. https://codereview.chromium.org/719983002/diff/120001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:88: scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_; |network_change_notifier_| is never used in this class. Can we remove it? https://codereview.chromium.org/719983002/diff/120001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/120001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1227: DCHECK(!host_id_.empty()); // assumming |ApplyConfig| has already been run nit: Comments should be English sentences. nit: Two spaces before // https://codereview.chromium.org/719983002/diff/120001/remoting/host/signaling... File remoting/host/signaling_connector.h (right): https://codereview.chromium.org/719983002/diff/120001/remoting/host/signaling... remoting/host/signaling_connector.h:71: scoped_ptr<OAuthTokenGetter> oauth_token_getter_; Is this change relevant to this CL?
Thanks for the feedback. Please take another look. https://codereview.chromium.org/719983002/diff/120001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.h (right): https://codereview.chromium.org/719983002/diff/120001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:143: const scoped_refptr<RsaKeyPair> key_pair_; On 2014/12/02 03:35:22, Lambros wrote: > Why change this? The assumption here is that presence of "const" is good: 1) It allows the callers to pass "const" arguments. 2) If it compiles it is probably safe (although C++ doesn't give any formal guarantees / allows to cast the constness away). Allowing callers to pass "const" arguments is desirable, because this: 1A) enables their callers to pass "const" arguments 1B) const arguments are sometimes unavoidable (i.e. retrieved from another part of the system that we cannot or don't want to change) and casting constness away is yucky 1C) presence of "const" is a good code readability aid (const = I promise I won't modify/delete this pointer). Anyway - this is unrelated to core changes here, but this is a small, beneficial refactoring, so I think this is ok. https://codereview.chromium.org/719983002/diff/120001/remoting/host/minimum_h... File remoting/host/minimum_heartbeat_supporter.cc (right): https://codereview.chromium.org/719983002/diff/120001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.cc:107: // Order of destroying fields below is important. On 2014/12/02 03:35:22, Lambros wrote: > Rather than manually calling reset(), why not just order them correctly in the > header file? C++ guarantees to destroy these objects in reverse order. And > please add a comment explaining what the dependencies actually are? Good point on the explicit comment - thanks for pointing this out. Documenting this in a comment is especially good now, when all of the 4 collaborating objects are embedded inside MinimumHeartbeatSupporter (meaning that hopefully the comment is unlikely to get out of sync with the implementation). Hmmm... I guess I can remove the explicit resets... My initial thinking was that having an explicit destructor with explicit order of resets was more straightforward and more easily readable by future maintainers. But I already saw some other classes with important order of field declarations, so I guess it is ok to do it this way. I think I still want an explicit destructor - both the assert and HOST_LOG seem desirable. https://codereview.chromium.org/719983002/diff/120001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.cc:113: HOST_LOG << "MinimumHeartbeatSupporter is ready to die " On 2014/12/02 03:35:22, Lambros wrote: > If you do need to log this, you could maybe simplify it to > "MinimumHeartbeatSupporter destroyed." I think I do want to log this to aid in investigating "why the host didn't exit" question. Presence of this log will indicate that this is not MinimumHeartbeatSupporter's fault. I will simplify the log message. https://codereview.chromium.org/719983002/diff/120001/remoting/host/minimum_h... File remoting/host/minimum_heartbeat_supporter.h (right): https://codereview.chromium.org/719983002/diff/120001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:48: static scoped_refptr<MinimumHeartbeatSupporter> Create( On 2014/12/02 03:35:23, Lambros wrote: > This feels like too many parameters to one method, which goes against Chromium > style. Could we simplify this interface? > For example, |host_id| and |key_pair| are used only to construct a > HeartbeatSender. Could the caller create the HeartbeatSender and pass that in? Can you comment on the things below please?: 1. I can introduce a MinimumHeartbeatSupporter::Callbacks struct that holds all the 3 callbacks. This will help reduce the number of parameters. OTOH, I feel that it just adds noise elsewhere (extra struct declaration + definition and filling out by the caller). 2. The caller could in theory create some of the objects here (i.e. HeartbeatSender), but they are closely coupled (i.e. to construct HeartbeatSender one needs to have a SignalStrategy) and I like how the current changelist keeps everything inside the static Create method. 3. Maybe it is possible to combine configuration values into a bigger, coherent bundle, but I am not sure how to do it: - talkgadget_prefix comes from policy (HostProcess::OnHostTalkGadgetPrefixPolicyUpdate) - host_id, key_pair, use_service_account comes from config (HostProcess::ApplyConfig) - oauth_refresh_token is read from a path specified in config - directory_bot_jid comes from commandline (HostProcess::InitWithCommandLine) I am also guessing that Chromium style frowns upon too many fields and one can treat the current changelist as simply exposing presence of too many fields and responsibilities inside HostProcess. It feels that there is one or more class hiding inside HostProcess that are waiting to be refactored out (HostConfigAndPolicyAndCommandLineValues?) but I am not quite sure how to cleanly do this refactoring + I would rather do this refactoring as a separate changelist. I think my proposal is to just leave a TODO here. What do you think? https://codereview.chromium.org/719983002/diff/120001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:56: const scoped_refptr<RsaKeyPair> key_pair, On 2014/12/02 03:35:23, Lambros wrote: > It is strange to have 'const' with a pass-by-value parameter. Either lose the > 'const', or pass by const-reference. Good catch - thanks. I blame it on: 1. My stupidity - trying to consistently, aesthetically have consts on all arguments, without thinking about details of what I am doing 2. Just copying the declaration from constructor of HeartbeatSender - it has identical parameter declaration with const applied to scoped_refptr (introduced by https://chromium.googlesource.com/chromium/src/+/8f1504bf1badb27cff0f1a71886f...). I see that HeartbeatSender only calls RsaKeyPair::SignMessage which is a const method - therefore I'll do two things: A. Convert the param into a const reference (i.e. s/const scoped_refptr<T>/const scoped_refptr<T>&/ ) both in HeartbeatSender and in MinimumHeartbeatSupporter. B. Enforce constness of RsaKeyPair by changing s/scoped_refptr<T>/scoped_refptr<const T>/). (without this HeartbeatSender can call non-const functions of RsaKeyPair). https://codereview.chromium.org/719983002/diff/120001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:63: SignalStrategy* GetSignalStrategy(); On 2014/12/02 03:35:23, Lambros wrote: > nit: This is a simple getter, so name it Unix-style: signal_strategy() and > inline the definition here. Done. https://codereview.chromium.org/719983002/diff/120001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:88: scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_; On 2014/12/02 03:35:22, Lambros wrote: > |network_change_notifier_| is never used in this class. Can we remove it? No - there is an implicit / magical / global dependency :-( From https://code.google.com/p/chromium/codesearch#chromium/src/net/base/network_c...: // Creates the process-wide, platform-specific NetworkChangeNotifier. The // caller owns the returned pointer. You may call this on any thread. You // may also avoid creating this entirely (in which case nothing will be // monitored), but if you do create it, you must do so before any other // threads try to access the API below, and it must outlive all other threads // which might try to use it. static NetworkChangeNotifier* Create(); And our SignalingConnector uses this magical/global/implicit dependency here: https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/sign... SignalingConnector::SignalingConnector( XmppSignalStrategy* signal_strategy, scoped_ptr<DnsBlackholeChecker> dns_blackhole_checker, const base::Closure& auth_failed_callback) : signal_strategy_(signal_strategy), auth_failed_callback_(auth_failed_callback), dns_blackhole_checker_(dns_blackhole_checker.Pass()), reconnect_attempts_(0) { DCHECK(!auth_failed_callback_.is_null()); DCHECK(dns_blackhole_checker_.get()); net::NetworkChangeNotifier::AddConnectionTypeObserver(this); net::NetworkChangeNotifier::AddIPAddressObserver(this); signal_strategy_->AddListener(this); ScheduleTryReconnect(); } SignalingConnector::~SignalingConnector() { signal_strategy_->RemoveListener(this); net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this); net::NetworkChangeNotifier::RemoveIPAddressObserver(this); } https://codereview.chromium.org/719983002/diff/120001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/120001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1227: DCHECK(!host_id_.empty()); // assumming |ApplyConfig| has already been run On 2014/12/02 03:35:23, Lambros wrote: > nit: Comments should be English sentences. > nit: Two spaces before // > Done. https://codereview.chromium.org/719983002/diff/120001/remoting/host/signaling... File remoting/host/signaling_connector.h (right): https://codereview.chromium.org/719983002/diff/120001/remoting/host/signaling... remoting/host/signaling_connector.h:71: scoped_ptr<OAuthTokenGetter> oauth_token_getter_; On 2014/12/02 03:35:23, Lambros wrote: > Is this change relevant to this CL? Not directly relevant, but please read on. Assumption1: scoped_ptr is better than raw pointer Assumption2: we should avoid raw pointers in new code If I kept the old code in signaling_connector, then I would have to have a raw pointer (or rather call scoped_ptr<T>.get()) in implementation of MinimumHeartbeatSupporter::Create. Assumption3: hygiene is good Assumption4: small refactorings won't happen if they require an overhead of a separate changelist Are these valid?
Hmmm... clicking "Publish+Mail Comments" again, as Rietveld still thinks that I have some unpublished drafts... Weird.
lambroslambrou@chromium.org changed reviewers: + wez@chromium.org
wez: Could you please have a look at the MockCallback stuff - I could do with an extra architectural brain on this :) In particular, any ideas on how to lessen the code-duplication between the 0- and 1- parameter versions? https://codereview.chromium.org/719983002/diff/120001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.h (right): https://codereview.chromium.org/719983002/diff/120001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:143: const scoped_refptr<RsaKeyPair> key_pair_; On 2014/12/02 20:08:01, lukasza wrote: > On 2014/12/02 03:35:22, Lambros wrote: > > Why change this? > > The assumption here is that presence of "const" is good: > 1) It allows the callers to pass "const" arguments. But this field is a value type (not a reference or pointer type). Callers can still pass "const" arguments even if the field is not const - it will be assigned by value. Adding "const" will discourage future maintainers from making certain changes (for example, moving the assignment of this field out of the ctor into an Init() method), for no obvious benefit. I suspect what you really want is scoped_refptr<const RsaKeyPair> ? https://codereview.chromium.org/719983002/diff/120001/remoting/host/minimum_h... File remoting/host/minimum_heartbeat_supporter.h (right): https://codereview.chromium.org/719983002/diff/120001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:48: static scoped_refptr<MinimumHeartbeatSupporter> Create( On 2014/12/02 20:08:01, lukasza wrote: > On 2014/12/02 03:35:23, Lambros wrote: > > This feels like too many parameters to one method, which goes against Chromium > > style. Could we simplify this interface? > > For example, |host_id| and |key_pair| are used only to construct a > > HeartbeatSender. Could the caller create the HeartbeatSender and pass that in? > > I think my proposal is to just leave a TODO here. What do you think? I think a TODO: is fine for now - I accept that refactoring is beyond the scope of this CL. https://codereview.chromium.org/719983002/diff/120001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:56: const scoped_refptr<RsaKeyPair> key_pair, On 2014/12/02 20:08:01, lukasza wrote: > On 2014/12/02 03:35:23, Lambros wrote: > > It is strange to have 'const' with a pass-by-value parameter. Either lose the > > 'const', or pass by const-reference. > I see that HeartbeatSender only calls RsaKeyPair::SignMessage which is a const > method - therefore I'll do two things: > A. Convert the param into a const reference (i.e. s/const scoped_refptr<T>/const > scoped_refptr<T>&/ ) both in HeartbeatSender and in MinimumHeartbeatSupporter. > B. Enforce constness of RsaKeyPair by changing > s/scoped_refptr<T>/scoped_refptr<const T>/). (without this HeartbeatSender can > call non-const functions of RsaKeyPair). Yes, I think scoped_refptr should be passed around by const-reference (but stored by value). https://codereview.chromium.org/719983002/diff/120001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:88: scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_; On 2014/12/02 20:08:01, lukasza wrote: > On 2014/12/02 03:35:22, Lambros wrote: > > |network_change_notifier_| is never used in this class. Can we remove it? > > No - there is an implicit / magical / global dependency :-( Fair enough :) I guess we had this problem before, and you're just moving the code around. https://codereview.chromium.org/719983002/diff/160001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter.cc (right): https://codereview.chromium.org/719983002/diff/160001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.cc:92: // Release outselves from memory - we won't be called anymore. typo: ourselves But the grammar is a bit dubious here - who are "we"? :) It's a good idea to avoid using "we" and "our" in code comments. In this case, I think you could remove this comment, and the one above it. https://codereview.chromium.org/719983002/diff/160001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter.h (right): https://codereview.chromium.org/719983002/diff/160001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.h:39: function_that_acks, nit: This should be indented +4 from previous line. https://codereview.chromium.org/719983002/diff/160001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.h:43: ack_or_timeout_callback); nit: Indentation https://codereview.chromium.org/719983002/diff/160001/remoting/host/mock_call... File remoting/host/mock_callback.h (right): https://codereview.chromium.org/719983002/diff/160001/remoting/host/mock_call... remoting/host/mock_callback.h:19: // MockCallback<Sig> can be used to mock Callback<Sig> from base/callback.h This basically looks fine, though I can't say I fully understand it all. My only concern is the amount of code-duplication between the 0-parameter and 1-parameter versions of these classes. https://codereview.chromium.org/719983002/diff/160001/remoting/host/mock_call... remoting/host/mock_callback.h:36: // << "Verify that FunctionUnderTest didn't store the callback " This feels like it should be a code comment, not a log message.
I addressed CR feedback. I also added documentation comments to MockCallback that explain why we cannot use WeakPtrFactory. I also added explicit handling (+ unit test) of what happens when Callback::Run is called when MockCallback is already destroyed. Labros / Wez - could you please take a look? https://codereview.chromium.org/719983002/diff/120001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.h (right): https://codereview.chromium.org/719983002/diff/120001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:143: const scoped_refptr<RsaKeyPair> key_pair_; On 2014/12/03 03:20:24, Lambros wrote: > On 2014/12/02 20:08:01, lukasza wrote: > > On 2014/12/02 03:35:22, Lambros wrote: > > > Why change this? > > > > The assumption here is that presence of "const" is good: > > 1) It allows the callers to pass "const" arguments. > But this field is a value type (not a reference or pointer type). Callers can > still pass "const" arguments even if the field is not const - it will be > assigned by value. > > Adding "const" will discourage future maintainers from making certain changes > (for example, moving the assignment of this field out of the ctor into an Init() > method), for no obvious benefit. > > I suspect what you really want is scoped_refptr<const RsaKeyPair> ? You're right. I don't know what I was thinking. https://codereview.chromium.org/719983002/diff/160001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter.cc (right): https://codereview.chromium.org/719983002/diff/160001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.cc:92: // Release outselves from memory - we won't be called anymore. On 2014/12/03 03:20:25, Lambros wrote: > typo: ourselves > But the grammar is a bit dubious here - who are "we"? :) > It's a good idea to avoid using "we" and "our" in code comments. > In this case, I think you could remove this comment, and the one above it. Done. https://codereview.chromium.org/719983002/diff/160001/remoting/host/mock_call... File remoting/host/mock_callback.h (right): https://codereview.chromium.org/719983002/diff/160001/remoting/host/mock_call... remoting/host/mock_callback.h:36: // << "Verify that FunctionUnderTest didn't store the callback " On 2014/12/03 03:20:25, Lambros wrote: > This feels like it should be a code comment, not a log message. Maybe. I think that having this message in the output of a failing test will help. I'll leave it in, unless you strongly object to this.
Publishing results of git cl format.
https://codereview.chromium.org/719983002/diff/200001/remoting/host/mock_call... File remoting/host/mock_callback.h (right): https://codereview.chromium.org/719983002/diff/200001/remoting/host/mock_call... remoting/host/mock_callback.h:26: // base::Callback<void(int)> callback = mock_callback.GetCallback(); Is it important that GetCallback() is only called after EXPECT_CALL() expectations have been set? How many times can GetCallback() be called on MockCallback? If it can be called multiple times then what are the rules around the expectations being matched? nit: MockCallback seems a misleading name, since it's not a mock callback in itself, but seemingly a factory for mocked callbacks? https://codereview.chromium.org/719983002/diff/200001/remoting/host/mock_call... remoting/host/mock_callback.h:31: // HasRemainingCallbacks usage example: HasRemainingCallbacks has not been introduced yet, so there is no context for the reader - perhaps you mean "Example of verifying that callback instances were correctly freed"? It seems that this capability is the only reason we need this class - test can otherwise just EXPECT_CALL on test fixture methods and then Bind() to those to get callbacks - is it actually useful enough to justify this extra complexity, given that we already have tools like ASan and TSan to trap threading/memory issues? https://codereview.chromium.org/719983002/diff/200001/remoting/host/mock_call... remoting/host/mock_callback.h:39: // << "worry about lifetime of arguments bound to the callback"; You need to be very clear on the threading semantics for a check like this, otherwise you may get a test that passes HasRemainingCallbacks() because thread timings happen to tear things down at the right times, but then it fails mysteriously now and again when the timings change and the object lives a little longer on some background thread. https://codereview.chromium.org/719983002/diff/200001/remoting/host/mock_call... remoting/host/mock_callback.h:45: // ON_CALL and EXPECT_CALL gMock macros). This is not very clear; you mean that the call expectations must have been set before any thread calls Run()? Or something else? https://codereview.chromium.org/719983002/diff/200001/remoting/host/mock_call... remoting/host/mock_callback.h:46: // - Is thread-safe wrt other Run calls and destroying MockCallback. What are the semantics if a callback still exists when the MockCallback is destroyed? The comment on GetCallback() explicitly states that the caller is responsible for guaranteeing that nothing calls Run() on the callback after the MockCallback has been destroyed? https://codereview.chromium.org/719983002/diff/200001/remoting/host/mock_call... remoting/host/mock_callback.h:52: // What happens when Callback::Run is called after MockCallback is destroyed: This contradicts the GetCallback comment, though. https://codereview.chromium.org/719983002/diff/200001/remoting/host/mock_call... remoting/host/mock_callback.h:65: // use WeakPtrFactory<T> and WeakPtr<T>, because: This comment is very confusing; it's not clear what similarities they share, especially since you then go on to describe differences in threading limitations, at least one of which is fundamental to what WeakPtr is. MockCallbackTracker also hasn't been mentioned in any of the preceding text, so it's confusing to introduce it here without any reference to its purpose. https://codereview.chromium.org/719983002/diff/200001/remoting/host/mock_call... remoting/host/mock_callback.h:80: class MockCallback<R()> { It looks like you should be able to put most of this in a MockCallbackBase and then have cardinality-specific MockCallback templates derived from that, which provide the GetCallback() and Run() implementations. You may even be able to make MockCallbackBase varargs-style so that you can then have the cardinality-specific templates just add the relevant MOCK_METHOD definitions and instantiate the base with the required cardinality. However, I'm still not clear on what this class does that can't be achieved via an EXPECT_CALL() and Bind() to a test fixture method, besides the HasRemainingCallbacks() capability, which itself seems flaky to me - if that's the only benefit then could you add a HasOneRefForTest() method to base::Callback that checks the ref-count of the internal callback storage to ensure that the test is the only thing holding on to it?
I addressed some review comments. To help decide whether or not keeping mock_callback.h is desirable, I removed it in patchset#11 - please take a look at the difference between patchset#10 and patchset #11 to see what having mock_callback.h is giving us (maybe not that much indeed). Wez / Lambros - can you take another look please? https://codereview.chromium.org/719983002/diff/200001/remoting/host/mock_call... File remoting/host/mock_callback.h (right): https://codereview.chromium.org/719983002/diff/200001/remoting/host/mock_call... remoting/host/mock_callback.h:26: // base::Callback<void(int)> callback = mock_callback.GetCallback(); On 2014/12/03 19:45:49, Wez wrote: > Is it important that GetCallback() is only called after EXPECT_CALL() > expectations have been set? Sort of. EXPECT_CALL is not thread-safe wrt Callback::Run. This is sort of covered already in "Thread safety notes". I've also added a comment next to the example. > How many times can GetCallback() be called on MockCallback? > > If it can be called multiple times then what are the rules around the > expectations being matched? Can be called multiple times. All Callbacks retrieved in this way are subject to gMock expectations set on MockCalback::Run method. I've added a comment where I try to explain this. > nit: MockCallback seems a misleading name, since it's not a mock callback in > itself, but seemingly a factory for mocked callbacks? Hmmm... interesting. Yes - maybe this should be renamed to MockCallbackFactory. https://codereview.chromium.org/719983002/diff/200001/remoting/host/mock_call... remoting/host/mock_callback.h:31: // HasRemainingCallbacks usage example: On 2014/12/03 19:45:49, Wez wrote: > HasRemainingCallbacks has not been introduced yet, so there is no context for > the reader - perhaps you mean "Example of verifying that callback instances were > correctly freed"? > > It seems that this capability is the only reason we need this class - test can > otherwise just EXPECT_CALL on test fixture methods and then Bind() to those to > get callbacks - is it actually useful enough to justify this extra complexity, > given that we already have tools like ASan and TSan to trap threading/memory > issues? Good point. I am not sure about this - Some tests (i.e. ack_or_timeout_reporter_unittest.cc) don't have a test fixture, just test functions. I also don't quite like the aesthetics of having to declare mocked Run in the test fixture (kind of globally) and using them in individual tests (although this particular aesthetics concern is easily addressed by the next item below). - Some tests decided to introduce MockCallback class (with the mocked method similar to the one you are suggesting to be embedded in a test fixture). This works but it seemed nice at one point to let the tests avoid having to declare a one-off class in every test that needs to mock a callback. - When other tests introduced separate MockCallback classes, they sometimes unnecessarily exposed ref-counting to the test (i.e. see: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/test_he...) But other than that, you are correct - this functionality can be achieved in other ways with some extra class and/or mocked-method declarations next to test suites (except for HasRemainingCallbacks). BTW: HasRemainingCallbacks can be done outside of Callback by having test maintain its own WeakPtrFactory (to an instance of whatever class declares a mocked Run), passing a weak_ptr to base::Bind and asserting weak_ptr_factory.HasWeakPtrs(). To help decide, I got rid of mock_callback.h in a separate patchset - the test code has to do a bit more in this case, but only slightly. If you think patchset#10 (with mock_callback.h) doesn't have any advantage over patchset#11 (without mock_callback.h) then let's get rid of mock_callback.h (especially since it would probably receive similar feedback when trying to move that to a chromium-wide location [which was my intention]). https://codereview.chromium.org/719983002/diff/200001/remoting/host/mock_call... remoting/host/mock_callback.h:39: // << "worry about lifetime of arguments bound to the callback"; On 2014/12/03 19:45:49, Wez wrote: > You need to be very clear on the threading semantics for a check like this, > otherwise you may get a test that passes HasRemainingCallbacks() because thread > timings happen to tear things down at the right times, but then it fails > mysteriously now and again when the timings change and the object lives a little > longer on some background thread. I don't understand: - HasRemainingCallbacks is thread-safe (by virtue of atomic/interlocked operations via RefCountedThreadSafe<T>). - HasRemainingCallbacks is subject to race conditions. This is sort of obvious. What would I say in a comment to document this? https://codereview.chromium.org/719983002/diff/200001/remoting/host/mock_call... remoting/host/mock_callback.h:45: // ON_CALL and EXPECT_CALL gMock macros). On 2014/12/03 19:45:49, Wez wrote: > This is not very clear; you mean that the call expectations must have been set > before any thread calls Run()? Or something else? One cannot call EXPECT_CALL and Run in parallel. It is ok to have a sequence of calls EXPECT_CALL, Run, EXPECT_CALL (overrides previous expectations), Run. https://codereview.chromium.org/719983002/diff/200001/remoting/host/mock_call... remoting/host/mock_callback.h:46: // - Is thread-safe wrt other Run calls and destroying MockCallback. On 2014/12/03 19:45:49, Wez wrote: > What are the semantics if a callback still exists when the MockCallback is > destroyed? This is documented in "What happens when Callback::Run is called after MockCallbackFactory is destroyed" comment. > > The comment on GetCallback() explicitly states that the caller is responsible > for guaranteeing that nothing calls Run() on the callback after the MockCallback > has been destroyed? Ooops... this is a stale comment. I'll fix this. https://codereview.chromium.org/719983002/diff/200001/remoting/host/mock_call... remoting/host/mock_callback.h:52: // What happens when Callback::Run is called after MockCallback is destroyed: On 2014/12/03 19:45:49, Wez wrote: > This contradicts the GetCallback comment, though. Acknowledged. https://codereview.chromium.org/719983002/diff/200001/remoting/host/mock_call... remoting/host/mock_callback.h:80: class MockCallback<R()> { On 2014/12/03 19:45:49, Wez wrote: > It looks like you should be able to put most of this in a MockCallbackBase and > then have cardinality-specific MockCallback templates derived from that, which > provide the GetCallback() and Run() implementations. > > You may even be able to make MockCallbackBase varargs-style so that you can then > have the cardinality-specific templates just add the relevant MOCK_METHOD > definitions and instantiate the base with the required cardinality. > > However, I'm still not clear on what this class does that can't be achieved via > an EXPECT_CALL() and Bind() to a test fixture method, besides the > HasRemainingCallbacks() capability, which itself seems flaky to me - if that's > the only benefit then could you add a HasOneRefForTest() method to > base::Callback that checks the ref-count of the internal callback storage to > ensure that the test is the only thing holding on to it? Ack - I'll try this out.
Ping?
lgtm without the MockCallback stuff - please take a look at my comments, mostly nits. https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter.cc (right): https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.cc:8: #include "base/callback.h" I don't think you need to repeat any #includes here that are already in ack_or_timeout_reporter.h. From https://code.google.com/p/include-what-you-use/ "either foo.cc or foo.h should #include a .h file that exports the declaration of that symbol" I interpret that as XOR. https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.cc:13: #include "base/sequenced_task_runner.h" Same here, can remove this. https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.cc:29: // Ref-counting helpers. You can remove this comment. https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter.h (right): https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.h:10: #include "base/time/time.h" nit: Remove this and forward-declare base::TimeDelta ? https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter_unittest.cc (right): https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter_unittest.cc:10: #include "base/sequenced_task_runner.h" Don't need here, already in ack_or_timeout_reporter.h https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter_unittest.cc:58: EXPECT_CALL(mock_function_that_acks, Run(testing::_)) nit: We normally add using... declarations for all these testing:: names. See for example remoting/protocol/jingle_session_unittest.cc https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter_unittest.cc:95: // This is expectation is depended on by |MinimumHeartbeatSupporter| to nit: This expectation... https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter_unittest.cc:100: // It should be safe to call the callbacks multiple times. Is this testing anything - there are no expectations here? Why would it not be safe? And why do we care about being able to run these more than once? https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter_unittest.cc:128: // |ack_or_timeout_callback| should be dropped after being called once. I think you can remove this? It's just a duplicate of the comment above. https://codereview.chromium.org/719983002/diff/240001/remoting/host/minimum_h... File remoting/host/minimum_heartbeat_supporter.h (right): https://codereview.chromium.org/719983002/diff/240001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:13: #include "base/time/time.h" Don't need time.h ? https://codereview.chromium.org/719983002/diff/240001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:37: // ensures that we keep trying to send heartbeats (which among other "among other things"? Remove this, or say what those things actually are. This whole phrasing is not very clear to me. It seems to me this class has two functions: 1) Keep sending regular heartbeats to the host. 2) Keep the host process alive during shutdown, until the reason-code for shutdown has been sent and acknowledged (subject to timeout). Am I right? https://codereview.chromium.org/719983002/diff/240001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:89: // Order fields below is important for destructing them in the right oder. nit: Order of the fields ... https://codereview.chromium.org/719983002/diff/240001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:96: // - |network_task_runner_| is used by all the other fields and has to be |network_task_runner_| is ref-counted, so it doesn't matter where it goes? https://codereview.chromium.org/719983002/diff/240001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/240001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:335: base::WeakPtrFactory<HostProcess> weak_factory_for_network_task_runner_; It seems strange to move network_task_runner_ into MinimumHeartbeatSupporter, but keep this WeakPtrFactory here. Maybe you could move this as well? Make this a TODO if you do it in another CL. https://codereview.chromium.org/719983002/diff/240001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1350: base::TimeDelta::FromSeconds(30) /* timeout */); 30s feels much too long to be delaying process exit. I think the Python script sends SIGTERM, waits 10s, then sends SIGKILL. So anything more than 10s is pointless here, unless you update the Python script as well. See https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/linu... https://codereview.chromium.org/719983002/diff/240001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1359: base::TimeDelta::FromSeconds(30) /* timeout */); Same here.
I addressed code review feedback from Lambros. Thanks! Wez - could you please take a look at mock_callback.h (i.e. if we should leave things as-is [without mock_callback.h] or whether unit tests were better off with mock_callback.h). You can see the differences in unit tests when comparing patchset #10 and patchset #11: https://codereview.chromium.org/719983002/diff2/220001:240001/remoting/host/a... https://codereview.chromium.org/719983002/diff2/220001:240001/remoting/host/h... Thanks, Lukasz https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter.cc (right): https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.cc:8: #include "base/callback.h" On 2014/12/05 22:19:48, Lambros wrote: > I don't think you need to repeat any #includes here that are already in > ack_or_timeout_reporter.h. > > From https://code.google.com/p/include-what-you-use/ > "either foo.cc or foo.h should #include a .h file that exports the declaration > of that symbol" > I interpret that as XOR. I applied your forward-declarations comment in the .h file to both the Callback and TimeDelta class (both only used via reference in the public declarations), so I had to keep the include in the .cc file. https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.cc:13: #include "base/sequenced_task_runner.h" On 2014/12/05 22:19:48, Lambros wrote: > Same here, can remove this. Done. https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.cc:29: // Ref-counting helpers. On 2014/12/05 22:19:48, Lambros wrote: > You can remove this comment. Done. https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter.h (right): https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.h:10: #include "base/time/time.h" On 2014/12/05 22:19:48, Lambros wrote: > nit: Remove this and forward-declare base::TimeDelta ? Done for TimeDelta and Callback/Closure. https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter_unittest.cc (right): https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter_unittest.cc:10: #include "base/sequenced_task_runner.h" On 2014/12/05 22:19:48, Lambros wrote: > Don't need here, already in ack_or_timeout_reporter.h Done. https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter_unittest.cc:58: EXPECT_CALL(mock_function_that_acks, Run(testing::_)) On 2014/12/05 22:19:48, Lambros wrote: > nit: We normally add using... declarations for all these testing:: names. See > for example remoting/protocol/jingle_session_unittest.cc Done. https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter_unittest.cc:95: // This is expectation is depended on by |MinimumHeartbeatSupporter| to On 2014/12/05 22:19:48, Lambros wrote: > nit: This expectation... Done. https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter_unittest.cc:100: // It should be safe to call the callbacks multiple times. On 2014/12/05 22:19:48, Lambros wrote: > Is this testing anything - there are no expectations here? Why would it not be > safe? And why do we care about being able to run these more than once? I'll add a comment. This tests 1) that ack_or_timeout_callback is not called anymore and 2) that nothing crashes / references bad memory when keeping calling the callbacks. https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter_unittest.cc:128: // |ack_or_timeout_callback| should be dropped after being called once. On 2014/12/05 22:19:48, Lambros wrote: > I think you can remove this? It's just a duplicate of the comment above. I don't see how this is a duplicate :-/ https://codereview.chromium.org/719983002/diff/240001/remoting/host/minimum_h... File remoting/host/minimum_heartbeat_supporter.h (right): https://codereview.chromium.org/719983002/diff/240001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:13: #include "base/time/time.h" On 2014/12/05 22:19:48, Lambros wrote: > Don't need time.h ? Done. https://codereview.chromium.org/719983002/diff/240001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:37: // ensures that we keep trying to send heartbeats (which among other On 2014/12/05 22:19:48, Lambros wrote: > "among other things"? > Remove this, or say what those things actually are. > > This whole phrasing is not very clear to me. It seems to me this class has two > functions: > 1) Keep sending regular heartbeats to the host. > 2) Keep the host process alive during shutdown, until the reason-code for > shutdown has been sent and acknowledged (subject to timeout). > Am I right? The comment was not trying to explain the purpose of *the class*, but rather trying to explain what is accomplished by keeping an instance of the class alive. I'll reword the comment as suggested. https://codereview.chromium.org/719983002/diff/240001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:89: // Order fields below is important for destructing them in the right oder. On 2014/12/05 22:19:48, Lambros wrote: > nit: Order of the fields ... Done. https://codereview.chromium.org/719983002/diff/240001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:96: // - |network_task_runner_| is used by all the other fields and has to be On 2014/12/05 22:19:48, Lambros wrote: > |network_task_runner_| is ref-counted, so it doesn't matter where it goes? The other objects don't hold a reference to network_task_runner_, but rather go through TLS storage to get a generic TaskRunner. For example SignalingConnector goes through base::Timer which then does the following: https://code.google.com/p/chromium/codesearch#chromium/src/base/timer/timer.c... : scoped_refptr<SingleThreadTaskRunner> Timer::GetTaskRunner() { return task_runner_.get() ? task_runner_ : ThreadTaskRunnerHandle::Get(); } which ends up here: https://code.google.com/p/chromium/codesearch#chromium/src/base/thread_task_r... : scoped_refptr<SingleThreadTaskRunner> ThreadTaskRunnerHandle::Get() { ThreadTaskRunnerHandle* current = lazy_tls_ptr.Pointer()->Get(); DCHECK(current); return current->task_runner_; } https://codereview.chromium.org/719983002/diff/240001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/240001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:335: base::WeakPtrFactory<HostProcess> weak_factory_for_network_task_runner_; On 2014/12/05 22:19:49, Lambros wrote: > It seems strange to move network_task_runner_ into MinimumHeartbeatSupporter, > but keep this WeakPtrFactory here. Maybe you could move this as well? Make this > a TODO if you do it in another CL. This is here to stay for the long term. 1. network_task_runner is not embedded/hidden inside MinimumHeartbeatSupporter (unlike signaling_connector, heartbeat_sender and network_change_notifier which are now only used inside MinimumHeartbeatSupporter). The network_task_runner is primarily controlled by ChromotingHostContext. 2. This WeakPtrFactory is needed to invalidate callbacks that HostProcess creates and passes to MinimumHeartbeatSupporter. I think the name is also fine, but that a comment is needed to explain what the name means - that this is a "weak factory for weak pointers to |this| that will to be referenced from the network thread". I did think for a moment, that this could be named "weak_factory_for_heartbeat_callbacks", because this is what it is used for at the moment, but affinity with network thread is a more generic, truer property of this field. https://codereview.chromium.org/719983002/diff/240001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1350: base::TimeDelta::FromSeconds(30) /* timeout */); On 2014/12/05 22:19:49, Lambros wrote: > 30s feels much too long to be delaying process exit. I think the Python script > sends SIGTERM, waits 10s, then sends SIGKILL. So anything more than 10s is > pointless here, unless you update the Python script as well. > See > https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/linu... I'll change this to 10 seconds. Note that this timeout has no relation to the timeout used by the python script - everything around SendHostOfflineReason happens within the host process, without input or interference from the python script (i.e. I assume that you pointed the python script's behavior as an example of a reasonable process shutdown timeout, not to point out that there is an interaction of some kind between the python script and the host sending a host-offline-reason). https://codereview.chromium.org/719983002/diff/240001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1359: base::TimeDelta::FromSeconds(30) /* timeout */); On 2014/12/05 22:19:48, Lambros wrote: > Same here. Done.
https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter_unittest.cc (right): https://codereview.chromium.org/719983002/diff/240001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter_unittest.cc:128: // |ack_or_timeout_callback| should be dropped after being called once. On 2014/12/05 23:59:28, lukasza wrote: > On 2014/12/05 22:19:48, Lambros wrote: > > I think you can remove this? It's just a duplicate of the comment above. > > I don't see how this is a duplicate :-/ This is the same comment as on line 99 (in patch set #12). I don't think we need to duplicate comments within the same file, but I don't feel strongly about it. https://codereview.chromium.org/719983002/diff/240001/remoting/host/minimum_h... File remoting/host/minimum_heartbeat_supporter.h (right): https://codereview.chromium.org/719983002/diff/240001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:96: // - |network_task_runner_| is used by all the other fields and has to be On 2014/12/05 23:59:28, lukasza wrote: > On 2014/12/05 22:19:48, Lambros wrote: > > |network_task_runner_| is ref-counted, so it doesn't matter where it goes? > > The other objects don't hold a reference to network_task_runner_, > but rather go through TLS storage to get a generic TaskRunner. > Hmm, that doesn't feel right, but I can see why you'd want to destroy (dereference) the AutoThreadTaskRunner last. https://codereview.chromium.org/719983002/diff/240001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/240001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1350: base::TimeDelta::FromSeconds(30) /* timeout */); On 2014/12/05 23:59:28, lukasza wrote: > On 2014/12/05 22:19:49, Lambros wrote: > > 30s feels much too long to be delaying process exit. I think the Python script > > sends SIGTERM, waits 10s, then sends SIGKILL. So anything more than 10s is > > pointless here, unless you update the Python script as well. > > See > > > https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/linu... > > I'll change this to 10 seconds. > > Note that this timeout has no relation to the timeout used by the python script > - everything around SendHostOfflineReason happens within the host process, > without input or interference from the python script (i.e. I assume that you > pointed the python script's behavior as an example of a reasonable process > shutdown timeout, not to point out that there is an interaction of some kind > between the python script and the host sending a host-offline-reason). No, there is an actual interaction. The Python script sends SIGTERM to this host process. That gets handled by SigTermHandler() defined in this file, which then calls through to this function here. Python will then wait only 10s before sending SIGKILL to this process, which cannot be trapped. https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter_unittest.cc (right): https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter_unittest.cc:106: // We are veryfying that such callbacks do not trigger nit: verifying https://codereview.chromium.org/719983002/diff/260001/remoting/host/minimum_h... File remoting/host/minimum_heartbeat_supporter.h (right): https://codereview.chromium.org/719983002/diff/260001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:36: // 1. Keep sending regulard heartbeats to the service. nit: regular
BTW: If tests in patchset#11 are slightly better off, then 1) complexity in mock_callback.h can be reduced if revisit earlier decision to switch away from WeakPtrFactory (i.e. mock_callback.h in https://codereview.chromium.org/734053003/patch/1/10004 is relatively simple when WeakPtrFactory is used) 2) we can consider whether replacing GetCallback with an implicit conversion operator further simplifies the tests (this would also mean that in the end MockCallback name is probably more appropriate than MockCallbackFactory).
I'm not convinced by the encapsulation here; it seems that the HostProcess could reasonably manage heart beating, and have a subcomponent responsible for running the actual host, that we can create and teardown as needed; that way the heartbeat sending stays the same regardless of the state the host is in, rather than having special cases during shutdown, for example. https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter.cc (right): https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.cc:16: class AckOrTimeoutReporter : public base::RefCounted<AckOrTimeoutReporter> { Why does this need to be ref-counted? The only thing that owns it is itself, and it deletes itself on ack or timeout, so there's no ambiguity there, surely? https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.cc:48: DCHECK(sequence_checker_.CalledOnValidSequencedThread()); This check is a no-op; the sequence checker binds to the thread on which it is created. Also, why do you have a SequenceChecker *and* a SequencedTaskRunner? If you hold a TaskRunner you can just check whether the current thread belongs to it via RunsTasksOnCurrentThread(). https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.cc:52: DCHECK(sequence_checker_.CalledOnValidSequencedThread()); SequenceChecker will do this in its own dtor. https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.cc:67: sequenced_task_runner_->PostDelayedTask(FROM_HERE, timeout_callback, timeout); Couldn't you use a OneShotTimer here rather than requiring the sequenced task runner and the WeakPtrFactory below? https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.cc:80: // of |ack_or_timeout_callback_|. How could anything that that callback does possibly trigger the others? https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter.h (right): https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.h:22: // acking, but doesn't support timeouts. By acking, do you mean a completion callback? Why would such a function be ACK but not provide a success/failure result? https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.h:37: void ReportAckOrTimeout( This name doesn't seem very helpful/descriptive; this function actually calls a supplied "callback" and then times-out if it doesn't "ack" within the specified |timeout|, so call this e.g. CallAsyncMethodWithTimeout() or similar. https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.h:37: void ReportAckOrTimeout( Why do you need this function at all? It seems it's only used to call the HeartbeatSender function - why not make that indicate success/failure and add a timeout there? https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.h:39: function_that_acks, nit: typedef this to something like AsyncFunction? https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.h:41: scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner, Why do you need this parameter, rather than using ThreadTaskRunnerHandle::Get()? https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter_unittest.cc (right): https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter_unittest.cc:66: .WillOnce(DoAll(SaveArg<1>(captured_timeout_callback), Return(true))); You shouldn't be checking for this call; tests should verify a specific intended behaviour, not a specific sequence of calls. In this case, you want to verify that ReportAckOrTimeout() will call you back with Ack if passed a function that Acks, or with Timeout if you call it with a function that never acks, or with one that acks, but too late - in the latter case you should ideally "fast forward" time via a fake base::TickClock, rather than actually waiting for a (short) timeout to expire. https://codereview.chromium.org/719983002/diff/260001/remoting/host/minimum_h... File remoting/host/minimum_heartbeat_supporter.h (right): https://codereview.chromium.org/719983002/diff/260001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:37: // 2. Keep the host process alive while sending host-offline-reason heartbeat. Would it be cleaner to leave HostProcess in charge of heartbeats and move the Chromoting-host related parts of that into a sub-component that can be created and torn-down as needed? https://codereview.chromium.org/719983002/diff/260001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:38: class MinimumHeartbeatSupporter This seems a not-very-helpful name - it's not clear at a first glance what this does that HeartbeatSender does not. https://codereview.chromium.org/719983002/diff/260001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:39: : public base::RefCounted<MinimumHeartbeatSupporter> { Noooo! Does this really need to be ref-counted? https://codereview.chromium.org/719983002/diff/260001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:43: // class to read and store config/policy/cmdline values. We store config in a dictionary; we specifically removed the sub-class for that because it looked just like a DictionaryValue. ;) https://codereview.chromium.org/719983002/diff/260001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/260001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:338: base::WeakPtrFactory<HostProcess> weak_factory_for_network_task_runner_; This should just be |weak_factory_| and the comment needn't describe the threading restriction on WeakPtrFactory, since that's inherent in what WeakPtrs are. https://codereview.chromium.org/719983002/diff/260001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:339: }; While you're here this should have DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/719983002/diff/260001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1237: weak_factory_for_network_task_runner_.GetWeakPtr()), Why are we passing three Callbacks here rather than encapsulating the functionality as an interface that HostProcess can then implement? https://codereview.chromium.org/719983002/diff/260001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1245: DCHECK(!minimum_heartbeat_supporter_.get()); nit: Remove the .get() https://codereview.chromium.org/719983002/diff/260001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1380: // Need to invalidate WeakPtrs on the same thread where they are dereferenced. nit: This is always a requirement for invalidating them, not an implementation limitation, so not really worth documenting here. https://codereview.chromium.org/719983002/diff/260001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1386: minimum_heartbeat_supporter_ = nullptr; Why are you NULLing this here?
I made small tweaks to address easy feedback from Wez and Lambros. I held back on making other changes, until we can agree on the direction of changes (i.e. I cannot at the same time address the comment that asks for removing explicit sequencing assert in AckOrTimeoutReporter's destructor *and* switch to using task_runner-based asserts [which require the explicit assert in the destructor]). Wez - could you please take another look (mostly at my replies in Rietveld where I am trying to address your feedback). Thanks, Lukasz https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter.cc (right): https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.cc:16: class AckOrTimeoutReporter : public base::RefCounted<AckOrTimeoutReporter> { On 2014/12/09 00:51:58, Wez wrote: > Why does this need to be ref-counted? The only thing that owns it is itself, and > it deletes itself on ack or timeout, so there's no ambiguity there, surely? "The only thing that owns it is itself" is not true. It is owned by 1) itself and 2) the ReportAckOrTimeout method. I think it needs to be owned from those 2 places: 1) holding a reference only from self can end up pulling a rag from under ReportAckOrTimeout (which needs the object while calling the Execute instance method); the rag can be pulled if function_that_acks ends up running a nested task message pump. 2) holding a reference only from the ReportAckOrTimeout method means that the AckOrTimeoutReporter object can be destroyed too early (i.e. causing callbacks to become no-ops [via weak ptrs] too early). I thought about holding a scoped_ptr in ReportAckOrTimeout function and "Pass"-ing it to AckOrTimeoutReporter, but this cannot work - the pointer cannot be simulteanously passed as an argument *and* at the same time used as "this" in instance method invocation. https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.cc:48: DCHECK(sequence_checker_.CalledOnValidSequencedThread()); On 2014/12/09 00:51:57, Wez wrote: > This check is a no-op; the sequence checker binds to the thread on which it is > created. > > Also, why do you have a SequenceChecker *and* a SequencedTaskRunner? If you hold > a TaskRunner you can just check whether the current thread belongs to it via > RunsTasksOnCurrentThread(). Good point. This was written this way before I was requiring a SequencedTaskRunner to be passed in. I'll switch to checking via task_runner (if we reach an agreement that using an explicit SequencedTaskRunner is a good idea - this is being discussed in other comments). https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.cc:52: DCHECK(sequence_checker_.CalledOnValidSequencedThread()); On 2014/12/09 00:51:57, Wez wrote: > SequenceChecker will do this in its own dtor. I'll keep this (because based on another comment I will be probably switching back to doing the checks explicitly / via task_runner). https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.cc:67: sequenced_task_runner_->PostDelayedTask(FROM_HERE, timeout_callback, timeout); On 2014/12/09 00:51:58, Wez wrote: > Couldn't you use a OneShotTimer here rather than requiring the sequenced task > runner and the WeakPtrFactory below? I don't understand the proposal. - Requirement #1: AckOrTimeoutReporter needs to cancel the ack_callback when timeout_callback happens (and the other way round as well). - Requirement #2: AckOrTimeoutReporter should be released from memory after the external ack_or_timeout_callback is called. - Design decision #3: ack_callback and timeout_callback need to happen in sequence (an alternative design decision would be to enforce thread safety in some other way) - Requirement #4: Make this easy to unit tests. I don't understand how a OneShotTimer can achieve #1 without requiring long-lived memory with a flag that flags whether a callback already happened (violating #2). I guess requirement #2 could be relaxed to say only |ack_or_timeout_callback| needs to be released. OneShotTimer will find task runner via TLS, but there is no guarantee that this is the same task runner that will be used for calling ack_callback (in fact, with the current code, it is perfectly fine to call ReportAckOrTimeout from *another* task runner). We could ask the caller to only call ReportAckOrTimeout from the same task_runner that will be sued to call ack_callback, but 1) explicit sequenced_task_runner seems to be a cleaner, more direct way of explaining this contract and 2) explicit sequenced_task_runner doesn't unnecessarily constraint on what task runner the ReportAckOrTimeout function can be called. Having an explicit timer-like object also helps with dependency injection for unit testing (#4). I guess I could always mock a timer stored in TLS, but it seems like more trouble than just explicitly passing a mock sequence task runner. https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.cc:80: // of |ack_or_timeout_callback_|. On 2014/12/09 00:51:57, Wez wrote: > How could anything that that callback does possibly trigger the others? i.e. it could start a nested TaskRunner pump. https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter.h (right): https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.h:22: // acking, but doesn't support timeouts. On 2014/12/09 00:51:58, Wez wrote: > By acking, do you mean a completion callback? > > Why would such a function be ACK but not provide a success/failure result? I am not sure if I understand the question. If by "provide [...] failure result" you mean "provide timeout result", then the "why" answer is: it is easier to unit test a generic ReportAckOrTimeout function, then timeout embedded inside HeartbeatSender or MinimumHeartbeatSender (the timeout is in MinimumHeartbeatSender in patchset#1, adding unit tests prompted extracting this functionality to a separate class). If the above misunderstands your question, then maybe we can discuss futher by looking at a concrete |function_that_acks| - a partially bound HeartbeatSender::SendHostOfflineReason. When SendHostOfflineReason calls ack_callback, then the host-offline-reason has been sent to and accepted by the bot and directory service. BTW: Do you think I should also rename ack_callback to completion_callback in HeartbeatSender::SendHostOfflineReason? https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.h:37: void ReportAckOrTimeout( On 2014/12/09 00:51:58, Wez wrote: > Why do you need this function at all? It seems it's only used to call the > HeartbeatSender function - why not make that indicate success/failure and add a > timeout there? I find it easier to unit test a generic ReportAckOrTimeout, than the same functionality hardwired inside HeartbeatSender or MinimumHeartbeatSupporter (in patchset#1 the timeout was handled inside MinimumHeartbeatSupporter and consequently there were no unit tests). https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.h:37: void ReportAckOrTimeout( On 2014/12/09 00:51:58, Wez wrote: > This name doesn't seem very helpful/descriptive; this function actually calls a > supplied "callback" and then times-out if it doesn't "ack" within the specified > |timeout|, so call this e.g. CallAsyncMethodWithTimeout() or similar. I can rename (if we agree that having a ReportAckOrTimeout functionality outside of HeartbeatSender / MinimumHeartbeatSupporter is desirable). https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.h:39: function_that_acks, On 2014/12/09 00:51:58, Wez wrote: > nit: typedef this to something like AsyncFunction? I like that the reader can see the signature of function_that_acks right here, without having to hunt down typedefs... I'd rather leave it as is. https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.h:41: scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner, On 2014/12/09 00:51:58, Wez wrote: > Why do you need this parameter, rather than using ThreadTaskRunnerHandle::Get()? This way I can avoid unnecessarily constraining ReportAckOrTimeout to only be called on the same task_runner as the runner used to invoke ack_callback. https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter_unittest.cc (right): https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter_unittest.cc:66: .WillOnce(DoAll(SaveArg<1>(captured_timeout_callback), Return(true))); On 2014/12/09 00:51:58, Wez wrote: > You shouldn't be checking for this call; tests should verify a specific intended > behaviour, not a specific sequence of calls. > > In this case, you want to verify that ReportAckOrTimeout() will call you back > with Ack if passed a function that Acks, or with Timeout if you call it with a > function that never acks, or with one that acks, but too late - in the latter > case you should ideally "fast forward" time via a fake base::TickClock, rather > than actually waiting for a (short) timeout to expire. I also don't like tight coupling between tests and production code in this case. Can you please explain more about basing the test on base::TickClock? How would I tie together a test-controlled TickClock with either sequenced_task_runner->PostDelayedTask and/or OneShotTimer? Also - I note that: - The test doesn't enforce a specific *sequence* of calls - the calls can happen in any order. - This is not that different from other, existing tests (i.e. HeartbeatSender uses a public interface of XmppSignalStrategy - this interface gets mocked in test code). https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter_unittest.cc:106: // We are veryfying that such callbacks do not trigger On 2014/12/06 01:25:05, Lambros wrote: > nit: verifying Done. https://codereview.chromium.org/719983002/diff/260001/remoting/host/minimum_h... File remoting/host/minimum_heartbeat_supporter.h (right): https://codereview.chromium.org/719983002/diff/260001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:36: // 1. Keep sending regulard heartbeats to the service. On 2014/12/06 01:25:05, Lambros wrote: > nit: regular Done. https://codereview.chromium.org/719983002/diff/260001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:37: // 2. Keep the host process alive while sending host-offline-reason heartbeat. On 2014/12/09 00:51:58, Wez wrote: > Would it be cleaner to leave HostProcess in charge of heartbeats and move the > Chromoting-host related parts of that into a sub-component that can be created > and torn-down as needed? I like the fact that MinimumHeartbeatSupporter has a relatively narrow interface - users call Create method and afterwards don't have to worry about creating and maintaining lifetime of the 4 objects that cooperate to send heartbeats - everything is taken care of by MinimumHeartbeatSupporter. I don't understand what you mean by "Chromoting-host related parts". Fields encapsulated by MinimumHeartbeatSupporter are what is needed to send heartbeats (including when sending a host-offline-reason). We want to tear down everything else when minimizing the security risk associated with malformed policy. https://codereview.chromium.org/719983002/diff/260001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:38: class MinimumHeartbeatSupporter On 2014/12/09 00:51:58, Wez wrote: > This seems a not-very-helpful name - it's not clear at a first glance what this > does that HeartbeatSender does not. What name would you suggest? RE: the difference from HeartbeatSender HeartbeatSender periodically sends heartbeats once something else connects a signal strategy. MinimumHeartbeatSender periodically sends heartbeats, without needing other objects for keeping the signal strategy connected - it itself takes care of connecting and authtenticating. RE: This seems a not-very-helpful name When picking MinimumHeartbeatSupporter I was trying to convey the fact that keeping this object alive is a minimum required to keep sending heartbeats (i.e. that this is a life-support system for heartbeats). https://codereview.chromium.org/719983002/diff/260001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:39: : public base::RefCounted<MinimumHeartbeatSupporter> { On 2014/12/09 00:51:58, Wez wrote: > Noooo! Does this really need to be ref-counted? It is kept alive by 1) HostProcess (wants to keep sending heartbeats as long as the host is running) and 2) itself (to give it a chance to send host-offline-reason during error-related shutdown). More than 1 owner = have to have ref-counting? I think we cannot pass scoped_ptr<MinimumHeartbeatSupporter> from HostProcess to MinimimHeartbeatSupporter - same comment as in report_ack_or_timeout.cc (cannot pass scoped_ptr<T>.Pass as an argument and at the same time use it as "this" when calling an instance method). https://codereview.chromium.org/719983002/diff/260001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:43: // class to read and store config/policy/cmdline values. On 2014/12/09 00:51:58, Wez wrote: > We store config in a dictionary; we specifically removed the sub-class for that > because it looked just like a DictionaryValue. ;) Acknowledged. OTOH, I thought more about strongly-typed interface (i.e. having a |host_id| property, rather than passing a "host_id" as a string to a dictionary-like interface). https://codereview.chromium.org/719983002/diff/260001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/260001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:338: base::WeakPtrFactory<HostProcess> weak_factory_for_network_task_runner_; On 2014/12/09 00:51:58, Wez wrote: > This should just be |weak_factory_| and the comment needn't describe the > threading restriction on WeakPtrFactory, since that's inherent in what WeakPtrs > are. Maybe. This is what I started with, but I wasn't explicitly destroying the factory, meaning that the thread asserts in the factory were triggered. When fixing this and adding explicit destroying of the factory, I changed its name, so reader of ShutdownOnNetworkThread can more easily understand why this weak_factory is being destroyed in ShutdownOnNetworkThread and not elsewhere. https://codereview.chromium.org/719983002/diff/260001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:339: }; On 2014/12/09 00:51:58, Wez wrote: > While you're here this should have DISALLOW_COPY_AND_ASSIGN Ok. https://codereview.chromium.org/719983002/diff/260001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1237: weak_factory_for_network_task_runner_.GetWeakPtr()), On 2014/12/09 00:51:58, Wez wrote: > Why are we passing three Callbacks here rather than encapsulating the > functionality as an interface that HostProcess can then implement? We rely on WeakPtrs translating the callbacks into no-ops when the HostProcess is no longer alive. Doing this via interface is possible, but I think not as clean (i.e. MinimumHeartbeatSupporter cannot just pass through the callbacks to other objects, but has to have a flag indicating whether the listener is still alive + code to forward the callbacks/listeners from encapsulated objects - look at patchset#1 to see how it would look like [i.e. see SetListener method on MinimumHeartbeatSupporter + forwarding methods in implementation of MinimumHeartbeatSupporter class]; I think I prefer current code to patchset#1). https://codereview.chromium.org/719983002/diff/260001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1245: DCHECK(!minimum_heartbeat_supporter_.get()); On 2014/12/09 00:51:58, Wez wrote: > nit: Remove the .get() Done.
Added one more unit test for ReportAckOrTimeout. https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter.cc (right): https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.cc:16: class AckOrTimeoutReporter : public base::RefCounted<AckOrTimeoutReporter> { On 2014/12/09 18:47:07, lukasza wrote: > On 2014/12/09 00:51:58, Wez wrote: > > Why does this need to be ref-counted? The only thing that owns it is itself, > and > > it deletes itself on ack or timeout, so there's no ambiguity there, surely? > > "The only thing that owns it is itself" is not true. It is owned by 1) itself > and 2) the ReportAckOrTimeout method. I think it needs to be owned from those 2 > places: > 1) holding a reference only from self can end up pulling a rag from under > ReportAckOrTimeout (which needs the object while calling the Execute instance > method); the rag can be pulled if function_that_acks ends up running a nested > task message pump. > 2) holding a reference only from the ReportAckOrTimeout method means that the > AckOrTimeoutReporter object can be destroyed too early (i.e. causing callbacks > to become no-ops [via weak ptrs] too early). > > I thought about holding a scoped_ptr in ReportAckOrTimeout function and > "Pass"-ing it to AckOrTimeoutReporter, but this cannot work - the pointer cannot > be simulteanously passed as an argument *and* at the same time used as "this" in > instance method invocation. I've added a unit test where ack_callback is synchronously called by function_that_acks. In this case when ack_callback runs, the Execute instance method is still on the callstack. When adding this case, I thought that it will help enforce proper behavior wrt lifetime management. After adding this case, I realized that the call to the function_that_acks is the last statement of Execute method, so in theory it might be ok to rcelease AckOrTimeoutReporter from within AckOrTimeoutReporter::AckOrTimeout. Still - the test case is useful anyway (helps validate a valid, separate scenario), so I'll leave it in.
I extracted a kHostOfflineReasonTimeoutSeconds constant to go next to the constant introduced in https://codereview.chromium.org/784243003/
https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter.cc (right): https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.cc:80: // of |ack_or_timeout_callback_|. On 2014/12/09 18:47:07, lukasza wrote: > On 2014/12/09 00:51:57, Wez wrote: > > How could anything that that callback does possibly trigger the others? > > i.e. it could start a nested TaskRunner pump. That's specifically disallowed in Chromium production code; it's only do-able in tests AFAIK. https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter.h (right): https://codereview.chromium.org/719983002/diff/260001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.h:41: scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner, On 2014/12/09 18:47:07, lukasza wrote: > On 2014/12/09 00:51:58, Wez wrote: > > Why do you need this parameter, rather than using > ThreadTaskRunnerHandle::Get()? > > This way I can avoid unnecessarily constraining ReportAckOrTimeout to only be > called on the same task_runner as the runner used to invoke ack_callback. It's already constrained to be the same thread by the implementation, though, since you bind a WeakPtr to the ACK-or-timeout reporting class into the delayed task that you post to |sequenced_task_runner_|.
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
https://codereview.chromium.org/719983002/diff/320001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter.h (right): https://codereview.chromium.org/719983002/diff/320001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.h:37: void ReportAckOrTimeout( Can this be replaced with something like CreateCallbackWithTimeout() that would just creates a callback which is expected to be called in certain period of time? That would make both the implementation and use of this function cleaner. https://codereview.chromium.org/719983002/diff/320001/remoting/host/minimum_h... File remoting/host/minimum_heartbeat_supporter.cc (right): https://codereview.chromium.org/719983002/diff/320001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.cc:103: // Passing partially bound |SetHostOfflineReason| as |function_that_acks| nit: Add . at the end. Comments should be full sentences, it's not a full sentence without . https://codereview.chromium.org/719983002/diff/320001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.cc:103: // Passing partially bound |SetHostOfflineReason| as |function_that_acks| nit: SetHostOfflineReason() instead of |SetHostOfflineReason| please. Pipes are normally used to denote |variable_names| while () marks a function name. https://codereview.chromium.org/719983002/diff/320001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.cc:104: base::Bind(&HeartbeatSender::SetHostOfflineReason, If you replace ReportAckOrTimeout() with CreateCallbackWithTimeout() this code would look much simpler and more readable, e.g.: heartbeat_sender_->SetHostOfflineReason( CreateCallbackWithTimeout( timeout, network_task_runner_, base::Bind(&MinimumHeartbeatSupporter::OnAckOrTimeout, this)); https://codereview.chromium.org/719983002/diff/320001/remoting/host/minimum_h... File remoting/host/minimum_heartbeat_supporter.h (right): https://codereview.chromium.org/719983002/diff/320001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:35: // MinimumHeartbeatSupporter has 2 functions: I think you can easily separate these two features. There is no reason they need to be part of the same class. You can have MinimumHeartbeatSupporter just keep all the pieces that are necessary for XMPP connection and then separate class (e.g. OfflineReasonSender) that sends offline state on shutdown. That way MinimumHeartbeatSupporter wouldn't need to be ref-counted and you would pass its ovnership to OfflineReasonSender when shutting down the host. https://codereview.chromium.org/719983002/diff/320001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:38: class MinimumHeartbeatSupporter IMO that's not very descriptive name. It's essentially a holder for all the pieces that we need for signaling on the host. Maybe call it HostSignaling? https://codereview.chromium.org/719983002/diff/320001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:41: // TODO(lukasza): Refactor to limit the number of parameters below. Do you really need this Create() method? I suggest just making the constructor public and keep the code that creates HeartbeatSender, SignalStrategy, etc. in HostProcess class. https://codereview.chromium.org/719983002/diff/320001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:76: scoped_ptr<XmppSignalStrategy> signal_strategy, s/XmppSignalStrategy/SignalStrategy/. There is nothing in this class that require |signal_strategy| to use XMPP. https://codereview.chromium.org/719983002/diff/320001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/320001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1355: scoped_refptr<MinimumHeartbeatSupporter> tmpHeartbeatSupporter = heartbeat_supporter?
I addressed feedback from Wez and Sergey. Please take another look? https://codereview.chromium.org/719983002/diff/320001/remoting/host/ack_or_ti... File remoting/host/ack_or_timeout_reporter.h (right): https://codereview.chromium.org/719983002/diff/320001/remoting/host/ack_or_ti... remoting/host/ack_or_timeout_reporter.h:37: void ReportAckOrTimeout( On 2014/12/15 22:39:19, Sergey Ulanov wrote: > Can this be replaced with something like CreateCallbackWithTimeout() that would > just creates a callback which is expected to be called in certain period of > time? That would make both the implementation and use of this function cleaner. I am working on removing ack_or_timeout_reporter*.* altogether (and replacing them with timeout implementation inside heartbeat_sender*.*). https://codereview.chromium.org/719983002/diff/320001/remoting/host/minimum_h... File remoting/host/minimum_heartbeat_supporter.cc (right): https://codereview.chromium.org/719983002/diff/320001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.cc:103: // Passing partially bound |SetHostOfflineReason| as |function_that_acks| On 2014/12/15 22:39:19, Sergey Ulanov wrote: > nit: SetHostOfflineReason() instead of |SetHostOfflineReason| please. Pipes are > normally used to denote |variable_names| while () marks a function name. N/A after removing ReportAckOrTimeout. https://codereview.chromium.org/719983002/diff/320001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.cc:103: // Passing partially bound |SetHostOfflineReason| as |function_that_acks| On 2014/12/15 22:39:19, Sergey Ulanov wrote: > nit: Add . at the end. Comments should be full sentences, it's not a full > sentence without . N/A after removing ReportAckOrTimeout. https://codereview.chromium.org/719983002/diff/320001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.cc:104: base::Bind(&HeartbeatSender::SetHostOfflineReason, On 2014/12/15 22:39:19, Sergey Ulanov wrote: > If you replace ReportAckOrTimeout() with CreateCallbackWithTimeout() this code > would look much simpler and more readable, e.g.: > > heartbeat_sender_->SetHostOfflineReason( > CreateCallbackWithTimeout( > timeout, network_task_runner_, > base::Bind(&MinimumHeartbeatSupporter::OnAckOrTimeout, this)); N/A after removing ReportAckOrTimeout. https://codereview.chromium.org/719983002/diff/320001/remoting/host/minimum_h... File remoting/host/minimum_heartbeat_supporter.h (right): https://codereview.chromium.org/719983002/diff/320001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:35: // MinimumHeartbeatSupporter has 2 functions: On 2014/12/15 22:39:19, Sergey Ulanov wrote: > I think you can easily separate these two features. There is no reason they need > to be part of the same class. You can have MinimumHeartbeatSupporter just keep > all the pieces that are necessary for XMPP connection and then separate class > (e.g. OfflineReasonSender) that sends offline state on shutdown. That way > MinimumHeartbeatSupporter wouldn't need to be ref-counted and you would pass its > ovnership to OfflineReasonSender when shutting down the host. I think ownership management is orthogonal to having 2 functions/responsibilities - even with a separate OfflineReasonSender, it should be the MinimumHeartbeatSupporter that keeps the network_task_runner alive, right? I am working on removing ref-counting, following the pattern below suggested by Wez: scoped_ptr<T> x_; x_.release()->DoWorkAndDelete(); I don't understand what we would gain (after ref-counting is removed) by having a separate OfflineReasonSender - the separate object would just forward calls to heartbeat_sender and to minimum_heartbeat_supporter (aka host_signaling_manager). https://codereview.chromium.org/719983002/diff/320001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:38: class MinimumHeartbeatSupporter On 2014/12/15 22:39:19, Sergey Ulanov wrote: > IMO that's not very descriptive name. It's essentially a holder for all the > pieces that we need for signaling on the host. Maybe call it HostSignaling? I'll go ahead with HostSignalingManager. https://codereview.chromium.org/719983002/diff/320001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:41: // TODO(lukasza): Refactor to limit the number of parameters below. On 2014/12/15 22:39:19, Sergey Ulanov wrote: > Do you really need this Create() method? I suggest just making the constructor > public and keep the code that creates HeartbeatSender, SignalStrategy, etc. in > HostProcess class. Without the Create method, the heartbeat_sender, signaling_connector, network_change_notifier, dns_blackhole_checker would no longer be internal implementation details of MinimumHeartbeatSupporter (right now only minimum_heartbeat_supporter.h/.cc refers to these 4 things - I think this encapsulation is desirable and I don't want to reintroduce these dependencies back to HostProcess). https://codereview.chromium.org/719983002/diff/320001/remoting/host/minimum_h... remoting/host/minimum_heartbeat_supporter.h:76: scoped_ptr<XmppSignalStrategy> signal_strategy, On 2014/12/15 22:39:19, Sergey Ulanov wrote: > s/XmppSignalStrategy/SignalStrategy/. There is nothing in this class that > require |signal_strategy| to use XMPP. Good point. Done. https://codereview.chromium.org/719983002/diff/320001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/320001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1355: scoped_refptr<MinimumHeartbeatSupporter> tmpHeartbeatSupporter = On 2014/12/15 22:39:19, Sergey Ulanov wrote: > heartbeat_supporter? Yikes. Muscle memory never dies... Thanks for catching this.
CL description needs overhauling! :D
https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:109: static void StaticCallHostOfflineReasonAckCallback( nit: Maybe just call it RunAckCallback(). nit: Move this to anonymous namespace, without any "static". https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:117: // We go through PostTask (rather than directly calling Run), I don't understand why you have to make this asynchronous. Doing it this way means you're breaking the atomicity of (calling the callback + stopping the timer), surely? https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:123: base::Bind(StaticCallHostOfflineReasonAckCallback, Are you sure you really need this? It seems very sad that base::Bind() can't take a Callback directly as its first parameter, and expects either a bare function or a class method. https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:265: host_offline_reason_timeout_timer_.Stop(); Maybe stop the timer first? https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.h (right): https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:104: // intiates sending a stanza right away. typo: initiates (maybe "initiating"? Or better: "..stanzas, and initiates..") https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:106: // See rem:host-offline-reason class-level comments for discussion of nit: What is "rem:host-offline-reason"? If it's an actual class, use the real name (with or without the remoting:: prefix). https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:161: base::OneShotTimer<HeartbeatSender> host_offline_reason_timeout_timer_; Maybe just host_offline_reason_timer_? https://codereview.chromium.org/719983002/diff/340001/remoting/host/host_sign... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/719983002/diff/340001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:37: class HostSignalingManager { Please update the CL description with the new class name. https://codereview.chromium.org/719983002/diff/340001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:67: void SendHostOfflineReasonAndDelete(const std::string& host_offline_reason, Won't you get double-deletion when the scoped_ptr returned from Create() goes out of scope? If the object is self-owned, perhaps Create() shouldn't be returning a scoped_ptr? Also, if the object is responsible for its own deletion, the dtor should be private? https://codereview.chromium.org/719983002/diff/340001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:97: HostSignalingManager* self_; There's no ref-counting, so you don't need this. https://codereview.chromium.org/719983002/diff/340001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/340001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1254: state_ == HOST_STOPPED) << state_; I don't think I've seen this construct before. Will it produce a readable log message?
I am not sure how I managed to miss the email notifying me of pending code review comments 2 days ago. Anyway - I addressed the comments (including adjusting the changelist description) and this is ready for another look please. https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:109: static void StaticCallHostOfflineReasonAckCallback( On 2014/12/16 02:14:05, Lambros wrote: > nit: Maybe just call it RunAckCallback(). > nit: Move this to anonymous namespace, without any "static". Done (except I renamed to RunHostOfflineReasonAckCallback). https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:117: // We go through PostTask (rather than directly calling Run), On 2014/12/16 02:14:05, Lambros wrote: > I don't understand why you have to make this asynchronous. Doing it this way > means you're breaking the atomicity of (calling the callback + stopping the > timer), surely? RE: why Requiring the callers (and callers of the callers) to always do the call as the last statement seems icky. RE: breaking the atomicity What I really care about is that when a bot ack, then the host_offline_reason_ack_callback will be run exactly once - I don't care about the slight delay introduced by PostTask - The "exactly once" is enforced by calling Reset on the callback field (this is independent from OneShotTimer::Stop call which is only needed for hygiene / releasing other memory when ok to do so). - I do care that it is ok for the callback to destroy HeartbeatSender (this is why PostTask to call the callback with no more frames with heartbeat sender on the callstack) https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:123: base::Bind(StaticCallHostOfflineReasonAckCallback, On 2014/12/16 02:14:04, Lambros wrote: > Are you sure you really need this? It seems very sad that base::Bind() can't > take a Callback directly as its first parameter, and expects either a bare > function or a class method. I tried passing the callback object by value - this didn't compile. I tried to look through the comments of base/callback.h and base/bind_helpers.h but didn't see how to achieve this. I'll send a post to chromium-dev to ask. https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:265: host_offline_reason_timeout_timer_.Stop(); On 2014/12/16 02:14:04, Lambros wrote: > Maybe stop the timer first? Done. https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.h (right): https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:104: // intiates sending a stanza right away. On 2014/12/16 02:14:05, Lambros wrote: > typo: initiates (maybe "initiating"? Or better: "..stanzas, and initiates..") Done. https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:106: // See rem:host-offline-reason class-level comments for discussion of On 2014/12/16 02:14:05, Lambros wrote: > nit: What is "rem:host-offline-reason"? If it's an actual class, use the real > name (with or without the remoting:: prefix). This is an xml attribute referred to in the class-level comments at the top. I'll qualify the comment here. https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:161: base::OneShotTimer<HeartbeatSender> host_offline_reason_timeout_timer_; On 2014/12/16 02:14:05, Lambros wrote: > Maybe just host_offline_reason_timer_? I think I'll just leave the current name - more accurate + not much worse in terms of readability/length from the proposal. https://codereview.chromium.org/719983002/diff/340001/remoting/host/host_sign... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/719983002/diff/340001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:37: class HostSignalingManager { On 2014/12/16 02:14:05, Lambros wrote: > Please update the CL description with the new class name. Ooops. Done. https://codereview.chromium.org/719983002/diff/340001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:67: void SendHostOfflineReasonAndDelete(const std::string& host_offline_reason, On 2014/12/16 02:14:05, Lambros wrote: > Won't you get double-deletion when the scoped_ptr returned from Create() goes > out of scope? If the object is self-owned, perhaps Create() shouldn't be > returning a scoped_ptr? > Also, if the object is responsible for its own deletion, the dtor should be > private? The caller of Create takes ownership of the returned scoped_ptr. Before calling SendHostOfflineReasonAndDelete, the owner is supposed to release the scoped_ptr first. https://codereview.chromium.org/719983002/diff/340001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:97: HostSignalingManager* self_; On 2014/12/16 02:14:05, Lambros wrote: > There's no ref-counting, so you don't need this. I do. I am calling "delete self_" in OnAckOrTimeout. https://codereview.chromium.org/719983002/diff/340001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/340001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1254: state_ == HOST_STOPPED) << state_; On 2014/12/16 02:14:05, Lambros wrote: > I don't think I've seen this construct before. Will it produce a readable log > message? Semi-readable. I'll fix by prepending "state = " to the message. For general usage see base/logging.h, in particular the comments here: https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&l=46 I double-checked that the streaming works the way I understood the comments in base/... by adding the following to 1 of unit tests: int x = 123; DCHECK(x == 456) << x; This produced a crash with the following as the top message. Notice the trailing "123": [10311:10311:1217/095226:84800221205:FATAL:heartbeat_sender_unittest.cc(293)] Check failed: x == 456. 123 BTW: I hope that the string literal added by me will be compiled-away on non-Debug builds and won't end up increasing the size of the host binary/executable.
And I also switched to better/proper callback currying in patchset #18.
Still lgtm, but please look at my comments. https://codereview.chromium.org/719983002/diff/340001/remoting/host/host_sign... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/719983002/diff/340001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:67: void SendHostOfflineReasonAndDelete(const std::string& host_offline_reason, On 2014/12/17 18:02:04, lukasza wrote: > On 2014/12/16 02:14:05, Lambros wrote: > > Won't you get double-deletion when the scoped_ptr returned from Create() goes > > out of scope? If the object is self-owned, perhaps Create() shouldn't be > > returning a scoped_ptr? > > Also, if the object is responsible for its own deletion, the dtor should be > > private? > > The caller of Create takes ownership of the returned scoped_ptr. Before calling > SendHostOfflineReasonAndDelete, the owner is supposed to release the scoped_ptr > first. So the caller owns the object until it calls SendHostOfflineReasonAndDelete() which effectively takes back ownership from the caller? In that case, I'd prefer if SendHostOfflineReasonAndDelete() accepted a scoped_ptr to the object. It could be a static method, or perhaps a non-static method with DCHECK(param.get() == this); Admittedly, this seems strange, but I think it's the right thing to do. Check with others on our team. Chromium does use the pattern of ptr.release()->DoStuffAndDelete() a lot, but bear in mind that such patterns were in common usage before Chromium gained a scoped_ptr with moveable semantics. https://codereview.chromium.org/719983002/diff/340001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:97: HostSignalingManager* self_; On 2014/12/17 18:02:04, lukasza wrote: > On 2014/12/16 02:14:05, Lambros wrote: > > There's no ref-counting, so you don't need this. > > I do. I am calling "delete self_" in OnAckOrTimeout. Couldn't you do "delete this;" instead? https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:112: base::Closure fully_bound_ack_callback = nit: Don't need the temporary. We do PostTask(FROM_HERE, base::Bind(... a lot in our code. https://codereview.chromium.org/719983002/diff/380001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1235: DCHECK(!host_id_.empty()); // Assumming |ApplyConfig| has already been run. typo: Assuming Maybe better: |ApplyConfig| should already have been run. https://codereview.chromium.org/719983002/diff/380001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1254: << "state_ = " << state_; nit: Will fit on previous line.
Thanks for the feedback Lambros - I appreciate it (especially catching the self_ sillyness :-/). Wez/Sergey - could you please take a look? https://codereview.chromium.org/719983002/diff/340001/remoting/host/host_sign... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/719983002/diff/340001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:67: void SendHostOfflineReasonAndDelete(const std::string& host_offline_reason, On 2014/12/17 23:44:34, Lambros wrote: > On 2014/12/17 18:02:04, lukasza wrote: > > On 2014/12/16 02:14:05, Lambros wrote: > > > Won't you get double-deletion when the scoped_ptr returned from Create() > goes > > > out of scope? If the object is self-owned, perhaps Create() shouldn't be > > > returning a scoped_ptr? > > > Also, if the object is responsible for its own deletion, the dtor should be > > > private? > > > > The caller of Create takes ownership of the returned scoped_ptr. Before > calling > > SendHostOfflineReasonAndDelete, the owner is supposed to release the > scoped_ptr > > first. > > So the caller owns the object until it calls SendHostOfflineReasonAndDelete() > which effectively takes back ownership from the caller? > In that case, I'd prefer if SendHostOfflineReasonAndDelete() accepted a > scoped_ptr to the object. It could be a static method, or perhaps a non-static > method with DCHECK(param.get() == this); > Admittedly, this seems strange, but I think it's the right thing to do. Check > with others on our team. > Chromium does use the pattern of ptr.release()->DoStuffAndDelete() a lot, but > bear in mind that such patterns were in common usage before Chromium gained a > scoped_ptr with moveable semantics. I don't have a strong opinion here (with no recent C++ experience under my belt). The current code is an implementation of option #4 as recommended by Wez: Re the ownership hand-off, I agree that it's not ideal. I'd suggest: 1) // Take a bare pointer to the XMPP manager before passing it to itself to delete. (Essentially the same as your suggestion). HostXmppManager* host_xmpp_manager_temp = host_xmpp_manager.get(); host_xmpp_manager_temp->SendShutdownReasonAndDeleteSelf(host_xmpp_manager.Pass()); or 2) // Pass the pointer-to-scoped_ptr so that the function can perform the Pass() internally. host_xmpp_manager->SendShutdownReasonAndDeleteSelf(&host_xmpp_manager); or 3) // Variant of #2 in which we use a static class method, which internally does a similar dance to #1, thereby keeping the caller simple. HostXmppManager::SendShutdownReasonAndDeleteManager(host_xmpp_manager.Pass()); or 4) // Release the manager as part of the call, so that its self-ownership becomes implicit. host_xmpp_manager.release()->SendShutdownReasonAndDelete(); #4 is probably the approach I'd take; I don't think having the object own itself via a scoped_ptr<> is necessary (I'm not even sure it'll work, since IIRC the object gets deleted by scoped_ptr<> before the contained pointer gets NULL'd, so you'd end up using-after-free) - instead the object can just |delete this| when it's done. Thanks for persevering with this; I appreciate the temptation of ref-counting in situations like this, but our experience in Chromium development has been that it leads to more problems if not used sparingly! https://codereview.chromium.org/719983002/diff/340001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:97: HostSignalingManager* self_; On 2014/12/17 23:44:34, Lambros wrote: > On 2014/12/17 18:02:04, lukasza wrote: > > On 2014/12/16 02:14:05, Lambros wrote: > > > There's no ref-counting, so you don't need this. > > > > I do. I am calling "delete self_" in OnAckOrTimeout. > > Couldn't you do "delete this;" instead? Done. https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:112: base::Closure fully_bound_ack_callback = On 2014/12/17 23:44:34, Lambros wrote: > nit: Don't need the temporary. We do > PostTask(FROM_HERE, base::Bind(... > a lot in our code. Acknowledged. https://codereview.chromium.org/719983002/diff/380001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1235: DCHECK(!host_id_.empty()); // Assumming |ApplyConfig| has already been run. On 2014/12/17 23:44:34, Lambros wrote: > typo: Assuming > Maybe better: |ApplyConfig| should already have been run. Done. https://codereview.chromium.org/719983002/diff/380001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1254: << "state_ = " << state_; On 2014/12/17 23:44:34, Lambros wrote: > nit: Will fit on previous line. It was on a previous line until I did "git cl format" :-P
https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:108: // work with base::Bind inside CallHostOfflineReasonAckCallback below. This description is not very helpful; I think all you really want to say is that this method takes a supplied |ack_callback| and Run()s it with the bound |success| value? https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:109: static void StaticCallHostOfflineReasonAckCallback( CallHostOfflineReasonAckCallback https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:115: void HeartbeatSender::CallHostOfflineReasonAckCallback( PostHostOfflineReasonAckCallback https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.h (right): https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:111: enum AckOrTimeout { Ack, Timeout }; Elsewhere we just return a |bool success| parameter to indicate success or failure; why not do that here? https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... File remoting/host/heartbeat_sender_unittest.cc (right): https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender_unittest.cc:276: base::Bind(&MockAckCallback::Run, base::Unretained(&mock_ack_callback))); How is this related to the purpose of this test? https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender_unittest.cc:320: .Times(1); Tests should not mix EXPECT_CALL() in with logic that touches the relevant object - the behaviour is then undefined. If you want to expect a sequence of callbacks from these various test-cases then use an InSequence to force the ordering of your EXPECT_CALL() calls and run things that way. Or create a separate callback object for each run. https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:111: // of our callers to ensure that they call us last. This is a common pattern, so doesn't need such a long explanation, e.g. "Run the ACK callback under a clean stack via PostTask()." https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:114: base::MessageLoop::current()->PostTask(FROM_HERE, fully_bound_ack_callback); nit: Use ThreadTaskRunnerHandle::Get() here. https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:130: base::Unretained(this), AckOrTimeout::Timeout)); Re-using CallHostOfflineReasonAckCallback here really doesn't seem to save you much/anything. Suggest simply having two methods e.g. OnHostOfflineReasonAck() and OnHostOfflineReasonTimeout(), which IMO will be more readable anyway. https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:253: host_offline_reason_timeout_timer_.Stop(); This seems wrong - surely you want to stop the timer even if the callback is already null? How can the ACK callback already be null, though? https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.h (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:112: enum AckOrTimeout { Ack, Timeout }; Why do we need this rather than a success/failure boolean? https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... File remoting/host/host_signaling_manager.cc (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:40: const ChromotingHostContext& host_context, You only use network_task_runner() and url_context_getter() so you should just pass those in directly, rather than the entire context. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:50: // Create a NetworkChangeNotifier for use by the signaling connector. This comment seems to add nothing? https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:54: scoped_ptr<XmppSignalStrategy> signal_strategy(new XmppSignalStrategy( nit: Here & below, IIRC style guide prefers wrapping at the outermost point that makes sense, e.g. in this case immediately after signal_strategy( unless that makes the wrapping much more cumbersome. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:65: if (!oauth_refresh_token.empty()) { What does it mean for the OAuth refresh token to be empty? What happens then? https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:91: HOST_LOG << "HostSignalingManager destroyed."; Is this really important enough to put in the host log? Maybe [D]VLOG(2) it? https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:102: self_ = this; // We own ourselves until the ack-or-timeout callback fires. This has no effect, though..? https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:110: HeartbeatSender::AckOrTimeout ack_or_timeout) { As noted elsewhere, suggest re-expressing this as a simple |bool success| https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:119: default: You're already handling all the possible values, so get rid of the "default" case. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:124: delete self_; delete this https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:35: // 1. Keep sending regular heartbeats to the service. You mean to the Chromoting Directory, I think? https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:36: // 2. Keep the host process alive while sending host-offline-reason heartbeat. nit: The second function is to make a best-effort to deliver the shutdown reason to the cloud - the need for keeping the process alive is just an implementation detail. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:41: // class to read and store config/policy/cmdline values. The parameters need documenting. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:45: const base::Closure& on_auth_failed_callback, As previously commented, these should be combined into a HostSignalingManager::EventHandler interface. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:46: const ChromotingHostContext& host_context, Ick... do you really need this? The ChromotingHostContext is really only intended as a convenience for creating the necessary threads, not to be passed around into other classes. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:51: const std::string& directory_bot_jid, what's this? https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:53: bool use_service_account); What does "use_service_account" mean? https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:57: // Gets signal strategy used for talking to the Chromoting bot. "Get the SignalStrategy to use to talk to..." https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:58: // Return value is valid until |this| gets deleted. nit: Suggest: "The SignalStrategy remains owned by the HostSignalingManager." https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:66: // process to exit. You don't need the sentence re the task-runner. The AutoThreadTaskRunner behaviour is implicitly relied upon by various bits of Chromoting, and we don't describe it everywhere; this class' responsibility is to keep itself alive until the offline reason is ack'd or times out, and it's the caller's responsibility to maintain the TaskRunners and process until it's done. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:80: // Order of fields below is important for destructing them in the right order. That cannot be true of |network_task_runner_|, so move it out of the commented block. It would probably be clearer to do the teardown explicitly in the destructor, with a comment before each .reset() to explain the ordering. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:81: // - |heartbeat_sender_| and |signaling_connector_| have to be destructed s/destructed/destroyed/ https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:88: // destructed last. This comment doesn't make sense, since all you're doing is dropping a reference, not destroying it. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:89: scoped_refptr<AutoThreadTaskRunner> network_task_runner_; You need to clarify what this task-runner is used for, i.e. the threading properties of the class. Looking at the implementation, you basically use it just to DCHECK that this class is always created, used & destroyed from the same thread, and to initialise the underlying signaling object - why not just use ThreadTaskRunnerHandle::Get() to grab the calling thread's |task_runner| in your constructor, so the caller needn't pass it in explicitly? Or use base::NonThreadSafe as a base-class and do checks based on that, and just use ThreadTaskRunnerHandle::Get() to get the TaskRunner to pass to the underlying signaling object. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:97: HostSignalingManager* self_; This is a bare pointer so it doesn't have any effect on the lifetime of anything, surely? https://codereview.chromium.org/719983002/diff/380001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:321: scoped_ptr<HostSignalingManager> host_signaling_manager_; nit: Add a brief comment to explain this member's purpose, e.g. "Used to send heartbeats while running, and the reason for going offline when shutting down." https://codereview.chromium.org/719983002/diff/380001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1242: weak_factory_for_network_task_runner_.GetWeakPtr()), When you replace this with a HostSignalingManager::EventHandler interface you can just have SendHostOfflineReasonAndDelete() NULL the |event_handler_| bare pointer that the HostSignalingManager holds, to ensure the HostProcess doesn't get used-after-free. https://codereview.chromium.org/719983002/diff/380001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1361: ShutdownOnNetworkThread(); This and the HOST_STARTED case are now almost identical, so I'd suggest folding their implementation into a small helper member function and having the HOST_INITIALIZING case just DCHECK, then create |host_signaling_manager_|, then call that same helper as the HostStarted case, to send the offline reason and shutdown.
Thanks for the feedback. I tried to address it in the latest patchset. Please take another look. https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:108: // work with base::Bind inside CallHostOfflineReasonAckCallback below. On 2014/12/18 00:46:20, Wez wrote: > This description is not very helpful; I think all you really want to say is that > this method takes a supplied |ack_callback| and Run()s it with the bound > |success| value? N/A - this function goes away in patchset #18 (it turns out that the way to bind a callback I just need to pass the callback as a first argument, rather than trying to pass Run method + callback pointer as I've been trying to do previously). https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:109: static void StaticCallHostOfflineReasonAckCallback( On 2014/12/18 00:46:20, Wez wrote: > CallHostOfflineReasonAckCallback N/A as of patchset #18 https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:115: void HeartbeatSender::CallHostOfflineReasonAckCallback( On 2014/12/18 00:46:20, Wez wrote: > PostHostOfflineReasonAckCallback Good point - thanks. https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.h (right): https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:111: enum AckOrTimeout { Ack, Timeout }; On 2014/12/18 00:46:20, Wez wrote: > Elsewhere we just return a |bool success| parameter to indicate success or > failure; why not do that here? Seemed cleaner this way (i.e. Timeout is more readable than "false" means in a call to base::Bind; OTOH, I can add a comment saying that "false" is a "success" value + I can see that bool is shorter and therefore results in cleaner declarations). Anyway - I replaced AckOrTimeout with bool in the latest patchset. https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... File remoting/host/heartbeat_sender_unittest.cc (right): https://codereview.chromium.org/719983002/diff/340001/remoting/host/heartbeat... remoting/host/heartbeat_sender_unittest.cc:320: .Times(1); On 2014/12/18 00:46:20, Wez wrote: > Tests should not mix EXPECT_CALL() in with logic that touches the relevant > object - the behaviour is then undefined. If you want to expect a sequence of > callbacks from these various test-cases then use an InSequence to force the > ordering of your EXPECT_CALL() calls and run things that way. Or create a > separate callback object for each run. According to GMock documentation the behavior is well defined. https://code.google.com/p/googlemock/wiki/ForDummies (I read that when writing the code to double-check my expectations of what would happen, not when replying to the CR feedback :-) has an explict section titled "Using Multiple Expectations" and says: "when a mock method is invoked, Google Mock will search the expectations in the reverse order they are defined, and stop when an active expectation that matches the arguments is found (you can think of it as "newer rules override older ones."). If the matching expectation cannot take any more calls, you will get an upper-bound-violated failure." So - in the test above I do not want to expect a *sequence* of calls. I want to waive the earlier expectation that ack_callback is not called. https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:111: // of our callers to ensure that they call us last. On 2014/12/18 00:46:20, Wez wrote: > This is a common pattern, so doesn't need such a long explanation, e.g. "Run the > ACK callback under a clean stack via PostTask()." I tried to shorten the comment following your suggestion. OTOH, Lambros was asking for an explanation why we need to go through the trouble of PostTask / why we care about a clean callstack, so I ended up inserting an explanation focusing on the "why". https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:114: base::MessageLoop::current()->PostTask(FROM_HERE, fully_bound_ack_callback); On 2014/12/18 00:46:20, Wez wrote: > nit: Use ThreadTaskRunnerHandle::Get() here. Done. Can you please give more details / help me understand why this is preferrable? I used message loop because it seemed more generic / more broadly applicabla (i.e. wasn't introducing a tight coupling between this PostTask call and SingleThreadTaskRunner [i.e. another SequencedTaskRunner would also work]). https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:130: base::Unretained(this), AckOrTimeout::Timeout)); On 2014/12/18 00:46:20, Wez wrote: > Re-using CallHostOfflineReasonAckCallback here really doesn't seem to save you > much/anything. Suggest simply having two methods e.g. OnHostOfflineReasonAck() > and OnHostOfflineReasonTimeout(), which IMO will be more readable anyway. Good point. Done. This also helped keep the timer and callback resetting logic in one place (only in the On...Timeout and On...Ack methods). Thanks. https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:253: host_offline_reason_timeout_timer_.Stop(); On 2014/12/18 00:46:20, Wez wrote: > This seems wrong - surely you want to stop the timer even if the callback is > already null? In this case the timer is already stopped. > How can the ACK callback already be null, though? Right - if the callback is null then the timer should have already been stopped. I've added explicit asserts in the new On...Timeout and On...Ack methods. https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.h (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:112: enum AckOrTimeout { Ack, Timeout }; On 2014/12/18 00:46:20, Wez wrote: > Why do we need this rather than a success/failure boolean? Done - I switched to bool. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... File remoting/host/host_signaling_manager.cc (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:40: const ChromotingHostContext& host_context, On 2014/12/18 00:46:21, Wez wrote: > You only use network_task_runner() and url_context_getter() so you should just > pass those in directly, rather than the entire context. I was passing them separately in the first draft, but 1) I prefer smaller number of consolidated arguments especially since 2) they are passed by *const*-ref and 3) Lambros already complained in his feedback that this method has too many parameters. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:50: // Create a NetworkChangeNotifier for use by the signaling connector. On 2014/12/18 00:46:21, Wez wrote: > This comment seems to add nothing? I am just copy&pasting from the code that was in HostProcess class. Somebody else thought that this comment was useful (when inserting it in the first place). I can remove, if you feel strongly about this. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:54: scoped_ptr<XmppSignalStrategy> signal_strategy(new XmppSignalStrategy( On 2014/12/18 00:46:21, Wez wrote: > nit: Here & below, IIRC style guide prefers wrapping at the outermost point that > makes sense, e.g. in this case immediately after signal_strategy( unless that > makes the wrapping much more cumbersome. IIRC Lambros complained that this fits on a single line so I folded. This also passes git cl format, so I'll just leave it as is. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:65: if (!oauth_refresh_token.empty()) { On 2014/12/18 00:46:21, Wez wrote: > What does it mean for the OAuth refresh token to be empty? What happens then? I don't know. Just copy&pasting existing code from HostProcess. If we suspect that something is wrong (or simply suspicious/smelly) here then I'd prefer to tackle this separately from this changelist. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:91: HOST_LOG << "HostSignalingManager destroyed."; On 2014/12/18 00:46:21, Wez wrote: > Is this really important enough to put in the host log? Maybe [D]VLOG(2) it? I hoped that this would help diagnose the case of a hang host (when you might not have an opportunity to restart with more verbose logging). OTOH, I don't feel strongly about this (and admittedly I don't have much experience judging how much logging is too little vs too much). I'll leave it in for now, but please push back if you think I should change to VLOG(2). https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:102: self_ = this; // We own ourselves until the ack-or-timeout callback fires. On 2014/12/18 00:46:21, Wez wrote: > This has no effect, though..? Yes. I don't know what I was thinking... :-( https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:110: HeartbeatSender::AckOrTimeout ack_or_timeout) { On 2014/12/18 00:46:21, Wez wrote: > As noted elsewhere, suggest re-expressing this as a simple |bool success| Done. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:119: default: On 2014/12/18 00:46:21, Wez wrote: > You're already handling all the possible values, so get rid of the "default" > case. N/A after the switch to bool. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:124: delete self_; On 2014/12/18 00:46:21, Wez wrote: > delete this Right. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:35: // 1. Keep sending regular heartbeats to the service. On 2014/12/18 00:46:22, Wez wrote: > You mean to the Chromoting Directory, I think? To Talk, Chromoting Directory and Chromoting Bot. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:36: // 2. Keep the host process alive while sending host-offline-reason heartbeat. On 2014/12/18 00:46:22, Wez wrote: > nit: The second function is to make a best-effort to deliver the shutdown reason > to the cloud - the need for keeping the process alive is just an implementation > detail. I think I'll leave it as-is. The ref-counting trick to keep the host process alive is something that found very useful but not obvious initially - I think it is useful to call this aspect of the implementation explicitly. Additionally - the main reason why I am grouping the signaling/heartbeat-objects together is so I can keep them alive together while the process's life is extended and everything else is dead or dying. If I didn't need to keep those objects together then I wouldn't have HostSignalingManager. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:41: // class to read and store config/policy/cmdline values. On 2014/12/18 00:46:21, Wez wrote: > The parameters need documenting. I documented some of the parameters. I didn't document the following: - host_context - self-explanatory / comment would be reduntant? - xmpp_server_config - self-explanatory / comment would be reduntant?? - host_id - self-explanatory / comment would be reduntant?? - oauth_refresh_token - self-explanatory / comment would be reduntant?? - use_service_account - I have no idea what this means / this is not documented in remoting_me2me_host.cc and in oauth_token_getter.h/cc :-( I added a shallow comment that nevertheless I hope helps here: // Controls which API key gets used. I just renamed (without adding a comment - I hope the name change is sufficient to make these parameters easier to understand) - key_pair to host_key_pair (also renaming in HeartbeatSender but not elsewhere) - talkgadget_prefix to talkgadget_prefix_policy (without renaming other occurences - i.e. in HostProcess). I am not sure what this does exactly. This seems to be part of a hostname in a http url that DnsBlackholeChecker tries to talk to. I am not sure why this needs to be controlled by a policy or what this is useful for :-( https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:45: const base::Closure& on_auth_failed_callback, On 2014/12/18 00:46:21, Wez wrote: > As previously commented, these should be combined into a > HostSignalingManager::EventHandler interface. Nooooo. As I pointed out earlier, this results in icky forwarding code in HostSignalingManager + necessity for an explicit listener/eventhandler deregistration. With callbacks, HostSignalingManager::Create can just treat the callbacks as a blackbox/opaque things and pass them to HeartbeatSender and SignalingConnector. With the event handler interface we have to hardwire the listener logic into HostSignalingManager. You can compare minimum_heartbeat_supporter.h/.cc and remoting_me2me_host.cc between patchset #1 and patchset #2 - I think things are much cleaner in patchset #2. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:46: const ChromotingHostContext& host_context, On 2014/12/18 00:46:21, Wez wrote: > Ick... do you really need this? The ChromotingHostContext is really only > intended as a convenience for creating the necessary threads, not to be passed > around into other classes. I think it is ok / not icky... 1. HostSignalingManager doesn't take ownership or mutate ChromotingHostContext (i.e. gets it via const ref). 2. This helps minimize the number of parameters. 3. It is ok for HostProcess to get the threads via ChromotingHostContext - should be ok here as well. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:51: const std::string& directory_bot_jid, On 2014/12/18 00:46:21, Wez wrote: > what's this? It's a JID to use when talking to Directory Bot. I thought that this is covered just by the name of the parameter. Anyway - I tried to improve this by adding a comment with an example: // Directory Bot's JID - i.e. "remoting@bot.talk.google.com". https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:53: bool use_service_account); On 2014/12/18 00:46:21, Wez wrote: > What does "use_service_account" mean? I don't know - I am just moving an already working code from HostProcess :-( remoting_me2_me_host.cc also didn't have any comment on the field. This gets passed to constructor of OAuthTokenGetter::OAuthCredentials struct (and doesn't have a comment/explanation there either. This is the code where it ends up used (in oauth_token_getter.cc): // Service accounts use different API keys, as they use the client app flow. google_apis::OAuth2Client oauth2_client = oauth_credentials_->is_service_account ? google_apis::CLIENT_REMOTING_HOST : google_apis::CLIENT_REMOTING; https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:57: // Gets signal strategy used for talking to the Chromoting bot. On 2014/12/18 00:46:22, Wez wrote: > "Get the SignalStrategy to use to talk to..." Done. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:58: // Return value is valid until |this| gets deleted. On 2014/12/18 00:46:21, Wez wrote: > nit: Suggest: "The SignalStrategy remains owned by the HostSignalingManager." Done. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:66: // process to exit. On 2014/12/18 00:46:21, Wez wrote: > You don't need the sentence re the task-runner. > > The AutoThreadTaskRunner behaviour is implicitly relied upon by various bits of > Chromoting, and we don't describe it everywhere; this class' responsibility is > to keep itself alive until the offline reason is ack'd or times out, and ... > it's the caller's responsibility to maintain the TaskRunners and process until it's > done. This is not true. The caller (HostProcess) shuts down and drops its references to all the task runners. The HostSignalingManager::SendHostOfflineReasonAndDelete is the only thing at this point that keeps the process alive. The extra comment doesn't hurt, right? If the comment is not true, then the whole HostSignalingManager / SendHostOfflineReason is broken so it is unlikely for the comment to get out of sync with the code, right? https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:80: // Order of fields below is important for destructing them in the right order. On 2014/12/18 00:46:21, Wez wrote: > That cannot be true of |network_task_runner_|, so move it out of the commented > block. > > It would probably be clearer to do the teardown explicitly in the destructor, > with a comment before each .reset() to explain the ordering. Arrrrgh... I had the teardown done explicitly in the destructor, but moved it here in response to code review feedback from Lambros :-) I'll leave it in as is for now. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:81: // - |heartbeat_sender_| and |signaling_connector_| have to be destructed On 2014/12/18 00:46:22, Wez wrote: > s/destructed/destroyed/ Done. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:88: // destructed last. On 2014/12/18 00:46:21, Wez wrote: > This comment doesn't make sense, since all you're doing is dropping a reference, > not destroying it. Dropping a ref-count to network_task_runner will drop a ref-count to ui_task_runner (held by the cleanup/stop/join callback of network_task_runner) and can therefore shutdown the process prematurely (although I understand that this will not somehow magically destroy the thread the destructor is running on; the process shutdown might though, right? or still wrong?). https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:89: scoped_refptr<AutoThreadTaskRunner> network_task_runner_; On 2014/12/18 00:46:21, Wez wrote: > You need to clarify what this task-runner is used for, i.e. the threading > properties of the class. > > Looking at the implementation, you basically use it just to DCHECK that this > class is always created, used & destroyed from the same thread, and to > initialise the underlying signaling object - why not just use > ThreadTaskRunnerHandle::Get() to grab the calling thread's |task_runner| in your > constructor, so the caller needn't pass it in explicitly? Or use > base::NonThreadSafe as a base-class and do checks based on that, and just use > ThreadTaskRunnerHandle::Get() to get the TaskRunner to pass to the underlying > signaling object. This field keeps the process alive. I've added a comment. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:97: HostSignalingManager* self_; On 2014/12/18 00:46:21, Wez wrote: > This is a bare pointer so it doesn't have any effect on the lifetime of > anything, surely? Yes, I don't know what I was thinking... :-( https://codereview.chromium.org/719983002/diff/380001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:321: scoped_ptr<HostSignalingManager> host_signaling_manager_; On 2014/12/18 00:46:22, Wez wrote: > nit: Add a brief comment to explain this member's purpose, e.g. "Used to send > heartbeats while running, and the reason for going offline when shutting down." Good point. Done. https://codereview.chromium.org/719983002/diff/380001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1242: weak_factory_for_network_task_runner_.GetWeakPtr()), On 2014/12/18 00:46:22, Wez wrote: > When you replace this with a HostSignalingManager::EventHandler interface you > can just have SendHostOfflineReasonAndDelete() NULL the |event_handler_| bare > pointer that the HostSignalingManager holds, to ensure the HostProcess doesn't > get used-after-free. Yes. I had that in patchset #1. I think I prefer the current code, because it keeps things more decoupled: - HostProcess manages its lifetime and lifetime of callbacks it creates - HostSignalingManager doesn't have any callback processing or listener logic at all; it just passes the callbacks to HeartbeatSender and SignalingConnector https://codereview.chromium.org/719983002/diff/380001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1361: ShutdownOnNetworkThread(); On 2014/12/18 00:46:22, Wez wrote: > This and the HOST_STARTED case are now almost identical, so I'd suggest folding > their implementation into a small helper member function and having the > HOST_INITIALIZING case just DCHECK, then create |host_signaling_manager_|, then > call that same helper as the HostStarted case, to send the offline reason and > shutdown. Ok.
https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:111: // of our callers to ensure that they call us last. On 2014/12/18 19:02:33, lukasza wrote: > On 2014/12/18 00:46:20, Wez wrote: > > This is a common pattern, so doesn't need such a long explanation, e.g. "Run > the > > ACK callback under a clean stack via PostTask()." > > I tried to shorten the comment following your suggestion. OTOH, Lambros was > asking for an explanation why we need to go through the trouble of PostTask / > why we care about a clean callstack, so I ended up inserting an explanation > focusing on the "why". Acknowledged. https://codereview.chromium.org/719983002/diff/380001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:114: base::MessageLoop::current()->PostTask(FROM_HERE, fully_bound_ack_callback); On 2014/12/18 19:02:33, lukasza wrote: > On 2014/12/18 00:46:20, Wez wrote: > > nit: Use ThreadTaskRunnerHandle::Get() here. > > Done. Can you please give more details / help me understand why this is > preferrable? I used message loop because it seemed more generic / more broadly > applicabla (i.e. wasn't introducing a tight coupling between this PostTask call > and SingleThreadTaskRunner [i.e. another SequencedTaskRunner would also work]). In using the current MessageLoop's TaskRunner you're intrinsically tying the operation to the SingleThreadTaskRunner for this thread, so it's equivalent. The idea is really that ThreadTaskRunnerHandle is the new shiny way to get the current thread's TaskRunner; the fact that that is a SingleThreadTaskRunner whereas you only really need a SequencedTaskRunner doesn't really make an difference. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... File remoting/host/host_signaling_manager.cc (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:40: const ChromotingHostContext& host_context, On 2014/12/18 19:02:33, lukasza wrote: > On 2014/12/18 00:46:21, Wez wrote: > > You only use network_task_runner() and url_context_getter() so you should just > > pass those in directly, rather than the entire context. > > I was passing them separately in the first draft, but 1) I prefer smaller number > of consolidated arguments especially since 2) they are passed by *const*-ref and > 3) Lambros already complained in his feedback that this method has too many > parameters. It has too many parameters in part because of the callbacks not being wrapped into an interface, and not using things like e.g. OAuthCredentials to encapsulate groups of related parameters, though. Also consider whether any of these parameters are really only needed for testing, or for special-cases, in which case those would be good candidates for separate setter functions to configure. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:50: // Create a NetworkChangeNotifier for use by the signaling connector. On 2014/12/18 19:02:33, lukasza wrote: > On 2014/12/18 00:46:21, Wez wrote: > > This comment seems to add nothing? > > I am just copy&pasting from the code that was in HostProcess class. Somebody > else thought that this comment was useful (when inserting it in the first > place). I can remove, if you feel strongly about this. Given that none of the code below is commented, I'd prefer we remove this for consistency, and because it adds nothing. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:54: scoped_ptr<XmppSignalStrategy> signal_strategy(new XmppSignalStrategy( On 2014/12/18 19:02:33, lukasza wrote: > On 2014/12/18 00:46:21, Wez wrote: > > nit: Here & below, IIRC style guide prefers wrapping at the outermost point > that > > makes sense, e.g. in this case immediately after signal_strategy( unless that > > makes the wrapping much more cumbersome. > > IIRC Lambros complained that this fits on a single line so I folded. This also > passes git cl format, so I'll just leave it as is. Acknowledged. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:65: if (!oauth_refresh_token.empty()) { On 2014/12/18 19:02:33, lukasza wrote: > On 2014/12/18 00:46:21, Wez wrote: > > What does it mean for the OAuth refresh token to be empty? What happens then? > > I don't know. Just copy&pasting existing code from HostProcess. If we suspect > that something is wrong (or simply suspicious/smelly) here then I'd prefer to > tackle this separately from this changelist. Acknowledged. Can you file a cleanup bug for us to review the code for that? https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:91: HOST_LOG << "HostSignalingManager destroyed."; On 2014/12/18 19:02:33, lukasza wrote: > On 2014/12/18 00:46:21, Wez wrote: > > Is this really important enough to put in the host log? Maybe [D]VLOG(2) it? > > I hoped that this would help diagnose the case of a hang host (when you might > not have an opportunity to restart with more verbose logging). OTOH, I don't > feel strongly about this (and admittedly I don't have much experience judging > how much logging is too little vs too much). > > I'll leave it in for now, but please push back if you think I should change to > VLOG(2). I think it's fine to remove this since you already log the ACK and timeout cases. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:102: self_ = this; // We own ourselves until the ack-or-timeout callback fires. On 2014/12/18 19:02:33, lukasza wrote: > On 2014/12/18 00:46:21, Wez wrote: > > This has no effect, though..? > > Yes. I don't know what I was thinking... :-( Acknowledged. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:35: // 1. Keep sending regular heartbeats to the service. On 2014/12/18 19:02:33, lukasza wrote: > On 2014/12/18 00:46:22, Wez wrote: > > You mean to the Chromoting Directory, I think? > > To Talk, Chromoting Directory and Chromoting Bot. We only send the heartbeat to the directory - it just happens to pass via Talk and the Bot. My point is that "service" is very vague and could, e.g. under Windows, be confused with the wrong concept. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:36: // 2. Keep the host process alive while sending host-offline-reason heartbeat. On 2014/12/18 19:02:34, lukasza wrote: > On 2014/12/18 00:46:22, Wez wrote: > > nit: The second function is to make a best-effort to deliver the shutdown > reason > > to the cloud - the need for keeping the process alive is just an > implementation > > detail. > > I think I'll leave it as-is. The ref-counting trick to keep the host process > alive is something that found very useful but not obvious initially - I think it > is useful to call this aspect of the implementation explicitly. Additionally - > the main reason why I am grouping the signaling/heartbeat-objects together is so > I can keep them alive together while the process's life is extended and > everything else is dead or dying. If I didn't need to keep those objects > together then I wouldn't have HostSignalingManager. Acknowledged. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:41: // class to read and store config/policy/cmdline values. On 2014/12/18 19:02:34, lukasza wrote: > On 2014/12/18 00:46:21, Wez wrote: > > The parameters need documenting. > > I documented some of the parameters. > > I didn't document the following: > - host_context - self-explanatory / comment would be reduntant? > - xmpp_server_config - self-explanatory / comment would be reduntant?? > - host_id - self-explanatory / comment would be reduntant?? > - oauth_refresh_token - self-explanatory / comment would be reduntant?? > > - use_service_account - I have no idea what this means / this is not documented > in remoting_me2me_host.cc and in oauth_token_getter.h/cc :-( I added a shallow > comment that nevertheless I hope helps here: // Controls which API key gets > used. > > I just renamed (without adding a comment - I hope the name change is sufficient > to make these parameters easier to understand) > - key_pair to host_key_pair (also renaming in HeartbeatSender but not elsewhere) > - talkgadget_prefix to talkgadget_prefix_policy (without renaming other > occurences - i.e. in HostProcess). I am not sure what this does exactly. This > seems to be part of a hostname in a http url that DnsBlackholeChecker tries to > talk to. I am not sure why this needs to be controlled by a policy or what this > is useful for :-( Please document the paremeters in the Create() method's block comment, not in-line. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:45: const base::Closure& on_auth_failed_callback, On 2014/12/18 19:02:34, lukasza wrote: > On 2014/12/18 00:46:21, Wez wrote: > > As previously commented, these should be combined into a > > HostSignalingManager::EventHandler interface. > > Nooooo. As I pointed out earlier, this results in icky forwarding code in > HostSignalingManager + necessity for an explicit listener/eventhandler > deregistration. With callbacks, HostSignalingManager::Create can just treat the > callbacks as a blackbox/opaque things and pass them to HeartbeatSender and > SignalingConnector. With the event handler interface we have to hardwire the > listener logic into HostSignalingManager. You can compare > minimum_heartbeat_supporter.h/.cc and remoting_me2me_host.cc between patchset #1 > and patchset #2 - I think things are much cleaner in patchset #2. > I disagree; the callback-based implementation has a little less boilerplate but is not cleaner. With the callbacks you have more parameters to Create(), making the call more cumbersome, and the semantics of those callback with respect to the lifetime of the HostSignalingManager are non-obvious. With an EventHandler interface the HostSignalingManager can Bind(&EventHandler::Foo, weak_event_handler_) using a WeakPtrFactory initialized with the EventHandler supplied to Create(). You can then define that the EventHandler will stop receiving notifications when either the HostSignalingManager is deleted directly, or SendHostOfflineReasonAndDelete() is called. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:46: const ChromotingHostContext& host_context, On 2014/12/18 19:02:34, lukasza wrote: > On 2014/12/18 00:46:21, Wez wrote: > > Ick... do you really need this? The ChromotingHostContext is really only > > intended as a convenience for creating the necessary threads, not to be passed > > around into other classes. > > I think it is ok / not icky... > 1. HostSignalingManager doesn't take ownership or mutate ChromotingHostContext > (i.e. gets it via const ref). > 2. This helps minimize the number of parameters. > 3. It is ok for HostProcess to get the threads via ChromotingHostContext - > should be ok here as well. HostProcess is a special-case in that it is an implementation detail of remoting_me2me_host - this class just needs a thread to run on and a URLRequestGetter, so passing in the entire ChromotingHostContext creates an artificial dependency. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:51: const std::string& directory_bot_jid, On 2014/12/18 19:02:34, lukasza wrote: > On 2014/12/18 00:46:21, Wez wrote: > > what's this? > > It's a JID to use when talking to Directory Bot. I thought that this is covered > just by the name of the parameter. Anyway - I tried to improve this by adding a > comment with an example: > // Directory Bot's JID - i.e. mailto:"remoting@bot.talk.google.com". Right; the question is not _what_ is the parameter, which the name covers, but _why_ is it here? Why is it not just a constant in the implementation? https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:53: bool use_service_account); On 2014/12/18 19:02:34, lukasza wrote: > On 2014/12/18 00:46:21, Wez wrote: > > What does "use_service_account" mean? > > I don't know - I am just moving an already working code from HostProcess :-( > remoting_me2_me_host.cc also didn't have any comment on the field. This gets > passed to constructor of OAuthTokenGetter::OAuthCredentials struct (and doesn't > have a comment/explanation there either. This is the code where it ends up used > (in oauth_token_getter.cc): > > // Service accounts use different API keys, as they use the client app flow. > google_apis::OAuth2Client oauth2_client = > oauth_credentials_->is_service_account ? > google_apis::CLIENT_REMOTING_HOST : google_apis::CLIENT_REMOTING; Yeah, that parameter is super badly documented; your comment is helpful here, IMO - suggest elaborating e.g. "True if the OAuth refresh token is for a service account, False for a user account, to allow the correct client-ID to be used." https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:66: // process to exit. On 2014/12/18 19:02:34, lukasza wrote: > On 2014/12/18 00:46:21, Wez wrote: > > You don't need the sentence re the task-runner. > > > > The AutoThreadTaskRunner behaviour is implicitly relied upon by various bits > of > > Chromoting, and we don't describe it everywhere; this class' responsibility is > > to keep itself alive until the offline reason is ack'd or times out, and > ... > > it's the caller's responsibility to maintain the TaskRunners and process until > it's > > done. > > This is not true. The caller (HostProcess) shuts down and drops its references > to all the task runners. The > HostSignalingManager::SendHostOfflineReasonAndDelete is the only thing at this > point that keeps the process alive. > > The extra comment doesn't hurt, right? If the comment is not true, then the > whole HostSignalingManager / SendHostOfflineReason is broken so it is unlikely > for the comment to get out of sync with the code, right? Acknowledged. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:88: // destructed last. On 2014/12/18 19:02:34, lukasza wrote: > On 2014/12/18 00:46:21, Wez wrote: > > This comment doesn't make sense, since all you're doing is dropping a > reference, > > not destroying it. > > Dropping a ref-count to network_task_runner will drop a ref-count to > ui_task_runner (held by the cleanup/stop/join callback of network_task_runner) > and can therefore shutdown the process prematurely (although I understand that > this will not somehow magically destroy the thread the destructor is running on; > the process shutdown might though, right? or still wrong?). That case should be safe - the AutoThreadTaskRunner will PostTask() the |stop_task| it was initialised with to the underlying thread's TaskRunner - in the case of AutoThread, the stop_task then posts to the owning thread to join() it, before dropping the reference to it. Because threads have SequentialTaskRunners, you're guaranteed that the tasks leading to process shutdown won't get processed until after you've finished working here, and returned control to your thread's MessageLoop to process those tasks. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:97: HostSignalingManager* self_; On 2014/12/18 19:02:34, lukasza wrote: > On 2014/12/18 00:46:21, Wez wrote: > > This is a bare pointer so it doesn't have any effect on the lifetime of > > anything, surely? > > Yes, I don't know what I was thinking... :-( Yeah, we all have code-review moments like that. :D https://codereview.chromium.org/719983002/diff/420001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/719983002/diff/420001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:117: PostHostOfflineReasonAckCallback(false); You don't need to post in this case, you can just take a copy of the callback, clear the callback member, then Run() the copy directly. That way the PostTask() in the previous method can be moved in-line into OnHostOfflineReasonAck. https://codereview.chromium.org/719983002/diff/420001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:122: if (!host_offline_reason_ack_callback_.is_null()) { nit: Suggest rewriting this as: if (host_callback_.is_null()) { DCHECK(...) return; } // Do other stuff here... https://codereview.chromium.org/719983002/diff/420001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.h (right): https://codereview.chromium.org/719983002/diff/420001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:99: // Invoked when the host ID is permanently not recognized by the server. These comments should be in the HeartbeatSender() comment block above, not in-line in the declaration - the comment block above basically needs to document how the caller should use this constructor. https://codereview.chromium.org/719983002/diff/420001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:145: void OnHostOfflineReasonAck(); nit: A single-line comment on this block to explain what these methods are for would be helpful, e.g. "Handlers for host-offline-reason completion and timeout." - otherwise this class just reads like a huge list of methods and members. :-/ https://codereview.chromium.org/719983002/diff/420001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:169: base::OneShotTimer<HeartbeatSender> host_offline_reason_timeout_timer_; Similarly here, it'd be good to separate the offline-reason related members into a block and introduce it with an explanation that these are used to manage communication w/ the cloud, and process lifetime, during shutdown. https://codereview.chromium.org/719983002/diff/420001/remoting/host/host_sign... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/719983002/diff/420001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:66: ~HostSignalingManager(); What is the behaviour of this class wrt the offline reason if it is explicitly deleted by the caller rather than via SendOfflineReason, below? https://codereview.chromium.org/719983002/diff/420001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/420001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:259: void SendOfflineReasonAndShutdownOnNetworkThread(HostExitCodes exit_code); This name is very specific but it's not clear to the caller why we need it versus ShutdownHost(), above... I'd suggest a comment to indicate that this is used by ShutdownHost() in the case where the host components are actively running when ShutdownHost() was called.
I addressed most code review feedback, except the callbacks-vs-listener issue. Wez - can you please take another look? https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... File remoting/host/host_signaling_manager.cc (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:40: const ChromotingHostContext& host_context, On 2014/12/19 22:05:36, Wez wrote: > On 2014/12/18 19:02:33, lukasza wrote: > > On 2014/12/18 00:46:21, Wez wrote: > > > You only use network_task_runner() and url_context_getter() so you should > just > > > pass those in directly, rather than the entire context. > > > > I was passing them separately in the first draft, but 1) I prefer smaller > number > > of consolidated arguments especially since 2) they are passed by *const*-ref > and > > 3) Lambros already complained in his feedback that this method has too many > > parameters. > > It has too many parameters in part because of the callbacks not being wrapped > into an interface, and not using things like e.g. OAuthCredentials to > encapsulate groups of related parameters, though. Point taken on OAuthCredentials - it will let me remove oauth_refresh_token and use_service_account from the parameters list (and replace them with oauth_credentials). I am not convinced about callbacks-vs-interface and not sure if there is anything to be done with the remaining parameters. Let's chat about it? > Also consider whether any of these parameters are really only needed for > testing, or for special-cases, in which case those would be good candidates for > separate setter functions to configure. All of the parameters are used in the main control path (i.e. all of them were originally in HostProcess and used to construct the fields that are now hidden behind HostSignalingManager). Therefore I think separate setters (or some kind of a "builder" pattern) are not necessary and would only complicate things unnecessarily. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:50: // Create a NetworkChangeNotifier for use by the signaling connector. On 2014/12/19 22:05:36, Wez wrote: > On 2014/12/18 19:02:33, lukasza wrote: > > On 2014/12/18 00:46:21, Wez wrote: > > > This comment seems to add nothing? > > > > I am just copy&pasting from the code that was in HostProcess class. Somebody > > else thought that this comment was useful (when inserting it in the first > > place). I can remove, if you feel strongly about this. > > Given that none of the code below is commented, I'd prefer we remove this for > consistency, and because it adds nothing. Done. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:65: if (!oauth_refresh_token.empty()) { On 2014/12/19 22:05:36, Wez wrote: > On 2014/12/18 19:02:33, lukasza wrote: > > On 2014/12/18 00:46:21, Wez wrote: > > > What does it mean for the OAuth refresh token to be empty? What happens > then? > > > > I don't know. Just copy&pasting existing code from HostProcess. If we > suspect > > that something is wrong (or simply suspicious/smelly) here then I'd prefer to > > tackle this separately from this changelist. > > Acknowledged. Can you file a cleanup bug for us to review the code for that? Done - crbug.com/446646 https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:91: HOST_LOG << "HostSignalingManager destroyed."; On 2014/12/19 22:05:37, Wez wrote: > On 2014/12/18 19:02:33, lukasza wrote: > > On 2014/12/18 00:46:21, Wez wrote: > > > Is this really important enough to put in the host log? Maybe [D]VLOG(2) it? > > > > I hoped that this would help diagnose the case of a hang host (when you might > > not have an opportunity to restart with more verbose logging). OTOH, I don't > > feel strongly about this (and admittedly I don't have much experience judging > > how much logging is too little vs too much). > > > > I'll leave it in for now, but please push back if you think I should change to > > VLOG(2). > > I think it's fine to remove this since you already log the ACK and timeout > cases. Done. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:35: // 1. Keep sending regular heartbeats to the service. On 2014/12/19 22:05:37, Wez wrote: > On 2014/12/18 19:02:33, lukasza wrote: > > On 2014/12/18 00:46:22, Wez wrote: > > > You mean to the Chromoting Directory, I think? > > > > To Talk, Chromoting Directory and Chromoting Bot. > > We only send the heartbeat to the directory - it just happens to pass via Talk > and the Bot. > > My point is that "service" is very vague and could, e.g. under Windows, be > confused with the wrong concept. Got it - done (s/service/Chromoting Directory/). https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:41: // class to read and store config/policy/cmdline values. On 2014/12/19 22:05:37, Wez wrote: > On 2014/12/18 19:02:34, lukasza wrote: > > On 2014/12/18 00:46:21, Wez wrote: > > > The parameters need documenting. > > > > I documented some of the parameters. > > > > I didn't document the following: > > - host_context - self-explanatory / comment would be reduntant? > > - xmpp_server_config - self-explanatory / comment would be reduntant?? > > - host_id - self-explanatory / comment would be reduntant?? > > - oauth_refresh_token - self-explanatory / comment would be reduntant?? > > > > - use_service_account - I have no idea what this means / this is not > documented > > in remoting_me2me_host.cc and in oauth_token_getter.h/cc :-( I added a > shallow > > comment that nevertheless I hope helps here: // Controls which API key gets > > used. > > > > I just renamed (without adding a comment - I hope the name change is > sufficient > > to make these parameters easier to understand) > > - key_pair to host_key_pair (also renaming in HeartbeatSender but not > elsewhere) > > - talkgadget_prefix to talkgadget_prefix_policy (without renaming other > > occurences - i.e. in HostProcess). I am not sure what this does exactly. > This > > seems to be part of a hostname in a http url that DnsBlackholeChecker tries to > > talk to. I am not sure why this needs to be controlled by a policy or what > this > > is useful for :-( > > Please document the paremeters in the Create() method's block comment, not > in-line. Done. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:45: const base::Closure& on_auth_failed_callback, On 2014/12/19 22:05:37, Wez wrote: > On 2014/12/18 19:02:34, lukasza wrote: > > On 2014/12/18 00:46:21, Wez wrote: > > > As previously commented, these should be combined into a > > > HostSignalingManager::EventHandler interface. > > > > Nooooo. As I pointed out earlier, this results in icky forwarding code in > > HostSignalingManager + necessity for an explicit listener/eventhandler > > deregistration. With callbacks, HostSignalingManager::Create can just treat > the > > callbacks as a blackbox/opaque things and pass them to HeartbeatSender and > > SignalingConnector. With the event handler interface we have to hardwire the > > listener logic into HostSignalingManager. You can compare > > minimum_heartbeat_supporter.h/.cc and remoting_me2me_host.cc between patchset > #1 > > and patchset #2 - I think things are much cleaner in patchset #2. > > > > I disagree; the callback-based implementation has a little less boilerplate but > is not cleaner. > > With the callbacks you have more parameters to Create(), making the call more > cumbersome, and the semantics of those callback with respect to the lifetime of > the HostSignalingManager are non-obvious. I think the lifetime semantics are cleaner with callbacks: - consumers of the callbacks (HostSignalingManager, HeartbeatSender, SignalingConnector) assume that the callbacks are valid/alive forever - creator of the callbacks (HostProcess) ensure the callbacks are safe wrt the lifetime of the creator (i.e. the creator is responsible to force the callbacks into no-ops before the creator gets deleted); I also like that the lifetime controlling by creator is done in a way that doesn't have to know who consumes the callbacks (i.e. the creator is not coupled with consumers wrt managing the lifetime of the callbacks). > With an EventHandler interface the HostSignalingManager can > Bind(&EventHandler::Foo, weak_event_handler_) using a WeakPtrFactory initialized > with the EventHandler supplied to Create(). You can then define that the > EventHandler will stop receiving notifications when either the > HostSignalingManager is deleted directly, or SendHostOfflineReasonAndDelete() is > called. This sounds like a lot of magic that users of HostSignalingManager have to know, before they can confidently/safely use the listener interface. Maybe we can chat about this (this did help convince me that ack_or_timeout_reporter is a bad idea - maybe this will also be most efficient in this case). https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:46: const ChromotingHostContext& host_context, On 2014/12/19 22:05:37, Wez wrote: > On 2014/12/18 19:02:34, lukasza wrote: > > On 2014/12/18 00:46:21, Wez wrote: > > > Ick... do you really need this? The ChromotingHostContext is really only > > > intended as a convenience for creating the necessary threads, not to be > passed > > > around into other classes. > > > > I think it is ok / not icky... > > 1. HostSignalingManager doesn't take ownership or mutate ChromotingHostContext > > (i.e. gets it via const ref). > > 2. This helps minimize the number of parameters. > > 3. It is ok for HostProcess to get the threads via ChromotingHostContext - > > should be ok here as well. > > HostProcess is a special-case in that it is an implementation detail of > remoting_me2me_host - this class just needs a thread to run on and a > URLRequestGetter, so passing in the entire ChromotingHostContext creates an > artificial dependency. Done. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:51: const std::string& directory_bot_jid, On 2014/12/19 22:05:37, Wez wrote: > On 2014/12/18 19:02:34, lukasza wrote: > > On 2014/12/18 00:46:21, Wez wrote: > > > what's this? > > > > It's a JID to use when talking to Directory Bot. I thought that this is > covered > > just by the name of the parameter. Anyway - I tried to improve this by adding > a > > comment with an example: > > // Directory Bot's JID - i.e. mailto:"remoting@bot.talk.google.com". > > Right; the question is not _what_ is the parameter, which the name covers, but > _why_ is it here? Why is it not just a constant in the implementation? Got it. As Feynman says (https://www.youtube.com/watch?v=36GT2zI8lVA) "why" questions are inherently difficult to answer :-) Answer#1: It's here because it is overridable on the command line (i.e. see https://code.google.com/p/chromium/codesearch#chromium/src/remoting/base/serv...). Answer#2: It's here to enable overriding it when testing against locally running talk/bot (i.e. see "host" section here: https://wiki.corp.google.com/twiki/bin/view/Main/RunningChromotingServersLocally). I don't think any of that needs to go into comments though, right? I'll leave them as-is for now. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:53: bool use_service_account); On 2014/12/19 22:05:37, Wez wrote: > On 2014/12/18 19:02:34, lukasza wrote: > > On 2014/12/18 00:46:21, Wez wrote: > > > What does "use_service_account" mean? > > > > I don't know - I am just moving an already working code from HostProcess :-( > > remoting_me2_me_host.cc also didn't have any comment on the field. This gets > > passed to constructor of OAuthTokenGetter::OAuthCredentials struct (and > doesn't > > have a comment/explanation there either. This is the code where it ends up > used > > (in oauth_token_getter.cc): > > > > // Service accounts use different API keys, as they use the client app flow. > > google_apis::OAuth2Client oauth2_client = > > oauth_credentials_->is_service_account ? > > google_apis::CLIENT_REMOTING_HOST : google_apis::CLIENT_REMOTING; > > Yeah, that parameter is super badly documented; your comment is helpful here, > IMO - suggest elaborating e.g. "True if the OAuth refresh token is for a service > account, False for a user account, to allow the correct client-ID to be used." I've added the comment you suggested to the constructor of OAuthCredentials in oauth_token_getter.h https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:88: // destructed last. On 2014/12/19 22:05:37, Wez wrote: > On 2014/12/18 19:02:34, lukasza wrote: > > On 2014/12/18 00:46:21, Wez wrote: > > > This comment doesn't make sense, since all you're doing is dropping a > > reference, > > > not destroying it. > > > > Dropping a ref-count to network_task_runner will drop a ref-count to > > ui_task_runner (held by the cleanup/stop/join callback of network_task_runner) > > and can therefore shutdown the process prematurely (although I understand that > > this will not somehow magically destroy the thread the destructor is running > on; > > the process shutdown might though, right? or still wrong?). > > That case should be safe - the AutoThreadTaskRunner will PostTask() the > |stop_task| it was initialised with to the underlying thread's TaskRunner - in > the case of AutoThread, the stop_task then posts to the owning thread to join() > it, before dropping the reference to it. > > Because threads have SequentialTaskRunners, you're guaranteed that the tasks > leading to process shutdown won't get processed until after you've finished > working here, and returned control to your thread's MessageLoop to process those > tasks. Okay, I think I get it - dropping the reference will post join to the other thread, but the task_runner wrapped by AutoThreadTaskRunner can still be used just fine, even after AutoThreadTaskRunner is dead and a join is pending on the other thread. I'll remove the comment. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:97: HostSignalingManager* self_; On 2014/12/19 22:05:37, Wez wrote: > On 2014/12/18 19:02:34, lukasza wrote: > > On 2014/12/18 00:46:21, Wez wrote: > > > This is a bare pointer so it doesn't have any effect on the lifetime of > > > anything, surely? > > > > Yes, I don't know what I was thinking... :-( > > Yeah, we all have code-review moments like that. :D Acknowledged. https://codereview.chromium.org/719983002/diff/420001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/719983002/diff/420001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:117: PostHostOfflineReasonAckCallback(false); On 2014/12/19 22:05:37, Wez wrote: > You don't need to post in this case, you can just take a copy of the callback, > clear the callback member, then Run() the copy directly. That way the PostTask() > in the previous method can be moved in-line into OnHostOfflineReasonAck. Oh, because we are getting called directly by the timer in this case and therefore there are no frames on the callstack using |this|... Hmmm... okay... (although I kind of liked the symmetry between OnHostOfflineReasonTimeout and OnHostOfflineReasonAck). https://codereview.chromium.org/719983002/diff/420001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:122: if (!host_offline_reason_ack_callback_.is_null()) { On 2014/12/19 22:05:37, Wez wrote: > nit: Suggest rewriting this as: > > if (host_callback_.is_null()) { > DCHECK(...) > return; > } > > // Do other stuff here... Done. https://codereview.chromium.org/719983002/diff/420001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.h (right): https://codereview.chromium.org/719983002/diff/420001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:99: // Invoked when the host ID is permanently not recognized by the server. On 2014/12/19 22:05:37, Wez wrote: > These comments should be in the HeartbeatSender() comment block above, not > in-line in the declaration - the comment block above basically needs to document > how the caller should use this constructor. Done. https://codereview.chromium.org/719983002/diff/420001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:145: void OnHostOfflineReasonAck(); On 2014/12/19 22:05:37, Wez wrote: > nit: A single-line comment on this block to explain what these methods are for > would be helpful, e.g. "Handlers for host-offline-reason completion and > timeout." - otherwise this class just reads like a huge list of methods and > members. :-/ Done. https://codereview.chromium.org/719983002/diff/420001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:169: base::OneShotTimer<HeartbeatSender> host_offline_reason_timeout_timer_; On 2014/12/19 22:05:37, Wez wrote: > Similarly here, it'd be good to separate the offline-reason related members into > a block and introduce it with an explanation that these are used to manage > communication w/ the cloud, and process lifetime, during shutdown. Done. https://codereview.chromium.org/719983002/diff/420001/remoting/host/host_sign... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/719983002/diff/420001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:66: ~HostSignalingManager(); On 2014/12/19 22:05:37, Wez wrote: > What is the behaviour of this class wrt the offline reason if it is explicitly > deleted by the caller rather than via SendOfflineReason, below? The HostSignalingManager gets deleted (without a call to SendHostOfflineReasonAndDelete) during normal (non-error-related) shutdown of HostProcess. In this case we simply close the connection. If HeartbeatSender::SendHostOfflineReason were called, without a callback that ends up shutting down the process (as in the HostSignalingManager::SendHostOfflineReasonAndDelete case), then each subsequent heartbeat will include host-offline-reason. This seems ok. https://codereview.chromium.org/719983002/diff/420001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/420001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:259: void SendOfflineReasonAndShutdownOnNetworkThread(HostExitCodes exit_code); On 2014/12/19 22:05:37, Wez wrote: > This name is very specific but it's not clear to the caller why we need it > versus ShutdownHost(), above... > > I'd suggest a comment to indicate that this is used by ShutdownHost() in the > case where the host components are actively running when ShutdownHost() was > called. Yes - this method should be "private" to ShutdownHost method (if such thing was possible). I've added a comment as you suggested. I guess one alternative would be to avoid having a separate method and just have a goto inside ShutdownHost (this seems palatable to me, despite general frowning upon gotos).
https://codereview.chromium.org/719983002/diff/420001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/420001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:259: void SendOfflineReasonAndShutdownOnNetworkThread(HostExitCodes exit_code); On 2015/01/07 01:24:04, lukasza wrote: > On 2014/12/19 22:05:37, Wez wrote: > > This name is very specific but it's not clear to the caller why we need it > > versus ShutdownHost(), above... > > > > I'd suggest a comment to indicate that this is used by ShutdownHost() in the > > case where the host components are actively running when ShutdownHost() was > > called. > > Yes - this method should be "private" to ShutdownHost method (if such thing was > possible). I've added a comment as you suggested. I guess one alternative > would be to avoid having a separate method and just have a goto inside > ShutdownHost (this seems palatable to me, despite general frowning upon gotos). Noooo, goto is eeeevil, just like exceptions. ;) Your comment explains things nicely, IMO. https://codereview.chromium.org/719983002/diff/440001/remoting/host/dns_black... File remoting/host/dns_blackhole_checker.h (right): https://codereview.chromium.org/719983002/diff/440001/remoting/host/dns_black... remoting/host/dns_blackhole_checker.h:26: url_request_context_getter, What's the rationale for making this const scoped_refptr&? https://codereview.chromium.org/719983002/diff/440001/remoting/host/dns_black... remoting/host/dns_blackhole_checker.h:27: std::string talkgadget_prefix); Where did this weird wrapping come from? https://codereview.chromium.org/719983002/diff/440001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/719983002/diff/440001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:110: local_callback.Run(false /* success */); s/success/failure :) https://codereview.chromium.org/719983002/diff/440001/remoting/host/host_sign... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/719983002/diff/440001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:77: // process to exit. Note here that the caller *must* release this object, so that it won't get double-deleted. https://codereview.chromium.org/719983002/diff/440001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:83: const scoped_refptr<AutoThreadTaskRunner>& network_task_runner, What prompted the const&?
This is looking much cleaner now - thanks for all the hard work. :)
I've made small formatting and commenting fixes. Wez - could you please take another look? https://codereview.chromium.org/719983002/diff/440001/remoting/host/dns_black... File remoting/host/dns_blackhole_checker.h (right): https://codereview.chromium.org/719983002/diff/440001/remoting/host/dns_black... remoting/host/dns_blackhole_checker.h:26: url_request_context_getter, On 2015/01/08 01:49:50, Wez wrote: > What's the rationale for making this const scoped_refptr&? When following your suggestion to pass network_task_runner + url_request_context_getter directly (rather than wrapped in chromoting_context), I've introduced parameters to HostSignalingManager::Create with const scoped_refptr<X>& declaration. This declaration avoids unnecessary addref/release and ref-count churn (I am not sure now where exactly I read the advise to pass scoped_refptr by const-ref rather than by value). When fixing usage of these parameters in HostSignalingManager::Create, I've noticed that we can also change the declarations in here (and a few other places). I've checked that in all of those cases the parameter (now changed to const scoped_refptr<X>&) is just used in the constructor to initialize a field of scoped_refptr<X> type, so this change seemed straightforward and safe. https://codereview.chromium.org/719983002/diff/440001/remoting/host/dns_black... remoting/host/dns_blackhole_checker.h:27: std::string talkgadget_prefix); On 2015/01/08 01:49:50, Wez wrote: > Where did this weird wrapping come from? git cl format The thing is that the 1st parameter won't fit on a single line even when indented by 4 spaces (rather than continuing right after the opening paren). I've changed "url_request_context_getter" to "request_context_getter" (this name is already used by xmpp_signal_strategy.h) and now it fits in 80 columns. https://codereview.chromium.org/719983002/diff/440001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/719983002/diff/440001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:110: local_callback.Run(false /* success */); On 2015/01/08 01:49:50, Wez wrote: > s/success/failure :) Ok, I can see the ambiguity now. What I meant was that success == false. Let me just remove this comment to avoid the ambiguity (and the comment for "true" below as well). https://codereview.chromium.org/719983002/diff/440001/remoting/host/host_sign... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/719983002/diff/440001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:77: // process to exit. On 2015/01/08 01:49:51, Wez wrote: > Note here that the caller *must* release this object, so that it won't get > double-deleted. Done. https://codereview.chromium.org/719983002/diff/440001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:83: const scoped_refptr<AutoThreadTaskRunner>& network_task_runner, On 2015/01/08 01:49:51, Wez wrote: > What prompted the const&? In general, scoped_refptr should be passed by const-ref to avoid unnecessary calls to AddRef/Release and ref-count churn. OTOH, I am not sure where exactly I found this advise (but pretty sure that it was recently and originating from within Chromium docs or code or forums). I did this when replacing chromoting_context in HostSignalingManager::Create with network_task_runner + url_request_context_getter (and noticing that Create passes its scoped_refptr references to places that also could declare the scoped_refptr parameter as a const-ref).
https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:45: const base::Closure& on_auth_failed_callback, On 2015/01/07 01:24:04, lukasza wrote: > On 2014/12/19 22:05:37, Wez wrote: > > On 2014/12/18 19:02:34, lukasza wrote: > > > On 2014/12/18 00:46:21, Wez wrote: > > > > As previously commented, these should be combined into a > > > > HostSignalingManager::EventHandler interface. > > > > > > Nooooo. As I pointed out earlier, this results in icky forwarding code in > > > HostSignalingManager + necessity for an explicit listener/eventhandler > > > deregistration. With callbacks, HostSignalingManager::Create can just treat > > the > > > callbacks as a blackbox/opaque things and pass them to HeartbeatSender and > > > SignalingConnector. With the event handler interface we have to hardwire > the > > > listener logic into HostSignalingManager. You can compare > > > minimum_heartbeat_supporter.h/.cc and remoting_me2me_host.cc between > patchset > > #1 > > > and patchset #2 - I think things are much cleaner in patchset #2. > > > > > > > I disagree; the callback-based implementation has a little less boilerplate > but > > is not cleaner. > > > > With the callbacks you have more parameters to Create(), making the call more > > cumbersome, and the semantics of those callback with respect to the lifetime > of > > the HostSignalingManager are non-obvious. > > I think the lifetime semantics are cleaner with callbacks: > - consumers of the callbacks (HostSignalingManager, HeartbeatSender, > SignalingConnector) assume that the callbacks are valid/alive forever > - creator of the callbacks (HostProcess) ensure the callbacks are safe wrt the > lifetime of the creator (i.e. the creator is responsible to force the callbacks > into no-ops before the creator gets deleted); > > I also like that the lifetime controlling by creator is done in a way that > doesn't have to know who consumes the callbacks (i.e. the creator is not coupled > with consumers wrt managing the lifetime of the callbacks). > > > With an EventHandler interface the HostSignalingManager can > > Bind(&EventHandler::Foo, weak_event_handler_) using a WeakPtrFactory > initialized > > with the EventHandler supplied to Create(). You can then define that the > > EventHandler will stop receiving notifications when either the > > HostSignalingManager is deleted directly, or SendHostOfflineReasonAndDelete() > is > > called. > > This sounds like a lot of magic that users of HostSignalingManager have to know, > before they can confidently/safely use the listener interface. Not really; it becomes a perfectly straightforward contract that the interface you pass in must remain valid until the HostSignalingManager is destroyed, or told to delete itself. That's much more explicit that passing in a set of callbacks that may or may not still be called after you've deleted this object. > > > Maybe we can chat about this (this did help convince me that > ack_or_timeout_reporter is a bad idea - maybe this will also be most efficient > in this case). https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:88: // destructed last. On 2015/01/07 01:24:04, lukasza wrote: > On 2014/12/19 22:05:37, Wez wrote: > > On 2014/12/18 19:02:34, lukasza wrote: > > > On 2014/12/18 00:46:21, Wez wrote: > > > > This comment doesn't make sense, since all you're doing is dropping a > > > reference, > > > > not destroying it. > > > > > > Dropping a ref-count to network_task_runner will drop a ref-count to > > > ui_task_runner (held by the cleanup/stop/join callback of > network_task_runner) > > > and can therefore shutdown the process prematurely (although I understand > that > > > this will not somehow magically destroy the thread the destructor is running > > on; > > > the process shutdown might though, right? or still wrong?). > > > > That case should be safe - the AutoThreadTaskRunner will PostTask() the > > |stop_task| it was initialised with to the underlying thread's TaskRunner - in > > the case of AutoThread, the stop_task then posts to the owning thread to > join() > > it, before dropping the reference to it. > > > > Because threads have SequentialTaskRunners, you're guaranteed that the tasks > > leading to process shutdown won't get processed until after you've finished > > working here, and returned control to your thread's MessageLoop to process > those > > tasks. > > Okay, I think I get it - dropping the reference will post join to the other > thread, but the task_runner wrapped by AutoThreadTaskRunner can still be used > just fine, even after AutoThreadTaskRunner is dead and a join is pending on the > other thread. Yes; sounds like we could have explained that more clearly in the AutoThread() implementation. https://codereview.chromium.org/719983002/diff/440001/remoting/host/dns_black... File remoting/host/dns_blackhole_checker.h (right): https://codereview.chromium.org/719983002/diff/440001/remoting/host/dns_black... remoting/host/dns_blackhole_checker.h:26: url_request_context_getter, On 2015/01/08 23:50:31, lukasza wrote: > On 2015/01/08 01:49:50, Wez wrote: > > What's the rationale for making this const scoped_refptr&? > > When following your suggestion to pass network_task_runner + > url_request_context_getter directly (rather than wrapped in chromoting_context), > I've introduced parameters to HostSignalingManager::Create with const > scoped_refptr<X>& declaration. This declaration avoids unnecessary > addref/release and ref-count churn (I am not sure now where exactly I read the > advise to pass scoped_refptr by const-ref rather than by value). > > When fixing usage of these parameters in HostSignalingManager::Create, I've > noticed that we can also change the declarations in here (and a few other > places). I've checked that in all of those cases the parameter (now changed to > const scoped_refptr<X>&) is just used in the constructor to initialize a field > of scoped_refptr<X> type, so this change seemed straightforward and safe. Acknowledged. https://codereview.chromium.org/719983002/diff/440001/remoting/host/dns_black... remoting/host/dns_blackhole_checker.h:27: std::string talkgadget_prefix); On 2015/01/08 23:50:31, lukasza wrote: > On 2015/01/08 01:49:50, Wez wrote: > > Where did this weird wrapping come from? > > git cl format > > The thing is that the 1st parameter won't fit on a single line even when > indented by 4 spaces (rather than continuing right after the opening paren). > > I've changed "url_request_context_getter" to "request_context_getter" (this name > is already used by xmpp_signal_strategy.h) and now it fits in 80 columns. Acknowledged. https://codereview.chromium.org/719983002/diff/440001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/719983002/diff/440001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:110: local_callback.Run(false /* success */); On 2015/01/08 23:50:31, lukasza wrote: > On 2015/01/08 01:49:50, Wez wrote: > > s/success/failure :) > > Ok, I can see the ambiguity now. What I meant was that success == false. Let > me just remove this comment to avoid the ambiguity (and the comment for "true" > below as well). Actually what I meant was that you comment /* success */ on both the true and false values! In general I'd expect a function with a bool return-value to convey success (i.e. true == succeeded) rather than failure. Of course, this is good motivation for using an enum after all, for clarity, I suppose. :-/ https://codereview.chromium.org/719983002/diff/480001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.h (right): https://codereview.chromium.org/719983002/diff/480001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:103: Why are there blank lines between these parameters? https://codereview.chromium.org/719983002/diff/480001/remoting/host/heartbeat... File remoting/host/heartbeat_sender_unittest.cc (right): https://codereview.chromium.org/719983002/diff/480001/remoting/host/heartbeat... remoting/host/heartbeat_sender_unittest.cc:319: EXPECT_CALL(mock_ack_callback, Run(true /* success */)).Times(1); In your other comments you indicated that true is failure, not success. https://codereview.chromium.org/719983002/diff/480001/remoting/host/host_sign... File remoting/host/host_signaling_manager.cc (right): https://codereview.chromium.org/719983002/diff/480001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:91: << " to the bot"; nit: No need for "to the bot" - it adds nothing. Similarly, "trying to send" -> "sending". https://codereview.chromium.org/719983002/diff/480001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:102: HOST_LOG << "SendHostOfflineReason: got ack"; got ack -> succeeded https://codereview.chromium.org/719983002/diff/480001/remoting/host/host_sign... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/719983002/diff/480001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:55: const base::Closure& on_auth_failed_callback, See earlier comments re interface https://codereview.chromium.org/719983002/diff/480001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:98: scoped_refptr<AutoThreadTaskRunner> network_task_runner_; I thought you were going to document that this was needed to keep the process alive? https://codereview.chromium.org/719983002/diff/480001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/480001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:372: // Factory of weak pointers to |this| that will to be referenced from the typo: that will to be https://codereview.chromium.org/719983002/diff/480001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:373: // network thread (weak pointers are constrained to a single thread). Weak pointers are constrained to a single thread is not necessary; it's implicit in the nature of their function. https://codereview.chromium.org/719983002/diff/480001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:374: base::WeakPtrFactory<HostProcess> weak_factory_for_network_task_runner_; Just weak_factory_, please - it's implicit that this is dispensing WeakPtrs for dereference on the network task runner, since that's the one this object is dereferenced on.
I addressed the feedback (including changing from callbacks to a listener interface in HostSignalingManager). Wez - can you please take another look? https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:45: const base::Closure& on_auth_failed_callback, On 2015/01/09 02:55:45, Wez wrote: > On 2015/01/07 01:24:04, lukasza wrote: > > On 2014/12/19 22:05:37, Wez wrote: > > > On 2014/12/18 19:02:34, lukasza wrote: > > > > On 2014/12/18 00:46:21, Wez wrote: > > > > > As previously commented, these should be combined into a > > > > > HostSignalingManager::EventHandler interface. > > > > > > > > Nooooo. As I pointed out earlier, this results in icky forwarding code in > > > > HostSignalingManager + necessity for an explicit listener/eventhandler > > > > deregistration. With callbacks, HostSignalingManager::Create can just > treat > > > the > > > > callbacks as a blackbox/opaque things and pass them to HeartbeatSender and > > > > SignalingConnector. With the event handler interface we have to hardwire > > the > > > > listener logic into HostSignalingManager. You can compare > > > > minimum_heartbeat_supporter.h/.cc and remoting_me2me_host.cc between > > patchset > > > #1 > > > > and patchset #2 - I think things are much cleaner in patchset #2. > > > > > > > > > > I disagree; the callback-based implementation has a little less boilerplate > > but > > > is not cleaner. > > > > > > With the callbacks you have more parameters to Create(), making the call > more > > > cumbersome, and the semantics of those callback with respect to the lifetime > > of > > > the HostSignalingManager are non-obvious. > > > > I think the lifetime semantics are cleaner with callbacks: > > - consumers of the callbacks (HostSignalingManager, HeartbeatSender, > > SignalingConnector) assume that the callbacks are valid/alive forever > > - creator of the callbacks (HostProcess) ensure the callbacks are safe wrt the > > lifetime of the creator (i.e. the creator is responsible to force the > callbacks > > into no-ops before the creator gets deleted); > > > > I also like that the lifetime controlling by creator is done in a way that > > doesn't have to know who consumes the callbacks (i.e. the creator is not > coupled > > with consumers wrt managing the lifetime of the callbacks). > > > > > With an EventHandler interface the HostSignalingManager can > > > Bind(&EventHandler::Foo, weak_event_handler_) using a WeakPtrFactory > > initialized > > > with the EventHandler supplied to Create(). You can then define that the > > > EventHandler will stop receiving notifications when either the > > > HostSignalingManager is deleted directly, or > SendHostOfflineReasonAndDelete() > > is > > > called. > > > > This sounds like a lot of magic that users of HostSignalingManager have to > know, > > before they can confidently/safely use the listener interface. > > Not really; it becomes a perfectly straightforward contract that the interface > you pass in must remain valid until the HostSignalingManager is destroyed, or > told to delete itself. > > That's much more explicit that passing in a set of callbacks that may or may not > still be called after you've deleted this object. > > > > > > > Maybe we can chat about this (this did help convince me that > > ack_or_timeout_reporter is a bad idea - maybe this will also be most efficient > > in this case). > Ok. https://codereview.chromium.org/719983002/diff/380001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:88: // destructed last. On 2015/01/09 02:55:45, Wez wrote: > On 2015/01/07 01:24:04, lukasza wrote: > > On 2014/12/19 22:05:37, Wez wrote: > > > On 2014/12/18 19:02:34, lukasza wrote: > > > > On 2014/12/18 00:46:21, Wez wrote: > > > > > This comment doesn't make sense, since all you're doing is dropping a > > > > reference, > > > > > not destroying it. > > > > > > > > Dropping a ref-count to network_task_runner will drop a ref-count to > > > > ui_task_runner (held by the cleanup/stop/join callback of > > network_task_runner) > > > > and can therefore shutdown the process prematurely (although I understand > > that > > > > this will not somehow magically destroy the thread the destructor is > running > > > on; > > > > the process shutdown might though, right? or still wrong?). > > > > > > That case should be safe - the AutoThreadTaskRunner will PostTask() the > > > |stop_task| it was initialised with to the underlying thread's TaskRunner - > in > > > the case of AutoThread, the stop_task then posts to the owning thread to > > join() > > > it, before dropping the reference to it. > > > > > > Because threads have SequentialTaskRunners, you're guaranteed that the tasks > > > leading to process shutdown won't get processed until after you've finished > > > working here, and returned control to your thread's MessageLoop to process > > those > > > tasks. > > > > Okay, I think I get it - dropping the reference will post join to the other > > thread, but the task_runner wrapped by AutoThreadTaskRunner can still be used > > just fine, even after AutoThreadTaskRunner is dead and a join is pending on > the > > other thread. > > Yes; sounds like we could have explained that more clearly in the AutoThread() > implementation. Acknowledged. https://codereview.chromium.org/719983002/diff/440001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/719983002/diff/440001/remoting/host/heartbeat... remoting/host/heartbeat_sender.cc:110: local_callback.Run(false /* success */); On 2015/01/09 02:55:45, Wez wrote: > On 2015/01/08 23:50:31, lukasza wrote: > > On 2015/01/08 01:49:50, Wez wrote: > > > s/success/failure :) > > > > Ok, I can see the ambiguity now. What I meant was that success == false. Let > > me just remove this comment to avoid the ambiguity (and the comment for "true" > > below as well). > > Actually what I meant was that you comment /* success */ on both the true and > false values! In general I'd expect a function with a bool return-value to > convey success (i.e. true == succeeded) rather than failure. Of course, this is > good motivation for using an enum after all, for clarity, I suppose. :-/ Yes - avoiding this kind of confusion was why I wanted to introduce the AckOrTimeout enum. The "/* success */" comment was meant to say that "false" is bound to "success" parameter, not that "false" indicates success. C#'s named parameters address this case nicely (+ give compile-time validation so that names used here do not get out of sync with parameter declaration [unlike what can easily happen to comments]). I'll leave it as-is, but without a /* success */ or a /* failure */ comment (saing "failure" would be just as confusing - is failure=false [and therefore success=true]? or does false mean failure [i.e. success=false]). https://codereview.chromium.org/719983002/diff/480001/remoting/host/heartbeat... File remoting/host/heartbeat_sender.h (right): https://codereview.chromium.org/719983002/diff/480001/remoting/host/heartbeat... remoting/host/heartbeat_sender.h:103: On 2015/01/09 02:55:45, Wez wrote: > Why are there blank lines between these parameters? Fixed. https://codereview.chromium.org/719983002/diff/480001/remoting/host/heartbeat... File remoting/host/heartbeat_sender_unittest.cc (right): https://codereview.chromium.org/719983002/diff/480001/remoting/host/heartbeat... remoting/host/heartbeat_sender_unittest.cc:319: EXPECT_CALL(mock_ack_callback, Run(true /* success */)).Times(1); On 2015/01/09 02:55:45, Wez wrote: > In your other comments you indicated that true is failure, not success. No. I was trying to indicate that "false" is bound to "success" argument, not that "false" means "success" (success=false means failure). https://codereview.chromium.org/719983002/diff/480001/remoting/host/host_sign... File remoting/host/host_signaling_manager.cc (right): https://codereview.chromium.org/719983002/diff/480001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:91: << " to the bot"; On 2015/01/09 02:55:45, Wez wrote: > nit: No need for "to the bot" - it adds nothing. > > Similarly, "trying to send" -> "sending". Done. https://codereview.chromium.org/719983002/diff/480001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:102: HOST_LOG << "SendHostOfflineReason: got ack"; On 2015/01/09 02:55:45, Wez wrote: > got ack -> succeeded Done. https://codereview.chromium.org/719983002/diff/480001/remoting/host/host_sign... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/719983002/diff/480001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:55: const base::Closure& on_auth_failed_callback, On 2015/01/09 02:55:45, Wez wrote: > See earlier comments re interface Acknowledged. https://codereview.chromium.org/719983002/diff/480001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:98: scoped_refptr<AutoThreadTaskRunner> network_task_runner_; On 2015/01/09 02:55:45, Wez wrote: > I thought you were going to document that this was needed to keep the process > alive? Done. https://codereview.chromium.org/719983002/diff/480001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/480001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:372: // Factory of weak pointers to |this| that will to be referenced from the On 2015/01/09 02:55:45, Wez wrote: > typo: that will to be Done. https://codereview.chromium.org/719983002/diff/480001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:373: // network thread (weak pointers are constrained to a single thread). On 2015/01/09 02:55:46, Wez wrote: > Weak pointers are constrained to a single thread is not necessary; it's implicit > in the nature of their function. Done. https://codereview.chromium.org/719983002/diff/480001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:374: base::WeakPtrFactory<HostProcess> weak_factory_for_network_task_runner_; On 2015/01/09 02:55:46, Wez wrote: > Just weak_factory_, please - it's implicit that this is dispensing WeakPtrs for > dereference on the network task runner, since that's the one this object is > dereferenced on. I thought that the qualification is useful because: 1. some HostProcess's instance methods can be invoked on other threads, for example: OnInitializePairingRegistry, StartOnUiThread 2. There is both ShutdownOnUiThread and ShutdownOnNetworkThread. I thought that without a qualification reader of ShutdownOnNetworkThread will wonder why a generic weak_factory is being invalidated here. FWIW, I dropped the suffix from the name.
LGTM once the remaining nits are addressed. :D https://codereview.chromium.org/719983002/diff/480001/remoting/host/heartbeat... File remoting/host/heartbeat_sender_unittest.cc (right): https://codereview.chromium.org/719983002/diff/480001/remoting/host/heartbeat... remoting/host/heartbeat_sender_unittest.cc:319: EXPECT_CALL(mock_ack_callback, Run(true /* success */)).Times(1); On 2015/01/09 19:00:13, lukasza wrote: > On 2015/01/09 02:55:45, Wez wrote: > > In your other comments you indicated that true is failure, not success. > > No. I was trying to indicate that "false" is bound to "success" argument, not > that "false" means "success" (success=false means failure). D'oh! OK. The problem is that the meaning is ambiguous, so I think as long as the API defines the meaning of the parameter, I'd remove the comment, to avoid confusion. If you'd prefer to just use an enum for extreme clarity then that's also fine with me - apologies for suggesting otherwise! https://codereview.chromium.org/719983002/diff/480001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/480001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:374: base::WeakPtrFactory<HostProcess> weak_factory_for_network_task_runner_; On 2015/01/09 19:00:14, lukasza wrote: > On 2015/01/09 02:55:46, Wez wrote: > > Just weak_factory_, please - it's implicit that this is dispensing WeakPtrs > for > > dereference on the network task runner, since that's the one this object is > > dereferenced on. > > I thought that the qualification is useful because: > 1. some HostProcess's instance methods can be invoked on other threads, for > example: OnInitializePairingRegistry, StartOnUiThread > 2. There is both ShutdownOnUiThread and ShutdownOnNetworkThread. I thought that > without a qualification reader of ShutdownOnNetworkThread will wonder why a > generic weak_factory is being invalidated here. > > FWIW, I dropped the suffix from the name. The point is that _any_ WeakPtrFactory _must_ be invalidated on the same thread on which the WeakPtrs it dispenses are checked & dereferenced. e.g. it wouldn't make sense to have a weak_factory_for_ui_thread_ and a weak_factory_for_network_thread_ on the same object, unless you were doing something really crazy! https://codereview.chromium.org/719983002/diff/520001/remoting/host/host_sign... File remoting/host/host_signaling_manager.cc (right): https://codereview.chromium.org/719983002/diff/520001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:96: weak_factory_for_listener_->InvalidateWeakPtrs(); nit: Suggest adding a comment e.g. "Ensure that any subsequent callbacks from the HeartbeatSender or SignalingConnector don't touch the Listener." https://codereview.chromium.org/719983002/diff/520001/remoting/host/host_sign... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/719983002/diff/520001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:102: // destoyed or when SendHostOfflineReasonAndDelete method is called. typo: destroyed https://codereview.chromium.org/719983002/diff/520001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:107: // where an instance of HostProcess has been already destroyed, but we want nit: already been https://codereview.chromium.org/719983002/diff/520001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:108: // to keep the process running while SendHostOfflineReasonAndDelete tries to nit: No need for the full method name; suggest "... while we try to ..." https://codereview.chromium.org/719983002/diff/520001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/520001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:800: // Overridden from HostSignalingManager::Listener nit: No need for this, or the similar comment below. https://codereview.chromium.org/719983002/diff/520001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1389: // Overridden from HostSignalingManager::Listener No need for this comment.
The CQ bit was checked by lukasza@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/719983002/560001
Oops... forgot to publish the comments and changes I made earlier yesterday. https://codereview.chromium.org/719983002/diff/480001/remoting/host/heartbeat... File remoting/host/heartbeat_sender_unittest.cc (right): https://codereview.chromium.org/719983002/diff/480001/remoting/host/heartbeat... remoting/host/heartbeat_sender_unittest.cc:319: EXPECT_CALL(mock_ack_callback, Run(true /* success */)).Times(1); On 2015/01/10 01:43:32, Wez wrote: > On 2015/01/09 19:00:13, lukasza wrote: > > On 2015/01/09 02:55:45, Wez wrote: > > > In your other comments you indicated that true is failure, not success. > > > > No. I was trying to indicate that "false" is bound to "success" argument, not > > that "false" means "success" (success=false means failure). > > D'oh! OK. The problem is that the meaning is ambiguous, so I think as long as > the API defines the meaning of the parameter, I'd remove the comment, to avoid > confusion. If you'd prefer to just use an enum for extreme clarity then that's > also fine with me - apologies for suggesting otherwise! :-) The declaration of SetHostOfflineReason names the parameter of the callback, so I think the meaning of the parameter should be easy to understand by future maintainers: void SetHostOfflineReason( ... const base::Callback<void(bool success)>& ack_callback); I could in theory add an extra comment, but I think it would not add much value and might just go unnoticed while definitely adding extra noise to the overall comments volume. https://codereview.chromium.org/719983002/diff/480001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/480001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:374: base::WeakPtrFactory<HostProcess> weak_factory_for_network_task_runner_; On 2015/01/10 01:43:32, Wez wrote: > On 2015/01/09 19:00:14, lukasza wrote: > > On 2015/01/09 02:55:46, Wez wrote: > > > Just weak_factory_, please - it's implicit that this is dispensing WeakPtrs > > for > > > dereference on the network task runner, since that's the one this object is > > > dereferenced on. > > > > I thought that the qualification is useful because: > > 1. some HostProcess's instance methods can be invoked on other threads, for > > example: OnInitializePairingRegistry, StartOnUiThread > > 2. There is both ShutdownOnUiThread and ShutdownOnNetworkThread. I thought > that > > without a qualification reader of ShutdownOnNetworkThread will wonder why a > > generic weak_factory is being invalidated here. > > > > FWIW, I dropped the suffix from the name. > > The point is that _any_ WeakPtrFactory _must_ be invalidated on the same thread > on which the WeakPtrs it dispenses are checked & dereferenced. e.g. it wouldn't > make sense to have a weak_factory_for_ui_thread_ and a > weak_factory_for_network_thread_ on the same object, unless you were doing > something really crazy! Oh, ok. I think I get your point: 1) there should be only one "main" thread associated with an object and 2) there is no point in explicitly referring to this thread everywhere. https://codereview.chromium.org/719983002/diff/520001/remoting/host/host_sign... File remoting/host/host_signaling_manager.cc (right): https://codereview.chromium.org/719983002/diff/520001/remoting/host/host_sign... remoting/host/host_signaling_manager.cc:96: weak_factory_for_listener_->InvalidateWeakPtrs(); On 2015/01/10 01:43:32, Wez wrote: > nit: Suggest adding a comment e.g. "Ensure that any subsequent callbacks from > the HeartbeatSender or SignalingConnector don't touch the Listener." Done. https://codereview.chromium.org/719983002/diff/520001/remoting/host/host_sign... File remoting/host/host_signaling_manager.h (right): https://codereview.chromium.org/719983002/diff/520001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:107: // where an instance of HostProcess has been already destroyed, but we want On 2015/01/10 01:43:32, Wez wrote: > nit: already been Done. I actually had this written the other way around and thought that this sounds more English-like this way... oh, well... :-) https://codereview.chromium.org/719983002/diff/520001/remoting/host/host_sign... remoting/host/host_signaling_manager.h:108: // to keep the process running while SendHostOfflineReasonAndDelete tries to On 2015/01/10 01:43:32, Wez wrote: > nit: No need for the full method name; suggest "... while we try to ..." Done. https://codereview.chromium.org/719983002/diff/520001/remoting/host/remoting_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/719983002/diff/520001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:800: // Overridden from HostSignalingManager::Listener On 2015/01/10 01:43:32, Wez wrote: > nit: No need for this, or the similar comment below. Done. https://codereview.chromium.org/719983002/diff/520001/remoting/host/remoting_... remoting/host/remoting_me2me_host.cc:1389: // Overridden from HostSignalingManager::Listener On 2015/01/10 01:43:32, Wez wrote: > No need for this comment. Done.
I mean earlier *today*... :-/
Message was sent while issue was closed.
Committed patchset #27 (id:560001)
Message was sent while issue was closed.
Patchset 27 (id:??) landed as https://crrev.com/806efecb817d9a4fd9dfe460b5ce7d132ace615d Cr-Commit-Position: refs/heads/master@{#311077} |
