From 8644dfa1f63dfc3a0d01132cd1b1287dde4b29ce Mon Sep 17 00:00:00 2001 From: "rlarocque@chromium.org" Date: Tue, 1 Apr 2014 01:20:14 +0000 Subject: [PATCH] sync: Buffer Protocol Events for about:sync page Allows the about:sync page to present the last six network events as soon as it's opened. Adds the ProtocolEventBuffer to the SyncManagerImpl. This class holds on to a few protocol events and returns them on demand. Modifies the interface to enable SyncBacknedHost protocol event forwarding. By default, it does not forward any events. There are now separate registration and unregistration functions. The registration function will send the set of buffered notifications to the UI thread, and also register it to receive incoming events in the future. It continues to forward events until the number of registration calls is matched by the number of un-registration calls. Makes about:sync's registration to receive events explicit. This was a long-time TODO. If we did not fix this issue, then the about:sync page could receive the initial set of buffered invalidations before it had its event listeners defined and registered. Makes the about:sync page keep track of known events and avoid adding duplicates to the list. Since the registration of a new event listener causes events to be distributed to all listeners, we must add this logic to ensure that opening one new about:sync tab will not cause previously opened tabs to display duplicate events. BUG=329373,349301 Review URL: https://codereview.chromium.org/212603007 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@260726 0039d316-1c4b-4281-b951-d872f2087c98 --- .../browser/resources/sync_internals/about.js | 62 ++++++++++----- .../resources/sync_internals/chrome_sync.js | 9 +++ chrome/browser/sync/glue/sync_backend_host.h | 10 ++- .../sync/glue/sync_backend_host_core.cc | 26 ++++++- .../sync/glue/sync_backend_host_core.h | 3 +- .../sync/glue/sync_backend_host_impl.cc | 16 +++- .../sync/glue/sync_backend_host_impl.h | 3 +- .../sync/glue/sync_backend_host_mock.cc | 4 +- .../sync/glue/sync_backend_host_mock.h | 3 +- chrome/browser/sync/profile_sync_service.cc | 10 +-- .../ui/webui/sync_internals_browsertest.js | 30 ++++++++ .../webui/sync_internals_message_handler.cc | 25 ++++-- .../ui/webui/sync_internals_message_handler.h | 3 + sync/internal_api/protocol_event_buffer.cc | 37 +++++++++ sync/internal_api/protocol_event_buffer.h | 42 ++++++++++ .../protocol_event_buffer_unittest.cc | 77 +++++++++++++++++++ .../public/events/commit_request_event.h | 3 +- .../public/events/commit_response_event.h | 3 +- .../configure_get_updates_request_event.h | 4 +- .../events/get_updates_response_event.h | 3 +- .../events/normal_get_updates_request_event.h | 3 +- .../events/poll_get_updates_request_event.h | 3 +- sync/internal_api/public/sync_manager.h | 4 + .../public/test/fake_sync_manager.h | 2 + sync/internal_api/sync_manager_impl.cc | 6 ++ sync/internal_api/sync_manager_impl.h | 4 + sync/internal_api/test/fake_sync_manager.cc | 5 ++ sync/sync_internal_api.gypi | 2 + sync/sync_tests.gypi | 1 + 29 files changed, 352 insertions(+), 51 deletions(-) create mode 100644 sync/internal_api/protocol_event_buffer.cc create mode 100644 sync/internal_api/protocol_event_buffer.h create mode 100644 sync/internal_api/protocol_event_buffer_unittest.cc diff --git a/chrome/browser/resources/sync_internals/about.js b/chrome/browser/resources/sync_internals/about.js index 172e690030afd5..61ad8aa2966893 100644 --- a/chrome/browser/resources/sync_internals/about.js +++ b/chrome/browser/resources/sync_internals/about.js @@ -19,12 +19,23 @@ cr.define('chrome.sync.about_tab', function() { /** Container for accumulated sync protocol events. */ var protocolEvents = []; + /** We may receive re-delivered events. Keep a record of ones we've seen. */ + var knownEventTimestamps = {}; + /** * Callback for incoming protocol events. * @param {Event} e The protocol event. */ function onReceivedProtocolEvent(e) { var details = e.details; + + // Return early if we've seen this event before. Assumes that timestamps + // are sufficiently high resolution to uniquely identify an event. + if (knownEventTimestamps.hasOwnProperty(details.time)) { + return; + } + + knownEventTimestamps[details.time] = true; protocolEvents.push(details); var context = new JsEvalContext({ events: protocolEvents }); @@ -43,29 +54,11 @@ cr.define('chrome.sync.about_tab', function() { } /** - * Toggles the given traffic event entry div's "expanded" state. - * @param {HTMLElement} element the element to toggle. - */ - function expandListener(element) { - element.target.classList.toggle('traffic-event-entry-expanded'); - } - - /** - * Attaches a listener to the given traffic event entry div. - * @param {HTMLElement} element the element to attach the listener to. + * Initializes listeners for status dump and import UI. */ - function addExpandListener(element) { - element.addEventListener('click', expandListener, false); - } - - function onLoad() { + function initStatusDumpButton() { $('status-data').hidden = true; - chrome.sync.events.addEventListener( - 'onAboutInfoUpdated', - onAboutInfoUpdatedEvent); - chrome.sync.requestUpdatedAboutInfo(); - var dumpStatusButton = $('dump-status'); dumpStatusButton.addEventListener('click', function(event) { var aboutInfo = chrome.sync.aboutInfo; @@ -111,8 +104,37 @@ cr.define('chrome.sync.about_tab', function() { var aboutInfo = JSON.parse(data); refreshAboutInfo(aboutInfo); }); + } + /** + * Toggles the given traffic event entry div's "expanded" state. + * @param {HTMLElement} element the element to toggle. + */ + function expandListener(element) { + element.target.classList.toggle('traffic-event-entry-expanded'); + } + + /** + * Attaches a listener to the given traffic event entry div. + * @param {HTMLElement} element the element to attach the listener to. + */ + function addExpandListener(element) { + element.addEventListener('click', expandListener, false); + } + + function onLoad() { + initStatusDumpButton(); initProtocolEventLog(); + + chrome.sync.events.addEventListener( + 'onAboutInfoUpdated', + onAboutInfoUpdatedEvent); + + // Register to receive a stream of event notifications. + chrome.sync.registerForEvents(); + + // Request an about info update event to initialize the page. + chrome.sync.requestUpdatedAboutInfo(); } return { diff --git a/chrome/browser/resources/sync_internals/chrome_sync.js b/chrome/browser/resources/sync_internals/chrome_sync.js index 81793d40a27297..f59c2bd8173661 100644 --- a/chrome/browser/resources/sync_internals/chrome_sync.js +++ b/chrome/browser/resources/sync_internals/chrome_sync.js @@ -82,6 +82,14 @@ cr.define('chrome.sync', function() { chrome.sync.events.dispatchEvent(e); }; + /** + * Registers to receive a stream of events through + * chrome.sync.dispatchEvent(). + */ + var registerForEvents = function() { + chrome.send('registerForEvents'); + }; + /** * Asks the browser to refresh our snapshot of sync state. Should result * in an onAboutInfoUpdated event being emitted. @@ -103,6 +111,7 @@ cr.define('chrome.sync', function() { dispatchEvent: dispatchEvent, events: new cr.EventTarget(), + registerForEvents: registerForEvents, requestUpdatedAboutInfo: requestUpdatedAboutInfo, requestListOfTypes: requestListOfTypes, }; diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h index a2861ef7f57ffe..caea2d08e4b9bb 100644 --- a/chrome/browser/sync/glue/sync_backend_host.h +++ b/chrome/browser/sync/glue/sync_backend_host.h @@ -192,8 +192,14 @@ class SyncBackendHost : public BackendDataTypeConfigurer { // Fetches the DeviceInfo tracker. virtual SyncedDeviceTracker* GetSyncedDeviceTracker() const = 0; - // Sets whether or not the frontend will be notified of network events. - virtual void SetForwardProtocolEvents(bool forward) = 0; + // Requests that the backend forward to the fronent any protocol events in + // its buffer and begin forwarding automatically from now on. Repeated calls + // to this function may result in the same events being emitted several + // times. + virtual void RequestBufferedProtocolEventsAndEnableForwarding() = 0; + + // Disables protocol event forwarding. + virtual void DisableProtocolEventForwarding() = 0; virtual base::MessageLoop* GetSyncLoopForTesting() = 0; diff --git a/chrome/browser/sync/glue/sync_backend_host_core.cc b/chrome/browser/sync/glue/sync_backend_host_core.cc index c433b0b3df9604..c49171a5404e2c 100644 --- a/chrome/browser/sync/glue/sync_backend_host_core.cc +++ b/chrome/browser/sync/glue/sync_backend_host_core.cc @@ -104,6 +104,7 @@ SyncBackendHostCore::SyncBackendHostCore( sync_loop_(NULL), registrar_(NULL), has_sync_setup_completed_(has_sync_setup_completed), + forward_protocol_events_(false), weak_ptr_factory_(this) { DCHECK(backend.get()); } @@ -605,9 +606,30 @@ void SyncBackendHostCore::DoRetryConfiguration( retry_callback); } -void SyncBackendHostCore::SetForwardProtocolEvents(bool enabled) { +void SyncBackendHostCore::SendBufferedProtocolEventsAndEnableForwarding() { DCHECK_EQ(base::MessageLoop::current(), sync_loop_); - forward_protocol_events_ = enabled; + forward_protocol_events_ = true; + + if (sync_manager_) { + // Grab our own copy of the buffered events. + // The buffer is not modified by this operation. + std::vector buffered_events; + sync_manager_->GetBufferedProtocolEvents().release(&buffered_events); + + // Send them all over the fence to the host. + for (std::vector::iterator it = + buffered_events.begin(); it != buffered_events.end(); ++it) { + // TODO(rlarocque): Make it explicit that host_ takes ownership. + host_.Call( + FROM_HERE, + &SyncBackendHostImpl::HandleProtocolEventOnFrontendLoop, + *it); + } + } +} + +void SyncBackendHostCore::DisableProtocolEventForwarding() { + forward_protocol_events_ = false; } void SyncBackendHostCore::DeleteSyncDataFolder() { diff --git a/chrome/browser/sync/glue/sync_backend_host_core.h b/chrome/browser/sync/glue/sync_backend_host_core.h index d246255698d09a..0c521002010b9e 100644 --- a/chrome/browser/sync/glue/sync_backend_host_core.h +++ b/chrome/browser/sync/glue/sync_backend_host_core.h @@ -209,7 +209,8 @@ class SyncBackendHostCore return synced_device_tracker_.get(); } - void SetForwardProtocolEvents(bool forward); + void SendBufferedProtocolEventsAndEnableForwarding(); + void DisableProtocolEventForwarding(); // Delete the sync data folder to cleanup backend data. Happens the first // time sync is enabled for a user (to prevent accidentally reusing old diff --git a/chrome/browser/sync/glue/sync_backend_host_impl.cc b/chrome/browser/sync/glue/sync_backend_host_impl.cc index de87a4939a3725..4c202f0d8e9038 100644 --- a/chrome/browser/sync/glue/sync_backend_host_impl.cc +++ b/chrome/browser/sync/glue/sync_backend_host_impl.cc @@ -475,12 +475,22 @@ SyncedDeviceTracker* SyncBackendHostImpl::GetSyncedDeviceTracker() const { return core_->synced_device_tracker(); } -void SyncBackendHostImpl::SetForwardProtocolEvents(bool forward) { +void SyncBackendHostImpl::RequestBufferedProtocolEventsAndEnableForwarding() { DCHECK(initialized()); registrar_->sync_thread()->message_loop()->PostTask( FROM_HERE, - base::Bind(&SyncBackendHostCore::SetForwardProtocolEvents, - core_, forward)); + base::Bind( + &SyncBackendHostCore::SendBufferedProtocolEventsAndEnableForwarding, + core_)); +} + +void SyncBackendHostImpl::DisableProtocolEventForwarding() { + DCHECK(initialized()); + registrar_->sync_thread()->message_loop()->PostTask( + FROM_HERE, + base::Bind( + &SyncBackendHostCore::DisableProtocolEventForwarding, + core_)); } void SyncBackendHostImpl::InitCore(scoped_ptr options) { diff --git a/chrome/browser/sync/glue/sync_backend_host_impl.h b/chrome/browser/sync/glue/sync_backend_host_impl.h index 24e621513239c4..d4568a245cb696 100644 --- a/chrome/browser/sync/glue/sync_backend_host_impl.h +++ b/chrome/browser/sync/glue/sync_backend_host_impl.h @@ -124,7 +124,8 @@ class SyncBackendHostImpl virtual void GetModelSafeRoutingInfo( syncer::ModelSafeRoutingInfo* out) const OVERRIDE; virtual SyncedDeviceTracker* GetSyncedDeviceTracker() const OVERRIDE; - virtual void SetForwardProtocolEvents(bool forward) OVERRIDE; + virtual void RequestBufferedProtocolEventsAndEnableForwarding() OVERRIDE; + virtual void DisableProtocolEventForwarding() OVERRIDE; virtual base::MessageLoop* GetSyncLoopForTesting() OVERRIDE; protected: diff --git a/chrome/browser/sync/glue/sync_backend_host_mock.cc b/chrome/browser/sync/glue/sync_backend_host_mock.cc index f7629b22227d2c..176652ace9f391 100644 --- a/chrome/browser/sync/glue/sync_backend_host_mock.cc +++ b/chrome/browser/sync/glue/sync_backend_host_mock.cc @@ -110,7 +110,9 @@ base::MessageLoop* SyncBackendHostMock::GetSyncLoopForTesting() { return NULL; } -void SyncBackendHostMock::SetForwardProtocolEvents(bool enabled) {} +void SyncBackendHostMock::RequestBufferedProtocolEventsAndEnableForwarding() {} + +void SyncBackendHostMock::DisableProtocolEventForwarding() {} void SyncBackendHostMock::set_fail_initial_download(bool should_fail) { fail_initial_download_ = should_fail; diff --git a/chrome/browser/sync/glue/sync_backend_host_mock.h b/chrome/browser/sync/glue/sync_backend_host_mock.h index b75a0879b69849..76e3ea7f7637ec 100644 --- a/chrome/browser/sync/glue/sync_backend_host_mock.h +++ b/chrome/browser/sync/glue/sync_backend_host_mock.h @@ -93,7 +93,8 @@ class SyncBackendHostMock : public SyncBackendHost { virtual SyncedDeviceTracker* GetSyncedDeviceTracker() const OVERRIDE; - virtual void SetForwardProtocolEvents(bool enabled) OVERRIDE; + virtual void RequestBufferedProtocolEventsAndEnableForwarding() OVERRIDE; + virtual void DisableProtocolEventForwarding() OVERRIDE; virtual base::MessageLoop* GetSyncLoopForTesting() OVERRIDE; diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc index 9166d4f395eff9..75ac00d7bc68b4 100644 --- a/chrome/browser/sync/profile_sync_service.cc +++ b/chrome/browser/sync/profile_sync_service.cc @@ -936,7 +936,7 @@ void ProfileSyncService::OnBackendInitialized( debug_info_listener_ = debug_info_listener; if (protocol_event_observers_.might_have_observers()) { - backend_->SetForwardProtocolEvents(true); + backend_->RequestBufferedProtocolEventsAndEnableForwarding(); } // If we have a cached passphrase use it to decrypt/encrypt data now that the @@ -2067,17 +2067,15 @@ void ProfileSyncService::AddProtocolEventObserver( browser_sync::ProtocolEventObserver* observer) { protocol_event_observers_.AddObserver(observer); if (backend_) { - backend_->SetForwardProtocolEvents( - protocol_event_observers_.might_have_observers()); + backend_->RequestBufferedProtocolEventsAndEnableForwarding(); } } void ProfileSyncService::RemoveProtocolEventObserver( browser_sync::ProtocolEventObserver* observer) { protocol_event_observers_.RemoveObserver(observer); - if (backend_) { - backend_->SetForwardProtocolEvents( - protocol_event_observers_.might_have_observers()); + if (backend_ && !protocol_event_observers_.might_have_observers()) { + backend_->DisableProtocolEventForwarding(); } } diff --git a/chrome/browser/ui/webui/sync_internals_browsertest.js b/chrome/browser/ui/webui/sync_internals_browsertest.js index 3e6aa24fbb4809..3a1160cbc04ed2 100644 --- a/chrome/browser/ui/webui/sync_internals_browsertest.js +++ b/chrome/browser/ui/webui/sync_internals_browsertest.js @@ -227,6 +227,20 @@ HARD_CODED_ABOUT_INFO = { "unrecoverable_error_detected": false }; +NETWORK_EVENT_DETAILS_1 = { + "details":"Notified types: Bookmarks, Autofill", + "proto":{}, + "time":1395874542192.407, + "type":"Normal GetUpdate request" +}; + +NETWORK_EVENT_DETAILS_2 = { + "details":"Received error: SYNC_AUTH_ERROR", + "proto":{}, + "time":1395874542192.837, + "type":"GetUpdates Response" +}; + TEST_F('SyncInternalsWebUITest', 'Uninitialized', function() { assertNotEquals(null, chrome.sync.aboutInfo); expectTrue(this.hasInDetails(true, 'Username', '')); @@ -246,6 +260,22 @@ TEST_F('SyncInternalsWebUITest', 'LoadPastedAboutInfo', function() { expectTrue(this.hasInDetails(true, 'Summary', 'Sync service initialized')); }); +TEST_F('SyncInternalsWebUITest', 'NetworkEventsTest', function() { + networkEvent1 = new Event('onProtocolEvent'); + networkEvent1.details = NETWORK_EVENT_DETAILS_1; + networkEvent2 = new Event('onProtocolEvent'); + networkEvent2.details = NETWORK_EVENT_DETAILS_2; + + chrome.sync.events.dispatchEvent(networkEvent1); + chrome.sync.events.dispatchEvent(networkEvent2); + + expectEquals(2, $('traffic-event-container').children.length); + + // Test that repeated events are not re-displayed. + chrome.sync.events.dispatchEvent(networkEvent1); + expectEquals(2, $('traffic-event-container').children.length); +}); + TEST_F('SyncInternalsWebUITest', 'SearchTabDoesntChangeOnItemSelect', function() { // Select the search tab. diff --git a/chrome/browser/ui/webui/sync_internals_message_handler.cc b/chrome/browser/ui/webui/sync_internals_message_handler.cc index e0b64f9bf081ee..3896a429ef7077 100644 --- a/chrome/browser/ui/webui/sync_internals_message_handler.cc +++ b/chrome/browser/ui/webui/sync_internals_message_handler.cc @@ -41,14 +41,10 @@ SyncInternalsMessageHandler::~SyncInternalsMessageHandler() { void SyncInternalsMessageHandler::RegisterMessages() { DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); - // Register for ProfileSyncService events. - ProfileSyncService* service = GetProfileSyncService(); - if (service) { - service->AddObserver(this); - service->AddProtocolEventObserver(this); - js_controller_ = service->GetJsController(); - js_controller_->AddJsEventHandler(this); - } + web_ui()->RegisterMessageCallback( + "registerForEvents", + base::Bind(&SyncInternalsMessageHandler::HandleRegisterForEvents, + base::Unretained(this))); web_ui()->RegisterMessageCallback( "requestUpdatedAboutInfo", @@ -64,6 +60,19 @@ void SyncInternalsMessageHandler::RegisterMessages() { RegisterJsControllerCallback("getClientServerTraffic"); } +void SyncInternalsMessageHandler::HandleRegisterForEvents( + const base::ListValue* args) { + DCHECK(args->empty()); + + ProfileSyncService* service = GetProfileSyncService(); + if (service) { + service->AddObserver(this); + service->AddProtocolEventObserver(this); + js_controller_ = service->GetJsController(); + js_controller_->AddJsEventHandler(this); + } +} + void SyncInternalsMessageHandler::HandleRequestUpdatedAboutInfo( const base::ListValue* args) { DCHECK(args->empty()); diff --git a/chrome/browser/ui/webui/sync_internals_message_handler.h b/chrome/browser/ui/webui/sync_internals_message_handler.h index 806e25d6455c40..80cc63ab04549f 100644 --- a/chrome/browser/ui/webui/sync_internals_message_handler.h +++ b/chrome/browser/ui/webui/sync_internals_message_handler.h @@ -37,6 +37,9 @@ class SyncInternalsMessageHandler // Forwards requests to the JsController. void ForwardToJsController(const std::string& name, const base::ListValue*); + // Sets up observers to receive events and forward them to the UI. + void HandleRegisterForEvents(const base::ListValue* args); + // Fires an event to send updated info back to the page. void HandleRequestUpdatedAboutInfo(const base::ListValue* args); diff --git a/sync/internal_api/protocol_event_buffer.cc b/sync/internal_api/protocol_event_buffer.cc new file mode 100644 index 00000000000000..2e9ba64407b44e --- /dev/null +++ b/sync/internal_api/protocol_event_buffer.cc @@ -0,0 +1,37 @@ +// Copyright 2014 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "sync/internal_api/protocol_event_buffer.h" + +#include "sync/internal_api/public/events/protocol_event.h" + +namespace syncer { + +const size_t ProtocolEventBuffer::kBufferSize = 6; + +ProtocolEventBuffer::ProtocolEventBuffer() + : buffer_deleter_(&buffer_) {} + +ProtocolEventBuffer::~ProtocolEventBuffer() {} + +void ProtocolEventBuffer::RecordProtocolEvent(const ProtocolEvent& event) { + buffer_.push_back(event.Clone().release()); + if (buffer_.size() > kBufferSize) { + ProtocolEvent* to_delete = buffer_.front(); + buffer_.pop_front(); + delete to_delete; + } +} + +ScopedVector +ProtocolEventBuffer::GetBufferedProtocolEvents() const { + ScopedVector ret; + for (std::deque::const_iterator it = buffer_.begin(); + it != buffer_.end(); ++it) { + ret.push_back((*it)->Clone().release()); + } + return ret.Pass(); +} + +} // namespace syncer diff --git a/sync/internal_api/protocol_event_buffer.h b/sync/internal_api/protocol_event_buffer.h new file mode 100644 index 00000000000000..8d82210f08eb12 --- /dev/null +++ b/sync/internal_api/protocol_event_buffer.h @@ -0,0 +1,42 @@ +// Copyright 2014 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef SYNC_INTERNAL_API_PROTOCOL_EVENT_BUFFER_H_ +#define SYNC_INTERNAL_API_PROTOCOL_EVENT_BUFFER_H_ + +#include + +#include "base/memory/scoped_vector.h" +#include "sync/base/sync_export.h" + +namespace syncer { + +class ProtocolEvent; + +// A container for ProtocolEvents. +// +// Stores at most kBufferSize events, then starts dropping the oldest events. +class SYNC_EXPORT_PRIVATE ProtocolEventBuffer { + public: + static const size_t kBufferSize; + + ProtocolEventBuffer(); + ~ProtocolEventBuffer(); + + // Records an event. May cause the oldest event to be dropped. + void RecordProtocolEvent(const ProtocolEvent& event); + + // Returns the buffered contents. Will not clear the buffer. + ScopedVector GetBufferedProtocolEvents() const; + + private: + std::deque buffer_; + STLElementDeleter > buffer_deleter_; + + DISALLOW_COPY_AND_ASSIGN(ProtocolEventBuffer); +}; + +} // namespace syncer + +#endif // SYNC_INTERNAL_API_PROTOCOL_EVENT_BUFFER_H_ diff --git a/sync/internal_api/protocol_event_buffer_unittest.cc b/sync/internal_api/protocol_event_buffer_unittest.cc new file mode 100644 index 00000000000000..d6d403401283ee --- /dev/null +++ b/sync/internal_api/protocol_event_buffer_unittest.cc @@ -0,0 +1,77 @@ +// Copyright 2014 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "base/memory/scoped_ptr.h" +#include "base/memory/scoped_vector.h" +#include "base/time/time.h" +#include "sync/internal_api/protocol_event_buffer.h" +#include "sync/internal_api/public/events/poll_get_updates_request_event.h" +#include "sync/internal_api/public/events/protocol_event.h" +#include "sync/protocol/sync.pb.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace syncer { + +class ProtocolEventBufferTest : public ::testing::Test { + public: + ProtocolEventBufferTest(); + virtual ~ProtocolEventBufferTest(); + + static scoped_ptr MakeTestEvent(int64 id); + static bool HasId(const ProtocolEvent& event, int64 id); + + protected: + ProtocolEventBuffer buffer_; +}; + +ProtocolEventBufferTest::ProtocolEventBufferTest() {} + +ProtocolEventBufferTest::~ProtocolEventBufferTest() {} + +scoped_ptr ProtocolEventBufferTest::MakeTestEvent(int64 id) { + sync_pb::ClientToServerMessage message; + return scoped_ptr( + new PollGetUpdatesRequestEvent( + base::Time::FromInternalValue(id), + message)); +} + +bool ProtocolEventBufferTest::HasId(const ProtocolEvent& event, int64 id) { + return event.GetTimestamp() == base::Time::FromInternalValue(id); +} + +TEST_F(ProtocolEventBufferTest, AddThenReturnEvents) { + scoped_ptr e1(MakeTestEvent(1)); + scoped_ptr e2(MakeTestEvent(2)); + + buffer_.RecordProtocolEvent(*e1); + buffer_.RecordProtocolEvent(*e2); + + ScopedVector buffered_events( + buffer_.GetBufferedProtocolEvents()); + + ASSERT_EQ(2U, buffered_events.size()); + EXPECT_TRUE(HasId(*(buffered_events[0]), 1)); + EXPECT_TRUE(HasId(*(buffered_events[1]), 2)); +} + +TEST_F(ProtocolEventBufferTest, AddThenOverflowThenReturnEvents) { + for (size_t i = 0; i < ProtocolEventBuffer::kBufferSize+1; ++i) { + scoped_ptr e(MakeTestEvent(static_cast(i))); + buffer_.RecordProtocolEvent(*e); + } + + ScopedVector buffered_events( + buffer_.GetBufferedProtocolEvents()); + ASSERT_EQ(ProtocolEventBuffer::kBufferSize, buffered_events.size()); + + for (size_t i = 1; i < ProtocolEventBuffer::kBufferSize+1; ++i) { + EXPECT_TRUE( + HasId(*(buffered_events[i-1]), static_cast(i))); + } + +} + + +} // namespace syncer diff --git a/sync/internal_api/public/events/commit_request_event.h b/sync/internal_api/public/events/commit_request_event.h index cf379a9835bd12..d79b69c50e3a6e 100644 --- a/sync/internal_api/public/events/commit_request_event.h +++ b/sync/internal_api/public/events/commit_request_event.h @@ -10,6 +10,7 @@ #include "base/memory/scoped_ptr.h" #include "base/time/time.h" #include "base/values.h" +#include "sync/base/sync_export.h" #include "sync/internal_api/public/base/model_type.h" #include "sync/internal_api/public/events/protocol_event.h" #include "sync/protocol/sync.pb.h" @@ -17,7 +18,7 @@ namespace syncer { // An event representing a commit request message sent to the server. -class CommitRequestEvent : public ProtocolEvent { +class SYNC_EXPORT_PRIVATE CommitRequestEvent : public ProtocolEvent { public: CommitRequestEvent( base::Time timestamp, diff --git a/sync/internal_api/public/events/commit_response_event.h b/sync/internal_api/public/events/commit_response_event.h index e1f1797f1252ae..b2330fa9b77e51 100644 --- a/sync/internal_api/public/events/commit_response_event.h +++ b/sync/internal_api/public/events/commit_response_event.h @@ -10,6 +10,7 @@ #include "base/memory/scoped_ptr.h" #include "base/time/time.h" #include "base/values.h" +#include "sync/base/sync_export.h" #include "sync/internal_api/public/events/protocol_event.h" #include "sync/internal_api/public/util/syncer_error.h" #include "sync/protocol/sync.pb.h" @@ -17,7 +18,7 @@ namespace syncer { // An event representing a commit response event from the server. -class CommitResponseEvent : public ProtocolEvent { +class SYNC_EXPORT_PRIVATE CommitResponseEvent : public ProtocolEvent { public: CommitResponseEvent( base::Time timestamp, diff --git a/sync/internal_api/public/events/configure_get_updates_request_event.h b/sync/internal_api/public/events/configure_get_updates_request_event.h index eb783ab98e3cce..dcec982d85fac1 100644 --- a/sync/internal_api/public/events/configure_get_updates_request_event.h +++ b/sync/internal_api/public/events/configure_get_updates_request_event.h @@ -8,13 +8,15 @@ #include "base/memory/scoped_ptr.h" #include "base/time/time.h" #include "base/values.h" +#include "sync/base/sync_export.h" #include "sync/internal_api/public/events/protocol_event.h" #include "sync/protocol/sync.pb.h" namespace syncer { // An event representing a configure GetUpdates request to the server. -class ConfigureGetUpdatesRequestEvent : public ProtocolEvent { +class SYNC_EXPORT_PRIVATE ConfigureGetUpdatesRequestEvent + : public ProtocolEvent { public: ConfigureGetUpdatesRequestEvent( base::Time timestamp, diff --git a/sync/internal_api/public/events/get_updates_response_event.h b/sync/internal_api/public/events/get_updates_response_event.h index 1d31cccde4ac57..c47fe9b51b1c1c 100644 --- a/sync/internal_api/public/events/get_updates_response_event.h +++ b/sync/internal_api/public/events/get_updates_response_event.h @@ -8,6 +8,7 @@ #include "base/memory/scoped_ptr.h" #include "base/time/time.h" #include "base/values.h" +#include "sync/base/sync_export.h" #include "sync/internal_api/public/events/protocol_event.h" #include "sync/internal_api/public/util/syncer_error.h" #include "sync/protocol/sync.pb.h" @@ -18,7 +19,7 @@ namespace syncer { // // Unlike the events for the request message, the response events are generic // and do not vary for each type of GetUpdate cycle. -class GetUpdatesResponseEvent : public ProtocolEvent { +class SYNC_EXPORT_PRIVATE GetUpdatesResponseEvent : public ProtocolEvent { public: GetUpdatesResponseEvent( base::Time timestamp, diff --git a/sync/internal_api/public/events/normal_get_updates_request_event.h b/sync/internal_api/public/events/normal_get_updates_request_event.h index 7d0b87401d42ea..ac3877e4bdf4b2 100644 --- a/sync/internal_api/public/events/normal_get_updates_request_event.h +++ b/sync/internal_api/public/events/normal_get_updates_request_event.h @@ -8,6 +8,7 @@ #include "base/memory/scoped_ptr.h" #include "base/time/time.h" #include "base/values.h" +#include "sync/base/sync_export.h" #include "sync/internal_api/public/base/model_type.h" #include "sync/internal_api/public/events/protocol_event.h" #include "sync/protocol/sync.pb.h" @@ -19,7 +20,7 @@ class NudgeTracker; } // namespace sessions // An event representing a 'normal mode' GetUpdate request to the server. -class NormalGetUpdatesRequestEvent : public ProtocolEvent { +class SYNC_EXPORT_PRIVATE NormalGetUpdatesRequestEvent : public ProtocolEvent { public: NormalGetUpdatesRequestEvent( base::Time timestamp, diff --git a/sync/internal_api/public/events/poll_get_updates_request_event.h b/sync/internal_api/public/events/poll_get_updates_request_event.h index d4a679ce5da3a8..f039c1de92d5e9 100644 --- a/sync/internal_api/public/events/poll_get_updates_request_event.h +++ b/sync/internal_api/public/events/poll_get_updates_request_event.h @@ -8,6 +8,7 @@ #include "base/memory/scoped_ptr.h" #include "base/time/time.h" #include "base/values.h" +#include "sync/base/sync_export.h" #include "sync/internal_api/public/base/model_type.h" #include "sync/internal_api/public/events/protocol_event.h" #include "sync/protocol/sync.pb.h" @@ -19,7 +20,7 @@ class NudgeTracker; } // namespace sessions // An event representing a poll request sent to the server. -class PollGetUpdatesRequestEvent : public ProtocolEvent { +class SYNC_EXPORT_PRIVATE PollGetUpdatesRequestEvent : public ProtocolEvent { public: PollGetUpdatesRequestEvent( base::Time timestamp, diff --git a/sync/internal_api/public/sync_manager.h b/sync/internal_api/public/sync_manager.h index 38bad0786d4983..db1e188119222f 100644 --- a/sync/internal_api/public/sync_manager.h +++ b/sync/internal_api/public/sync_manager.h @@ -12,6 +12,7 @@ #include "base/callback_forward.h" #include "base/files/file_path.h" #include "base/memory/ref_counted.h" +#include "base/memory/scoped_vector.h" #include "base/task_runner.h" #include "base/threading/thread_checker.h" #include "sync/base/sync_export.h" @@ -353,6 +354,9 @@ class SYNC_EXPORT SyncManager : public syncer::InvalidationHandler { // Ask the SyncManager to fetch updates for the given types. virtual void RefreshTypes(ModelTypeSet types) = 0; + + // Returns any buffered protocol events. Does not clear the buffer. + virtual ScopedVector GetBufferedProtocolEvents() = 0; }; } // namespace syncer diff --git a/sync/internal_api/public/test/fake_sync_manager.h b/sync/internal_api/public/test/fake_sync_manager.h index 81b71600cbd82a..655caefd59f5fc 100644 --- a/sync/internal_api/public/test/fake_sync_manager.h +++ b/sync/internal_api/public/test/fake_sync_manager.h @@ -124,6 +124,8 @@ class FakeSyncManager : public SyncManager { virtual bool ReceivedExperiment(Experiments* experiments) OVERRIDE; virtual bool HasUnsyncedItems() OVERRIDE; virtual SyncEncryptionHandler* GetEncryptionHandler() OVERRIDE; + virtual ScopedVector + GetBufferedProtocolEvents() OVERRIDE; virtual void RefreshTypes(ModelTypeSet types) OVERRIDE; private: diff --git a/sync/internal_api/sync_manager_impl.cc b/sync/internal_api/sync_manager_impl.cc index 224b38b8ae7523..7350066a586f54 100644 --- a/sync/internal_api/sync_manager_impl.cc +++ b/sync/internal_api/sync_manager_impl.cc @@ -946,6 +946,7 @@ void SyncManagerImpl::OnMigrationRequested(ModelTypeSet types) { } void SyncManagerImpl::OnProtocolEvent(const ProtocolEvent& event) { + protocol_event_buffer_.RecordProtocolEvent(event); FOR_EACH_OBSERVER(SyncManager::Observer, observers_, OnProtocolEvent(event)); } @@ -1161,6 +1162,11 @@ SyncEncryptionHandler* SyncManagerImpl::GetEncryptionHandler() { return sync_encryption_handler_.get(); } +ScopedVector + SyncManagerImpl::GetBufferedProtocolEvents() { + return protocol_event_buffer_.GetBufferedProtocolEvents(); +} + // static. int SyncManagerImpl::GetDefaultNudgeDelay() { return kDefaultNudgeDelayMilliseconds; diff --git a/sync/internal_api/sync_manager_impl.h b/sync/internal_api/sync_manager_impl.h index 5cb39088b73e84..8219605fea4d0c 100644 --- a/sync/internal_api/sync_manager_impl.h +++ b/sync/internal_api/sync_manager_impl.h @@ -19,6 +19,7 @@ #include "sync/internal_api/js_mutation_event_observer.h" #include "sync/internal_api/js_sync_encryption_handler_observer.h" #include "sync/internal_api/js_sync_manager_observer.h" +#include "sync/internal_api/protocol_event_buffer.h" #include "sync/internal_api/public/sync_manager.h" #include "sync/internal_api/public/user_share.h" #include "sync/internal_api/sync_encryption_handler_impl.h" @@ -115,6 +116,8 @@ class SYNC_EXPORT_PRIVATE SyncManagerImpl : virtual bool ReceivedExperiment(Experiments* experiments) OVERRIDE; virtual bool HasUnsyncedItems() OVERRIDE; virtual SyncEncryptionHandler* GetEncryptionHandler() OVERRIDE; + virtual ScopedVector + GetBufferedProtocolEvents() OVERRIDE; // SyncEncryptionHandler::Observer implementation. virtual void OnPassphraseRequired( @@ -358,6 +361,7 @@ class SYNC_EXPORT_PRIVATE SyncManagerImpl : // This is for keeping track of client events to send to the server. DebugInfoEventListener debug_info_event_listener_; + ProtocolEventBuffer protocol_event_buffer_; TrafficRecorder traffic_recorder_; Encryptor* encryptor_; diff --git a/sync/internal_api/test/fake_sync_manager.cc b/sync/internal_api/test/fake_sync_manager.cc index 883da74972a491..3fdbefad7f9bf5 100644 --- a/sync/internal_api/test/fake_sync_manager.cc +++ b/sync/internal_api/test/fake_sync_manager.cc @@ -239,6 +239,11 @@ SyncEncryptionHandler* FakeSyncManager::GetEncryptionHandler() { return fake_encryption_handler_.get(); } +ScopedVector +FakeSyncManager::GetBufferedProtocolEvents() { + return ScopedVector(); +} + void FakeSyncManager::RefreshTypes(ModelTypeSet types) { last_refresh_request_types_ = types; } diff --git a/sync/sync_internal_api.gypi b/sync/sync_internal_api.gypi index 20ac6a59e284c1..b0a476ba0b141a 100644 --- a/sync/sync_internal_api.gypi +++ b/sync/sync_internal_api.gypi @@ -41,6 +41,8 @@ 'internal_api/js_sync_manager_observer.cc', 'internal_api/js_sync_manager_observer.h', 'internal_api/non_blocking_type_processor.cc', + 'internal_api/protocol_event_buffer.cc', + 'internal_api/protocol_event_buffer.h', 'internal_api/public/base/ack_handle.cc', 'internal_api/public/base/ack_handle.h', 'internal_api/public/base/cancelation_observer.cc', diff --git a/sync/sync_tests.gypi b/sync/sync_tests.gypi index a8ac4a0a57a094..a2366a98c7dc1a 100644 --- a/sync/sync_tests.gypi +++ b/sync/sync_tests.gypi @@ -417,6 +417,7 @@ 'internal_api/js_mutation_event_observer_unittest.cc', 'internal_api/js_sync_encryption_handler_observer_unittest.cc', 'internal_api/js_sync_manager_observer_unittest.cc', + 'internal_api/protocol_event_buffer_unittest.cc', 'internal_api/public/change_record_unittest.cc', 'internal_api/public/sessions/sync_session_snapshot_unittest.cc', 'internal_api/sync_core_proxy_unittest.cc',