Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6621062:
Refactor sync notifier out of sync api. (Closed)
|
Created:
9 years, 9 months ago by Agrawal Modified:
9 years, 7 months ago CC:
chromium-reviews, Raghu Simha, ncarter (slow), Paweł Hajdan Jr., tim (not reviewing), idana Visibility:
Public. |
DescriptionRefactor sync notifier out of sync api.
We now have a clearly defined interface for notifier.
Old p2p notifications are still used by tests.
BUG=
TEST=
Patch Set 1 #
Total comments: 24
Patch Set 2 : Add SendNotification method. #Patch Set 3 : Address few comments (no interface changes). #Patch Set 4 : Renamed SyncNotifierCallback to SyncNotifierObserver #Patch Set 5 : Parsing Incoming notification data in the syncnotifier. #Patch Set 6 : Fix build. #
Total comments: 32
Patch Set 7 : Split Login method and remove notification logic from syncapi. #
Total comments: 43
Patch Set 8 : Addressing comments. #
Total comments: 16
Patch Set 9 : Minox fixes. #Messages
Total messages: 18 (0 generated)
Some initial comments. Thanks for doing this! http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/engine/sync... File chrome/browser/sync/engine/syncapi.cc (right): http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/engine/sync... chrome/browser/sync/engine/syncapi.cc:2590: scoped_ptr<SyncNotifierFactory> sync_notifier_factory( no need for this to be a scoped_ptr, just create it on the stack http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... File chrome/browser/sync/notifier/sync_notifier.h (right): http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier.h:16: virtual void Login( have a public virtual destructor http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier.h:17: const std::string& email, const std::string& token, need #include string http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier.h:19: SyncNotifierCallback* sync_notifier_callback) = 0; I think that instead of bundling SyncNotifierCallback into Login, there should be separate functions: void AddObserver(SyncNotifierCallback*); void RemoveObserver(SyncNotifierCallback*); I think decoupling delegate/observer setting from initialization makes the class easier to test/work with, and eventually I may want to attach javascript listeners to the notifier (for about:sync), so supporting multiple observers would be nice. ObserverList should make this easy. http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier.h:22: virtual void Logout() = 0; I think there's no need for an explicit Logout() interface -- we can just have the destructor do the work. http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... File chrome/browser/sync/notifier/sync_notifier_callback.h (right): http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier_callback.h:18: class SyncNotifierCallback { I think it would be better to rename this SyncNotifierObserver. In fact, the usual pattern would be to make this an inner class of SyncNotifier named Observer, but having it outside is fine, too. http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier_callback.h:24: const IncomingNotificationData& notification_data) = 0; I think this should take a browser_sync::sessions::TypePayloadMap instead; the details of marshaling the data from the string should be an implementation detail. http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier_callback.h:26: virtual void OnOutgoingNotification() = 0; No need for an outgoing notification. http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier_callback.h:27: virtual void StoreCookie(const std::string& cookie) = 0; I don't think this should be part of the observer interface; a separate object should be passed to SyncNotifier. http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... File chrome/browser/sync/notifier/sync_notifier_factory.cc (right): http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier_factory.cc:42: const CommandLine& command_line = *CommandLine::ForCurrentProcess(); It would help testability to make command_line be a parameter to this function http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier_factory.cc:85: return (SyncNotifier*) new SyncNotifierImpl(notifier_options); no need for this cast http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... File chrome/browser/sync/notifier/sync_notifier_impl.h (right): http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier_impl.h:59: void SendNotification(); I think this should be part of the SyncNotifier interface, actually, maybe suitably renamed. It's gross, but I'd rather have the grossness visible instead of hiding it in the implementation. Also, eventually, I hope to have two separate implementations, one that uses ServerNotifierThread and another that uses MediatorThread.
Addressed few comments. Working on the interface changes as suggested by Fred. http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/engine/sync... File chrome/browser/sync/engine/syncapi.cc (right): http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/engine/sync... chrome/browser/sync/engine/syncapi.cc:2590: scoped_ptr<SyncNotifierFactory> sync_notifier_factory( On 2011/03/08 02:48:30, akalin wrote: > no need for this to be a scoped_ptr, just create it on the stack Done. http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... File chrome/browser/sync/notifier/sync_notifier.h (right): http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier.h:16: virtual void Login( On 2011/03/08 02:48:30, akalin wrote: > have a public virtual destructor Done. Thanks for pointing out. http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier.h:17: const std::string& email, const std::string& token, On 2011/03/08 02:48:30, akalin wrote: > need #include string Done. http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... File chrome/browser/sync/notifier/sync_notifier_factory.cc (right): http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier_factory.cc:42: const CommandLine& command_line = *CommandLine::ForCurrentProcess(); On 2011/03/08 02:48:30, akalin wrote: > It would help testability to make command_line be a parameter to this function Done. http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier_factory.cc:85: return (SyncNotifier*) new SyncNotifierImpl(notifier_options); On 2011/03/08 02:48:30, akalin wrote: > no need for this cast Done. http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... File chrome/browser/sync/notifier/sync_notifier_impl.h (right): http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier_impl.h:59: void SendNotification(); On 2011/03/08 02:48:30, akalin wrote: > I think this should be part of the SyncNotifier interface, actually, maybe > suitably renamed. It's gross, but I'd rather have the grossness visible instead > of hiding it in the implementation. > > Also, eventually, I hope to have two separate implementations, one that uses > ServerNotifierThread and another that uses MediatorThread. Added SendNotification to the interface.
Renamed SyncNotifierCallback to SyncNotifierObserver. Added support for multiple observers. http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... File chrome/browser/sync/notifier/sync_notifier.h (right): http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier.h:19: SyncNotifierCallback* sync_notifier_callback) = 0; On 2011/03/08 02:48:30, akalin wrote: > I think that instead of bundling SyncNotifierCallback into Login, there should > be separate functions: > > void AddObserver(SyncNotifierCallback*); > void RemoveObserver(SyncNotifierCallback*); > > I think decoupling delegate/observer setting from initialization makes the class > easier to test/work with, and eventually I may want to attach javascript > listeners to the notifier (for about:sync), so supporting multiple observers > would be nice. ObserverList should make this easy. Done. Thanks for the suggestion. http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier.h:22: virtual void Logout() = 0; On 2011/03/08 02:48:30, akalin wrote: > I think there's no need for an explicit Logout() interface -- we can just have > the destructor do the work. Done. http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... File chrome/browser/sync/notifier/sync_notifier_callback.h (right): http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier_callback.h:18: class SyncNotifierCallback { On 2011/03/08 02:48:30, akalin wrote: > I think it would be better to rename this SyncNotifierObserver. In fact, the > usual pattern would be to make this an inner class of SyncNotifier named > Observer, but having it outside is fine, too. Done. http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier_callback.h:24: const IncomingNotificationData& notification_data) = 0; On 2011/03/08 02:48:30, akalin wrote: > I think this should take a browser_sync::sessions::TypePayloadMap instead; the > details of marshaling the data from the string should be an implementation > detail. The data parsing also needs ModelSafeWorkerRegistrar for p2p notifications. p2p notifications logic looks at the service_url and then it just generates the map for ALL enabled datatypes. If service_url check is not important we can just override the type_map in syncapi.cc (for p2p notifications). http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier_callback.h:26: virtual void OnOutgoingNotification() = 0; On 2011/03/08 02:48:30, akalin wrote: > No need for an outgoing notification. SyncInternal::OnOutgoingNotification increments some stats on this. Is this no longer used? http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... chrome/browser/sync/notifier/sync_notifier_callback.h:27: virtual void StoreCookie(const std::string& cookie) = 0; On 2011/03/08 02:48:30, akalin wrote: > I don't think this should be part of the observer interface; a separate object > should be passed to SyncNotifier. This is not clear to me. The state/cookie is currently just a string in the notifier interface, and an implementation may decide to not use it at all. What are the benefits for removing this from here?
> The data parsing also needs ModelSafeWorkerRegistrar for p2p > notifications. p2p notifications logic looks at the service_url and then > it just generates the map for ALL enabled datatypes. If service_url > check is not important we can just override the type_map in syncapi.cc > (for p2p notifications). Hmm...since the interface has UpdateEnabledTypes, I think it's fine to store that and use it instead of ModelSafeWorkerRegistrar for enabled types. > SyncInternal::OnOutgoingNotification increments some stats on this. Is > this no longer used? Hmm...not really. But I think you can move the stat incrementing to the code that actually sends the outgoing notification. > http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... > chrome/browser/sync/notifier/sync_notifier_callback.h:27: virtual void > StoreCookie(const std::string& cookie) = 0; > On 2011/03/08 02:48:30, akalin wrote: >> >> I don't think this should be part of the observer interface; a > > separate object >> >> should be passed to SyncNotifier. > > This is not clear to me. > The state/cookie is currently just a string in the notifier interface, > and an implementation may decide to not use it at all. What are the > benefits for removing this from here? Just separation of concerns, especially since there can logically be many observers, but there should be at most one data writer. Ideally, the handling of the state wouldn't be part of the notifications interface (since it's really just a detail of one of the implementations). If there's an easy way to hide the state handling in the implementation, then I'd prefer that. If not, you should add a TODO to do so.
Should I wait for another upload or should I make another pass? :) On 2011/03/09 02:07:25, akalin wrote: > > The data parsing also needs ModelSafeWorkerRegistrar for p2p > > notifications. p2p notifications logic looks at the service_url and then > > it just generates the map for ALL enabled datatypes. If service_url > > check is not important we can just override the type_map in syncapi.cc > > (for p2p notifications). > > Hmm...since the interface has UpdateEnabledTypes, I think it's fine to > store that and use it instead of ModelSafeWorkerRegistrar for enabled > types. > > > SyncInternal::OnOutgoingNotification increments some stats on this. Is > > this no longer used? > > Hmm...not really. But I think you can move the stat incrementing to > the code that actually sends the outgoing notification. > > > > http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... > > chrome/browser/sync/notifier/sync_notifier_callback.h:27: virtual void > > StoreCookie(const std::string& cookie) = 0; > > On 2011/03/08 02:48:30, akalin wrote: > >> > >> I don't think this should be part of the observer interface; a > > > > separate object > >> > >> should be passed to SyncNotifier. > > > > This is not clear to me. > > The state/cookie is currently just a string in the notifier interface, > > and an implementation may decide to not use it at all. What are the > > benefits for removing this from here? > > Just separation of concerns, especially since there can logically be > many observers, but there should be at most one data writer. > > Ideally, the handling of the state wouldn't be part of the > notifications interface (since it's really just a detail of one of the > implementations). If there's an easy way to hide the state handling > in the implementation, then I'd prefer that. If not, you should add a > TODO to do so.
On Wed, Mar 9, 2011 at 2:39 PM, <akalin@chromium.org> wrote: > Should I wait for another upload or should I make another pass? :) I have a new patch almost ready. I will ping you after the upload. (I also did a rebase and there were some conflicts). > > On 2011/03/09 02:07:25, akalin wrote: > >> > The data parsing also needs ModelSafeWorkerRegistrar for p2p >> > notifications. p2p notifications logic looks at the service_url and then >> > it just generates the map for ALL enabled datatypes. If service_url >> > check is not important we can just override the type_map in syncapi.cc >> > (for p2p notifications). >> > > Hmm...since the interface has UpdateEnabledTypes, I think it's fine to >> store that and use it instead of ModelSafeWorkerRegistrar for enabled >> types. >> > Done > > > SyncInternal::OnOutgoingNotification increments some stats on this. Is >> > this no longer used? >> > > Hmm...not really. But I think you can move the stat incrementing to >> the code that actually sends the outgoing notification. >> > > Done > > >> > > > http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... > >> > chrome/browser/sync/notifier/sync_notifier_callback.h:27: virtual void >> > StoreCookie(const std::string& cookie) = 0; >> > On 2011/03/08 02:48:30, akalin wrote: >> >> >> >> I don't think this should be part of the observer interface; a >> > >> > separate object >> >> >> >> should be passed to SyncNotifier. >> > >> > This is not clear to me. >> > The state/cookie is currently just a string in the notifier interface, >> > and an implementation may decide to not use it at all. What are the >> > benefits for removing this from here? >> > > Just separation of concerns, especially since there can logically be >> many observers, but there should be at most one data writer. >> > > Ideally, the handling of the state wouldn't be part of the >> notifications interface (since it's really just a detail of one of the >> implementations). If there's an easy way to hide the state handling >> in the implementation, then I'd prefer that. If not, you should add a >> TODO to do so. >> > > Added a TODO. > > > http://codereview.chromium.org/6621062/ >
On Wed, Mar 9, 2011 at 2:39 PM, <akalin@chromium.org> wrote: > Should I wait for another upload or should I make another pass? :) Patch uploaded. PTAL. Thanks for the feedback so far. > > > On 2011/03/09 02:07:25, akalin wrote: > >> > The data parsing also needs ModelSafeWorkerRegistrar for p2p >> > notifications. p2p notifications logic looks at the service_url and then >> > it just generates the map for ALL enabled datatypes. If service_url >> > check is not important we can just override the type_map in syncapi.cc >> > (for p2p notifications). >> > > Hmm...since the interface has UpdateEnabledTypes, I think it's fine to >> store that and use it instead of ModelSafeWorkerRegistrar for enabled >> types. >> > > > SyncInternal::OnOutgoingNotification increments some stats on this. Is >> > this no longer used? >> > > Hmm...not really. But I think you can move the stat incrementing to >> the code that actually sends the outgoing notification. >> > > > >> > > > http://codereview.chromium.org/6621062/diff/1/chrome/browser/sync/notifier/sy... > >> > chrome/browser/sync/notifier/sync_notifier_callback.h:27: virtual void >> > StoreCookie(const std::string& cookie) = 0; >> > On 2011/03/08 02:48:30, akalin wrote: >> >> >> >> I don't think this should be part of the observer interface; a >> > >> > separate object >> >> >> >> should be passed to SyncNotifier. >> > >> > This is not clear to me. >> > The state/cookie is currently just a string in the notifier interface, >> > and an implementation may decide to not use it at all. What are the >> > benefits for removing this from here? >> > > Just separation of concerns, especially since there can logically be >> many observers, but there should be at most one data writer. >> > > Ideally, the handling of the state wouldn't be part of the >> notifications interface (since it's really just a detail of one of the >> implementations). If there's an easy way to hide the state handling >> in the implementation, then I'd prefer that. If not, you should add a >> TODO to do so. >> > > > > http://codereview.chromium.org/6621062/ >
Looking better! A few more comments. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncapi.cc (right): http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:41: #include "chrome/browser/sync/notifier/sync_notifier_observer.h" fix include order to be alphabetical http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:1653: const CommandLine& command_line = *CommandLine::ForCurrentProcess(); this logic should be either in the factory or the notifier implementation; ideally, the syncapi shouldn't have to worry about the notification method. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:1776: DCHECK(using_p2p_notifications_); this can be omitted http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:1783: sync_notifier_->SendNotification(¬ification_pending_); since this is only used in tests, just assume the notification was successful and set notification_pending_ to false. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:2400: if (using_p2p_notifications_) { You can remove this check; just add a comment that this only takes effect during sync integration tests. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:2565: if (syncer_thread()) { This can also be removed, I think, but you should run the integration tests before and after to make sure there's no hidden dependency on this. If there is, let me know and I can fix in in a separate CL. In fact, I might just remove this block in a separate CL, but do it in this one too. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:2569: // Send a notification as soon as subscriptions are on This can be removed http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:2597: SyncNotifierFactory sync_notifier_factory; It's been a defect that the talk mediator gets reinitialized on new credentials. I think we can fix this now, and provide a cleaner interface. Basically, replace the Login() method with an UpdateCredentials() method that takes email/token and can be called multiple times, and a SetState() function that should only be called once (before any call to UpdateCredentials()). (For now it's okay to reinitialize TalkMediator in the implementation of UpdateCredentials(), but we can change that later). Then we can simply call sync_notifier_->UpdateCredentials() here. So basically sync_notifier_ will be constructed from the factory at Init() instead of here, and the state will be set there. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/notifie... File chrome/browser/sync/notifier/sync_notifier.h (right): http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier.h:5: // Interface to the sync notifier. Expand this comment, maybe something like: Interface for a sync notifier, which is an object that receives notifications when updates are available for a set of sync types. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier.h:12: #include "chrome/browser/sync/notifier/sync_notifier_observer.h" forward-declare SyncNotifierObserver http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier.h:25: virtual void Login( See comment in syncapi.cc re. changing Login() to UpdateCredentials(). http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier.h:30: virtual void SendNotification(bool* notification_pending) = 0; Is there a way to avoid having to send this bool? http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier.h:30: virtual void SendNotification(bool* notification_pending) = 0; Add a comment saying that this is here only to support the old p2p notification implementation, which is still used by sync integration tests. Add a TODO(akalin) to remove this once we move the integration tests off p2p notifications. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/notifie... File chrome/browser/sync/notifier/sync_notifier_observer.h (right): http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_observer.h:8: #include <string> http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_observer.h:24: virtual void StoreCookie(const std::string& cookie) = 0; Rename to WriteState or StoreState, matching existing terminology. (I don't think changing to "cookie" really adds anything.)
http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncapi.cc (right): http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:2565: if (syncer_thread()) { On 2011/03/10 04:26:45, akalin wrote: > This can also be removed, I think, but you should run the integration tests > before and after to make sure there's no hidden dependency on this. If there > is, let me know and I can fix in in a separate CL. In fact, I might just remove > this block in a separate CL, but do it in this one too. Correction: This actually shouldn't be removed. However, this can be moved to the notifier implementation; basically, if notification method is p2p, then the notifier implementation should synthesize a notification for all data types whenever it connects (or reconnects).
On Thu, Mar 10, 2011 at 2:10 PM, <akalin@chromium.org> wrote: > > > http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... > File chrome/browser/sync/engine/syncapi.cc (right): > > > http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... > chrome/browser/sync/engine/syncapi.cc:2565: if (syncer_thread()) { > On 2011/03/10 04:26:45, akalin wrote: > >> This can also be removed, I think, but you should run the integration >> > tests > >> before and after to make sure there's no hidden dependency on this. >> > If there > >> is, let me know and I can fix in in a separate CL. In fact, I might >> > just remove > >> this block in a separate CL, but do it in this one too. >> > > Correction: This actually shouldn't be removed. However, this can be > moved to the notifier implementation; basically, if notification method > is p2p, then the notifier implementation should synthesize a > notification for all data types whenever it connects (or reconnects). Yeah, some integration test cases were failing. I will move this to the implementation. > > > http://codereview.chromium.org/6621062/ >
Thanks for the feedback. Addressed all comments. PTAL. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncapi.cc (right): http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:41: #include "chrome/browser/sync/notifier/sync_notifier_observer.h" On 2011/03/10 04:26:45, akalin wrote: > fix include order to be alphabetical Done. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:1653: const CommandLine& command_line = *CommandLine::ForCurrentProcess(); On 2011/03/10 04:26:45, akalin wrote: > this logic should be either in the factory or the notifier implementation; > ideally, the syncapi shouldn't have to worry about the notification method. I was using this to prevent calling SendNotification for the normal code path. I am now calling SendNotification but doing noting in the implementation if we are using p2p notifications (the implementation knows about the notification method). http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:1776: DCHECK(using_p2p_notifications_); On 2011/03/10 04:26:45, akalin wrote: > this can be omitted Done. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:1783: sync_notifier_->SendNotification(¬ification_pending_); On 2011/03/10 04:26:45, akalin wrote: > since this is only used in tests, just assume the notification was successful > and set notification_pending_ to false. Done. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:2400: if (using_p2p_notifications_) { On 2011/03/10 04:26:45, akalin wrote: > You can remove this check; just add a comment that this only takes effect during > sync integration tests. Done. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:2565: if (syncer_thread()) { On 2011/03/10 22:10:30, akalin wrote: > On 2011/03/10 04:26:45, akalin wrote: > > This can also be removed, I think, but you should run the integration tests > > before and after to make sure there's no hidden dependency on this. If there > > is, let me know and I can fix in in a separate CL. In fact, I might just > remove > > this block in a separate CL, but do it in this one too. > > Correction: This actually shouldn't be removed. However, this can be moved to > the notifier implementation; basically, if notification method is p2p, then the > notifier implementation should synthesize a notification for all data types > whenever it connects (or reconnects). Yeah, some integration test cases were failing. I have moved this to the implementation. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:2565: if (syncer_thread()) { On 2011/03/10 04:26:45, akalin wrote: > This can also be removed, I think, but you should run the integration tests > before and after to make sure there's no hidden dependency on this. If there > is, let me know and I can fix in in a separate CL. In fact, I might just remove > this block in a separate CL, but do it in this one too. Move this to the notifier implementation. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:2569: // Send a notification as soon as subscriptions are on On 2011/03/10 04:26:45, akalin wrote: > This can be removed Done. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:2597: SyncNotifierFactory sync_notifier_factory; On 2011/03/10 04:26:45, akalin wrote: > It's been a defect that the talk mediator gets reinitialized on new credentials. > I think we can fix this now, and provide a cleaner interface. Basically, > replace the Login() method with an UpdateCredentials() method that takes > email/token and can be called multiple times, and a SetState() function that > should only be called once (before any call to UpdateCredentials()). (For now > it's okay to reinitialize TalkMediator in the implementation of > UpdateCredentials(), but we can change that later). Then we can simply call > sync_notifier_->UpdateCredentials() here. > > So basically sync_notifier_ will be constructed from the factory at Init() > instead of here, and the state will be set there. Done. As fas as I understand, we need to call setstate after OpenDirectory is called in SignIn. I have moved sync notifier construction there too. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/notifie... File chrome/browser/sync/notifier/sync_notifier.h (right): http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier.h:5: // Interface to the sync notifier. On 2011/03/10 04:26:45, akalin wrote: > Expand this comment, maybe something like: > > Interface for a sync notifier, which is an object that receives notifications > when updates are available for a set of sync types. Done. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier.h:12: #include "chrome/browser/sync/notifier/sync_notifier_observer.h" On 2011/03/10 04:26:45, akalin wrote: > forward-declare SyncNotifierObserver Done. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier.h:25: virtual void Login( On 2011/03/10 04:26:45, akalin wrote: > See comment in syncapi.cc re. changing Login() to UpdateCredentials(). Done. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier.h:30: virtual void SendNotification(bool* notification_pending) = 0; On 2011/03/10 04:26:45, akalin wrote: > Add a comment saying that this is here only to support the old p2p notification > implementation, which is still used by sync integration tests. Add a > TODO(akalin) to remove this once we move the integration tests off p2p > notifications. Done. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier.h:30: virtual void SendNotification(bool* notification_pending) = 0; On 2011/03/10 04:26:45, akalin wrote: > Is there a way to avoid having to send this bool? Done. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/notifie... File chrome/browser/sync/notifier/sync_notifier_observer.h (right): http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_observer.h:8: On 2011/03/10 04:26:45, akalin wrote: > #include <string> Done. http://codereview.chromium.org/6621062/diff/15001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_observer.h:24: virtual void StoreCookie(const std::string& cookie) = 0; On 2011/03/10 04:26:45, akalin wrote: > Rename to WriteState or StoreState, matching existing terminology. (I don't > think changing to "cookie" really adds anything.) Done.
More comments, but I think i'm satisfied with the interface http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncapi.cc (left): http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:1795: if (!talk_mediator()) { Restore this check, but instead check sync_notifier_ http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncapi.cc (right): http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:1451: // SyncNotifier to setup notifications. Change comment to something like: // The SyncNotifier which notifies us when updates need to be downloaded. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:1765: // Since this is used only in tests, we assume that the notification no need for this comment http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:1768: notification_pending_ = false; I think you can get rid of notification_pending_ entirely and rename this function to SendNotification(). http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:1812: SyncNotifierFactory sync_notifier_factory; Add a TODO to make sync_notifier_factory be a parameter to the constructor/Init() for testability. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:1819: if (lookup.good()) add braces http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:1821: else here too http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:2415: core_message_loop_->PostTask( this can change to: bool new_notification ...; if (new_notification) { Post(SendNotification...); } http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:2688: // Here we rely on the default notification method being SERVER. Remove this now-obsolete comment http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... File chrome/browser/sync/notifier/sync_notifier.h (right): http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier.h:27: virtual void UpdateCredentials( Add a comment saying that notifications won't be sent until UpdateCredentials() is called at least once, and that it can be called multiple times. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier.h:30: virtual void SetState(const std::string& state) = 0; Add a comment saying that this must be called before any call to UpdateCredentials. (In fact, move this to be before UpdateCredentials) http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... File chrome/browser/sync/notifier/sync_notifier_factory.cc (right): http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_factory.cc:44: notifier::NotifierOptions* notifier_options = i think it's fine to pass this by value (i.e., not heap-allocated) http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... File chrome/browser/sync/notifier/sync_notifier_factory.h (right): http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_factory.h:19: // Creates the appropriate sync notifier. Add a comment about the caller taking ownership http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... File chrome/browser/sync/notifier/sync_notifier_impl.cc (right): http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_impl.cc:22: : notifier_options_(notifier_options) { } null out server_notifier_thread_, too http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_impl.cc:25: // We NULL out talk_mediator_ so that any pending tasks do not this comment should be moved to SyncManager::Shutdown (sync_notifier instead of talk_mediator) http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_impl.cc:63: model_types_with_payloads = combine w/ previous line http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_impl.cc:108: model_types_with_payloads = remove repeated line http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_impl.cc:127: if (notifier_options_->notification_method == reset talk_mediator_ before creating the ServerNotifierThread/MediatorThread, to avoid any possible problems with having two of those threads at the same time. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_impl.cc:177: VLOG(1) << "Not sending notification: shutting down (talk_mediator_ is " replace this VLOG with NOTREACHED(), since it shouldn't happen (and since we do this check in syncapi.cc) http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... File chrome/browser/sync/notifier/sync_notifier_impl.h (right): http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_impl.h:53: // Should be called only once, before any calls to UpdateCredentials. move this comment to interface file http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_impl.h:66: notifier::TalkMediator* talk_mediator() { return talk_mediator_.get(); } no need for this accessor
http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncapi.cc (left): http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:1795: if (!talk_mediator()) { On 2011/03/11 04:17:14, akalin wrote: > Restore this check, but instead check sync_notifier_ Done. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncapi.cc (right): http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:1451: // SyncNotifier to setup notifications. On 2011/03/11 04:17:14, akalin wrote: > Change comment to something like: > > // The SyncNotifier which notifies us when updates need to be downloaded. Done. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:1765: // Since this is used only in tests, we assume that the notification On 2011/03/11 04:17:14, akalin wrote: > no need for this comment Done. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:1768: notification_pending_ = false; On 2011/03/11 04:17:14, akalin wrote: > I think you can get rid of notification_pending_ entirely and rename this > function to SendNotification(). Done. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:1812: SyncNotifierFactory sync_notifier_factory; On 2011/03/11 04:17:14, akalin wrote: > Add a TODO to make sync_notifier_factory be a parameter to the > constructor/Init() for testability. Done. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:1819: if (lookup.good()) On 2011/03/11 04:17:14, akalin wrote: > add braces Done. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:1821: else On 2011/03/11 04:17:14, akalin wrote: > here too Done. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:2415: core_message_loop_->PostTask( On 2011/03/11 04:17:14, akalin wrote: > this can change to: > > bool new_notification ...; > if (new_notification) { > Post(SendNotification...); > } Thanks. Got rid of the argument (new_pending_notification) http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:2688: // Here we rely on the default notification method being SERVER. On 2011/03/11 04:17:14, akalin wrote: > Remove this now-obsolete comment Done. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... File chrome/browser/sync/notifier/sync_notifier.h (right): http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier.h:27: virtual void UpdateCredentials( On 2011/03/11 04:17:14, akalin wrote: > Add a comment saying that notifications won't be sent until UpdateCredentials() > is called at least once, and that it can be called multiple times. Done. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier.h:30: virtual void SetState(const std::string& state) = 0; On 2011/03/11 04:17:14, akalin wrote: > Add a comment saying that this must be called before any call to > UpdateCredentials. (In fact, move this to be before UpdateCredentials) Done. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... File chrome/browser/sync/notifier/sync_notifier_factory.cc (right): http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_factory.cc:44: notifier::NotifierOptions* notifier_options = On 2011/03/11 04:17:14, akalin wrote: > i think it's fine to pass this by value (i.e., not heap-allocated) Ok. Now I am creating notifier_options_ on the stack and passing it by reference and then copying it in the SyncNotifierImpl constructor. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... File chrome/browser/sync/notifier/sync_notifier_factory.h (right): http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_factory.h:19: // Creates the appropriate sync notifier. On 2011/03/11 04:17:14, akalin wrote: > Add a comment about the caller taking ownership Done. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... File chrome/browser/sync/notifier/sync_notifier_impl.cc (right): http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_impl.cc:22: : notifier_options_(notifier_options) { } On 2011/03/11 04:17:14, akalin wrote: > null out server_notifier_thread_, too Done. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_impl.cc:25: // We NULL out talk_mediator_ so that any pending tasks do not On 2011/03/11 04:17:14, akalin wrote: > this comment should be moved to SyncManager::Shutdown (sync_notifier instead of > talk_mediator) Done. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_impl.cc:63: model_types_with_payloads = On 2011/03/11 04:17:14, akalin wrote: > combine w/ previous line Done. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_impl.cc:108: model_types_with_payloads = On 2011/03/11 04:17:14, akalin wrote: > remove repeated line Done. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_impl.cc:127: if (notifier_options_->notification_method == On 2011/03/11 04:17:14, akalin wrote: > reset talk_mediator_ before creating the ServerNotifierThread/MediatorThread, to > avoid any possible problems with having two of those threads at the same time. Done. Do you think that we should call talk_mediator_->logout as well, whenever we update the credentials with new values? http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_impl.cc:177: VLOG(1) << "Not sending notification: shutting down (talk_mediator_ is " On 2011/03/11 04:17:14, akalin wrote: > replace this VLOG with NOTREACHED(), since it shouldn't happen (and since we do > this check in syncapi.cc) Done. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... File chrome/browser/sync/notifier/sync_notifier_impl.h (right): http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_impl.h:53: // Should be called only once, before any calls to UpdateCredentials. On 2011/03/11 04:17:14, akalin wrote: > move this comment to interface file Done. http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_impl.h:66: notifier::TalkMediator* talk_mediator() { return talk_mediator_.get(); } On 2011/03/11 04:17:14, akalin wrote: > no need for this accessor Done.
LGTM after nits
Make sure to put through the {linux,mac,win}_sync trybots, and keep an eye on
the sync buildbots when you check in
http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie...
File chrome/browser/sync/notifier/sync_notifier_impl.cc (right):
http://codereview.chromium.org/6621062/diff/17002/chrome/browser/sync/notifie...
chrome/browser/sync/notifier/sync_notifier_impl.cc:127: if
(notifier_options_->notification_method ==
On 2011/03/11 21:42:20, nileshagrawal wrote:
> On 2011/03/11 04:17:14, akalin wrote:
> > reset talk_mediator_ before creating the
ServerNotifierThread/MediatorThread,
> to
> > avoid any possible problems with having two of those threads at the same
time.
>
> Done. Do you think that we should call talk_mediator_->logout as well,
whenever
> we update the credentials with new values?
Nah, that's okay -- the destructor calls it if necessary
http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncapi.cc (right):
http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.cc:2678: model_types_with_payloads =
combine with previous declaration
http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie...
File chrome/browser/sync/notifier/sync_notifier_factory.cc (right):
http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie...
chrome/browser/sync/notifier/sync_notifier_factory.cc:21: net::HostPortPair
StringToHostPortPair(const std::string& host_port_str,
add include for HostPortPair
http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie...
chrome/browser/sync/notifier/sync_notifier_factory.cc:23: std::string::size_type
colon_index = host_port_str.find(':');
add include for string
http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie...
File chrome/browser/sync/notifier/sync_notifier_factory.h (right):
http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie...
chrome/browser/sync/notifier/sync_notifier_factory.h:16: SyncNotifierFactory()
{}
move constructor/destructor bodies to .cc file
http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie...
chrome/browser/sync/notifier/sync_notifier_factory.h:22: private:
no need for this line
http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie...
File chrome/browser/sync/notifier/sync_notifier_impl.cc (right):
http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie...
chrome/browser/sync/notifier/sync_notifier_impl.cc:15:
Add a TODO(akalin) to split this class into two implementations -- one for p2p,
and one for server
http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie...
chrome/browser/sync/notifier/sync_notifier_impl.cc:26: // TODO(akalin): NULL the
other member variables defensively, too.
this TODO belongs in syncapi, where we null out sync_notifier_
http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie...
chrome/browser/sync/notifier/sync_notifier_impl.cc:174: NOTREACHED() << "Can not
send notification: talk_mediator_ is NULL";
Can not -> cannot
Thanks for the in-depth review and suggestions. http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncapi.cc (right): http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncapi.cc:2678: model_types_with_payloads = On 2011/03/11 22:27:44, akalin wrote: > combine with previous declaration Done. http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... File chrome/browser/sync/notifier/sync_notifier_factory.cc (right): http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_factory.cc:21: net::HostPortPair StringToHostPortPair(const std::string& host_port_str, On 2011/03/11 22:27:44, akalin wrote: > add include for HostPortPair Done. http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_factory.cc:23: std::string::size_type colon_index = host_port_str.find(':'); On 2011/03/11 22:27:44, akalin wrote: > add include for string Done. http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... File chrome/browser/sync/notifier/sync_notifier_factory.h (right): http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_factory.h:16: SyncNotifierFactory() {} On 2011/03/11 22:27:44, akalin wrote: > move constructor/destructor bodies to .cc file Done. http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_factory.h:22: private: On 2011/03/11 22:27:44, akalin wrote: > no need for this line Done. http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... File chrome/browser/sync/notifier/sync_notifier_impl.cc (right): http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_impl.cc:15: On 2011/03/11 22:27:44, akalin wrote: > Add a TODO(akalin) to split this class into two implementations -- one for p2p, > and one for server Done. http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_impl.cc:26: // TODO(akalin): NULL the other member variables defensively, too. On 2011/03/11 22:27:44, akalin wrote: > this TODO belongs in syncapi, where we null out sync_notifier_ Done. http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... chrome/browser/sync/notifier/sync_notifier_impl.cc:174: NOTREACHED() << "Can not send notification: talk_mediator_ is NULL"; On 2011/03/11 22:27:44, akalin wrote: > Can not -> cannot Done.
Patch for try/submit at http://codereview.chromium.org/6621062/ On 2011/03/11 22:54:26, nileshagrawal wrote: > Thanks for the in-depth review and suggestions. > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/engine/... > File chrome/browser/sync/engine/syncapi.cc (right): > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/engine/... > chrome/browser/sync/engine/syncapi.cc:2678: model_types_with_payloads = > On 2011/03/11 22:27:44, akalin wrote: > > combine with previous declaration > > Done. > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... > File chrome/browser/sync/notifier/sync_notifier_factory.cc (right): > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... > chrome/browser/sync/notifier/sync_notifier_factory.cc:21: net::HostPortPair > StringToHostPortPair(const std::string& host_port_str, > On 2011/03/11 22:27:44, akalin wrote: > > add include for HostPortPair > > Done. > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... > chrome/browser/sync/notifier/sync_notifier_factory.cc:23: std::string::size_type > colon_index = host_port_str.find(':'); > On 2011/03/11 22:27:44, akalin wrote: > > add include for string > > Done. > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... > File chrome/browser/sync/notifier/sync_notifier_factory.h (right): > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... > chrome/browser/sync/notifier/sync_notifier_factory.h:16: SyncNotifierFactory() > {} > On 2011/03/11 22:27:44, akalin wrote: > > move constructor/destructor bodies to .cc file > > Done. > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... > chrome/browser/sync/notifier/sync_notifier_factory.h:22: private: > On 2011/03/11 22:27:44, akalin wrote: > > no need for this line > > Done. > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... > File chrome/browser/sync/notifier/sync_notifier_impl.cc (right): > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... > chrome/browser/sync/notifier/sync_notifier_impl.cc:15: > On 2011/03/11 22:27:44, akalin wrote: > > Add a TODO(akalin) to split this class into two implementations -- one for > p2p, > > and one for server > > Done. > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... > chrome/browser/sync/notifier/sync_notifier_impl.cc:26: // TODO(akalin): NULL the > other member variables defensively, too. > On 2011/03/11 22:27:44, akalin wrote: > > this TODO belongs in syncapi, where we null out sync_notifier_ > > Done. > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... > chrome/browser/sync/notifier/sync_notifier_impl.cc:174: NOTREACHED() << "Can not > send notification: talk_mediator_ is NULL"; > On 2011/03/11 22:27:44, akalin wrote: > > Can not -> cannot > > Done.
Oops, I mean http://codereview.chromium.org/6683015/ On 2011/03/11 23:08:00, akalin wrote: > Patch for try/submit at http://codereview.chromium.org/6621062/ > > On 2011/03/11 22:54:26, nileshagrawal wrote: > > Thanks for the in-depth review and suggestions. > > > > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/engine/... > > File chrome/browser/sync/engine/syncapi.cc (right): > > > > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/engine/... > > chrome/browser/sync/engine/syncapi.cc:2678: model_types_with_payloads = > > On 2011/03/11 22:27:44, akalin wrote: > > > combine with previous declaration > > > > Done. > > > > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... > > File chrome/browser/sync/notifier/sync_notifier_factory.cc (right): > > > > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... > > chrome/browser/sync/notifier/sync_notifier_factory.cc:21: net::HostPortPair > > StringToHostPortPair(const std::string& host_port_str, > > On 2011/03/11 22:27:44, akalin wrote: > > > add include for HostPortPair > > > > Done. > > > > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... > > chrome/browser/sync/notifier/sync_notifier_factory.cc:23: > std::string::size_type > > colon_index = host_port_str.find(':'); > > On 2011/03/11 22:27:44, akalin wrote: > > > add include for string > > > > Done. > > > > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... > > File chrome/browser/sync/notifier/sync_notifier_factory.h (right): > > > > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... > > chrome/browser/sync/notifier/sync_notifier_factory.h:16: SyncNotifierFactory() > > {} > > On 2011/03/11 22:27:44, akalin wrote: > > > move constructor/destructor bodies to .cc file > > > > Done. > > > > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... > > chrome/browser/sync/notifier/sync_notifier_factory.h:22: private: > > On 2011/03/11 22:27:44, akalin wrote: > > > no need for this line > > > > Done. > > > > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... > > File chrome/browser/sync/notifier/sync_notifier_impl.cc (right): > > > > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... > > chrome/browser/sync/notifier/sync_notifier_impl.cc:15: > > On 2011/03/11 22:27:44, akalin wrote: > > > Add a TODO(akalin) to split this class into two implementations -- one for > > p2p, > > > and one for server > > > > Done. > > > > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... > > chrome/browser/sync/notifier/sync_notifier_impl.cc:26: // TODO(akalin): NULL > the > > other member variables defensively, too. > > On 2011/03/11 22:27:44, akalin wrote: > > > this TODO belongs in syncapi, where we null out sync_notifier_ > > > > Done. > > > > > http://codereview.chromium.org/6621062/diff/20001/chrome/browser/sync/notifie... > > chrome/browser/sync/notifier/sync_notifier_impl.cc:174: NOTREACHED() << "Can > not > > send notification: talk_mediator_ is NULL"; > > On 2011/03/11 22:27:44, akalin wrote: > > > Can not -> cannot > > > > Done.
Checked in in other patch |
