diff options
38 files changed, 1327 insertions, 1625 deletions
diff --git a/chrome/browser/resources/sync_internals/chrome_sync.js b/chrome/browser/resources/sync_internals/chrome_sync.js index 4fa04a5..016cb33 100644 --- a/chrome/browser/resources/sync_internals/chrome_sync.js +++ b/chrome/browser/resources/sync_internals/chrome_sync.js @@ -118,7 +118,7 @@ function makeSyncFunction(name) { }; // Handle a reply, assuming that messages are processed in FIFO order. - // Called by SyncInternalsUI::HandleJsMessageReply(). + // Called by SyncInternalsUI::HandleJsReply(). fn.handleReply = function() { var args = Array.prototype.slice.call(arguments); // Remove the callback before we call it since the callback may diff --git a/chrome/browser/sync/README.js b/chrome/browser/sync/README.js index d8c86aa..492c5c3 100644 --- a/chrome/browser/sync/README.js +++ b/chrome/browser/sync/README.js @@ -4,13 +4,17 @@ Overview of chrome://sync-internals This note explains how chrome://sync-internals (also known as about:sync) interacts with the sync service/backend. -Basically, chrome://sync-internals sends asynchronous messages to the -sync backend and the sync backend asynchronously raises events and -message replies to chrome://sync-internals. +Basically, chrome://sync-internals sends messages to the sync backend +and the sync backend sends the reply asynchronously. The sync backend +also asynchronously raises events which chrome://sync-internals listen +to. -Both messages and events have a name and a list of arguments, the -latter of which is represented by a JsArgList (js_arg_list.h) object, -which is basically a wrapper around an immutable ListValue. +A message and its reply has a name and a list of arguments, which is +basically a wrapper around an immutable ListValue. + +An event has a name and a details object, which is represented by a +JsEventDetails (js_event_details.h) object, which is basically a +wrapper around an immutable DictionaryValue. TODO(akalin): Move all the js_* files into a js/ subdirectory. @@ -19,37 +23,28 @@ Message/event flow chrome://sync-internals is represented by SyncInternalsUI (chrome/browser/ui/webui/sync_internals_ui.h). SyncInternalsUI -interacts with the sync service via a JsFrontend (js_frontend.h) -object, which has a ProcessMessage() method. The JsFrontend can -handle some messages itself, but it can also delegate the rest to a -JsBackend instance (js_backend.h), which also has a ProcessMessage() -method. A JsBackend can in turn handle some messages itself and -delegate to other JsBackend instances. - -Essentially, there is a tree with a JsFrontend as the root and -JsBackend as non-root internal nodes and leaf nodes (although -currently, the tree is more like a simple list). The sets of messages -handled by the JsBackends and the JsFrontend are disjoint, which means -that at most one node handles a given message type. Also, the -JsBackends may live on different threads, but JsArgList is thread-safe -so that's okay. - -SyncInternalsUI is a JsEventHandler (js_event_handler.h), which means -that it has a HandleJsEvent() method and a HandleJsMessageReply() -method, but JsBackends cannot easily access those objects. Instead, -each JsBackend keeps track of its parent router, which is a -JsEventRouter object (js_event_router.h). Basically, a JsEventRouter -is another JsBackend object or a JsFrontend object. So an event or -message reply travels up through the JsEventRouter until it reaches -the JsFrontend, which knows about the existing JsEventHandlers (via -AddHandler()/RemoveHandler()) and so can delegate to the right one. - -A diagram of the flow of a message and its reply: - -msg(args) -> F -> B -> B -> B - | | | - H <- R <- R <- R <- reply-event(args) - -F = JsFrontend, B = JsBackend, R = JsEventRouter, H = JsEventHandler - -Non-reply events are percolated up similarly. +interacts with the sync service via a JsController (js_controller.h) +object, which has a ProcessJsMessage() method that just delegates to +an underlying JsBackend instance (js_backend.h). The SyncInternalsUI +object also registers itself (as a JsEventHandler +[js_event_handler.h]) to the JsController object, and any events +raised by the JsBackend are propagated to the JsController and then to +the registered JsEventHandlers. + +The ProcessJsMessage() takes a WeakHandle (weak_handle.h) to a +JsReplyHandler (js_reply_handler.h), which the backend uses to send +replies safely across threads. SyncInternalsUI implements +JsReplyHandler, so it simply passes itself as the reply handler when +it calls ProcessJsMessage() on the JsController. + +The following objects live on the UI thread: + +- SyncInternalsUI (implements JsEventHandler, JsReplyHandler) +- SyncJsController (implements JsController, JsEventHandler) + +The following objects live on the sync thread: + +- SyncManager::SyncInternal (implements JsBackend) + +Of course, none of these objects need to know where the other objects +live, since they interact via WeakHandles. diff --git a/chrome/browser/sync/engine/syncapi.cc b/chrome/browser/sync/engine/syncapi.cc index df7e7f5..229d342 100644 --- a/chrome/browser/sync/engine/syncapi.cc +++ b/chrome/browser/sync/engine/syncapi.cc @@ -43,9 +43,11 @@ #include "chrome/browser/sync/engine/http_post_provider_factory.h" #include "chrome/browser/sync/js_arg_list.h" #include "chrome/browser/sync/js_backend.h" -#include "chrome/browser/sync/js_transaction_observer.h" #include "chrome/browser/sync/js_event_details.h" -#include "chrome/browser/sync/js_event_router.h" +#include "chrome/browser/sync/js_event_handler.h" +#include "chrome/browser/sync/js_reply_handler.h" +#include "chrome/browser/sync/js_sync_manager_observer.h" +#include "chrome/browser/sync/js_transaction_observer.h" #include "chrome/browser/sync/notifier/sync_notifier.h" #include "chrome/browser/sync/notifier/sync_notifier_observer.h" #include "chrome/browser/sync/protocol/app_specifics.pb.h" @@ -77,6 +79,15 @@ using base::TimeDelta; using browser_sync::AllStatus; using browser_sync::Cryptographer; +using browser_sync::JsArgList; +using browser_sync::JsBackend; +using browser_sync::JsEventDetails; +using browser_sync::JsEventHandler; +using browser_sync::JsReplyHandler; +using browser_sync::JsSyncManagerObserver; +using browser_sync::JsTransactionObserver; +using browser_sync::MakeWeakHandle; +using browser_sync::WeakHandle; using browser_sync::KeyParams; using browser_sync::ModelSafeRoutingInfo; using browser_sync::ModelSafeWorker; @@ -1206,8 +1217,7 @@ syncable::ModelTypeSet GetEncryptedTypes( class SyncManager::SyncInternal : public net::NetworkChangeNotifier::IPAddressObserver, public sync_notifier::SyncNotifierObserver, - public browser_sync::JsBackend, - public browser_sync::JsEventRouter, + public JsBackend, public SyncEngineEventListener, public ServerConnectionEventListener, public syncable::DirectoryChangeDelegate { @@ -1216,12 +1226,10 @@ class SyncManager::SyncInternal public: explicit SyncInternal(const std::string& name) : weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)), - parent_router_(NULL), registrar_(NULL), initialized_(false), setup_for_test_mode_(false), - observing_ip_address_changes_(false), - js_transaction_observer_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) { + observing_ip_address_changes_(false) { // Pre-fill |notification_info_map_|. for (int i = syncable::FIRST_REAL_MODEL_TYPE; i < syncable::MODEL_TYPE_COUNT; ++i) { @@ -1258,6 +1266,7 @@ class SyncManager::SyncInternal } bool Init(const FilePath& database_location, + const WeakHandle<JsEventHandler>& event_handler, const std::string& sync_server_and_path, int port, bool use_ssl, @@ -1405,25 +1414,12 @@ class SyncManager::SyncInternal // ServerConnectionEventListener implementation. virtual void OnServerConnectionEvent(const ServerConnectionEvent& event); - // browser_sync::JsBackend implementation. - virtual void SetParentJsEventRouter( - browser_sync::JsEventRouter* router) OVERRIDE; - virtual void RemoveParentJsEventRouter() OVERRIDE; - virtual const browser_sync::JsEventRouter* - GetParentJsEventRouter() const OVERRIDE; - virtual void ProcessMessage( - const std::string& name, - const browser_sync::JsArgList& args, - const browser_sync::JsEventHandler* sender) OVERRIDE; - - // JsEventRouter implementation. - virtual void RouteJsEvent( - const std::string& name, - const browser_sync::JsEventDetails& details) OVERRIDE; - virtual void RouteJsMessageReply( - const std::string& name, - const browser_sync::JsArgList& args, - const browser_sync::JsEventHandler* target) OVERRIDE; + // JsBackend implementation. + virtual void SetJsEventHandler( + const WeakHandle<JsEventHandler>& event_handler) OVERRIDE; + virtual void ProcessJsMessage( + const std::string& name, const JsArgList& args, + const WeakHandle<JsReplyHandler>& reply_handler) OVERRIDE; private: struct NotificationInfo { @@ -1444,11 +1440,9 @@ class SyncManager::SyncInternal }; typedef std::map<syncable::ModelType, NotificationInfo> NotificationInfoMap; - typedef browser_sync::JsArgList - (SyncManager::SyncInternal::*UnboundJsMessageHandler)( - const browser_sync::JsArgList&); - typedef base::Callback<browser_sync::JsArgList(browser_sync::JsArgList)> - JsMessageHandler; + typedef JsArgList + (SyncManager::SyncInternal::*UnboundJsMessageHandler)(const JsArgList&); + typedef base::Callback<JsArgList(JsArgList)> JsMessageHandler; typedef std::map<std::string, JsMessageHandler> JsMessageHandlerMap; // Helper to call OnAuthError when no authentication credentials are @@ -1550,34 +1544,20 @@ class SyncManager::SyncInternal // Helper function used only by the constructor. void BindJsMessageHandler( - const std::string& name, - UnboundJsMessageHandler unbound_message_handler); + const std::string& name, UnboundJsMessageHandler unbound_message_handler); // Returned pointer is owned by the caller. static DictionaryValue* NotificationInfoToValue( const NotificationInfoMap& notification_info); // JS message handlers. - browser_sync::JsArgList GetNotificationState( - const browser_sync::JsArgList& args); - - browser_sync::JsArgList GetNotificationInfo( - const browser_sync::JsArgList& args); - - browser_sync::JsArgList GetRootNodeDetails( - const browser_sync::JsArgList& args); - - browser_sync::JsArgList GetNodeSummariesById( - const browser_sync::JsArgList& args); - - browser_sync::JsArgList GetNodeDetailsById( - const browser_sync::JsArgList& args); - - browser_sync::JsArgList GetChildNodeIds( - const browser_sync::JsArgList& args); - - browser_sync::JsArgList FindNodesContainingString( - const browser_sync::JsArgList& args); + JsArgList GetNotificationState(const JsArgList& args); + JsArgList GetNotificationInfo(const JsArgList& args); + JsArgList GetRootNodeDetails(const JsArgList& args); + JsArgList GetNodeSummariesById(const JsArgList& args); + JsArgList GetNodeDetailsById(const JsArgList& args); + JsArgList GetChildNodeIds(const JsArgList& args); + JsArgList FindNodesContainingString(const JsArgList& args); const std::string name_; @@ -1594,7 +1574,7 @@ class SyncManager::SyncInternal // we'd have another worker class which implements // HandleCalculateChangesChangeEventFromSyncApi() and we'd pass it a // WeakHandle when we construct it. - browser_sync::WeakHandle<SyncInternal> weak_handle_this_; + WeakHandle<SyncInternal> weak_handle_this_; // We couple the DirectoryManager and username together in a UserShare member // so we can return a handle to share_ to clients of the API for use when @@ -1606,8 +1586,6 @@ class SyncManager::SyncInternal mutable base::Lock observers_lock_; ObserverList<SyncManager::Observer> observers_; - browser_sync::JsEventRouter* parent_router_; - // The ServerConnectionManager used to abstract communication between the // client (the Syncer) and the sync server. scoped_ptr<SyncAPIServerConnectionManager> connection_manager_; @@ -1649,9 +1627,11 @@ class SyncManager::SyncInternal // about:sync page. NotificationInfoMap notification_info_map_; - browser_sync::JsTransactionObserver js_transaction_observer_; - + // These are for interacting with chrome://sync-internals. JsMessageHandlerMap js_message_handlers_; + WeakHandle<JsEventHandler> js_event_handler_; + JsSyncManagerObserver js_sync_manager_observer_; + JsTransactionObserver js_transaction_observer_; }; const int SyncManager::SyncInternal::kDefaultNudgeDelayMilliseconds = 200; const int SyncManager::SyncInternal::kPreferencesNudgeDelayMilliseconds = 2000; @@ -1661,21 +1641,24 @@ SyncManager::Observer::~Observer() {} SyncManager::SyncManager(const std::string& name) : data_(new SyncInternal(name)) {} -bool SyncManager::Init(const FilePath& database_location, - const std::string& sync_server_and_path, - int sync_server_port, - bool use_ssl, - HttpPostProviderFactory* post_factory, - ModelSafeWorkerRegistrar* registrar, - const std::string& user_agent, - const SyncCredentials& credentials, - sync_notifier::SyncNotifier* sync_notifier, - const std::string& restored_key_for_bootstrapping, - bool setup_for_test_mode) { +bool SyncManager::Init( + const FilePath& database_location, + const WeakHandle<JsEventHandler>& event_handler, + const std::string& sync_server_and_path, + int sync_server_port, + bool use_ssl, + HttpPostProviderFactory* post_factory, + ModelSafeWorkerRegistrar* registrar, + const std::string& user_agent, + const SyncCredentials& credentials, + sync_notifier::SyncNotifier* sync_notifier, + const std::string& restored_key_for_bootstrapping, + bool setup_for_test_mode) { DCHECK(post_factory); VLOG(1) << "SyncManager starting Init..."; string server_string(sync_server_and_path); return data_->Init(database_location, + event_handler, server_string, sync_server_port, use_ssl, @@ -1759,6 +1742,7 @@ const std::string& SyncManager::GetAuthenticatedUsername() { bool SyncManager::SyncInternal::Init( const FilePath& database_location, + const WeakHandle<JsEventHandler>& event_handler, const std::string& sync_server_and_path, int port, bool use_ssl, @@ -1775,14 +1759,16 @@ bool SyncManager::SyncInternal::Init( VLOG(1) << "Starting SyncInternal initialization."; - weak_handle_this_ = - browser_sync::MakeWeakHandle(weak_ptr_factory_.GetWeakPtr()); + weak_handle_this_ = MakeWeakHandle(weak_ptr_factory_.GetWeakPtr()); registrar_ = model_safe_worker_registrar; setup_for_test_mode_ = setup_for_test_mode; sync_notifier_.reset(sync_notifier); + AddObserver(&js_sync_manager_observer_); + SetJsEventHandler(event_handler); + share_.dir_manager.reset(new DirectoryManager(database_location)); connection_manager_.reset(new SyncAPIServerConnectionManager( @@ -1829,7 +1815,8 @@ bool SyncManager::SyncInternal::Init( ObserverList<SyncManager::Observer> temp_obs_list; CopyObservers(&temp_obs_list); FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list, - OnInitializationComplete()); + OnInitializationComplete( + WeakHandle<JsBackend>(weak_ptr_factory_.GetWeakPtr()))); // The following calls check that initialized_ is true. @@ -1912,12 +1899,7 @@ bool SyncManager::SyncInternal::OpenDirectory() { } connection_manager()->set_client_id(lookup->cache_guid()); - - if (parent_router_) { - // Make sure we add the observer at most once. - lookup->RemoveTransactionObserver(&js_transaction_observer_); - lookup->AddTransactionObserver(&js_transaction_observer_); - } + lookup->AddTransactionObserver(&js_transaction_observer_); return true; } @@ -2220,10 +2202,6 @@ void SyncManager::RemoveObserver(Observer* observer) { data_->RemoveObserver(observer); } -browser_sync::JsBackend* SyncManager::GetJsBackend() { - return data_; -} - void SyncManager::RequestEarlyExit() { data_->RequestEarlyExit(); } @@ -2248,6 +2226,9 @@ void SyncManager::SyncInternal::Shutdown() { // Automatically stops the scheduler. scheduler_.reset(); + SetJsEventHandler(WeakHandle<JsEventHandler>()); + RemoveObserver(&js_sync_manager_observer_); + if (sync_notifier_.get()) { sync_notifier_->RemoveObserver(this); } @@ -2262,6 +2243,12 @@ void SyncManager::SyncInternal::Shutdown() { observing_ip_address_changes_ = false; if (dir_manager()) { + syncable::ScopedDirLookup lookup(dir_manager(), username_for_share()); + if (lookup.good()) { + lookup->RemoveTransactionObserver(&js_transaction_observer_); + } else { + NOTREACHED(); + } dir_manager()->FinalSaveChangesForAll(); dir_manager()->Close(username_for_share()); } @@ -2271,7 +2258,6 @@ void SyncManager::SyncInternal::Shutdown() { share_.dir_manager.reset(); setup_for_test_mode_ = false; - parent_router_ = NULL; registrar_ = NULL; initialized_ = false; @@ -2643,52 +2629,24 @@ void SyncManager::SyncInternal::OnSyncEngineEvent( } } -void SyncManager::SyncInternal::SetParentJsEventRouter( - browser_sync::JsEventRouter* router) { - DCHECK(router); - parent_router_ = router; - - // We might be called before OpenDirectory() or after shutdown. - if (!dir_manager()) { - return; - } - syncable::ScopedDirLookup lookup(dir_manager(), username_for_share()); - if (!lookup.good()) { - return; - } - - // Make sure we add the observer at most once. - lookup->RemoveTransactionObserver(&js_transaction_observer_); - lookup->AddTransactionObserver(&js_transaction_observer_); +void SyncManager::SyncInternal::SetJsEventHandler( + const WeakHandle<JsEventHandler>& event_handler) { + js_event_handler_ = event_handler; + js_sync_manager_observer_.SetJsEventHandler(js_event_handler_); + js_transaction_observer_.SetJsEventHandler(js_event_handler_); } -void SyncManager::SyncInternal::RemoveParentJsEventRouter() { - parent_router_ = NULL; - - // We might be called before OpenDirectory() or after shutdown. - if (!dir_manager()) { - return; - } - syncable::ScopedDirLookup lookup(dir_manager(), username_for_share()); - if (!lookup.good()) { +void SyncManager::SyncInternal::ProcessJsMessage( + const std::string& name, const JsArgList& args, + const WeakHandle<JsReplyHandler>& reply_handler) { + if (!initialized_) { + NOTREACHED(); return; } - lookup->RemoveTransactionObserver(&js_transaction_observer_); -} - -const browser_sync::JsEventRouter* - SyncManager::SyncInternal::GetParentJsEventRouter() const { - return parent_router_; -} - -void SyncManager::SyncInternal::ProcessMessage( - const std::string& name, const browser_sync::JsArgList& args, - const browser_sync::JsEventHandler* sender) { - DCHECK(initialized_); - if (!parent_router_) { - VLOG(1) << "No parent router; not replying to message " << name - << " with args " << args.ToString(); + if (!reply_handler.IsInitialized()) { + VLOG(1) << "Uninitialized reply handler; dropping unknown message " + << name << " with args " << args.ToString(); return; } @@ -2699,27 +2657,9 @@ void SyncManager::SyncInternal::ProcessMessage( return; } - parent_router_->RouteJsMessageReply( - name, js_message_handler.Run(args), sender); -} - -void SyncManager::SyncInternal::RouteJsEvent( - const std::string& name, - const browser_sync::JsEventDetails& details) { - if (!parent_router_) { - return; - } - parent_router_->RouteJsEvent(name, details); -} - -void SyncManager::SyncInternal::RouteJsMessageReply( - const std::string& name, - const browser_sync::JsArgList& args, - const browser_sync::JsEventHandler* target) { - if (!parent_router_) { - return; - } - parent_router_->RouteJsMessageReply(name, args, target); + reply_handler.Call(FROM_HERE, + &JsReplyHandler::HandleJsReply, + name, js_message_handler.Run(args)); } void SyncManager::SyncInternal::BindJsMessageHandler( @@ -2743,32 +2683,29 @@ DictionaryValue* SyncManager::SyncInternal::NotificationInfoToValue( return value; } -browser_sync::JsArgList - SyncManager::SyncInternal::GetNotificationState( - const browser_sync::JsArgList& args) { +JsArgList SyncManager::SyncInternal::GetNotificationState( + const JsArgList& args) { bool notifications_enabled = allstatus_.status().notifications_enabled; ListValue return_args; return_args.Append(Value::CreateBooleanValue(notifications_enabled)); - return browser_sync::JsArgList(&return_args); + return JsArgList(&return_args); } -browser_sync::JsArgList - SyncManager::SyncInternal::GetNotificationInfo( - const browser_sync::JsArgList& args) { +JsArgList SyncManager::SyncInternal::GetNotificationInfo( + const JsArgList& args) { ListValue return_args; return_args.Append(NotificationInfoToValue(notification_info_map_)); - return browser_sync::JsArgList(&return_args); + return JsArgList(&return_args); } -browser_sync::JsArgList - SyncManager::SyncInternal::GetRootNodeDetails( - const browser_sync::JsArgList& args) { +JsArgList SyncManager::SyncInternal::GetRootNodeDetails( + const JsArgList& args) { ReadTransaction trans(FROM_HERE, GetUserShare()); ReadNode root(&trans); root.InitByRootLookup(); ListValue return_args; return_args.Append(root.GetDetailsAsValue()); - return browser_sync::JsArgList(&return_args); + return JsArgList(&return_args); } namespace { @@ -2785,10 +2722,9 @@ int64 GetId(const ListValue& ids, int i) { return id; } -browser_sync::JsArgList GetNodeInfoById( - const browser_sync::JsArgList& args, - UserShare* user_share, - DictionaryValue* (BaseNode::*info_getter)() const) { +JsArgList GetNodeInfoById(const JsArgList& args, + UserShare* user_share, + DictionaryValue* (BaseNode::*info_getter)() const) { CHECK(info_getter); ListValue return_args; ListValue* node_summaries = new ListValue(); @@ -2809,26 +2745,23 @@ browser_sync::JsArgList GetNodeInfoById( node_summaries->Append((node.*info_getter)()); } } - return browser_sync::JsArgList(&return_args); + return JsArgList(&return_args); } } // namespace -browser_sync::JsArgList - SyncManager::SyncInternal::GetNodeSummariesById( - const browser_sync::JsArgList& args) { +JsArgList SyncManager::SyncInternal::GetNodeSummariesById( + const JsArgList& args) { return GetNodeInfoById(args, GetUserShare(), &BaseNode::GetSummaryAsValue); } -browser_sync::JsArgList - SyncManager::SyncInternal::GetNodeDetailsById( - const browser_sync::JsArgList& args) { +JsArgList SyncManager::SyncInternal::GetNodeDetailsById( + const JsArgList& args) { return GetNodeInfoById(args, GetUserShare(), &BaseNode::GetDetailsAsValue); } -browser_sync::JsArgList SyncManager::SyncInternal:: - GetChildNodeIds( - const browser_sync::JsArgList& args) { +JsArgList SyncManager::SyncInternal::GetChildNodeIds( + const JsArgList& args) { ListValue return_args; ListValue* child_ids = new ListValue(); return_args.Append(child_ids); @@ -2844,17 +2777,16 @@ browser_sync::JsArgList SyncManager::SyncInternal:: base::Int64ToString(*it))); } } - return browser_sync::JsArgList(&return_args); + return JsArgList(&return_args); } -browser_sync::JsArgList SyncManager::SyncInternal:: - FindNodesContainingString( - const browser_sync::JsArgList& args) { +JsArgList SyncManager::SyncInternal::FindNodesContainingString( + const JsArgList& args) { std::string query; ListValue return_args; if (!args.Get().GetString(0, &query)) { return_args.Append(new ListValue()); - return browser_sync::JsArgList(&return_args); + return JsArgList(&return_args); } // Convert the query string to lower case to perform case insensitive @@ -2878,7 +2810,7 @@ browser_sync::JsArgList SyncManager::SyncInternal:: } } - return browser_sync::JsArgList(&return_args); + return JsArgList(&return_args); } void SyncManager::SyncInternal::OnNotificationStateChange( @@ -2889,11 +2821,13 @@ void SyncManager::SyncInternal::OnNotificationStateChange( if (scheduler()) { scheduler()->set_notifications_enabled(notifications_enabled); } - if (parent_router_) { + if (js_event_handler_.IsInitialized()) { DictionaryValue details; details.Set("enabled", Value::CreateBooleanValue(notifications_enabled)); - parent_router_->RouteJsEvent("onNotificationStateChange", - browser_sync::JsEventDetails(&details)); + js_event_handler_.Call(FROM_HERE, + &JsEventHandler::HandleJsEvent, + "onNotificationStateChange", + JsEventDetails(&details)); } } @@ -2922,7 +2856,7 @@ void SyncManager::SyncInternal::OnIncomingNotification( LOG(WARNING) << "Sync received notification without any type information."; } - if (parent_router_) { + if (js_event_handler_.IsInitialized()) { DictionaryValue details; ListValue* changed_types = new ListValue(); details.Set("changedTypes", changed_types); @@ -2933,8 +2867,10 @@ void SyncManager::SyncInternal::OnIncomingNotification( syncable::ModelTypeToString(it->first); changed_types->Append(Value::CreateStringValue(model_type_str)); } - parent_router_->RouteJsEvent("onIncomingNotification", - browser_sync::JsEventDetails(&details)); + js_event_handler_.Call(FROM_HERE, + &JsEventHandler::HandleJsEvent, + "onIncomingNotification", + JsEventDetails(&details)); } } diff --git a/chrome/browser/sync/engine/syncapi.h b/chrome/browser/sync/engine/syncapi.h index 98129fb..a132fdf 100644 --- a/chrome/browser/sync/engine/syncapi.h +++ b/chrome/browser/sync/engine/syncapi.h @@ -52,6 +52,7 @@ #include "chrome/browser/sync/protocol/password_specifics.pb.h" #include "chrome/browser/sync/syncable/model_type.h" #include "chrome/browser/sync/util/cryptographer.h" +#include "chrome/browser/sync/weak_handle.h" #include "chrome/common/net/gaia/google_service_auth_error.h" #include "googleurl/src/gurl.h" @@ -63,6 +64,7 @@ class DictionaryValue; namespace browser_sync { class JsBackend; +class JsEventHandler; class ModelSafeWorkerRegistrar; namespace sessions { @@ -834,7 +836,78 @@ class SyncManager { // notification is illegal! // WARNING: Calling methods on the SyncManager before receiving this // message, unless otherwise specified, produces undefined behavior. - virtual void OnInitializationComplete() = 0; + // + // The given backend can emit the following events: + + /** + * @param {{ enabled: boolean }} details A dictionary containing: + * - enabled: whether or not notifications are enabled. + */ + // function onNotificationStateChange(details); + + /** + * @param {{ changedTypes: Array.<string> }} details A dictionary + * containing: + * - changedTypes: a list of types (as strings) for which there + are new updates. + */ + // function onIncomingNotification(details); + + // The given backend responds to the following messages (all other + // messages are ignored): + + /** + * Gets the current notification state. + * + * @param {function(boolean)} callback Called with whether or not + * notifications are enabled. + */ + // function getNotificationState(callback); + + /** + * Gets details about the root node. + * + * @param {function(!Object)} callback Called with details about the + * root node. + */ + // TODO(akalin): Change this to getRootNodeId or eliminate it + // entirely. + // function getRootNodeDetails(callback); + + /** + * Gets summary information for a list of ids. + * + * @param {Array.<string>} idList List of 64-bit ids in decimal + * string form. + * @param {Array.<{id: string, title: string, isFolder: boolean}>} + * callback Called with summaries for the nodes in idList that + * exist. + */ + // function getNodeSummariesById(idList, callback); + + /** + * Gets detailed information for a list of ids. + * + * @param {Array.<string>} idList List of 64-bit ids in decimal + * string form. + * @param {Array.<!Object>} callback Called with detailed + * information for the nodes in idList that exist. + */ + // function getNodeDetailsById(idList, callback); + + /** + * Gets child ids for a given id. + * + * @param {string} id 64-bit id in decimal string form of the parent + * node. + * @param {Array.<string>} callback Called with the (possibly empty) + * list of child ids. + */ + // function getChildNodeIds(id); + + virtual void OnInitializationComplete( + const browser_sync::WeakHandle<browser_sync::JsBackend>& + js_backend) = 0; // We are no longer permitted to communicate with the server. Sync should // be disabled and state cleaned up at once. This can happen for a number @@ -865,6 +938,8 @@ class SyncManager { // the directory in which to locate a sqlite repository storing the syncer // backend state. Initialization will open the database, or create it if it // does not already exist. Returns false on failure. + // |event_handler| is the JsEventHandler used to propagate events to + // chrome://sync-internals. |event_handler| may be uninitialized. // |sync_server_and_path| and |sync_server_port| represent the Chrome sync // server to use, and |use_ssl| specifies whether to communicate securely; // the default is false. @@ -875,6 +950,8 @@ class SyncManager { // HTTP header. Used internally when collecting stats to classify clients. // |sync_notifier| is owned and used to listen for notifications. bool Init(const FilePath& database_location, + const browser_sync::WeakHandle<browser_sync::JsEventHandler>& + event_handler, const std::string& sync_server_and_path, int sync_server_port, bool use_ssl, @@ -949,76 +1026,6 @@ class SyncManager { // potentially dereference garbage. void RemoveObserver(Observer* observer); - // Returns a pointer to the JsBackend (which is owned by the sync - // manager). Never returns NULL. jsDocs for raised events are below: - - /** - * @param {{ enabled: boolean }} details enabled is set to whether - * or not notifications are enabled. - */ - // function onNotificationStateChange(details); - - /** - * @param {{ changedTypes: Array.<string> }} details changedTypes is - * a list of types (as strings) for which there are new updates. - */ - // function onIncomingNotification(details); - - // jsDocs for handled messages are below (all other messages are - // ignored). - - /** - * Gets the current notification state. - * - * @param {function(boolean)} callback Called with whether or not - * notifications are enabled. - */ - // function getNotificationState(callback); - - /** - * Gets details about the root node. - * - * @param {function(!Object)} callback Called with details about the - * root node. - */ - // TODO(akalin): Change this to getRootNodeId or eliminate it - // entirely. - // - // function getRootNodeDetails(callback); - - /** - * Gets summary information for a list of ids. - * - * @param {Array.<string>} idList List of 64-bit ids in decimal - * string form. - * @param {Array.<{id: string, title: string, isFolder: boolean}>} - * callback Called with summaries for the nodes in idList that - * exist. - */ - // function getNodeSummariesById(idList, callback); - - /** - * Gets detailed information for a list of ids. - * - * @param {Array.<string>} idList List of 64-bit ids in decimal - * string form. - * @param {Array.<!Object>} callback Called with detailed - * information for the nodes in idList that exist. - */ - // function getNodeDetailsById(idList, callback); - - /** - * Gets child ids for a given id. - * - * @param {string} id 64-bit id in decimal string form of the parent - * node. - * @param {Array.<string>} callback Called with the (possibly empty) - * list of child ids. - */ - // function getChildNodeIds(id); - - browser_sync::JsBackend* GetJsBackend(); - // Status-related getters. Typically GetStatusSummary will suffice, but // GetDetailedSyncStatus can be useful for gathering debug-level details of // the internals of the sync engine. diff --git a/chrome/browser/sync/engine/syncapi_unittest.cc b/chrome/browser/sync/engine/syncapi_unittest.cc index 4af502e..7be2af3 100644 --- a/chrome/browser/sync/engine/syncapi_unittest.cc +++ b/chrome/browser/sync/engine/syncapi_unittest.cc @@ -10,6 +10,7 @@ #include <map> #include "base/basictypes.h" +#include "base/compiler_specific.h" #include "base/format_macros.h" #include "base/memory/scoped_ptr.h" #include "base/message_loop.h" @@ -27,7 +28,7 @@ #include "chrome/browser/sync/js_arg_list.h" #include "chrome/browser/sync/js_backend.h" #include "chrome/browser/sync/js_event_handler.h" -#include "chrome/browser/sync/js_event_router.h" +#include "chrome/browser/sync/js_reply_handler.h" #include "chrome/browser/sync/js_test_util.h" #include "chrome/browser/sync/notifier/sync_notifier.h" #include "chrome/browser/sync/notifier/sync_notifier_observer.h" @@ -52,17 +53,22 @@ using browser_sync::HasArgsAsList; using browser_sync::HasDetailsAsDictionary; using browser_sync::KeyParams; using browser_sync::JsArgList; +using browser_sync::JsBackend; +using browser_sync::JsEventHandler; +using browser_sync::JsReplyHandler; using browser_sync::MockJsEventHandler; -using browser_sync::MockJsEventRouter; +using browser_sync::MockJsReplyHandler; using browser_sync::ModelSafeRoutingInfo; using browser_sync::ModelSafeWorker; using browser_sync::ModelSafeWorkerRegistrar; using browser_sync::sessions::SyncSessionSnapshot; +using browser_sync::WeakHandle; using syncable::ModelType; using syncable::ModelTypeSet; using test::ExpectDictDictionaryValue; using test::ExpectDictStringValue; using testing::_; +using testing::AnyNumber; using testing::AtLeast; using testing::InSequence; using testing::Invoke; @@ -638,15 +644,41 @@ TEST_F(SyncApiTest, ChangeRecordToValue) { namespace { +class TestHttpPostProviderInterface : public HttpPostProviderInterface { + public: + virtual ~TestHttpPostProviderInterface() {} + + virtual void SetUserAgent(const char* user_agent) OVERRIDE {} + virtual void SetExtraRequestHeaders(const char* headers) OVERRIDE {} + virtual void SetURL(const char* url, int port) OVERRIDE {} + virtual void SetPostPayload(const char* content_type, + int content_length, + const char* content) OVERRIDE {} + virtual bool MakeSynchronousPost(int* os_error_code, int* response_code) + OVERRIDE { + return false; + } + virtual int GetResponseContentLength() const OVERRIDE { + return 0; + } + virtual const char* GetResponseContent() const OVERRIDE { + return ""; + } + virtual const std::string GetResponseHeaderValue( + const std::string& name) const OVERRIDE { + return ""; + } + virtual void Abort() OVERRIDE {} +}; + class TestHttpPostProviderFactory : public HttpPostProviderFactory { public: virtual ~TestHttpPostProviderFactory() {} - virtual HttpPostProviderInterface* Create() { - NOTREACHED(); - return NULL; + virtual HttpPostProviderInterface* Create() OVERRIDE { + return new TestHttpPostProviderInterface(); } - virtual void Destroy(HttpPostProviderInterface* http) { - NOTREACHED(); + virtual void Destroy(HttpPostProviderInterface* http) OVERRIDE { + delete http; } }; @@ -660,7 +692,8 @@ class SyncManagerObserverMock : public SyncManager::Observer { MOCK_METHOD1(OnChangesComplete, void(ModelType)); // NOLINT MOCK_METHOD1(OnSyncCycleCompleted, void(const SyncSessionSnapshot*)); // NOLINT - MOCK_METHOD0(OnInitializationComplete, void()); // NOLINT + MOCK_METHOD1(OnInitializationComplete, + void(const WeakHandle<JsBackend>&)); // NOLINT MOCK_METHOD1(OnAuthError, void(const GoogleServiceAuthError&)); // NOLINT MOCK_METHOD1(OnPassphraseRequired, void(sync_api::PassphraseRequiredReason)); // NOLINT @@ -722,16 +755,23 @@ class SyncManagerTest : public testing::Test, EXPECT_CALL(*sync_notifier_mock_, RemoveObserver(_)). WillOnce(Invoke(this, &SyncManagerTest::SyncNotifierRemoveObserver)); + sync_manager_.AddObserver(&observer_); + EXPECT_CALL(observer_, OnInitializationComplete(_)). + WillOnce(SaveArg<0>(&js_backend_)); + EXPECT_FALSE(sync_notifier_observer_); + EXPECT_FALSE(js_backend_.IsInitialized()); // Takes ownership of |sync_notifier_mock_|. - sync_manager_.Init(temp_dir_.path(), "bogus", 0, false, + sync_manager_.Init(temp_dir_.path(), + WeakHandle<JsEventHandler>(), + "bogus", 0, false, new TestHttpPostProviderFactory(), this, "bogus", credentials, sync_notifier_mock_, "", true /* setup_for_test_mode */); EXPECT_TRUE(sync_notifier_observer_); - sync_manager_.AddObserver(&observer_); + EXPECT_TRUE(js_backend_.IsInitialized()); EXPECT_EQ(1, update_enabled_types_call_count_); @@ -746,6 +786,7 @@ class SyncManagerTest : public testing::Test, type_roots_[i->first] = MakeServerNodeForType( sync_manager_.GetUserShare(), i->first); } + PumpLoop(); } void TearDown() { @@ -753,6 +794,7 @@ class SyncManagerTest : public testing::Test, sync_manager_.Shutdown(); sync_notifier_mock_ = NULL; EXPECT_FALSE(sync_notifier_observer_); + PumpLoop(); } // ModelSafeWorkerRegistrar implementation. @@ -827,6 +869,23 @@ class SyncManagerTest : public testing::Test, ++update_enabled_types_call_count_; } + void PumpLoop() { + ui_loop_.RunAllPending(); + } + + void SendJsMessage(const std::string& name, const JsArgList& args, + const WeakHandle<JsReplyHandler>& reply_handler) { + js_backend_.Call(FROM_HERE, &JsBackend::ProcessJsMessage, + name, args, reply_handler); + PumpLoop(); + } + + void SetJsEventHandler(const WeakHandle<JsEventHandler>& event_handler) { + js_backend_.Call(FROM_HERE, &JsBackend::SetJsEventHandler, + event_handler); + PumpLoop(); + } + private: // Needed by |ui_thread_|. MessageLoopForUI ui_loop_; @@ -840,6 +899,7 @@ class SyncManagerTest : public testing::Test, protected: SyncManager sync_manager_; + WeakHandle<JsBackend> js_backend_; StrictMock<SyncManagerObserverMock> observer_; sync_notifier::SyncNotifierObserver* sync_notifier_observer_; int update_enabled_types_call_count_; @@ -852,84 +912,36 @@ TEST_F(SyncManagerTest, UpdateEnabledTypes) { EXPECT_EQ(2, update_enabled_types_call_count_); } -TEST_F(SyncManagerTest, ParentJsEventRouter) { - StrictMock<MockJsEventRouter> event_router; - browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend(); - EXPECT_EQ(NULL, js_backend->GetParentJsEventRouter()); - js_backend->SetParentJsEventRouter(&event_router); - EXPECT_EQ(&event_router, js_backend->GetParentJsEventRouter()); - js_backend->RemoveParentJsEventRouter(); - EXPECT_EQ(NULL, js_backend->GetParentJsEventRouter()); -} - -TEST_F(SyncManagerTest, ProcessMessage) { +TEST_F(SyncManagerTest, ProcessJsMessage) { const JsArgList kNoArgs; - browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend(); - - // Messages sent without any parent router should be dropped. - { - StrictMock<MockJsEventHandler> event_handler; - js_backend->ProcessMessage("unknownMessage", - kNoArgs, &event_handler); - js_backend->ProcessMessage("getNotificationState", - kNoArgs, &event_handler); - } - - { - StrictMock<MockJsEventHandler> event_handler; - StrictMock<MockJsEventRouter> event_router; - - ListValue false_args; - false_args.Append(Value::CreateBooleanValue(false)); - - EXPECT_CALL(event_router, - RouteJsMessageReply("getNotificationState", - HasArgsAsList(false_args), - &event_handler)); - - js_backend->SetParentJsEventRouter(&event_router); + StrictMock<MockJsReplyHandler> reply_handler; - // This message should be dropped. - js_backend->ProcessMessage("unknownMessage", - kNoArgs, &event_handler); + ListValue false_args; + false_args.Append(Value::CreateBooleanValue(false)); - // This should trigger the reply. - js_backend->ProcessMessage("getNotificationState", - kNoArgs, &event_handler); + EXPECT_CALL(reply_handler, + HandleJsReply("getNotificationState", + HasArgsAsList(false_args))); - js_backend->RemoveParentJsEventRouter(); - } + // This message should be dropped. + SendJsMessage("unknownMessage", kNoArgs, reply_handler.AsWeakHandle()); - // Messages sent after a parent router has been removed should be - // dropped. - { - StrictMock<MockJsEventHandler> event_handler; - js_backend->ProcessMessage("unknownMessage", - kNoArgs, &event_handler); - js_backend->ProcessMessage("getNotificationState", - kNoArgs, &event_handler); - } + SendJsMessage("getNotificationState", kNoArgs, reply_handler.AsWeakHandle()); } -TEST_F(SyncManagerTest, ProcessMessageGetRootNodeDetails) { +TEST_F(SyncManagerTest, ProcessJsMessageGetRootNodeDetails) { const JsArgList kNoArgs; - browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend(); - - StrictMock<MockJsEventHandler> event_handler; - StrictMock<MockJsEventRouter> event_router; + StrictMock<MockJsReplyHandler> reply_handler; JsArgList return_args; - EXPECT_CALL(event_router, - RouteJsMessageReply("getRootNodeDetails", _, &event_handler)) + EXPECT_CALL(reply_handler, + HandleJsReply("getRootNodeDetails", _)) .WillOnce(SaveArg<1>(&return_args)); - js_backend->SetParentJsEventRouter(&event_router); - - // Should trigger the reply. - js_backend->ProcessMessage("getRootNodeDetails", kNoArgs, &event_handler); + SendJsMessage("getRootNodeDetails", kNoArgs, reply_handler.AsWeakHandle()); EXPECT_EQ(1u, return_args.Get().GetSize()); DictionaryValue* node_info = NULL; @@ -942,8 +954,6 @@ TEST_F(SyncManagerTest, ProcessMessageGetRootNodeDetails) { } else { ADD_FAILURE(); } - - js_backend->RemoveParentJsEventRouter(); } void CheckGetNodesByIdReturnArgs(const SyncManager& sync_manager, @@ -981,64 +991,51 @@ class SyncManagerGetNodesByIdTest : public SyncManagerTest { MakeNode(sync_manager_.GetUserShare(), syncable::BOOKMARKS, "testtag"); - browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend(); - - StrictMock<MockJsEventHandler> event_handler; - StrictMock<MockJsEventRouter> event_router; + StrictMock<MockJsReplyHandler> reply_handler; JsArgList return_args; const int64 ids[] = { root_id, child_id }; - EXPECT_CALL(event_router, - RouteJsMessageReply(message_name, _, &event_handler)) + EXPECT_CALL(reply_handler, + HandleJsReply(message_name, _)) .Times(arraysize(ids)).WillRepeatedly(SaveArg<1>(&return_args)); - js_backend->SetParentJsEventRouter(&event_router); - for (size_t i = 0; i < arraysize(ids); ++i) { ListValue args; ListValue* id_values = new ListValue(); args.Append(id_values); id_values->Append(Value::CreateStringValue(base::Int64ToString(ids[i]))); - js_backend->ProcessMessage(message_name, - JsArgList(&args), &event_handler); + SendJsMessage(message_name, + JsArgList(&args), reply_handler.AsWeakHandle()); CheckGetNodesByIdReturnArgs(sync_manager_, return_args, ids[i], is_detailed); } - - js_backend->RemoveParentJsEventRouter(); } void RunGetNodesByIdFailureTest(const char* message_name) { - browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend(); - - StrictMock<MockJsEventHandler> event_handler; - StrictMock<MockJsEventRouter> event_router; + StrictMock<MockJsReplyHandler> reply_handler; ListValue empty_list_args; empty_list_args.Append(new ListValue()); - EXPECT_CALL(event_router, - RouteJsMessageReply(message_name, - HasArgsAsList(empty_list_args), - &event_handler)) + EXPECT_CALL(reply_handler, + HandleJsReply(message_name, + HasArgsAsList(empty_list_args))) .Times(6); - js_backend->SetParentJsEventRouter(&event_router); - { ListValue args; - js_backend->ProcessMessage(message_name, - JsArgList(&args), &event_handler); + SendJsMessage(message_name, + JsArgList(&args), reply_handler.AsWeakHandle()); } { ListValue args; args.Append(new ListValue()); - js_backend->ProcessMessage(message_name, - JsArgList(&args), &event_handler); + SendJsMessage(message_name, + JsArgList(&args), reply_handler.AsWeakHandle()); } { @@ -1046,8 +1043,8 @@ class SyncManagerGetNodesByIdTest : public SyncManagerTest { ListValue* ids = new ListValue(); args.Append(ids); ids->Append(Value::CreateStringValue("")); - js_backend->ProcessMessage(message_name, - JsArgList(&args), &event_handler); + SendJsMessage(message_name, + JsArgList(&args), reply_handler.AsWeakHandle()); } { @@ -1055,8 +1052,8 @@ class SyncManagerGetNodesByIdTest : public SyncManagerTest { ListValue* ids = new ListValue(); args.Append(ids); ids->Append(Value::CreateStringValue("nonsense")); - js_backend->ProcessMessage(message_name, - JsArgList(&args), &event_handler); + SendJsMessage(message_name, + JsArgList(&args), reply_handler.AsWeakHandle()); } { @@ -1064,8 +1061,8 @@ class SyncManagerGetNodesByIdTest : public SyncManagerTest { ListValue* ids = new ListValue(); args.Append(ids); ids->Append(Value::CreateStringValue("0")); - js_backend->ProcessMessage(message_name, - JsArgList(&args), &event_handler); + SendJsMessage(message_name, + JsArgList(&args), reply_handler.AsWeakHandle()); } { @@ -1073,11 +1070,9 @@ class SyncManagerGetNodesByIdTest : public SyncManagerTest { ListValue* ids = new ListValue(); args.Append(ids); ids->Append(Value::CreateStringValue("9999")); - js_backend->ProcessMessage(message_name, - JsArgList(&args), &event_handler); + SendJsMessage(message_name, + JsArgList(&args), reply_handler.AsWeakHandle()); } - - js_backend->RemoveParentJsEventRouter(); } }; @@ -1098,25 +1093,19 @@ TEST_F(SyncManagerGetNodesByIdTest, GetNodeDetailsByIdFailure) { } TEST_F(SyncManagerTest, GetChildNodeIds) { - browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend(); - - StrictMock<MockJsEventHandler> event_handler; - StrictMock<MockJsEventRouter> event_router; + StrictMock<MockJsReplyHandler> reply_handler; JsArgList return_args; - EXPECT_CALL(event_router, - RouteJsMessageReply("getChildNodeIds", _, &event_handler)) + EXPECT_CALL(reply_handler, + HandleJsReply("getChildNodeIds", _)) .Times(1).WillRepeatedly(SaveArg<1>(&return_args)); - js_backend->SetParentJsEventRouter(&event_router); - - // Should trigger the reply. { ListValue args; args.Append(Value::CreateStringValue("1")); - js_backend->ProcessMessage("getChildNodeIds", - JsArgList(&args), &event_handler); + SendJsMessage("getChildNodeIds", + JsArgList(&args), reply_handler.AsWeakHandle()); } EXPECT_EQ(1u, return_args.Get().GetSize()); @@ -1124,98 +1113,89 @@ TEST_F(SyncManagerTest, GetChildNodeIds) { ASSERT_TRUE(return_args.Get().GetList(0, &nodes)); ASSERT_TRUE(nodes); EXPECT_EQ(5u, nodes->GetSize()); - - js_backend->RemoveParentJsEventRouter(); } TEST_F(SyncManagerTest, GetChildNodeIdsFailure) { - browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend(); - - StrictMock<MockJsEventHandler> event_handler; - StrictMock<MockJsEventRouter> event_router; + StrictMock<MockJsReplyHandler> reply_handler; ListValue empty_list_args; empty_list_args.Append(new ListValue()); - EXPECT_CALL(event_router, - RouteJsMessageReply("getChildNodeIds", - HasArgsAsList(empty_list_args), - &event_handler)) + EXPECT_CALL(reply_handler, + HandleJsReply("getChildNodeIds", + HasArgsAsList(empty_list_args))) .Times(5); - js_backend->SetParentJsEventRouter(&event_router); - { ListValue args; - js_backend->ProcessMessage("getChildNodeIds", - JsArgList(&args), &event_handler); + SendJsMessage("getChildNodeIds", + JsArgList(&args), reply_handler.AsWeakHandle()); } { ListValue args; args.Append(Value::CreateStringValue("")); - js_backend->ProcessMessage("getChildNodeIds", - JsArgList(&args), &event_handler); + SendJsMessage("getChildNodeIds", + JsArgList(&args), reply_handler.AsWeakHandle()); } { ListValue args; args.Append(Value::CreateStringValue("nonsense")); - js_backend->ProcessMessage("getChildNodeIds", - JsArgList(&args), &event_handler); + SendJsMessage("getChildNodeIds", + JsArgList(&args), reply_handler.AsWeakHandle()); } { ListValue args; args.Append(Value::CreateStringValue("0")); - js_backend->ProcessMessage("getChildNodeIds", - JsArgList(&args), &event_handler); + SendJsMessage("getChildNodeIds", + JsArgList(&args), reply_handler.AsWeakHandle()); } { ListValue args; args.Append(Value::CreateStringValue("9999")); - js_backend->ProcessMessage("getChildNodeIds", - JsArgList(&args), &event_handler); + SendJsMessage("getChildNodeIds", + JsArgList(&args), reply_handler.AsWeakHandle()); } - - js_backend->RemoveParentJsEventRouter(); } // TODO(akalin): Add unit tests for findNodesContainingString message. TEST_F(SyncManagerTest, OnNotificationStateChange) { InSequence dummy; - StrictMock<MockJsEventRouter> event_router; + StrictMock<MockJsEventHandler> event_handler; DictionaryValue true_details; true_details.SetBoolean("enabled", true); DictionaryValue false_details; false_details.SetBoolean("enabled", false); - EXPECT_CALL(event_router, - RouteJsEvent("onNotificationStateChange", - HasDetailsAsDictionary(true_details))); - EXPECT_CALL(event_router, - RouteJsEvent("onNotificationStateChange", - HasDetailsAsDictionary(false_details))); - - browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend(); + EXPECT_CALL(event_handler, + HandleJsEvent("onNotificationStateChange", + HasDetailsAsDictionary(true_details))); + EXPECT_CALL(event_handler, + HandleJsEvent("onNotificationStateChange", + HasDetailsAsDictionary(false_details))); sync_manager_.TriggerOnNotificationStateChangeForTest(true); sync_manager_.TriggerOnNotificationStateChangeForTest(false); - js_backend->SetParentJsEventRouter(&event_router); + SetJsEventHandler(event_handler.AsWeakHandle()); sync_manager_.TriggerOnNotificationStateChangeForTest(true); sync_manager_.TriggerOnNotificationStateChangeForTest(false); - js_backend->RemoveParentJsEventRouter(); + SetJsEventHandler(WeakHandle<JsEventHandler>()); sync_manager_.TriggerOnNotificationStateChangeForTest(true); sync_manager_.TriggerOnNotificationStateChangeForTest(false); + + // Should trigger the replies. + PumpLoop(); } TEST_F(SyncManagerTest, OnIncomingNotification) { - StrictMock<MockJsEventRouter> event_router; + StrictMock<MockJsEventHandler> event_handler; const syncable::ModelTypeBitSet empty_model_types; syncable::ModelTypeBitSet model_types; @@ -1239,21 +1219,22 @@ TEST_F(SyncManagerTest, OnIncomingNotification) { } } - EXPECT_CALL(event_router, - RouteJsEvent("onIncomingNotification", + EXPECT_CALL(event_handler, + HandleJsEvent("onIncomingNotification", HasDetailsAsDictionary(expected_details))); - browser_sync::JsBackend* js_backend = sync_manager_.GetJsBackend(); - sync_manager_.TriggerOnIncomingNotificationForTest(empty_model_types); sync_manager_.TriggerOnIncomingNotificationForTest(model_types); - js_backend->SetParentJsEventRouter(&event_router); + SetJsEventHandler(event_handler.AsWeakHandle()); sync_manager_.TriggerOnIncomingNotificationForTest(model_types); - js_backend->RemoveParentJsEventRouter(); + SetJsEventHandler(WeakHandle<JsEventHandler>()); sync_manager_.TriggerOnIncomingNotificationForTest(empty_model_types); sync_manager_.TriggerOnIncomingNotificationForTest(model_types); + + // Should trigger the replies. + PumpLoop(); } TEST_F(SyncManagerTest, RefreshEncryptionReady) { diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc index ebd2cd9..468b3d3 100644 --- a/chrome/browser/sync/glue/sync_backend_host.cc +++ b/chrome/browser/sync/glue/sync_backend_host.cc @@ -28,6 +28,7 @@ #include "chrome/browser/sync/glue/sync_backend_host.h" #include "chrome/browser/sync/js_arg_list.h" #include "chrome/browser/sync/js_event_details.h" +#include "chrome/browser/sync/js_event_handler.h" #include "chrome/browser/sync/notifier/sync_notifier.h" #include "chrome/browser/sync/sessions/session_state.h" // TODO(tim): Remove this! We should have a syncapi pass-thru instead. @@ -93,6 +94,7 @@ SyncBackendHost::~SyncBackendHost() { void SyncBackendHost::Initialize( SyncFrontend* frontend, + const WeakHandle<JsEventHandler>& event_handler, const GURL& sync_service_url, const syncable::ModelTypeSet& types, const SyncCredentials& credentials, @@ -132,6 +134,7 @@ void SyncBackendHost::Initialize( } InitCore(Core::DoInitializeOptions( + event_handler, sync_service_url, profile_->GetRequestContext(), credentials, @@ -171,15 +174,6 @@ bool SyncBackendHost::IsCryptographerReady( return initialized() && trans->GetCryptographer()->is_ready(); } -JsBackend* SyncBackendHost::GetJsBackend() { - if (initialized()) { - return core_.get(); - } else { - NOTREACHED(); - return NULL; - } -} - sync_api::HttpPostProviderFactory* SyncBackendHost::MakeHttpBridgeFactory( const scoped_refptr<net::URLRequestContextGetter>& getter) { return new HttpBridgeFactory(getter); @@ -563,6 +557,7 @@ void SyncBackendHost::Core::FinishConfigureDataTypesOnFrontendLoop() { SyncBackendHost::Core::DoInitializeOptions::DoInitializeOptions( + const WeakHandle<JsEventHandler>& event_handler, const GURL& service_url, const scoped_refptr<net::URLRequestContextGetter>& request_context_getter, @@ -570,7 +565,8 @@ SyncBackendHost::Core::DoInitializeOptions::DoInitializeOptions( bool delete_sync_data_folder, const std::string& restored_key_for_bootstrapping, bool setup_for_test_mode) - : service_url(service_url), + : event_handler(event_handler), + service_url(service_url), request_context_getter(request_context_getter), credentials(credentials), delete_sync_data_folder(delete_sync_data_folder), @@ -636,8 +632,6 @@ void SyncBackendHost::LogUnsyncedItems(int level) const { SyncBackendHost::Core::Core(const std::string& name, SyncBackendHost* backend) : name_(name), host_(backend), - sync_manager_observer_(ALLOW_THIS_IN_INITIALIZER_LIST(this)), - parent_router_(NULL), processing_passphrase_(false) { } @@ -691,6 +685,7 @@ void SyncBackendHost::Core::DoInitialize(const DoInitializeOptions& options) { const FilePath& path_str = host_->sync_data_folder_path(); success = sync_manager_->Init( path_str, + options.event_handler, options.service_url.host() + options.service_url.path(), options.service_url.EffectiveIntPort(), options.service_url.SchemeIsSecure(), @@ -767,7 +762,6 @@ void SyncBackendHost::Core::DoShutdown(bool sync_disabled) { save_changes_timer_.Stop(); sync_manager_->Shutdown(); // Stops the SyncerThread. sync_manager_->RemoveObserver(this); - DisconnectChildJsEventRouter(); sync_manager_.reset(); host_->ui_worker()->OnSyncerShutdownComplete(); @@ -883,7 +877,8 @@ void SyncBackendHost::Core::HandleSyncCycleCompletedOnFrontendLoop( host_->frontend_->OnSyncCycleCompleted(); } -void SyncBackendHost::Core::OnInitializationComplete() { +void SyncBackendHost::Core::OnInitializationComplete( + const WeakHandle<JsBackend>& js_backend) { if (!host_ || !host_->frontend_) return; // We may have been told to Shutdown before initialization // completed. @@ -893,7 +888,7 @@ void SyncBackendHost::Core::OnInitializationComplete() { host_->frontend_loop_->PostTask(FROM_HERE, NewRunnableMethod(this, &Core::HandleInitializationCompletedOnFrontendLoop, - true)); + js_backend, true)); // Initialization is complete, so we can schedule recurring SaveChanges. host_->sync_thread_.message_loop()->PostTask(FROM_HERE, @@ -901,18 +896,19 @@ void SyncBackendHost::Core::OnInitializationComplete() { } void SyncBackendHost::Core::HandleInitializationCompletedOnFrontendLoop( + const WeakHandle<JsBackend>& js_backend, bool success) { if (!host_) return; - host_->HandleInitializationCompletedOnFrontendLoop(success); + host_->HandleInitializationCompletedOnFrontendLoop(js_backend, success); } void SyncBackendHost::HandleInitializationCompletedOnFrontendLoop( - bool success) { + const WeakHandle<JsBackend>& js_backend, bool success) { if (!frontend_) return; if (!success) { - frontend_->OnBackendInitialized(false); + frontend_->OnBackendInitialized(WeakHandle<JsBackend>(), false); return; } @@ -926,7 +922,7 @@ void SyncBackendHost::HandleInitializationCompletedOnFrontendLoop( if (setup_completed) core_->sync_manager()->RefreshEncryption(); initialization_state_ = INITIALIZED; - frontend_->OnBackendInitialized(true); + frontend_->OnBackendInitialized(js_backend, true); return; } @@ -937,7 +933,7 @@ void SyncBackendHost::HandleInitializationCompletedOnFrontendLoop( sync_api::CONFIGURE_REASON_NEW_CLIENT, base::Bind( &SyncBackendHost::Core::HandleInitializationCompletedOnFrontendLoop, - core_.get()), + core_.get(), js_backend), true); } @@ -1005,22 +1001,6 @@ void SyncBackendHost::Core::OnEncryptionComplete( encrypted_types)); } -void SyncBackendHost::Core::RouteJsEvent( - const std::string& name, const JsEventDetails& details) { - host_->frontend_loop_->PostTask( - FROM_HERE, NewRunnableMethod( - this, &Core::RouteJsEventOnFrontendLoop, name, details)); -} - -void SyncBackendHost::Core::RouteJsMessageReply( - const std::string& name, const JsArgList& args, - const JsEventHandler* target) { - host_->frontend_loop_->PostTask( - FROM_HERE, NewRunnableMethod( - this, &Core::RouteJsMessageReplyOnFrontendLoop, - name, args, target)); -} - void SyncBackendHost::Core::HandleStopSyncingPermanentlyOnFrontendLoop() { if (!host_ || !host_->frontend_) return; @@ -1050,27 +1030,6 @@ void SyncBackendHost::Core::HandleAuthErrorEventOnFrontendLoop( host_->frontend_->OnAuthError(); } -void SyncBackendHost::Core::RouteJsEventOnFrontendLoop( - const std::string& name, const JsEventDetails& details) { - if (!host_ || !parent_router_) - return; - - DCHECK_EQ(MessageLoop::current(), host_->frontend_loop_); - - parent_router_->RouteJsEvent(name, details); -} - -void SyncBackendHost::Core::RouteJsMessageReplyOnFrontendLoop( - const std::string& name, const JsArgList& args, - const JsEventHandler* target) { - if (!host_ || !parent_router_) - return; - - DCHECK_EQ(MessageLoop::current(), host_->frontend_loop_); - - parent_router_->RouteJsMessageReply(name, args, target); -} - void SyncBackendHost::Core::StartSavingChanges() { save_changes_timer_.Start( base::TimeDelta::FromSeconds(kSaveChangesIntervalSeconds), @@ -1096,68 +1055,4 @@ void SyncBackendHost::Core::DeleteSyncDataFolder() { } } -void SyncBackendHost::Core::SetParentJsEventRouter(JsEventRouter* router) { - DCHECK_EQ(MessageLoop::current(), host_->frontend_loop_); - DCHECK(router); - parent_router_ = router; - MessageLoop* sync_loop = host_->sync_thread_.message_loop(); - CHECK(sync_loop); - sync_loop->PostTask( - FROM_HERE, - NewRunnableMethod(this, - &SyncBackendHost::Core::ConnectChildJsEventRouter)); -} - -void SyncBackendHost::Core::RemoveParentJsEventRouter() { - DCHECK_EQ(MessageLoop::current(), host_->frontend_loop_); - parent_router_ = NULL; - MessageLoop* sync_loop = host_->sync_thread_.message_loop(); - CHECK(sync_loop); - sync_loop->PostTask( - FROM_HERE, - NewRunnableMethod(this, - &SyncBackendHost::Core::DisconnectChildJsEventRouter)); -} - -const JsEventRouter* SyncBackendHost::Core::GetParentJsEventRouter() const { - DCHECK_EQ(MessageLoop::current(), host_->frontend_loop_); - return parent_router_; -} - -void SyncBackendHost::Core::ProcessMessage( - const std::string& name, const JsArgList& args, - const JsEventHandler* sender) { - DCHECK_EQ(MessageLoop::current(), host_->frontend_loop_); - MessageLoop* sync_loop = host_->sync_thread_.message_loop(); - CHECK(sync_loop); - sync_loop->PostTask( - FROM_HERE, - NewRunnableMethod(this, - &SyncBackendHost::Core::DoProcessMessage, - name, args, sender)); -} - -void SyncBackendHost::Core::ConnectChildJsEventRouter() { - DCHECK_EQ(MessageLoop::current(), host_->sync_thread_.message_loop()); - // We need this check since AddObserver() can be called at most once - // for a given observer. - if (!sync_manager_->GetJsBackend()->GetParentJsEventRouter()) { - sync_manager_->GetJsBackend()->SetParentJsEventRouter(this); - sync_manager_->AddObserver(&sync_manager_observer_); - } -} - -void SyncBackendHost::Core::DisconnectChildJsEventRouter() { - DCHECK_EQ(MessageLoop::current(), host_->sync_thread_.message_loop()); - sync_manager_->GetJsBackend()->RemoveParentJsEventRouter(); - sync_manager_->RemoveObserver(&sync_manager_observer_); -} - -void SyncBackendHost::Core::DoProcessMessage( - const std::string& name, const JsArgList& args, - const JsEventHandler* sender) { - DCHECK_EQ(MessageLoop::current(), host_->sync_thread_.message_loop()); - sync_manager_->GetJsBackend()->ProcessMessage(name, args, sender); -} - } // namespace browser_sync diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h index b0aa548..3304b8b 100644 --- a/chrome/browser/sync/glue/sync_backend_host.h +++ b/chrome/browser/sync/glue/sync_backend_host.h @@ -24,12 +24,11 @@ #include "chrome/browser/sync/engine/configure_reason.h" #include "chrome/browser/sync/engine/model_safe_worker.h" #include "chrome/browser/sync/js_backend.h" -#include "chrome/browser/sync/js_sync_manager_observer.h" #include "chrome/browser/sync/glue/data_type_controller.h" #include "chrome/browser/sync/glue/ui_model_worker.h" -#include "chrome/browser/sync/js_event_router.h" #include "chrome/browser/sync/notifier/sync_notifier_factory.h" #include "chrome/browser/sync/syncable/model_type.h" +#include "chrome/browser/sync/weak_handle.h" #include "chrome/common/net/gaia/google_service_auth_error.h" #include "googleurl/src/gurl.h" #include "net/url_request/url_request_context_getter.h" @@ -49,7 +48,7 @@ struct SyncSessionSnapshot; class ChangeProcessor; class DataTypeController; -class JsArgList; +class JsEventHandler; // SyncFrontend is the interface used by SyncBackendHost to communicate with // the entity that created it and, presumably, is interested in sync-related @@ -63,7 +62,8 @@ class SyncFrontend { // The backend has completed initialization and it is now ready to accept and // process changes. If success is false, initialization wasn't able to be // completed and should be retried. - virtual void OnBackendInitialized(bool success) = 0; + virtual void OnBackendInitialized( + const WeakHandle<JsBackend>& js_backend, bool success) = 0; // The backend queried the server recently and received some updates. virtual void OnSyncCycleCompleted() = 0; @@ -132,6 +132,7 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { // bootstrap authentication using |lsid|, if it isn't empty. // Optionally delete the Sync Data folder (if it's corrupt). void Initialize(SyncFrontend* frontend, + const WeakHandle<JsEventHandler>& event_handler, const GURL& service_url, const syncable::ModelTypeSet& types, const sync_api::SyncCredentials& credentials, @@ -239,15 +240,6 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { // using a token previously received. bool IsCryptographerReady(const sync_api::BaseTransaction* trans) const; - // Returns a pointer to the JsBackend (which is owned by the - // service). Must be called only after the sync backend has been - // initialized, and never returns NULL if you do so. Overrideable - // for testing purposes. - virtual JsBackend* GetJsBackend(); - - // TODO(akalin): Write unit tests for the JsBackend, finding a way - // to make this class testable in general. - protected: // An enum representing the steps to initializing the SyncBackendHost. enum InitializationState { @@ -260,9 +252,7 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { // The real guts of SyncBackendHost, to keep the public client API clean. class Core : public base::RefCountedThreadSafe<SyncBackendHost::Core>, - public sync_api::SyncManager::Observer, - public JsBackend, - public JsEventRouter { + public sync_api::SyncManager::Observer { public: Core(const std::string& name, SyncBackendHost* backend); @@ -277,7 +267,8 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { virtual void OnChangesComplete(syncable::ModelType model_type); virtual void OnSyncCycleCompleted( const sessions::SyncSessionSnapshot* snapshot); - virtual void OnInitializationComplete(); + virtual void OnInitializationComplete( + const WeakHandle<JsBackend>& js_backend); virtual void OnAuthError(const GoogleServiceAuthError& auth_error); virtual void OnPassphraseRequired( sync_api::PassphraseRequiredReason reason); @@ -289,22 +280,9 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { virtual void OnEncryptionComplete( const syncable::ModelTypeSet& encrypted_types); - // JsBackend implementation. - virtual void SetParentJsEventRouter(JsEventRouter* router) OVERRIDE; - virtual void RemoveParentJsEventRouter() OVERRIDE; - virtual const JsEventRouter* GetParentJsEventRouter() const OVERRIDE; - virtual void ProcessMessage(const std::string& name, const JsArgList& args, - const JsEventHandler* sender) OVERRIDE; - - // JsEventRouter implementation. - virtual void RouteJsEvent(const std::string& event_name, - const JsEventDetails& details) OVERRIDE; - virtual void RouteJsMessageReply(const std::string& event_name, - const JsArgList& args, - const JsEventHandler* target) OVERRIDE; - struct DoInitializeOptions { DoInitializeOptions( + const WeakHandle<JsEventHandler>& event_handler, const GURL& service_url, const scoped_refptr<net::URLRequestContextGetter>& request_context_getter, @@ -314,6 +292,7 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { bool setup_for_test_mode); ~DoInitializeOptions(); + WeakHandle<JsEventHandler> event_handler; GURL service_url; scoped_refptr<net::URLRequestContextGetter> request_context_getter; sync_api::SyncCredentials credentials; @@ -396,20 +375,14 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { // sync databases), as well as shutdown when you're no longer syncing. void DeleteSyncDataFolder(); - void ConnectChildJsEventRouter(); - - void DisconnectChildJsEventRouter(); - - void DoProcessMessage( - const std::string& name, const JsArgList& args, - const JsEventHandler* sender); - // A callback from the SyncerThread when it is safe to continue config. void FinishConfigureDataTypes(); // Called to handle updating frontend thread components whenever we may // need to alert the frontend that the backend is intialized. - void HandleInitializationCompletedOnFrontendLoop(bool success); + void HandleInitializationCompletedOnFrontendLoop( + const WeakHandle<JsBackend>& js_backend, + bool success); #if defined(UNIT_TEST) // Special form of initialization that does not try and authenticate the @@ -423,7 +396,8 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { sync_api::SyncCredentials credentials; credentials.email = WideToUTF8(test_user); credentials.sync_token = "token"; - DoInitialize(DoInitializeOptions(GURL(), getter, credentials, + DoInitialize(DoInitializeOptions(WeakHandle<JsEventHandler>(), + GURL(), getter, credentials, delete_sync_data_folder, "", true)); } @@ -483,13 +457,6 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { void HandleClearServerDataSucceededOnFrontendLoop(); void HandleClearServerDataFailedOnFrontendLoop(); - void RouteJsEventOnFrontendLoop( - const std::string& name, const JsEventDetails& details); - - void RouteJsMessageReplyOnFrontendLoop( - const std::string& name, const JsArgList& args, - const JsEventHandler* target); - void FinishConfigureDataTypesOnFrontendLoop(); // Return true if a model lives on the current thread. @@ -507,10 +474,6 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { // The top-level syncapi entry point. Lives on the sync thread. scoped_ptr<sync_api::SyncManager> sync_manager_; - JsSyncManagerObserver sync_manager_observer_; - - JsEventRouter* parent_router_; - // Denotes if the core is currently attempting to set a passphrase. While // this is true, OnPassphraseRequired calls are dropped. // Note: after initialization, this variable should only ever be accessed or @@ -523,7 +486,9 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { // InitializationComplete passes through the SyncBackendHost to forward // on to |frontend_|, and so that tests can intercept here if they need to // set up initial conditions. - virtual void HandleInitializationCompletedOnFrontendLoop(bool success); + virtual void HandleInitializationCompletedOnFrontendLoop( + const WeakHandle<JsBackend>& js_backend, + bool success); // Called to finish the job of ConfigureDataTypes once the syncer is in // configuration mode. diff --git a/chrome/browser/sync/glue/sync_backend_host_unittest.cc b/chrome/browser/sync/glue/sync_backend_host_unittest.cc index 51fc369..eef2f37 100644 --- a/chrome/browser/sync/glue/sync_backend_host_unittest.cc +++ b/chrome/browser/sync/glue/sync_backend_host_unittest.cc @@ -33,7 +33,7 @@ class MockSyncFrontend : public SyncFrontend { public: virtual ~MockSyncFrontend() {} - MOCK_METHOD1(OnBackendInitialized, void(bool)); + MOCK_METHOD2(OnBackendInitialized, void(const WeakHandle<JsBackend>&, bool)); MOCK_METHOD0(OnSyncCycleCompleted, void()); MOCK_METHOD0(OnAuthError, void()); MOCK_METHOD0(OnStopSyncingPermanently, void()); @@ -101,6 +101,7 @@ TEST_F(SyncBackendHostTest, InitShutdown) { credentials.email = "user@example.com"; credentials.sync_token = "sync_token"; backend.Initialize(&mock_frontend, + WeakHandle<JsEventHandler>(), GURL(k_mock_url), syncable::ModelTypeSet(), credentials, diff --git a/chrome/browser/sync/js_backend.h b/chrome/browser/sync/js_backend.h index 6d5ec92..bd5db52 100644 --- a/chrome/browser/sync/js_backend.h +++ b/chrome/browser/sync/js_backend.h @@ -14,26 +14,23 @@ namespace browser_sync { class JsArgList; class JsEventHandler; -class JsEventRouter; +class JsReplyHandler; +template <typename T> class WeakHandle; +// Interface representing the backend of chrome://sync-internals. A +// JsBackend can handle messages and can emit events to a +// JsEventHandler. class JsBackend { public: - // Sets the JS event router to which all backend events will be - // sent. - virtual void SetParentJsEventRouter(JsEventRouter* router) = 0; + // Starts emitting events to the given handler, if initialized. + virtual void SetJsEventHandler( + const WeakHandle<JsEventHandler>& event_handler) = 0; - // Removes any existing JS event router. - virtual void RemoveParentJsEventRouter() = 0; - - // Gets the crurent JS event router, or NULL if there is none. Used - // for testing. - virtual const JsEventRouter* GetParentJsEventRouter() const = 0; - - // Processes the given message. All reply events are sent to the - // parent JS event router (if set). - virtual void ProcessMessage( + // Processes the given message and replies via the given handler, if + // initialized. + virtual void ProcessJsMessage( const std::string& name, const JsArgList& args, - const JsEventHandler* sender) = 0; + const WeakHandle<JsReplyHandler>& reply_handler) = 0; protected: virtual ~JsBackend() {} diff --git a/chrome/browser/sync/js_controller.h b/chrome/browser/sync/js_controller.h new file mode 100644 index 0000000..65a43da --- /dev/null +++ b/chrome/browser/sync/js_controller.h @@ -0,0 +1,50 @@ +// Copyright (c) 2011 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 CHROME_BROWSER_SYNC_JS_CONTROLLER_H_ +#define CHROME_BROWSER_SYNC_JS_CONTROLLER_H_ +#pragma once + +// See README.js for design comments. + +#include <string> + +namespace browser_sync { + +class JsArgList; +class JsEventHandler; +class JsReplyHandler; +template <typename T> class WeakHandle; + +// An interface for objects that JsEventHandlers directly interact +// with. JsEventHandlers can add themselves to receive events and +// also send messages which will eventually reach the backend. +class JsController { + public: + // Adds an event handler which will start receiving JS events (not + // immediately, so this can be called in the handler's constructor). + // Multiple event handlers are supported, but each event handler + // must be added at most once. + // + // Ideally, we'd take WeakPtrs, but we need the raw pointer values + // to be able to look them up for removal. + virtual void AddJsEventHandler(JsEventHandler* event_handler) = 0; + + // Removes the given event handler if it has been added. It will + // immediately stop receiving any JS events. + virtual void RemoveJsEventHandler(JsEventHandler* event_handler) = 0; + + // Processes a JS message. The reply (if any) will be sent to + // |reply_handler| if it is initialized. + virtual void ProcessJsMessage( + const std::string& name, const JsArgList& args, + const WeakHandle<JsReplyHandler>& reply_handler) = 0; + + protected: + virtual ~JsController() {} +}; + +} // namespace browser_sync + +#endif // CHROME_BROWSER_SYNC_JS_CONTROLLER_H_ diff --git a/chrome/browser/sync/js_event_handler.h b/chrome/browser/sync/js_event_handler.h index 9a16e65..5c3f504 100644 --- a/chrome/browser/sync/js_event_handler.h +++ b/chrome/browser/sync/js_event_handler.h @@ -12,7 +12,6 @@ namespace browser_sync { -class JsArgList; class JsEventDetails; // An interface for objects that handle Javascript events (e.g., @@ -22,9 +21,6 @@ class JsEventHandler { virtual void HandleJsEvent( const std::string& name, const JsEventDetails& details) = 0; - virtual void HandleJsMessageReply( - const std::string& name, const JsArgList& args) = 0; - protected: virtual ~JsEventHandler() {} }; diff --git a/chrome/browser/sync/js_event_handler_list.cc b/chrome/browser/sync/js_event_handler_list.cc deleted file mode 100644 index 984f2a6..0000000 --- a/chrome/browser/sync/js_event_handler_list.cc +++ /dev/null @@ -1,96 +0,0 @@ -// Copyright (c) 2011 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 "chrome/browser/sync/js_event_handler_list.h" - -#include <cstddef> - -#include "base/logging.h" -#include "chrome/browser/sync/js_backend.h" -#include "chrome/browser/sync/js_event_details.h" -#include "chrome/browser/sync/js_event_handler.h" - -namespace browser_sync { - -JsEventHandlerList::PendingMessage::PendingMessage( - const std::string& name, const JsArgList& args, - const JsEventHandler* sender) - : name(name), args(args), sender(sender) {} - -JsEventHandlerList::JsEventHandlerList() : backend_(NULL) {} - -JsEventHandlerList::~JsEventHandlerList() { - RemoveBackend(); -} - -// We connect to the backend only when necessary, i.e. when there is -// at least one handler. - -void JsEventHandlerList::AddHandler(JsEventHandler* handler) { - handlers_.AddObserver(handler); - if (backend_) { - backend_->SetParentJsEventRouter(this); - } -} - -void JsEventHandlerList::RemoveHandler(JsEventHandler* handler) { - handlers_.RemoveObserver(handler); - if (backend_ && handlers_.size() == 0) { - backend_->RemoveParentJsEventRouter(); - } -} - -void JsEventHandlerList::SetBackend(JsBackend* backend) { - DCHECK(!backend_); - DCHECK(backend); - backend_ = backend; - - if (handlers_.size() > 0) { - backend_->SetParentJsEventRouter(this); - - // Process any queued messages. - PendingMessageList pending_messages; - pending_messages_.swap(pending_messages); - for (PendingMessageList::const_iterator it = pending_messages.begin(); - it != pending_messages.end(); ++it) { - backend_->ProcessMessage(it->name, it->args, it->sender); - } - } -} - -void JsEventHandlerList::RemoveBackend() { - if (backend_) { - backend_->RemoveParentJsEventRouter(); - backend_ = NULL; - } -} - -void JsEventHandlerList::ProcessMessage( - const std::string& name, const JsArgList& args, - const JsEventHandler* sender) { - if (backend_) { - backend_->ProcessMessage(name, args, sender); - } else { - pending_messages_.push_back(PendingMessage(name, args, sender)); - } -} - -void JsEventHandlerList::RouteJsEvent(const std::string& name, - const JsEventDetails& details) { - FOR_EACH_OBSERVER(JsEventHandler, handlers_, HandleJsEvent(name, details)); -} - -void JsEventHandlerList::RouteJsMessageReply(const std::string& name, - const JsArgList& args, - const JsEventHandler* target) { - JsEventHandler* non_const_target(const_cast<JsEventHandler*>(target)); - if (handlers_.HasObserver(non_const_target)) { - non_const_target->HandleJsMessageReply(name, args); - } else { - VLOG(1) << "Unknown target; dropping message reply " << name - << " with args " << args.ToString(); - } -} - -} // namespace browser_sync diff --git a/chrome/browser/sync/js_event_handler_list.h b/chrome/browser/sync/js_event_handler_list.h deleted file mode 100644 index 1fd68c1..0000000 --- a/chrome/browser/sync/js_event_handler_list.h +++ /dev/null @@ -1,79 +0,0 @@ -// Copyright (c) 2011 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 CHROME_BROWSER_SYNC_JS_EVENT_HANDLER_LIST_H_ -#define CHROME_BROWSER_SYNC_JS_EVENT_HANDLER_LIST_H_ -#pragma once - -#include <string> -#include <vector> - -#include "base/basictypes.h" -#include "base/compiler_specific.h" -#include "base/observer_list.h" -#include "chrome/browser/sync/js_arg_list.h" -#include "chrome/browser/sync/js_event_router.h" -#include "chrome/browser/sync/js_frontend.h" - -namespace browser_sync { - -class JsBackend; -class JsEventHandler; - -// A beefed-up ObserverList<JsEventHandler> that transparently handles -// the communication between the handlers and a backend. -class JsEventHandlerList : public JsFrontend, public JsEventRouter { - public: - JsEventHandlerList(); - - virtual ~JsEventHandlerList(); - - // Sets the backend to route all messages to. Should be called only - // if a backend has not already been set. - void SetBackend(JsBackend* backend); - - // Removes any existing backend. - void RemoveBackend(); - - // JsFrontend implementation. Routes messages to any attached - // backend; if there is none, queues up the message for processing - // when the next backend is attached. - virtual void AddHandler(JsEventHandler* handler) OVERRIDE; - virtual void RemoveHandler(JsEventHandler* handler) OVERRIDE; - virtual void ProcessMessage( - const std::string& name, const JsArgList& args, - const JsEventHandler* sender) OVERRIDE; - - // JsEventRouter implementation. Routes the event to the - // appropriate handler(s). - virtual void RouteJsEvent(const std::string& name, - const JsEventDetails& details) OVERRIDE; - virtual void RouteJsMessageReply(const std::string& name, - const JsArgList& args, - const JsEventHandler* target) OVERRIDE; - - private: - // A struct used to hold the arguments to ProcessMessage() for - // future invocation. - struct PendingMessage { - std::string name; - JsArgList args; - const JsEventHandler* sender; - - PendingMessage(const std::string& name, const JsArgList& args, - const JsEventHandler* sender); - }; - - typedef std::vector<PendingMessage> PendingMessageList; - - JsBackend* backend_; - ObserverList<JsEventHandler> handlers_; - PendingMessageList pending_messages_; - - DISALLOW_COPY_AND_ASSIGN(JsEventHandlerList); -}; - -} // namespace browser_sync - -#endif // CHROME_BROWSER_SYNC_JS_EVENT_HANDLER_LIST_H_ diff --git a/chrome/browser/sync/js_event_handler_list_unittest.cc b/chrome/browser/sync/js_event_handler_list_unittest.cc deleted file mode 100644 index 15687d8..0000000 --- a/chrome/browser/sync/js_event_handler_list_unittest.cc +++ /dev/null @@ -1,128 +0,0 @@ -// Copyright (c) 2011 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 "chrome/browser/sync/js_event_handler_list.h" - -#include "base/values.h" -#include "chrome/browser/sync/js_arg_list.h" -#include "chrome/browser/sync/js_event_details.h" -#include "chrome/browser/sync/js_test_util.h" -#include "testing/gmock/include/gmock/gmock.h" -#include "testing/gtest/include/gtest/gtest.h" - -namespace browser_sync { -namespace { - -using ::testing::InSequence; -using ::testing::StrictMock; - -class JsEventHandlerListTest : public testing::Test {}; - -TEST_F(JsEventHandlerListTest, Basic) { - InSequence dummy; - // |backend| must outlive |event_handlers|. - StrictMock<MockJsBackend> backend; - - JsEventHandlerList event_handlers; - - ListValue arg_list1, arg_list2; - arg_list1.Append(Value::CreateBooleanValue(false)); - arg_list2.Append(Value::CreateIntegerValue(5)); - JsArgList args1(&arg_list1), args2(&arg_list2); - - DictionaryValue details_dict1, details_dict2; - details_dict1.SetString("foo", "bar"); - details_dict2.SetInteger("baz", 5); - JsEventDetails details1(&details_dict1), details2(&details_dict2); - - StrictMock<MockJsEventHandler> handler1, handler2; - - // Once from each call to AddHandler(). - EXPECT_CALL(backend, SetParentJsEventRouter(&event_handlers)).Times(2); - - EXPECT_CALL(backend, ProcessMessage("test1", HasArgs(args2), &handler1)); - - EXPECT_CALL(handler2, HandleJsMessageReply("reply2", HasArgs(args1))); - EXPECT_CALL(handler1, HandleJsMessageReply("reply1", HasArgs(args2))); - EXPECT_CALL(handler1, HandleJsEvent("event", HasDetails(details1))); - EXPECT_CALL(handler2, HandleJsEvent("event", HasDetails(details1))); - - EXPECT_CALL(backend, ProcessMessage("test2", HasArgs(args1), &handler2)); - - EXPECT_CALL(handler2, HandleJsMessageReply("anotherreply2", HasArgs(args2))); - EXPECT_CALL(handler2, HandleJsEvent("anotherevent", HasDetails(details2))); - - // Once from the second call to RemoveHandler(), once from the - // destructor. - EXPECT_CALL(backend, RemoveParentJsEventRouter()).Times(2); - - event_handlers.SetBackend(&backend); - - event_handlers.AddHandler(&handler1); - event_handlers.AddHandler(&handler2); - - event_handlers.ProcessMessage("test1", args2, &handler1); - - event_handlers.RouteJsMessageReply("reply2", args1, &handler2); - event_handlers.RouteJsMessageReply("reply1", args2, &handler1); - event_handlers.RouteJsEvent("event", details1); - - event_handlers.RemoveHandler(&handler1); - - event_handlers.ProcessMessage("test2", args1, &handler2); - - event_handlers.RouteJsMessageReply("droppedreply1", args1, &handler1); - event_handlers.RouteJsMessageReply("anotherreply2", args2, &handler2); - event_handlers.RouteJsEvent("anotherevent", details2); - - event_handlers.RemoveHandler(&handler2); - - event_handlers.RouteJsMessageReply("anotherdroppedreply1", - args1, &handler1); - event_handlers.RouteJsMessageReply("anotheranotherreply2", - args2, &handler2); - event_handlers.RouteJsEvent("droppedevent", details2); - - // Let destructor of |event_handlers| call RemoveBackend(). -} - -TEST_F(JsEventHandlerListTest, QueuedMessages) { - // |backend| must outlive |event_handlers|. - StrictMock<MockJsBackend> backend; - - JsEventHandlerList event_handlers; - - ListValue arg_list1, arg_list2; - arg_list1.Append(Value::CreateBooleanValue(false)); - arg_list2.Append(Value::CreateIntegerValue(5)); - JsArgList args1(&arg_list1), args2(&arg_list2); - - StrictMock<MockJsEventHandler> handler1, handler2; - - // Once from the call to SetBackend(). - EXPECT_CALL(backend, SetParentJsEventRouter(&event_handlers)); - // Once from the first call to RemoveBackend(). - EXPECT_CALL(backend, RemoveParentJsEventRouter()); - // Once from the call to SetBackend(). - EXPECT_CALL(backend, ProcessMessage("test1", HasArgs(args2), &handler1)); - // Once from the call to SetBackend(). - EXPECT_CALL(backend, ProcessMessage("test2", HasArgs(args1), &handler2)); - - event_handlers.AddHandler(&handler1); - event_handlers.AddHandler(&handler2); - - // Should queue messages. - event_handlers.ProcessMessage("test1", args2, &handler1); - event_handlers.ProcessMessage("test2", args1, &handler2); - - // Should call the queued messages. - event_handlers.SetBackend(&backend); - - event_handlers.RemoveBackend(); - // Should do nothing. - event_handlers.RemoveBackend(); -} - -} // namespace -} // namespace browser_sync diff --git a/chrome/browser/sync/js_event_router.h b/chrome/browser/sync/js_event_router.h deleted file mode 100644 index 89839ab..0000000 --- a/chrome/browser/sync/js_event_router.h +++ /dev/null @@ -1,40 +0,0 @@ -// Copyright (c) 2011 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 CHROME_BROWSER_SYNC_JS_EVENT_ROUTER_H_ -#define CHROME_BROWSER_SYNC_JS_EVENT_ROUTER_H_ -#pragma once - -// See README.js for design comments. - -#include <string> - -namespace browser_sync { - -class JsArgList; -class JsEventDetails; -class JsEventHandler; - -// An interface for objects that don't directly handle Javascript -// events but can pass them to JsEventHandlers or route them to other -// JsEventRouters. -class JsEventRouter { - public: - virtual void RouteJsEvent( - const std::string& name, const JsEventDetails& details) = 0; - - // |target| is const because it shouldn't be used except by the - // router that directly knows about it (which can then safely cast - // away the constness). - virtual void RouteJsMessageReply( - const std::string& name, const JsArgList& args, - const JsEventHandler* target) = 0; - - protected: - virtual ~JsEventRouter() {} -}; - -} // namespace browser_sync - -#endif // CHROME_BROWSER_SYNC_JS_EVENT_ROUTER_H_ diff --git a/chrome/browser/sync/js_frontend.h b/chrome/browser/sync/js_frontend.h deleted file mode 100644 index e57cacd..0000000 --- a/chrome/browser/sync/js_frontend.h +++ /dev/null @@ -1,43 +0,0 @@ -// Copyright (c) 2011 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 CHROME_BROWSER_SYNC_JS_FRONTEND_H_ -#define CHROME_BROWSER_SYNC_JS_FRONTEND_H_ -#pragma once - -// See README.js for design comments. - -#include <string> - -namespace browser_sync { - -class JsArgList; -class JsEventHandler; - -// An interface for objects that interact directly with -// JsEventHandlers. -class JsFrontend { - public: - // Adds a handler which will start receiving JS events (not - // immediately, so this can be called in the handler's constructor). - // A handler must be added at most once. - virtual void AddHandler(JsEventHandler* handler) = 0; - - // Removes the given handler if it has been added. It will - // immediately stop receiving any JS events. - virtual void RemoveHandler(JsEventHandler* handler) = 0; - - // Sends a JS message. The reply (if any) will be sent to |sender| - // if it has been added. - virtual void ProcessMessage( - const std::string& name, const JsArgList& args, - const JsEventHandler* sender) = 0; - - protected: - virtual ~JsFrontend() {} -}; - -} // namespace browser_sync - -#endif // CHROME_BROWSER_SYNC_JS_FRONTEND_H_ diff --git a/chrome/browser/sync/js_reply_handler.h b/chrome/browser/sync/js_reply_handler.h new file mode 100644 index 0000000..5d50fc8 --- /dev/null +++ b/chrome/browser/sync/js_reply_handler.h @@ -0,0 +1,30 @@ +// Copyright (c) 2011 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 CHROME_BROWSER_SYNC_JS_REPLY_HANDLER_H_ +#define CHROME_BROWSER_SYNC_JS_REPLY_HANDLER_H_ +#pragma once + +// See README.js for design comments. + +#include <string> + +namespace browser_sync { + +class JsArgList; + +// An interface for objects that handle Javascript message replies +// (e.g., WebUIs). +class JsReplyHandler { + public: + virtual void HandleJsReply( + const std::string& name, const JsArgList& args) = 0; + + protected: + virtual ~JsReplyHandler() {} +}; + +} // namespace browser_sync + +#endif // CHROME_BROWSER_SYNC_JS_REPLY_HANDLER_H_ diff --git a/chrome/browser/sync/js_sync_manager_observer.cc b/chrome/browser/sync/js_sync_manager_observer.cc index 7faf4ec..4c2ff6a 100644 --- a/chrome/browser/sync/js_sync_manager_observer.cc +++ b/chrome/browser/sync/js_sync_manager_observer.cc @@ -7,27 +7,33 @@ #include <cstddef> #include "base/logging.h" +#include "base/tracked.h" #include "base/values.h" #include "chrome/browser/sync/js_arg_list.h" #include "chrome/browser/sync/js_event_details.h" -#include "chrome/browser/sync/js_event_router.h" +#include "chrome/browser/sync/js_event_handler.h" #include "chrome/browser/sync/sessions/session_state.h" #include "chrome/browser/sync/syncable/model_type.h" namespace browser_sync { -JsSyncManagerObserver::JsSyncManagerObserver(JsEventRouter* parent_router) - : parent_router_(parent_router) { - DCHECK(parent_router_); -} +JsSyncManagerObserver::JsSyncManagerObserver() {} JsSyncManagerObserver::~JsSyncManagerObserver() {} +void JsSyncManagerObserver::SetJsEventHandler( + const WeakHandle<JsEventHandler>& event_handler) { + event_handler_ = event_handler; +} + void JsSyncManagerObserver::OnChangesApplied( syncable::ModelType model_type, const sync_api::BaseTransaction* trans, const sync_api::SyncManager::ChangeRecord* changes, int change_count) { + if (!event_handler_.IsInitialized()) { + return; + } DictionaryValue details; details.SetString("modelType", syncable::ModelTypeToString(model_type)); ListValue* change_values = new ListValue(); @@ -35,85 +41,131 @@ void JsSyncManagerObserver::OnChangesApplied( for (int i = 0; i < change_count; ++i) { change_values->Append(changes[i].ToValue(trans)); } - parent_router_->RouteJsEvent("onChangesApplied", JsEventDetails(&details)); + HandleJsEvent(FROM_HERE, "onChangesApplied", JsEventDetails(&details)); } void JsSyncManagerObserver::OnChangesComplete( syncable::ModelType model_type) { + if (!event_handler_.IsInitialized()) { + return; + } DictionaryValue details; details.SetString("modelType", syncable::ModelTypeToString(model_type)); - parent_router_->RouteJsEvent("onChangesComplete", JsEventDetails(&details)); + HandleJsEvent(FROM_HERE, "onChangesComplete", JsEventDetails(&details)); } void JsSyncManagerObserver::OnSyncCycleCompleted( const sessions::SyncSessionSnapshot* snapshot) { + if (!event_handler_.IsInitialized()) { + return; + } DictionaryValue details; details.Set("snapshot", snapshot->ToValue()); - parent_router_->RouteJsEvent("onSyncCycleCompleted", - JsEventDetails(&details)); + HandleJsEvent(FROM_HERE, "onSyncCycleCompleted", JsEventDetails(&details)); } void JsSyncManagerObserver::OnAuthError( const GoogleServiceAuthError& auth_error) { + if (!event_handler_.IsInitialized()) { + return; + } DictionaryValue details; details.Set("authError", auth_error.ToValue()); - parent_router_->RouteJsEvent("onAuthError", JsEventDetails(&details)); + HandleJsEvent(FROM_HERE, "onAuthError", JsEventDetails(&details)); } void JsSyncManagerObserver::OnUpdatedToken(const std::string& token) { + if (!event_handler_.IsInitialized()) { + return; + } DictionaryValue details; details.SetString("token", "<redacted>"); - parent_router_->RouteJsEvent("onUpdatedToken", JsEventDetails(&details)); + HandleJsEvent(FROM_HERE, "onUpdatedToken", JsEventDetails(&details)); } void JsSyncManagerObserver::OnPassphraseRequired( sync_api::PassphraseRequiredReason reason) { + if (!event_handler_.IsInitialized()) { + return; + } DictionaryValue details; details.SetString("reason", sync_api::PassphraseRequiredReasonToString(reason)); - parent_router_->RouteJsEvent("onPassphraseRequired", - JsEventDetails(&details)); + HandleJsEvent(FROM_HERE, "onPassphraseRequired", JsEventDetails(&details)); } void JsSyncManagerObserver::OnPassphraseAccepted( const std::string& bootstrap_token) { + if (!event_handler_.IsInitialized()) { + return; + } DictionaryValue details; details.SetString("bootstrapToken", "<redacted>"); - parent_router_->RouteJsEvent("onPassphraseAccepted", - JsEventDetails(&details)); + HandleJsEvent(FROM_HERE, "onPassphraseAccepted", JsEventDetails(&details)); } void JsSyncManagerObserver::OnEncryptionComplete( const syncable::ModelTypeSet& encrypted_types) { + if (!event_handler_.IsInitialized()) { + return; + } DictionaryValue details; details.Set("encryptedTypes", syncable::ModelTypeSetToValue(encrypted_types)); - parent_router_->RouteJsEvent("onEncryptionComplete", - JsEventDetails(&details)); + HandleJsEvent(FROM_HERE, "onEncryptionComplete", JsEventDetails(&details)); } void JsSyncManagerObserver::OnMigrationNeededForTypes( const syncable::ModelTypeSet& types) { + if (!event_handler_.IsInitialized()) { + return; + } DictionaryValue details; details.Set("types", syncable::ModelTypeSetToValue(types)); - parent_router_->RouteJsEvent("onMigrationNeededForTypes", - JsEventDetails(&details)); + HandleJsEvent(FROM_HERE, "onMigrationNeededForTypes", + JsEventDetails(&details)); } -void JsSyncManagerObserver::OnInitializationComplete() { - parent_router_->RouteJsEvent("onInitializationComplete", JsEventDetails()); +void JsSyncManagerObserver::OnInitializationComplete( + const WeakHandle<JsBackend>& js_backend) { + if (!event_handler_.IsInitialized()) { + return; + } + // Ignore the |js_backend| argument; it's not really convertible to + // JSON anyway. + HandleJsEvent(FROM_HERE, "onInitializationComplete", JsEventDetails()); } void JsSyncManagerObserver::OnStopSyncingPermanently() { - parent_router_->RouteJsEvent("onStopSyncingPermanently", JsEventDetails()); + if (!event_handler_.IsInitialized()) { + return; + } + HandleJsEvent(FROM_HERE, "onStopSyncingPermanently", JsEventDetails()); } void JsSyncManagerObserver::OnClearServerDataSucceeded() { - parent_router_->RouteJsEvent("onClearServerDataSucceeded", JsEventDetails()); + if (!event_handler_.IsInitialized()) { + return; + } + HandleJsEvent(FROM_HERE, "onClearServerDataSucceeded", JsEventDetails()); } void JsSyncManagerObserver::OnClearServerDataFailed() { - parent_router_->RouteJsEvent("onClearServerDataFailed", JsEventDetails()); + if (!event_handler_.IsInitialized()) { + return; + } + HandleJsEvent(FROM_HERE, "onClearServerDataFailed", JsEventDetails()); +} + +void JsSyncManagerObserver::HandleJsEvent( + const tracked_objects::Location& from_here, + const std::string& name, const JsEventDetails& details) { + if (!event_handler_.IsInitialized()) { + NOTREACHED(); + return; + } + event_handler_.Call(from_here, + &JsEventHandler::HandleJsEvent, name, details); } } // namespace browser_sync diff --git a/chrome/browser/sync/js_sync_manager_observer.h b/chrome/browser/sync/js_sync_manager_observer.h index 597b3a8..29422e8 100644 --- a/chrome/browser/sync/js_sync_manager_observer.h +++ b/chrome/browser/sync/js_sync_manager_observer.h @@ -10,19 +10,25 @@ #include "base/basictypes.h" #include "chrome/browser/sync/engine/syncapi.h" +#include "chrome/browser/sync/weak_handle.h" + +namespace tracked_objects { +class Location; +} // namespace tracked_objects namespace browser_sync { -class JsEventRouter; +class JsEventDetails; +class JsEventHandler; -// Routes SyncManager events to a JsEventRouter. +// Routes SyncManager events to a JsEventHandler. class JsSyncManagerObserver : public sync_api::SyncManager::Observer { public: - // |parent_router| must be non-NULL and must outlive this object. - explicit JsSyncManagerObserver(JsEventRouter* parent_router); - + JsSyncManagerObserver(); virtual ~JsSyncManagerObserver(); + void SetJsEventHandler(const WeakHandle<JsEventHandler>& event_handler); + // sync_api::SyncManager::Observer implementation. virtual void OnChangesApplied( syncable::ModelType model_type, @@ -38,14 +44,18 @@ class JsSyncManagerObserver : public sync_api::SyncManager::Observer { virtual void OnPassphraseAccepted(const std::string& bootstrap_token); virtual void OnEncryptionComplete( const syncable::ModelTypeSet& encrypted_types); - virtual void OnInitializationComplete(); + virtual void OnInitializationComplete( + const WeakHandle<JsBackend>& js_backend); virtual void OnStopSyncingPermanently(); virtual void OnClearServerDataSucceeded(); virtual void OnClearServerDataFailed(); virtual void OnMigrationNeededForTypes(const syncable::ModelTypeSet& types); private: - JsEventRouter* parent_router_; + void HandleJsEvent(const tracked_objects::Location& from_here, + const std::string& name, const JsEventDetails& details); + + WeakHandle<JsEventHandler> event_handler_; DISALLOW_COPY_AND_ASSIGN(JsSyncManagerObserver); }; diff --git a/chrome/browser/sync/js_sync_manager_observer_unittest.cc b/chrome/browser/sync/js_sync_manager_observer_unittest.cc index e71c059..74a2a07 100644 --- a/chrome/browser/sync/js_sync_manager_observer_unittest.cc +++ b/chrome/browser/sync/js_sync_manager_observer_unittest.cc @@ -7,6 +7,7 @@ #include <cstddef> #include "base/basictypes.h" +#include "base/message_loop.h" #include "base/tracked.h" #include "base/values.h" #include "chrome/browser/sync/engine/syncapi.h" @@ -15,6 +16,7 @@ #include "chrome/browser/sync/js_test_util.h" #include "chrome/browser/sync/sessions/session_state.h" #include "chrome/browser/sync/syncable/model_type.h" +#include "chrome/browser/sync/weak_handle.h" #include "chrome/test/sync/engine/test_user_share.h" #include "testing/gtest/include/gtest/gtest.h" @@ -26,32 +28,43 @@ using ::testing::StrictMock; class JsSyncManagerObserverTest : public testing::Test { protected: - JsSyncManagerObserverTest() : sync_manager_observer_(&mock_router_) {} + JsSyncManagerObserverTest() { + js_sync_manager_observer_.SetJsEventHandler( + mock_js_event_handler_.AsWeakHandle()); + } + + StrictMock<MockJsEventHandler> mock_js_event_handler_; + JsSyncManagerObserver js_sync_manager_observer_; + + void PumpLoop() { + message_loop_.RunAllPending(); + } - StrictMock<MockJsEventRouter> mock_router_; - JsSyncManagerObserver sync_manager_observer_; + private: + MessageLoop message_loop_; }; TEST_F(JsSyncManagerObserverTest, NoArgNotifiations) { InSequence dummy; - EXPECT_CALL(mock_router_, - RouteJsEvent("onInitializationComplete", - HasDetails(JsEventDetails()))); - EXPECT_CALL(mock_router_, - RouteJsEvent("onStopSyncingPermanently", - HasDetails(JsEventDetails()))); - EXPECT_CALL(mock_router_, - RouteJsEvent("onClearServerDataSucceeded", - HasDetails(JsEventDetails()))); - EXPECT_CALL(mock_router_, - RouteJsEvent("onClearServerDataFailed", - HasDetails(JsEventDetails()))); - - sync_manager_observer_.OnInitializationComplete(); - sync_manager_observer_.OnStopSyncingPermanently(); - sync_manager_observer_.OnClearServerDataSucceeded(); - sync_manager_observer_.OnClearServerDataFailed(); + EXPECT_CALL(mock_js_event_handler_, + HandleJsEvent("onInitializationComplete", + HasDetails(JsEventDetails()))); + EXPECT_CALL(mock_js_event_handler_, + HandleJsEvent("onStopSyncingPermanently", + HasDetails(JsEventDetails()))); + EXPECT_CALL(mock_js_event_handler_, + HandleJsEvent("onClearServerDataSucceeded", + HasDetails(JsEventDetails()))); + EXPECT_CALL(mock_js_event_handler_, + HandleJsEvent("onClearServerDataFailed", + HasDetails(JsEventDetails()))); + + js_sync_manager_observer_.OnInitializationComplete(WeakHandle<JsBackend>()); + js_sync_manager_observer_.OnStopSyncingPermanently(); + js_sync_manager_observer_.OnClearServerDataSucceeded(); + js_sync_manager_observer_.OnClearServerDataFailed(); + PumpLoop(); } TEST_F(JsSyncManagerObserverTest, OnChangesComplete) { @@ -63,15 +76,16 @@ TEST_F(JsSyncManagerObserverTest, OnChangesComplete) { expected_details.SetString( "modelType", syncable::ModelTypeToString(syncable::ModelTypeFromInt(i))); - EXPECT_CALL(mock_router_, - RouteJsEvent("onChangesComplete", + EXPECT_CALL(mock_js_event_handler_, + HandleJsEvent("onChangesComplete", HasDetailsAsDictionary(expected_details))); } for (int i = syncable::FIRST_REAL_MODEL_TYPE; i < syncable::MODEL_TYPE_COUNT; ++i) { - sync_manager_observer_.OnChangesComplete(syncable::ModelTypeFromInt(i)); + js_sync_manager_observer_.OnChangesComplete(syncable::ModelTypeFromInt(i)); } + PumpLoop(); } TEST_F(JsSyncManagerObserverTest, OnSyncCycleCompleted) { @@ -93,11 +107,12 @@ TEST_F(JsSyncManagerObserverTest, OnSyncCycleCompleted) { DictionaryValue expected_details; expected_details.Set("snapshot", snapshot.ToValue()); - EXPECT_CALL(mock_router_, - RouteJsEvent("onSyncCycleCompleted", + EXPECT_CALL(mock_js_event_handler_, + HandleJsEvent("onSyncCycleCompleted", HasDetailsAsDictionary(expected_details))); - sync_manager_observer_.OnSyncCycleCompleted(&snapshot); + js_sync_manager_observer_.OnSyncCycleCompleted(&snapshot); + PumpLoop(); } TEST_F(JsSyncManagerObserverTest, OnAuthError) { @@ -105,11 +120,12 @@ TEST_F(JsSyncManagerObserverTest, OnAuthError) { DictionaryValue expected_details; expected_details.Set("authError", error.ToValue()); - EXPECT_CALL(mock_router_, - RouteJsEvent("onAuthError", + EXPECT_CALL(mock_js_event_handler_, + HandleJsEvent("onAuthError", HasDetailsAsDictionary(expected_details))); - sync_manager_observer_.OnAuthError(error); + js_sync_manager_observer_.OnAuthError(error); + PumpLoop(); } TEST_F(JsSyncManagerObserverTest, OnPassphraseRequired) { @@ -135,27 +151,28 @@ TEST_F(JsSyncManagerObserverTest, OnPassphraseRequired) { sync_api::PassphraseRequiredReasonToString( sync_api::REASON_SET_PASSPHRASE_FAILED)); - EXPECT_CALL(mock_router_, - RouteJsEvent("onPassphraseRequired", + EXPECT_CALL(mock_js_event_handler_, + HandleJsEvent("onPassphraseRequired", HasDetailsAsDictionary( reason_passphrase_not_required_details))); - EXPECT_CALL(mock_router_, - RouteJsEvent("onPassphraseRequired", + EXPECT_CALL(mock_js_event_handler_, + HandleJsEvent("onPassphraseRequired", HasDetailsAsDictionary(reason_encryption_details))); - EXPECT_CALL(mock_router_, - RouteJsEvent("onPassphraseRequired", + EXPECT_CALL(mock_js_event_handler_, + HandleJsEvent("onPassphraseRequired", HasDetailsAsDictionary(reason_decryption_details))); - EXPECT_CALL(mock_router_, - RouteJsEvent("onPassphraseRequired", + EXPECT_CALL(mock_js_event_handler_, + HandleJsEvent("onPassphraseRequired", HasDetailsAsDictionary( reason_set_passphrase_failed_details))); - sync_manager_observer_.OnPassphraseRequired( + js_sync_manager_observer_.OnPassphraseRequired( sync_api::REASON_PASSPHRASE_NOT_REQUIRED); - sync_manager_observer_.OnPassphraseRequired(sync_api::REASON_ENCRYPTION); - sync_manager_observer_.OnPassphraseRequired(sync_api::REASON_DECRYPTION); - sync_manager_observer_.OnPassphraseRequired( + js_sync_manager_observer_.OnPassphraseRequired(sync_api::REASON_ENCRYPTION); + js_sync_manager_observer_.OnPassphraseRequired(sync_api::REASON_DECRYPTION); + js_sync_manager_observer_.OnPassphraseRequired( sync_api::REASON_SET_PASSPHRASE_FAILED); + PumpLoop(); } TEST_F(JsSyncManagerObserverTest, SensitiveNotifiations) { @@ -164,16 +181,17 @@ TEST_F(JsSyncManagerObserverTest, SensitiveNotifiations) { DictionaryValue redacted_bootstrap_token_details; redacted_bootstrap_token_details.SetString("bootstrapToken", "<redacted>"); - EXPECT_CALL(mock_router_, - RouteJsEvent("onUpdatedToken", + EXPECT_CALL(mock_js_event_handler_, + HandleJsEvent("onUpdatedToken", HasDetailsAsDictionary(redacted_token_details))); - EXPECT_CALL(mock_router_, - RouteJsEvent( + EXPECT_CALL(mock_js_event_handler_, + HandleJsEvent( "onPassphraseAccepted", HasDetailsAsDictionary(redacted_bootstrap_token_details))); - sync_manager_observer_.OnUpdatedToken("sensitive_token"); - sync_manager_observer_.OnPassphraseAccepted("sensitive_token"); + js_sync_manager_observer_.OnUpdatedToken("sensitive_token"); + js_sync_manager_observer_.OnPassphraseAccepted("sensitive_token"); + PumpLoop(); } TEST_F(JsSyncManagerObserverTest, OnEncryptionComplete) { @@ -190,11 +208,12 @@ TEST_F(JsSyncManagerObserverTest, OnEncryptionComplete) { syncable::ModelTypeToString(type))); } - EXPECT_CALL(mock_router_, - RouteJsEvent("onEncryptionComplete", + EXPECT_CALL(mock_js_event_handler_, + HandleJsEvent("onEncryptionComplete", HasDetailsAsDictionary(expected_details))); - sync_manager_observer_.OnEncryptionComplete(encrypted_types); + js_sync_manager_observer_.OnEncryptionComplete(encrypted_types); + PumpLoop(); } TEST_F(JsSyncManagerObserverTest, OnMigrationNeededForTypes) { @@ -211,11 +230,12 @@ TEST_F(JsSyncManagerObserverTest, OnMigrationNeededForTypes) { syncable::ModelTypeToString(type))); } - EXPECT_CALL(mock_router_, - RouteJsEvent("onMigrationNeededForTypes", + EXPECT_CALL(mock_js_event_handler_, + HandleJsEvent("onMigrationNeededForTypes", HasDetailsAsDictionary(expected_details))); - sync_manager_observer_.OnMigrationNeededForTypes(types); + js_sync_manager_observer_.OnMigrationNeededForTypes(types); + PumpLoop(); } namespace { @@ -289,8 +309,8 @@ TEST_F(JsSyncManagerObserverTest, OnChangesApplied) { sync_api::ReadTransaction trans(FROM_HERE, test_user_share.user_share()); expected_changes->Append(changes[j].ToValue(&trans)); } - EXPECT_CALL(mock_router_, - RouteJsEvent("onChangesApplied", + EXPECT_CALL(mock_js_event_handler_, + HandleJsEvent("onChangesApplied", HasDetailsAsDictionary(expected_details))); } @@ -298,12 +318,13 @@ TEST_F(JsSyncManagerObserverTest, OnChangesApplied) { for (int i = syncable::AUTOFILL_PROFILE; i < syncable::MODEL_TYPE_COUNT; ++i) { sync_api::ReadTransaction trans(FROM_HERE, test_user_share.user_share()); - sync_manager_observer_.OnChangesApplied(syncable::ModelTypeFromInt(i), - &trans, &changes[i], - syncable::MODEL_TYPE_COUNT - i); + js_sync_manager_observer_.OnChangesApplied(syncable::ModelTypeFromInt(i), + &trans, &changes[i], + syncable::MODEL_TYPE_COUNT - i); } test_user_share.TearDown(); + PumpLoop(); } } // namespace diff --git a/chrome/browser/sync/js_test_util.cc b/chrome/browser/sync/js_test_util.cc index 7884ea2..eea7576 100644 --- a/chrome/browser/sync/js_test_util.cc +++ b/chrome/browser/sync/js_test_util.cc @@ -109,17 +109,29 @@ MockJsBackend::MockJsBackend() {} MockJsBackend::~MockJsBackend() {} -MockJsFrontend::MockJsFrontend() {} +WeakHandle<JsBackend> MockJsBackend::AsWeakHandle() { + return WeakHandle<JsBackend>(AsWeakPtr()); +} + +MockJsController::MockJsController() {} -MockJsFrontend::~MockJsFrontend() {} +MockJsController::~MockJsController() {} MockJsEventHandler::MockJsEventHandler() {} +WeakHandle<JsEventHandler> MockJsEventHandler::AsWeakHandle() { + return WeakHandle<JsEventHandler>(AsWeakPtr()); +} + MockJsEventHandler::~MockJsEventHandler() {} -MockJsEventRouter::MockJsEventRouter() {} +MockJsReplyHandler::MockJsReplyHandler() {} + +MockJsReplyHandler::~MockJsReplyHandler() {} -MockJsEventRouter::~MockJsEventRouter() {} +WeakHandle<JsReplyHandler> MockJsReplyHandler::AsWeakHandle() { + return WeakHandle<JsReplyHandler>(AsWeakPtr()); +} } // namespace browser_sync diff --git a/chrome/browser/sync/js_test_util.h b/chrome/browser/sync/js_test_util.h index e9a6ca3..6ac1a99 100644 --- a/chrome/browser/sync/js_test_util.h +++ b/chrome/browser/sync/js_test_util.h @@ -9,10 +9,12 @@ #include <ostream> #include <string> +#include "base/memory/weak_ptr.h" #include "chrome/browser/sync/js_backend.h" -#include "chrome/browser/sync/js_frontend.h" +#include "chrome/browser/sync/js_controller.h" #include "chrome/browser/sync/js_event_handler.h" -#include "chrome/browser/sync/js_event_router.h" +#include "chrome/browser/sync/js_reply_handler.h" +#include "chrome/browser/sync/weak_handle.h" #include "testing/gmock/include/gmock/gmock.h" namespace base { @@ -31,7 +33,7 @@ void PrintTo(const JsEventDetails& details, ::std::ostream* os); // A gmock matcher for JsArgList. Use like: // -// EXPECT_CALL(mock, HandleJsMessageReply("foo", HasArgs(expected_args))); +// EXPECT_CALL(mock, HandleJsReply("foo", HasArgs(expected_args))); ::testing::Matcher<const JsArgList&> HasArgs(const JsArgList& expected_args); // Like HasArgs() but takes a ListValue instead. @@ -50,51 +52,56 @@ void PrintTo(const JsEventDetails& details, ::std::ostream* os); // Mocks. -class MockJsBackend : public JsBackend { +class MockJsBackend : public JsBackend, + public base::SupportsWeakPtr<MockJsBackend> { public: MockJsBackend(); - ~MockJsBackend(); + virtual ~MockJsBackend(); - MOCK_METHOD1(SetParentJsEventRouter, void(JsEventRouter*)); - MOCK_METHOD0(RemoveParentJsEventRouter, void()); - MOCK_CONST_METHOD0(GetParentJsEventRouter, const JsEventRouter*()); - MOCK_METHOD3(ProcessMessage, void(const ::std::string&, const JsArgList&, - const JsEventHandler*)); + WeakHandle<JsBackend> AsWeakHandle(); + + MOCK_METHOD1(SetJsEventHandler, void(const WeakHandle<JsEventHandler>&)); + MOCK_METHOD3(ProcessJsMessage, void(const ::std::string&, const JsArgList&, + const WeakHandle<JsReplyHandler>&)); }; -class MockJsFrontend : public JsFrontend { +class MockJsController : public JsController, + public base::SupportsWeakPtr<MockJsController> { public: - MockJsFrontend(); - ~MockJsFrontend(); + MockJsController(); + virtual ~MockJsController(); - MOCK_METHOD1(AddHandler, void(JsEventHandler*)); - MOCK_METHOD1(RemoveHandler, void(JsEventHandler*)); - MOCK_METHOD3(ProcessMessage, + MOCK_METHOD1(AddJsEventHandler, void(JsEventHandler*)); + MOCK_METHOD1(RemoveJsEventHandler, void(JsEventHandler*)); + MOCK_METHOD3(ProcessJsMessage, void(const ::std::string&, const JsArgList&, - const JsEventHandler*)); + const WeakHandle<JsReplyHandler>&)); }; -class MockJsEventHandler : public JsEventHandler { +class MockJsEventHandler + : public JsEventHandler, + public base::SupportsWeakPtr<MockJsEventHandler> { public: MockJsEventHandler(); - ~MockJsEventHandler(); + virtual ~MockJsEventHandler(); + + WeakHandle<JsEventHandler> AsWeakHandle(); MOCK_METHOD2(HandleJsEvent, void(const ::std::string&, const JsEventDetails&)); - MOCK_METHOD2(HandleJsMessageReply, - void(const ::std::string&, const JsArgList&)); }; -class MockJsEventRouter : public JsEventRouter { +class MockJsReplyHandler + : public JsReplyHandler, + public base::SupportsWeakPtr<MockJsReplyHandler> { public: - MockJsEventRouter(); - ~MockJsEventRouter(); + MockJsReplyHandler(); + virtual ~MockJsReplyHandler(); - MOCK_METHOD2(RouteJsEvent, - void(const ::std::string&, const JsEventDetails&)); - MOCK_METHOD3(RouteJsMessageReply, - void(const ::std::string&, const JsArgList&, - const JsEventHandler*)); + WeakHandle<JsReplyHandler> AsWeakHandle(); + + MOCK_METHOD2(HandleJsReply, + void(const ::std::string&, const JsArgList&)); }; } // namespace browser_sync diff --git a/chrome/browser/sync/js_transaction_observer.cc b/chrome/browser/sync/js_transaction_observer.cc index 7eefaaf..e0700fc 100644 --- a/chrome/browser/sync/js_transaction_observer.cc +++ b/chrome/browser/sync/js_transaction_observer.cc @@ -11,20 +11,21 @@ #include "base/tracked.h" #include "base/values.h" #include "chrome/browser/sync/js_event_details.h" -#include "chrome/browser/sync/js_event_router.h" +#include "chrome/browser/sync/js_event_handler.h" namespace browser_sync { -JsTransactionObserver::JsTransactionObserver( - JsEventRouter* parent_router) - : parent_router_(parent_router) { - DCHECK(parent_router_); -} +JsTransactionObserver::JsTransactionObserver() {} JsTransactionObserver::~JsTransactionObserver() { DCHECK(non_thread_safe_.CalledOnValidThread()); } +void JsTransactionObserver::SetJsEventHandler( + const WeakHandle<JsEventHandler>& event_handler) { + event_handler_ = event_handler; +} + namespace { std::string GetLocationString(const tracked_objects::Location& location) { @@ -40,11 +41,13 @@ void JsTransactionObserver::OnTransactionStart( const tracked_objects::Location& location, const syncable::WriterTag& writer) { DCHECK(non_thread_safe_.CalledOnValidThread()); + if (!event_handler_.IsInitialized()) { + return; + } DictionaryValue details; details.SetString("location", GetLocationString(location)); details.SetString("writer", syncable::WriterTagToString(writer)); - parent_router_->RouteJsEvent("onTransactionStart", - JsEventDetails(&details)); + HandleJsEvent(FROM_HERE, "onTransactionStart", JsEventDetails(&details)); } void JsTransactionObserver::OnTransactionMutate( @@ -53,25 +56,40 @@ void JsTransactionObserver::OnTransactionMutate( const syncable::EntryKernelMutationSet& mutations, const syncable::ModelTypeBitSet& models_with_changes) { DCHECK(non_thread_safe_.CalledOnValidThread()); + if (!event_handler_.IsInitialized()) { + return; + } DictionaryValue details; details.SetString("location", GetLocationString(location)); details.SetString("writer", syncable::WriterTagToString(writer)); details.Set("mutations", syncable::EntryKernelMutationSetToValue(mutations)); details.Set("modelsWithChanges", syncable::ModelTypeBitSetToValue(models_with_changes)); - parent_router_->RouteJsEvent("onTransactionMutate", - JsEventDetails(&details)); + HandleJsEvent(FROM_HERE, "onTransactionMutate", JsEventDetails(&details)); } void JsTransactionObserver::OnTransactionEnd( const tracked_objects::Location& location, const syncable::WriterTag& writer) { DCHECK(non_thread_safe_.CalledOnValidThread()); + if (!event_handler_.IsInitialized()) { + return; + } DictionaryValue details; details.SetString("location", GetLocationString(location)); details.SetString("writer", syncable::WriterTagToString(writer)); - parent_router_->RouteJsEvent("onTransactionEnd", - JsEventDetails(&details)); + HandleJsEvent(FROM_HERE, "onTransactionEnd", JsEventDetails(&details)); +} + +void JsTransactionObserver::HandleJsEvent( + const tracked_objects::Location& from_here, + const std::string& name, const JsEventDetails& details) { + if (!event_handler_.IsInitialized()) { + NOTREACHED(); + return; + } + event_handler_.Call(from_here, + &JsEventHandler::HandleJsEvent, name, details); } } // namespace browser_sync diff --git a/chrome/browser/sync/js_transaction_observer.h b/chrome/browser/sync/js_transaction_observer.h index bf08390..bf59a6d 100644 --- a/chrome/browser/sync/js_transaction_observer.h +++ b/chrome/browser/sync/js_transaction_observer.h @@ -12,19 +12,26 @@ #include "base/compiler_specific.h" #include "base/threading/non_thread_safe.h" #include "chrome/browser/sync/syncable/transaction_observer.h" +#include "chrome/browser/sync/weak_handle.h" + +namespace tracked_objects { +class Location; +} // namespace tracked_objects namespace browser_sync { -class JsEventRouter; +class JsEventDetails; +class JsEventHandler; -// Routes SyncManager events to a JsEventRouter. +// Routes SyncManager events to a JsEventHandler. class JsTransactionObserver : public syncable::TransactionObserver { public: - // |parent_router| must be non-NULL and must outlive this object. - explicit JsTransactionObserver(JsEventRouter* parent_router); + JsTransactionObserver(); virtual ~JsTransactionObserver(); + void SetJsEventHandler(const WeakHandle<JsEventHandler>& event_handler); + // syncable::TransactionObserver implementation. virtual void OnTransactionStart( const tracked_objects::Location& location, @@ -40,7 +47,11 @@ class JsTransactionObserver : public syncable::TransactionObserver { private: base::NonThreadSafe non_thread_safe_; - JsEventRouter* parent_router_; + WeakHandle<JsEventHandler> event_handler_; + + void HandleJsEvent( + const tracked_objects::Location& from_here, + const std::string& name, const JsEventDetails& details); DISALLOW_COPY_AND_ASSIGN(JsTransactionObserver); }; diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc index 061ded4..1562d79 100644 --- a/chrome/browser/sync/profile_sync_service.cc +++ b/chrome/browser/sync/profile_sync_service.cc @@ -54,7 +54,12 @@ using browser_sync::ChangeProcessor; using browser_sync::DataTypeController; using browser_sync::DataTypeManager; +using browser_sync::JsBackend; +using browser_sync::JsController; +using browser_sync::JsEventDetails; +using browser_sync::JsEventHandler; using browser_sync::SyncBackendHost; +using browser_sync::WeakHandle; using sync_api::SyncCredentials; typedef GoogleServiceAuthError AuthError; @@ -356,11 +361,13 @@ void ProfileSyncService::InitializeBackend(bool delete_sync_data_folder) { scoped_refptr<net::URLRequestContextGetter> request_context_getter( profile_->GetRequestContext()); - backend_->Initialize(this, - sync_service_url_, - types, - credentials, - delete_sync_data_folder); + backend_->Initialize( + this, + WeakHandle<JsEventHandler>(sync_js_controller_.AsWeakPtr()), + sync_service_url_, + types, + credentials, + delete_sync_data_folder); } void ProfileSyncService::CreateBackend() { @@ -419,7 +426,7 @@ void ProfileSyncService::Shutdown(bool sync_disabled) { // Shutdown the migrator before the backend to ensure it doesn't pull a null // snapshot. migrator_.reset(); - js_event_handlers_.RemoveBackend(); + sync_js_controller_.AttachJsBackend(WeakHandle<JsBackend>()); // Move aside the backend so nobody else tries to use it while we are // shutting it down. @@ -486,8 +493,8 @@ void ProfileSyncService::NotifyObservers() { FOR_EACH_OBSERVER(Observer, observers_, OnStateChanged()); // TODO(akalin): Make an Observer subclass that listens and does the // event routing. - js_event_handlers_.RouteJsEvent( - "onServiceStateChanged", browser_sync::JsEventDetails()); + sync_js_controller_.HandleJsEvent( + "onServiceStateChanged", JsEventDetails()); } // static @@ -550,7 +557,8 @@ void ProfileSyncService::OnUnrecoverableError( &ProfileSyncService::Shutdown, true)); } -void ProfileSyncService::OnBackendInitialized(bool success) { +void ProfileSyncService::OnBackendInitialized( + const WeakHandle<JsBackend>& js_backend, bool success) { if (!success) { // If backend initialization failed, abort. We only want to blow away // state (DBs, etc) if this was a first-time scenario that failed. @@ -561,7 +569,7 @@ void ProfileSyncService::OnBackendInitialized(bool success) { backend_initialized_ = true; - js_event_handlers_.SetBackend(backend_->GetJsBackend()); + sync_js_controller_.AttachJsBackend(js_backend); // The very first time the backend initializes is effectively the first time // we can say we successfully "synced". last_synced_time_ will only be null @@ -1344,8 +1352,8 @@ bool ProfileSyncService::HasObserver(Observer* observer) const { return observers_.HasObserver(observer); } -browser_sync::JsFrontend* ProfileSyncService::GetJsFrontend() { - return &js_event_handlers_; +base::WeakPtr<JsController> ProfileSyncService::GetJsController() { + return sync_js_controller_.AsWeakPtr(); } void ProfileSyncService::SyncEvent(SyncEventCodes code) { diff --git a/chrome/browser/sync/profile_sync_service.h b/chrome/browser/sync/profile_sync_service.h index cef7d618..b24503b 100644 --- a/chrome/browser/sync/profile_sync_service.h +++ b/chrome/browser/sync/profile_sync_service.h @@ -9,8 +9,10 @@ #include <string> #include "base/basictypes.h" +#include "base/compiler_specific.h" #include "base/gtest_prod_util.h" #include "base/memory/scoped_ptr.h" +#include "base/memory/weak_ptr.h" #include "base/observer_list.h" #include "base/string16.h" #include "base/task.h" @@ -21,8 +23,8 @@ #include "chrome/browser/sync/engine/model_safe_worker.h" #include "chrome/browser/sync/glue/data_type_controller.h" #include "chrome/browser/sync/glue/sync_backend_host.h" -#include "chrome/browser/sync/js_event_handler_list.h" #include "chrome/browser/sync/profile_sync_service_observer.h" +#include "chrome/browser/sync/sync_js_controller.h" #include "chrome/browser/sync/sync_setup_wizard.h" #include "chrome/browser/sync/syncable/model_type.h" #include "chrome/browser/sync/unrecoverable_error_handler.h" @@ -43,7 +45,7 @@ namespace browser_sync { class BackendMigrator; class ChangeProcessor; class DataTypeManager; -class JsFrontend; +class JsController; class SessionModelAssociator; namespace sessions { struct SyncSessionSnapshot; } } @@ -189,19 +191,23 @@ class ProfileSyncService : public browser_sync::SyncFrontend, virtual void SetSyncSetupCompleted(); // SyncFrontend implementation. - virtual void OnBackendInitialized(bool success); - virtual void OnSyncCycleCompleted(); - virtual void OnAuthError(); - virtual void OnStopSyncingPermanently(); - virtual void OnClearServerDataFailed(); - virtual void OnClearServerDataTimeout(); - virtual void OnClearServerDataSucceeded(); - virtual void OnPassphraseRequired(sync_api::PassphraseRequiredReason reason); - virtual void OnPassphraseAccepted(); + virtual void OnBackendInitialized( + const browser_sync::WeakHandle<browser_sync::JsBackend>& js_backend, + bool success) OVERRIDE; + virtual void OnSyncCycleCompleted() OVERRIDE; + virtual void OnAuthError() OVERRIDE; + virtual void OnStopSyncingPermanently() OVERRIDE; + virtual void OnClearServerDataFailed() OVERRIDE; + virtual void OnClearServerDataSucceeded() OVERRIDE; + virtual void OnPassphraseRequired( + sync_api::PassphraseRequiredReason reason) OVERRIDE; + virtual void OnPassphraseAccepted() OVERRIDE; virtual void OnEncryptionComplete( - const syncable::ModelTypeSet& encrypted_types); + const syncable::ModelTypeSet& encrypted_types) OVERRIDE; virtual void OnMigrationNeededForTypes( - const syncable::ModelTypeSet& types); + const syncable::ModelTypeSet& types) OVERRIDE; + + void OnClearServerDataTimeout(); // Called when a user enters credentials through UI. virtual void OnUserSubmittedAuth(const std::string& username, @@ -324,10 +330,9 @@ class ProfileSyncService : public browser_sync::SyncFrontend, // Returns true if |observer| has already been added as an observer. bool HasObserver(Observer* observer) const; - // Returns a pointer to the service's JsFrontend (which is owned by - // the service). Never returns NULL. Overrideable for testing - // purposes. - virtual browser_sync::JsFrontend* GetJsFrontend(); + // Returns a weak pointer to the service's JsController. + // Overrideable for testing purposes. + virtual base::WeakPtr<browser_sync::JsController> GetJsController(); // Record stats on various events. static void SyncEvent(SyncEventCodes code); @@ -589,7 +594,7 @@ class ProfileSyncService : public browser_sync::SyncFrontend, ObserverList<Observer> observers_; - browser_sync::JsEventHandlerList js_event_handlers_; + browser_sync::SyncJsController sync_js_controller_; NotificationRegistrar registrar_; diff --git a/chrome/browser/sync/profile_sync_service_mock.h b/chrome/browser/sync/profile_sync_service_mock.h index 4464c3f..69785ff 100644 --- a/chrome/browser/sync/profile_sync_service_mock.h +++ b/chrome/browser/sync/profile_sync_service_mock.h @@ -8,6 +8,7 @@ #include <string> +#include "base/memory/weak_ptr.h" #include "base/string16.h" #include "chrome/browser/sync/glue/change_processor.h" #include "chrome/browser/sync/glue/data_type_controller.h" @@ -21,7 +22,9 @@ class ProfileSyncServiceMock : public ProfileSyncService { virtual ~ProfileSyncServiceMock(); MOCK_METHOD0(DisableForUser, void()); - MOCK_METHOD1(OnBackendInitialized, void(bool)); + MOCK_METHOD2(OnBackendInitialized, + void(const browser_sync::WeakHandle<browser_sync::JsBackend>&, + bool)); MOCK_METHOD0(OnSyncCycleCompleted, void()); MOCK_METHOD0(OnAuthError, void()); MOCK_METHOD4(OnUserSubmittedAuth, @@ -44,7 +47,7 @@ class ProfileSyncServiceMock : public ProfileSyncService { MOCK_METHOD0(InitializeBackend, void()); MOCK_METHOD1(AddObserver, void(Observer*)); MOCK_METHOD1(RemoveObserver, void(Observer*)); - MOCK_METHOD0(GetJsFrontend, browser_sync::JsFrontend*()); + MOCK_METHOD0(GetJsController, base::WeakPtr<browser_sync::JsController>()); MOCK_CONST_METHOD0(HasSyncSetupCompleted, bool()); MOCK_METHOD1(ChangePreferredDataTypes, diff --git a/chrome/browser/sync/profile_sync_service_unittest.cc b/chrome/browser/sync/profile_sync_service_unittest.cc index dd76d40..bd55a12 100644 --- a/chrome/browser/sync/profile_sync_service_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_unittest.cc @@ -145,30 +145,19 @@ TEST_F(ProfileSyncServiceTest, AbortedByShutdown) { service_.reset(); } -TEST_F(ProfileSyncServiceTest, JsFrontendHandlersBasic) { +TEST_F(ProfileSyncServiceTest, JsControllerHandlersBasic) { StartSyncService(); - - StrictMock<MockJsEventHandler> event_handler; - - SyncBackendHostForProfileSyncTest* test_backend = - service_->GetBackendForTest(); - EXPECT_TRUE(service_->sync_initialized()); - ASSERT_TRUE(test_backend != NULL); - ASSERT_TRUE(test_backend->GetJsBackend() != NULL); - EXPECT_EQ(NULL, test_backend->GetJsBackend()->GetParentJsEventRouter()); + EXPECT_TRUE(service_->GetBackendForTest() != NULL); - JsFrontend* js_backend = service_->GetJsFrontend(); - js_backend->AddHandler(&event_handler); - ASSERT_TRUE(test_backend->GetJsBackend() != NULL); - EXPECT_TRUE(test_backend->GetJsBackend()->GetParentJsEventRouter() != NULL); - - js_backend->RemoveHandler(&event_handler); - EXPECT_EQ(NULL, test_backend->GetJsBackend()->GetParentJsEventRouter()); + JsController* js_controller = service_->GetJsController(); + StrictMock<MockJsEventHandler> event_handler; + js_controller->AddJsEventHandler(&event_handler); + js_controller->RemoveJsEventHandler(&event_handler); } TEST_F(ProfileSyncServiceTest, - JsFrontendHandlersDelayedBackendInitialization) { + JsControllerHandlersDelayedBackendInitialization) { StartSyncServiceAndSetInitialSyncEnded(true, false, false, true); StrictMock<MockJsEventHandler> event_handler; @@ -193,153 +182,62 @@ TEST_F(ProfileSyncServiceTest, EXPECT_EQ(NULL, service_->GetBackendForTest()); EXPECT_FALSE(service_->sync_initialized()); - JsFrontend* js_backend = service_->GetJsFrontend(); - js_backend->AddHandler(&event_handler); + JsController* js_controller = service_->GetJsController(); + js_controller->AddJsEventHandler(&event_handler); // Since we're doing synchronous initialization, backend should be // initialized by this call. profile_->GetTokenService()->IssueAuthTokenForTest( GaiaConstants::kSyncService, "token"); - - SyncBackendHostForProfileSyncTest* test_backend = - service_->GetBackendForTest(); - EXPECT_TRUE(service_->sync_initialized()); - ASSERT_TRUE(test_backend != NULL); - ASSERT_TRUE(test_backend->GetJsBackend() != NULL); - EXPECT_TRUE(test_backend->GetJsBackend()->GetParentJsEventRouter() != NULL); - - js_backend->RemoveHandler(&event_handler); - EXPECT_EQ(NULL, test_backend->GetJsBackend()->GetParentJsEventRouter()); + js_controller->RemoveJsEventHandler(&event_handler); } -TEST_F(ProfileSyncServiceTest, JsFrontendProcessMessageBasic) { +TEST_F(ProfileSyncServiceTest, JsControllerProcessJsMessageBasic) { StartSyncService(); - StrictMock<MockJsEventHandler> event_handler; - // For some reason, these events may or may not fire. - EXPECT_CALL(event_handler, HandleJsEvent("onChangesApplied", _)) - .Times(AtMost(1)); - EXPECT_CALL(event_handler, HandleJsEvent("onChangesComplete", _)) - .Times(AtMost(1)); - EXPECT_CALL(event_handler, HandleJsEvent("onNotificationStateChange", _)) - .Times(AtMost(1)); + StrictMock<MockJsReplyHandler> reply_handler; ListValue arg_list1; - arg_list1.Append(Value::CreateBooleanValue(true)); - arg_list1.Append(Value::CreateIntegerValue(5)); + arg_list1.Append(Value::CreateBooleanValue(false)); JsArgList args1(&arg_list1); - EXPECT_CALL(event_handler, - HandleJsMessageReply("testMessage1", HasArgs(args1))); - - ListValue arg_list2; - arg_list2.Append(Value::CreateStringValue("test")); - arg_list2.Append(arg_list1.DeepCopy()); - JsArgList args2(&arg_list2); - EXPECT_CALL(event_handler, - HandleJsMessageReply("delayTestMessage2", HasArgs(args2))); - - ListValue arg_list3; - arg_list3.Append(arg_list1.DeepCopy()); - arg_list3.Append(arg_list2.DeepCopy()); - JsArgList args3(&arg_list3); - - JsFrontend* js_backend = service_->GetJsFrontend(); - - // Never replied to. - js_backend->ProcessMessage("notRepliedTo", args3, &event_handler); - - // Replied to later. - js_backend->ProcessMessage("delayTestMessage2", args2, &event_handler); + EXPECT_CALL(reply_handler, + HandleJsReply("getNotificationState", HasArgs(args1))); - js_backend->AddHandler(&event_handler); - - // Replied to immediately. - js_backend->ProcessMessage("testMessage1", args1, &event_handler); - - // Fires off reply for delayTestMessage2. - ui_loop_.RunAllPending(); - - // Never replied to. - js_backend->ProcessMessage("delayNotRepliedTo", args3, &event_handler); - - js_backend->RemoveHandler(&event_handler); + { + JsController* js_controller = service_->GetJsController(); + js_controller->ProcessJsMessage("getNotificationState", args1, + reply_handler.AsWeakHandle()); + } + // This forces the sync thread to process the message and reply. + service_.reset(); ui_loop_.RunAllPending(); - - // Never replied to. - js_backend->ProcessMessage("notRepliedTo", args3, &event_handler); } TEST_F(ProfileSyncServiceTest, - JsFrontendProcessMessageBasicDelayedBackendInitialization) { + JsControllerProcessJsMessageBasicDelayedBackendInitialization) { StartSyncServiceAndSetInitialSyncEnded(true, false, false, true); - StrictMock<MockJsEventHandler> event_handler; - EXPECT_CALL(event_handler, - HandleJsEvent("onTransactionStart", _)).Times(AnyNumber()); - EXPECT_CALL(event_handler, - HandleJsEvent("onTransactionEnd", _)).Times(AnyNumber()); - // For some reason, these events may or may not fire. - EXPECT_CALL(event_handler, HandleJsEvent("onChangesApplied", _)) - .Times(AtMost(1)); - EXPECT_CALL(event_handler, HandleJsEvent("onChangesComplete", _)) - .Times(AtMost(1)); - EXPECT_CALL(event_handler, HandleJsEvent("onNotificationStateChange", _)) - .Times(AtMost(1)); + StrictMock<MockJsReplyHandler> reply_handler; ListValue arg_list1; - arg_list1.Append(Value::CreateBooleanValue(true)); - arg_list1.Append(Value::CreateIntegerValue(5)); + arg_list1.Append(Value::CreateBooleanValue(false)); JsArgList args1(&arg_list1); - EXPECT_CALL(event_handler, - HandleJsMessageReply("testMessage1", HasArgs(args1))); - - ListValue arg_list2; - arg_list2.Append(Value::CreateStringValue("test")); - arg_list2.Append(arg_list1.DeepCopy()); - JsArgList args2(&arg_list2); - EXPECT_CALL(event_handler, - HandleJsMessageReply("testMessage2", HasArgs(args2))); + EXPECT_CALL(reply_handler, + HandleJsReply("getNotificationState", HasArgs(args1))); - ListValue arg_list3; - arg_list3.Append(arg_list1.DeepCopy()); - arg_list3.Append(arg_list2.DeepCopy()); - JsArgList args3(&arg_list3); - EXPECT_CALL(event_handler, - HandleJsMessageReply("delayTestMessage3", HasArgs(args3))); - - const JsEventDetails kNoDetails; - - EXPECT_CALL(event_handler, HandleJsEvent("onServiceStateChanged", - HasDetails(kNoDetails))).Times(AtLeast(3)); - - JsFrontend* js_backend = service_->GetJsFrontend(); - - // We expect a reply for this message, even though its sent before - // |event_handler| is added as a handler. - js_backend->ProcessMessage("testMessage1", args1, &event_handler); - - js_backend->AddHandler(&event_handler); - - js_backend->ProcessMessage("testMessage2", args2, &event_handler); - js_backend->ProcessMessage("delayTestMessage3", args3, &event_handler); + { + JsController* js_controller = service_->GetJsController(); + js_controller->ProcessJsMessage("getNotificationState", + args1, reply_handler.AsWeakHandle()); + } - // Fires testMessage1 and testMessage2. profile_->GetTokenService()->IssueAuthTokenForTest( GaiaConstants::kSyncService, "token"); - // Fires delayTestMessage3. - ui_loop_.RunAllPending(); - - const JsArgList kNoArgs; - - js_backend->ProcessMessage("delayNotRepliedTo", kNoArgs, &event_handler); - - js_backend->RemoveHandler(&event_handler); - + // This forces the sync thread to process the message and reply. + service_.reset(); ui_loop_.RunAllPending(); - - js_backend->ProcessMessage("notRepliedTo", kNoArgs, &event_handler); } // Make sure that things still work if sync is not enabled, but some old sync diff --git a/chrome/browser/sync/sync_js_controller.cc b/chrome/browser/sync/sync_js_controller.cc new file mode 100644 index 0000000..166611c --- /dev/null +++ b/chrome/browser/sync/sync_js_controller.cc @@ -0,0 +1,83 @@ +// Copyright (c) 2011 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 "chrome/browser/sync/sync_js_controller.h" + +#include "chrome/browser/sync/js_backend.h" +#include "chrome/browser/sync/js_event_details.h" + +namespace browser_sync { + +SyncJsController::PendingJsMessage::PendingJsMessage( + const std::string& name, const JsArgList& args, + const WeakHandle<JsReplyHandler>& reply_handler) + : name(name), args(args), reply_handler(reply_handler) {} + +SyncJsController::PendingJsMessage::~PendingJsMessage() {} + +SyncJsController::SyncJsController() {} + +SyncJsController::~SyncJsController() { + AttachJsBackend(WeakHandle<JsBackend>()); +} + +void SyncJsController::AddJsEventHandler(JsEventHandler* event_handler) { + js_event_handlers_.AddObserver(event_handler); + UpdateBackendEventHandler(); +} + +void SyncJsController::RemoveJsEventHandler(JsEventHandler* event_handler) { + js_event_handlers_.RemoveObserver(event_handler); + UpdateBackendEventHandler(); +} + +void SyncJsController::AttachJsBackend( + const WeakHandle<JsBackend>& js_backend) { + js_backend_ = js_backend; + UpdateBackendEventHandler(); + + if (js_backend_.IsInitialized()) { + // Process any queued messages. + for (PendingJsMessageList::const_iterator it = + pending_js_messages_.begin(); + it != pending_js_messages_.end(); ++it) { + js_backend_.Call(FROM_HERE, &JsBackend::ProcessJsMessage, + it->name, it->args, it->reply_handler); + } + } +} + +void SyncJsController::ProcessJsMessage( + const std::string& name, const JsArgList& args, + const WeakHandle<JsReplyHandler>& reply_handler) { + if (js_backend_.IsInitialized()) { + js_backend_.Call(FROM_HERE, &JsBackend::ProcessJsMessage, + name, args, reply_handler); + } else { + pending_js_messages_.push_back( + PendingJsMessage(name, args, reply_handler)); + } +} + +void SyncJsController::HandleJsEvent(const std::string& name, + const JsEventDetails& details) { + FOR_EACH_OBSERVER(JsEventHandler, js_event_handlers_, + HandleJsEvent(name, details)); +} + +void SyncJsController::UpdateBackendEventHandler() { + if (js_backend_.IsInitialized()) { + // To avoid making the backend send useless events, we clear the + // event handler we pass to it if we don't have any event + // handlers. + WeakHandle<JsEventHandler> backend_event_handler = + (js_event_handlers_.size() > 0) ? + WeakHandle<JsEventHandler>(AsWeakPtr()) : + WeakHandle<JsEventHandler>(); + js_backend_.Call(FROM_HERE, &JsBackend::SetJsEventHandler, + backend_event_handler); + } +} + +} // namespace browser_sync diff --git a/chrome/browser/sync/sync_js_controller.h b/chrome/browser/sync/sync_js_controller.h new file mode 100644 index 0000000..0b31f11 --- /dev/null +++ b/chrome/browser/sync/sync_js_controller.h @@ -0,0 +1,81 @@ +// Copyright (c) 2011 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 CHROME_BROWSER_SYNC_SYNC_JS_CONTROLLER_H_ +#define CHROME_BROWSER_SYNC_SYNC_JS_CONTROLLER_H_ +#pragma once + +#include <string> +#include <vector> + +#include "base/basictypes.h" +#include "base/compiler_specific.h" +#include "base/memory/weak_ptr.h" +#include "base/observer_list.h" +#include "chrome/browser/sync/js_arg_list.h" +#include "chrome/browser/sync/js_controller.h" +#include "chrome/browser/sync/js_event_handler.h" +#include "chrome/browser/sync/weak_handle.h" + +namespace browser_sync { + +class JsBackend; + +// A class that mediates between the sync JsEventHandlers and the sync +// JsBackend. +class SyncJsController + : public JsController, public JsEventHandler, + public base::SupportsWeakPtr<SyncJsController> { + public: + SyncJsController(); + + virtual ~SyncJsController(); + + // Sets the backend to route all messages to (if initialized). + // Sends any queued-up messages if |backend| is initialized. + void AttachJsBackend(const WeakHandle<JsBackend>& js_backend); + + // JsController implementation. + virtual void AddJsEventHandler(JsEventHandler* event_handler) OVERRIDE; + virtual void RemoveJsEventHandler(JsEventHandler* event_handler) OVERRIDE; + // Queues up any messages that are sent when there is no attached + // initialized backend. + virtual void ProcessJsMessage( + const std::string& name, const JsArgList& args, + const WeakHandle<JsReplyHandler>& reply_handler) OVERRIDE; + + // JsEventHandler implementation. + virtual void HandleJsEvent(const std::string& name, + const JsEventDetails& details) OVERRIDE; + + private: + // A struct used to hold the arguments to ProcessJsMessage() for + // future invocation. + struct PendingJsMessage { + std::string name; + JsArgList args; + WeakHandle<JsReplyHandler> reply_handler; + + PendingJsMessage(const std::string& name, const JsArgList& args, + const WeakHandle<JsReplyHandler>& reply_handler); + + ~PendingJsMessage(); + }; + + typedef std::vector<PendingJsMessage> PendingJsMessageList; + + // Sets |js_backend_|'s event handler depending on how many + // underlying event handlers we have. + void UpdateBackendEventHandler(); + + WeakHandle<JsBackend> js_backend_; + ObserverList<JsEventHandler> js_event_handlers_; + PendingJsMessageList pending_js_messages_; + + DISALLOW_COPY_AND_ASSIGN(SyncJsController); +}; + +} // namespace browser_sync + +#endif // CHROME_BROWSER_SYNC_SYNC_JS_CONTROLLER_H_ diff --git a/chrome/browser/sync/sync_js_controller_unittest.cc b/chrome/browser/sync/sync_js_controller_unittest.cc new file mode 100644 index 0000000..1dd5ca2 --- /dev/null +++ b/chrome/browser/sync/sync_js_controller_unittest.cc @@ -0,0 +1,126 @@ +// Copyright (c) 2011 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 "chrome/browser/sync/sync_js_controller.h" + +#include "base/message_loop.h" +#include "base/values.h" +#include "chrome/browser/sync/js_arg_list.h" +#include "chrome/browser/sync/js_event_details.h" +#include "chrome/browser/sync/js_test_util.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace browser_sync { +namespace { + +using ::testing::_; +using ::testing::InSequence; +using ::testing::Mock; +using ::testing::StrictMock; + +class SyncJsControllerTest : public testing::Test { + protected: + void PumpLoop() { + message_loop_.RunAllPending(); + } + + private: + MessageLoop message_loop_; +}; + +TEST_F(SyncJsControllerTest, Messages) { + InSequence dummy; + // |mock_backend| needs to outlive |sync_js_controller|. + StrictMock<MockJsBackend> mock_backend; + SyncJsController sync_js_controller; + + ListValue arg_list1, arg_list2; + arg_list1.Append(Value::CreateBooleanValue(false)); + arg_list2.Append(Value::CreateIntegerValue(5)); + JsArgList args1(&arg_list1), args2(&arg_list2); + + // TODO(akalin): Write matchers for WeakHandle and use them here + // instead of _. + EXPECT_CALL(mock_backend, SetJsEventHandler(_)); + EXPECT_CALL(mock_backend, ProcessJsMessage("test1", HasArgs(args2), _)); + EXPECT_CALL(mock_backend, ProcessJsMessage("test2", HasArgs(args1), _)); + + sync_js_controller.AttachJsBackend(mock_backend.AsWeakHandle()); + sync_js_controller.ProcessJsMessage("test1", args2, + WeakHandle<JsReplyHandler>()); + sync_js_controller.ProcessJsMessage("test2", args1, + WeakHandle<JsReplyHandler>()); + PumpLoop(); + + // Let destructor of |sync_js_controller| call RemoveBackend(). +} + +TEST_F(SyncJsControllerTest, QueuedMessages) { + // |mock_backend| needs to outlive |sync_js_controller|. + StrictMock<MockJsBackend> mock_backend; + SyncJsController sync_js_controller; + + ListValue arg_list1, arg_list2; + arg_list1.Append(Value::CreateBooleanValue(false)); + arg_list2.Append(Value::CreateIntegerValue(5)); + JsArgList args1(&arg_list1), args2(&arg_list2); + + // Should queue messages. + sync_js_controller.ProcessJsMessage("test1", args2, + WeakHandle<JsReplyHandler>()); + sync_js_controller.ProcessJsMessage("test2", args1, + WeakHandle<JsReplyHandler>()); + + Mock::VerifyAndClearExpectations(&mock_backend); + + // TODO(akalin): Write matchers for WeakHandle and use them here + // instead of _. + EXPECT_CALL(mock_backend, SetJsEventHandler(_)); + EXPECT_CALL(mock_backend, ProcessJsMessage("test1", HasArgs(args2), _)); + EXPECT_CALL(mock_backend, ProcessJsMessage("test2", HasArgs(args1), _)); + + // Should call the queued messages. + sync_js_controller.AttachJsBackend(mock_backend.AsWeakHandle()); + PumpLoop(); + + // Should do nothing. + sync_js_controller.AttachJsBackend(WeakHandle<JsBackend>()); + PumpLoop(); + + // Should also do nothing. + sync_js_controller.AttachJsBackend(WeakHandle<JsBackend>()); + PumpLoop(); +} + +TEST_F(SyncJsControllerTest, Events) { + InSequence dummy; + SyncJsController sync_js_controller; + + DictionaryValue details_dict1, details_dict2; + details_dict1.SetString("foo", "bar"); + details_dict2.SetInteger("baz", 5); + JsEventDetails details1(&details_dict1), details2(&details_dict2); + + StrictMock<MockJsEventHandler> event_handler1, event_handler2; + EXPECT_CALL(event_handler1, HandleJsEvent("event", HasDetails(details1))); + EXPECT_CALL(event_handler2, HandleJsEvent("event", HasDetails(details1))); + EXPECT_CALL(event_handler1, + HandleJsEvent("anotherevent", HasDetails(details2))); + EXPECT_CALL(event_handler2, + HandleJsEvent("anotherevent", HasDetails(details2))); + + sync_js_controller.AddJsEventHandler(&event_handler1); + sync_js_controller.AddJsEventHandler(&event_handler2); + sync_js_controller.HandleJsEvent("event", details1); + sync_js_controller.HandleJsEvent("anotherevent", details2); + sync_js_controller.RemoveJsEventHandler(&event_handler1); + sync_js_controller.RemoveJsEventHandler(&event_handler2); + sync_js_controller.HandleJsEvent("droppedevent", details2); + + PumpLoop(); +} + +} // namespace +} // namespace browser_sync diff --git a/chrome/browser/sync/test_profile_sync_service.cc b/chrome/browser/sync/test_profile_sync_service.cc index 8549bfb..0beeac5 100644 --- a/chrome/browser/sync/test_profile_sync_service.cc +++ b/chrome/browser/sync/test_profile_sync_service.cc @@ -8,6 +8,7 @@ #include "chrome/browser/sync/engine/syncapi.h" #include "chrome/browser/sync/glue/data_type_controller.h" #include "chrome/browser/sync/glue/sync_backend_host.h" +#include "chrome/browser/sync/js_reply_handler.h" #include "chrome/browser/sync/profile_sync_factory.h" #include "chrome/browser/sync/signin_manager.h" #include "chrome/browser/sync/sessions/session_state.h" @@ -45,7 +46,6 @@ class FakeSigninManager : public SigninManager { namespace browser_sync { -using ::testing::_; SyncBackendHostForProfileSyncTest::SyncBackendHostForProfileSyncTest( Profile* profile, bool set_initial_sync_ended_on_init, @@ -57,16 +57,6 @@ SyncBackendHostForProfileSyncTest::SyncBackendHostForProfileSyncTest( SyncBackendHostForProfileSyncTest::~SyncBackendHostForProfileSyncTest() {} -void SyncBackendHostForProfileSyncTest::ConfigureDataTypes( - const DataTypeController::TypeMap& data_type_controllers, - const syncable::ModelTypeSet& types, - sync_api::ConfigureReason reason, - base::Callback<void(bool)> ready_task, - bool nigori_enabled) { - SyncBackendHost::ConfigureDataTypes(data_type_controllers, types, - reason, ready_task, nigori_enabled); -} - void SyncBackendHostForProfileSyncTest:: SimulateSyncCycleCompletedInitialSyncEnded( const tracked_objects::Location& location) { @@ -113,40 +103,6 @@ void SyncBackendHostForProfileSyncTest::InitCore( } } -JsBackend* SyncBackendHostForProfileSyncTest::GetJsBackend() { - // Return a non-NULL result only when the overridden function does. - if (SyncBackendHost::GetJsBackend()) { - return this; - } else { - NOTREACHED(); - return NULL; - } -} - -void SyncBackendHostForProfileSyncTest::SetParentJsEventRouter( - JsEventRouter* router) { - core_->SetParentJsEventRouter(router); -} - -void SyncBackendHostForProfileSyncTest::RemoveParentJsEventRouter() { - core_->RemoveParentJsEventRouter(); -} - -const JsEventRouter* - SyncBackendHostForProfileSyncTest::GetParentJsEventRouter() const { - return core_->GetParentJsEventRouter(); -} - -void SyncBackendHostForProfileSyncTest::ProcessMessage( - const std::string& name, const JsArgList& args, - const JsEventHandler* sender) { - if (name.find("delay") != name.npos) { - core_->RouteJsMessageReply(name, args, sender); - } else { - core_->RouteJsMessageReplyOnFrontendLoop(name, args, sender); - } -} - void SyncBackendHostForProfileSyncTest::StartConfiguration( Callback0::Type* callback) { scoped_ptr<Callback0::Type> scoped_callback(callback); @@ -223,7 +179,9 @@ void TestProfileSyncService::SetInitialSyncEndedForEnabledTypes() { } } -void TestProfileSyncService::OnBackendInitialized(bool success) { +void TestProfileSyncService::OnBackendInitialized( + const browser_sync::WeakHandle<browser_sync::JsBackend>& backend, + bool success) { bool send_passphrase_required = false; if (success) { // Set this so below code can access GetUserShare(). @@ -259,7 +217,7 @@ void TestProfileSyncService::OnBackendInitialized(bool success) { } } - ProfileSyncService::OnBackendInitialized(success); + ProfileSyncService::OnBackendInitialized(backend, success); if (success && send_passphrase_required) OnPassphraseRequired(sync_api::REASON_DECRYPTION); @@ -296,7 +254,3 @@ void TestProfileSyncService::CreateBackend() { synchronous_backend_initialization_, fail_initial_download_)); } - -std::string TestProfileSyncService::GetLsidForAuthBootstraping() { - return "foo"; -} diff --git a/chrome/browser/sync/test_profile_sync_service.h b/chrome/browser/sync/test_profile_sync_service.h index 2f0ecf0..56b4e95 100644 --- a/chrome/browser/sync/test_profile_sync_service.h +++ b/chrome/browser/sync/test_profile_sync_service.h @@ -10,7 +10,6 @@ #include "base/compiler_specific.h" #include "chrome/browser/sync/glue/data_type_manager_impl.h" -#include "chrome/browser/sync/js_backend.h" #include "chrome/browser/sync/profile_sync_service.h" #include "chrome/test/base/profile_mock.h" #include "chrome/test/sync/engine/test_id_factory.h" @@ -26,12 +25,7 @@ ACTION(ReturnNewDataTypeManager) { namespace browser_sync { -// Mocks out the SyncerThread operations (Pause/Resume) since no thread is -// running in these tests, and allows tests to provide a task on construction -// to set up initial nodes to mock out an actual server initial sync -// download. -class SyncBackendHostForProfileSyncTest - : public SyncBackendHost, public JsBackend { +class SyncBackendHostForProfileSyncTest : public SyncBackendHost { public: // |synchronous_init| causes initialization to block until the syncapi has // completed setting itself up and called us back. @@ -44,40 +38,22 @@ class SyncBackendHostForProfileSyncTest MOCK_METHOD1(RequestNudge, void(const tracked_objects::Location&)); - virtual void ConfigureDataTypes( - const DataTypeController::TypeMap& data_type_controllers, - const syncable::ModelTypeSet& types, - sync_api::ConfigureReason reason, - base::Callback<void(bool)> ready_task, - bool nigori_enabled); - // Called when a nudge comes in. void SimulateSyncCycleCompletedInitialSyncEnded( const tracked_objects::Location&); virtual sync_api::HttpPostProviderFactory* MakeHttpBridgeFactory( - const scoped_refptr<net::URLRequestContextGetter>& getter); - - virtual void InitCore(const Core::DoInitializeOptions& options); - - virtual JsBackend* GetJsBackend(); - - // JsBackend implementation. - virtual void SetParentJsEventRouter(JsEventRouter* router) OVERRIDE; - virtual void RemoveParentJsEventRouter() OVERRIDE; - virtual const JsEventRouter* GetParentJsEventRouter() const OVERRIDE; - // Fires an event identical to the message unless the message has - // "delay" as a prefix, in which case a task to fire the identical - // event is posted instead. - virtual void ProcessMessage(const std::string& name, const JsArgList& args, - const JsEventHandler* sender) OVERRIDE; + const scoped_refptr<net::URLRequestContextGetter>& getter) OVERRIDE; - virtual void StartConfiguration(Callback0::Type* callback); + virtual void StartConfiguration(Callback0::Type* callback) OVERRIDE; static void SetDefaultExpectationsForWorkerCreation(ProfileMock* profile); static void SetHistoryServiceExpectations(ProfileMock* profile); + protected: + virtual void InitCore(const Core::DoInitializeOptions& options) OVERRIDE; + private: bool synchronous_init_; bool fail_initial_download_; @@ -99,11 +75,13 @@ class TestProfileSyncService : public ProfileSyncService { void SetInitialSyncEndedForEnabledTypes(); - virtual void OnBackendInitialized(bool success); + virtual void OnBackendInitialized( + const browser_sync::WeakHandle<browser_sync::JsBackend>& backend, + bool success) OVERRIDE; virtual void Observe(int type, const NotificationSource& source, - const NotificationDetails& details); + const NotificationDetails& details) OVERRIDE; // If this is called, configuring data types will require a syncer // nudge. @@ -118,16 +96,12 @@ class TestProfileSyncService : public ProfileSyncService { // specific return type (since C++ supports covariant return types) // that is made public. virtual browser_sync::SyncBackendHostForProfileSyncTest* - GetBackendForTest(); + GetBackendForTest() OVERRIDE; protected: - virtual void CreateBackend(); + virtual void CreateBackend() OVERRIDE; private: - // When testing under ChromiumOS, this method must not return an empty - // value value in order for the profile sync service to start. - virtual std::string GetLsidForAuthBootstraping(); - browser_sync::TestIdFactory id_factory_; bool synchronous_backend_initialization_; diff --git a/chrome/browser/ui/webui/sync_internals_ui.cc b/chrome/browser/ui/webui/sync_internals_ui.cc index a559788..529304e 100644 --- a/chrome/browser/ui/webui/sync_internals_ui.cc +++ b/chrome/browser/ui/webui/sync_internals_ui.cc @@ -13,34 +13,51 @@ #include "base/values.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/sync/js_arg_list.h" +#include "chrome/browser/sync/js_controller.h" #include "chrome/browser/sync/js_event_details.h" -#include "chrome/browser/sync/js_frontend.h" #include "chrome/browser/sync/profile_sync_service.h" #include "chrome/browser/sync/sync_ui_util.h" +#include "chrome/browser/sync/weak_handle.h" #include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h" #include "chrome/browser/ui/webui/chrome_url_data_manager.h" #include "chrome/browser/ui/webui/sync_internals_html_source.h" #include "chrome/common/extensions/extension_messages.h" #include "content/browser/browser_thread.h" +using browser_sync::JsArgList; +using browser_sync::JsEventDetails; +using browser_sync::JsReplyHandler; +using browser_sync::WeakHandle; + +namespace { + +// Gets the ProfileSyncService of the underlying original profile. +// May return NULL (e.g., if sync is disabled on the command line). +ProfileSyncService* GetProfileSyncService(Profile* profile) { + return profile->GetOriginalProfile()->GetProfileSyncService(); +} + +} // namespace + SyncInternalsUI::SyncInternalsUI(TabContents* contents) - : ChromeWebUI(contents) { - browser_sync::JsFrontend* backend = GetJsFrontend(); - if (backend) { - backend->AddHandler(this); - } - // If this PostTask() call fails, it's most likely because this is - // being run from a unit test. The created objects will be cleaned - // up, anyway. + : ChromeWebUI(contents), + weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) { + // TODO(akalin): Fix. Profile* profile = Profile::FromBrowserContext(contents->browser_context()); profile->GetChromeURLDataManager()->AddDataSource( new SyncInternalsHTMLSource()); + ProfileSyncService* sync_service = GetProfileSyncService(profile); + if (sync_service) { + js_controller_ = sync_service->GetJsController(); + } + if (js_controller_.get()) { + js_controller_->AddJsEventHandler(this); + } } SyncInternalsUI::~SyncInternalsUI() { - browser_sync::JsFrontend* backend = GetJsFrontend(); - if (backend) { - backend->RemoveHandler(this); + if (js_controller_.get()) { + js_controller_->RemoveJsEventHandler(this); } } @@ -48,7 +65,7 @@ void SyncInternalsUI::OnWebUISend(const GURL& source_url, const std::string& name, const ListValue& content) { scoped_ptr<ListValue> content_copy(content.DeepCopy()); - browser_sync::JsArgList args(content_copy.get()); + JsArgList args(content_copy.get()); VLOG(1) << "Received message: " << name << " with args " << args.ToString(); // We handle this case directly because it needs to work even if @@ -57,13 +74,14 @@ void SyncInternalsUI::OnWebUISend(const GURL& source_url, ListValue return_args; DictionaryValue* about_info = new DictionaryValue(); return_args.Append(about_info); - ProfileSyncService* service = GetProfile()->GetProfileSyncService(); + ProfileSyncService* service = GetProfileSyncService(GetProfile()); sync_ui_util::ConstructAboutInformation(service, about_info); - HandleJsMessageReply(name, browser_sync::JsArgList(&return_args)); + HandleJsReply(name, JsArgList(&return_args)); } else { - browser_sync::JsFrontend* backend = GetJsFrontend(); - if (backend) { - backend->ProcessMessage(name, args, this); + if (js_controller_.get()) { + js_controller_->ProcessJsMessage( + name, args, + WeakHandle<JsReplyHandler>(weak_ptr_factory_.GetWeakPtr())); } else { LOG(WARNING) << "No sync service; dropping message " << name << " with args " << args.ToString(); @@ -72,8 +90,7 @@ void SyncInternalsUI::OnWebUISend(const GURL& source_url, } void SyncInternalsUI::HandleJsEvent( - const std::string& name, - const browser_sync::JsEventDetails& details) { + const std::string& name, const JsEventDetails& details) { VLOG(1) << "Handling event: " << name << " with details " << details.ToString(); const std::string& event_handler = "chrome.sync." + name + ".fire"; @@ -81,19 +98,11 @@ void SyncInternalsUI::HandleJsEvent( CallJavascriptFunction(event_handler, arg_list); } -void SyncInternalsUI::HandleJsMessageReply( - const std::string& name, - const browser_sync::JsArgList& args) { +void SyncInternalsUI::HandleJsReply( + const std::string& name, const JsArgList& args) { VLOG(1) << "Handling reply for " << name << " message with args " << args.ToString(); const std::string& reply_handler = "chrome.sync." + name + ".handleReply"; std::vector<const Value*> arg_list(args.Get().begin(), args.Get().end()); CallJavascriptFunction(reply_handler, arg_list); } - -browser_sync::JsFrontend* SyncInternalsUI::GetJsFrontend() { - // If this returns NULL that means that sync is disabled for - // whatever reason. - ProfileSyncService* sync_service = GetProfile()->GetProfileSyncService(); - return sync_service ? sync_service->GetJsFrontend() : NULL; -} diff --git a/chrome/browser/ui/webui/sync_internals_ui.h b/chrome/browser/ui/webui/sync_internals_ui.h index 31157a4..e114592 100644 --- a/chrome/browser/ui/webui/sync_internals_ui.h +++ b/chrome/browser/ui/webui/sync_internals_ui.h @@ -10,16 +10,21 @@ #include "base/compiler_specific.h" #include "base/basictypes.h" +#include "base/memory/weak_ptr.h" #include "chrome/browser/sync/js_event_handler.h" +#include "chrome/browser/sync/js_reply_handler.h" #include "chrome/browser/ui/webui/chrome_web_ui.h" +class ProfileSyncService; + namespace browser_sync { -class JsFrontend; +class JsController; } // namespace browser_sync // The implementation for the chrome://sync-internals page. class SyncInternalsUI : public ChromeWebUI, - public browser_sync::JsEventHandler { + public browser_sync::JsEventHandler, + public browser_sync::JsReplyHandler { public: explicit SyncInternalsUI(TabContents* contents); virtual ~SyncInternalsUI(); @@ -45,14 +50,15 @@ class SyncInternalsUI : public ChromeWebUI, virtual void HandleJsEvent( const std::string& name, const browser_sync::JsEventDetails& details) OVERRIDE; - virtual void HandleJsMessageReply( + + // browser_sync::JsReplyHandler implementation. + virtual void HandleJsReply( const std::string& name, const browser_sync::JsArgList& args) OVERRIDE; private: - // Returns the sync service's JsFrontend object, or NULL if the sync - // service does not exist. - browser_sync::JsFrontend* GetJsFrontend(); + base::WeakPtrFactory<SyncInternalsUI> weak_ptr_factory_; + base::WeakPtr<browser_sync::JsController> js_controller_; DISALLOW_COPY_AND_ASSIGN(SyncInternalsUI); }; diff --git a/chrome/browser/ui/webui/sync_internals_ui_unittest.cc b/chrome/browser/ui/webui/sync_internals_ui_unittest.cc index b0f8b80..1728f33 100644 --- a/chrome/browser/ui/webui/sync_internals_ui_unittest.cc +++ b/chrome/browser/ui/webui/sync_internals_ui_unittest.cc @@ -8,6 +8,7 @@ #include <string> #include "base/message_loop.h" +#include "base/utf_string_conversions.h" #include "base/values.h" #include "chrome/browser/sync/js_arg_list.h" #include "chrome/browser/sync/js_event_details.h" @@ -25,6 +26,8 @@ namespace { using browser_sync::HasArgsAsList; using browser_sync::JsArgList; using browser_sync::JsEventDetails; +using testing::_; +using testing::Mock; using testing::NiceMock; using testing::Return; using testing::StrictMock; @@ -39,204 +42,158 @@ class TestSyncInternalsUI : public SyncInternalsUI { MOCK_METHOD1(ExecuteJavascript, void(const string16&)); }; -class SyncInternalsUITest : public RenderViewHostTestHarness { +// Tests with non-NULL ProfileSyncService. +class SyncInternalsUITestWithService : public RenderViewHostTestHarness { protected: - // We allocate memory for |sync_internals_ui_| but we don't - // construct it. This is because we want to set mock expectations - // with its address before we construct it, and its constructor - // calls into our mocks. - SyncInternalsUITest() - // The message loop is provided by RenderViewHostTestHarness. - : ui_thread_(BrowserThread::UI, MessageLoopForUI::current()), - test_sync_internals_ui_buf_(NULL), - test_sync_internals_ui_constructor_called_(false) {} + SyncInternalsUITestWithService() {} - virtual void SetUp() { - test_sync_internals_ui_buf_ = operator new(sizeof(TestSyncInternalsUI)); - test_sync_internals_ui_constructor_called_ = false; - profile_.reset(new NiceMock<ProfileMock>()); - RenderViewHostTestHarness::SetUp(); - } + virtual ~SyncInternalsUITestWithService() {} - virtual void TearDown() { - if (test_sync_internals_ui_constructor_called_) { - GetTestSyncInternalsUI()->~TestSyncInternalsUI(); - } - operator delete(test_sync_internals_ui_buf_); - RenderViewHostTestHarness::TearDown(); - } - - NiceMock<ProfileMock>* GetProfileMock() { - return static_cast<NiceMock<ProfileMock>*>(profile()); - } + virtual void SetUp() { + NiceMock<ProfileMock>* profile_mock = new NiceMock<ProfileMock>(); + StrictMock<ProfileSyncServiceMock> profile_sync_service_mock; + EXPECT_CALL(*profile_mock, GetProfileSyncService()) + .WillOnce(Return(&profile_sync_service_mock)); + profile_.reset(profile_mock); - // Set up boilerplate expectations for calls done during - // SyncInternalUI's construction/destruction. - void ExpectSetupTeardownCalls() { - EXPECT_CALL(*GetProfileMock(), GetProfileSyncService()) - .WillRepeatedly(Return(&profile_sync_service_mock_)); + RenderViewHostTestHarness::SetUp(); - EXPECT_CALL(profile_sync_service_mock_, GetJsFrontend()) - .WillRepeatedly(Return(&mock_js_backend_)); + EXPECT_CALL(profile_sync_service_mock, GetJsController()) + .WillOnce(Return(mock_js_controller_.AsWeakPtr())); - // Called by sync_ui_util::ConstructAboutInformation(). - EXPECT_CALL(profile_sync_service_mock_, HasSyncSetupCompleted()) - .WillRepeatedly(Return(false)); + EXPECT_CALL(mock_js_controller_, AddJsEventHandler(_)); - // Called by SyncInternalsUI's constructor. - EXPECT_CALL(mock_js_backend_, - AddHandler(GetTestSyncInternalsUIAddress())); + { + // Needed by |test_sync_internals_ui_|'s constructor. The + // message loop is provided by RenderViewHostTestHarness. + BrowserThread ui_thread_(BrowserThread::UI, + MessageLoopForUI::current()); + // |test_sync_internals_ui_|'s constructor triggers all the + // expectations above. + test_sync_internals_ui_.reset(new TestSyncInternalsUI(contents())); + } - // Called by SyncInternalUI's destructor. - EXPECT_CALL(mock_js_backend_, - RemoveHandler(GetTestSyncInternalsUIAddress())); + Mock::VerifyAndClearExpectations(profile_mock); + Mock::VerifyAndClearExpectations(&mock_js_controller_); } - // Like ExpectSetupTeardownCalls() but with a NULL - // ProfileSyncService. - void ExpectSetupTeardownCallsNullService() { - EXPECT_CALL(*GetProfileMock(), GetProfileSyncService()) - .WillRepeatedly(Return(static_cast<ProfileSyncService*>(NULL))); - } + virtual void TearDown() { + Mock::VerifyAndClearExpectations(&mock_js_controller_); - void ConstructTestSyncInternalsUI() { - if (test_sync_internals_ui_constructor_called_) { - ADD_FAILURE() << "ConstructTestSyncInternalsUI() should be called " - << "at most once per test"; - return; - } - new(test_sync_internals_ui_buf_) TestSyncInternalsUI(contents()); - test_sync_internals_ui_constructor_called_ = true; - } + // Called by |test_sync_internals_ui_|'s destructor. + EXPECT_CALL(mock_js_controller_, + RemoveJsEventHandler(test_sync_internals_ui_.get())); + test_sync_internals_ui_.reset(); - TestSyncInternalsUI* GetTestSyncInternalsUI() { - if (!test_sync_internals_ui_constructor_called_) { - ADD_FAILURE() << "ConstructTestSyncInternalsUI() should be called " - << "before GetTestSyncInternalsUI()"; - return NULL; - } - return GetTestSyncInternalsUIAddress(); - } - - // Used for passing into EXPECT_CALL(). - TestSyncInternalsUI* GetTestSyncInternalsUIAddress() { - EXPECT_TRUE(test_sync_internals_ui_buf_); - return static_cast<TestSyncInternalsUI*>(test_sync_internals_ui_buf_); + RenderViewHostTestHarness::TearDown(); } - StrictMock<ProfileSyncServiceMock> profile_sync_service_mock_; - StrictMock<browser_sync::MockJsFrontend> mock_js_backend_; - - private: - // Needed by |contents()|. - BrowserThread ui_thread_; - void* test_sync_internals_ui_buf_; - bool test_sync_internals_ui_constructor_called_; + StrictMock<browser_sync::MockJsController> mock_js_controller_; + scoped_ptr<TestSyncInternalsUI> test_sync_internals_ui_; }; -TEST_F(SyncInternalsUITest, HandleJsEvent) { - ExpectSetupTeardownCalls(); - - ConstructTestSyncInternalsUI(); - - EXPECT_CALL(*GetTestSyncInternalsUI(), +TEST_F(SyncInternalsUITestWithService, HandleJsEvent) { + EXPECT_CALL(*test_sync_internals_ui_, ExecuteJavascript( ASCIIToUTF16("chrome.sync.testMessage.fire({});"))); - GetTestSyncInternalsUI()->HandleJsEvent("testMessage", JsEventDetails()); + test_sync_internals_ui_->HandleJsEvent("testMessage", JsEventDetails()); } -TEST_F(SyncInternalsUITest, HandleJsEventNullService) { - ExpectSetupTeardownCallsNullService(); - - ConstructTestSyncInternalsUI(); - - EXPECT_CALL(*GetTestSyncInternalsUI(), - ExecuteJavascript( - ASCIIToUTF16("chrome.sync.testMessage.fire({});"))); - - GetTestSyncInternalsUI()->HandleJsEvent("testMessage", JsEventDetails()); -} - -TEST_F(SyncInternalsUITest, HandleJsMessageReply) { - ExpectSetupTeardownCalls(); - - ConstructTestSyncInternalsUI(); - +TEST_F(SyncInternalsUITestWithService, HandleJsReply) { EXPECT_CALL( - *GetTestSyncInternalsUI(), + *test_sync_internals_ui_, ExecuteJavascript( ASCIIToUTF16("chrome.sync.testMessage.handleReply(5,true);"))); ListValue args; args.Append(Value::CreateIntegerValue(5)); args.Append(Value::CreateBooleanValue(true)); - GetTestSyncInternalsUI()->HandleJsMessageReply( - "testMessage", JsArgList(&args)); + test_sync_internals_ui_->HandleJsReply("testMessage", JsArgList(&args)); } -TEST_F(SyncInternalsUITest, HandleJsMessageReplyNullService) { - ExpectSetupTeardownCallsNullService(); - - ConstructTestSyncInternalsUI(); +TEST_F(SyncInternalsUITestWithService, OnWebUISendBasic) { + const std::string& name = "testName"; + ListValue args; + args.Append(Value::CreateIntegerValue(10)); - EXPECT_CALL( - *GetTestSyncInternalsUI(), - ExecuteJavascript( - ASCIIToUTF16("chrome.sync.testMessage.handleReply(5,true);"))); + EXPECT_CALL(mock_js_controller_, + ProcessJsMessage(name, HasArgsAsList(args), _)); - ListValue args; - args.Append(Value::CreateIntegerValue(5)); - args.Append(Value::CreateBooleanValue(true)); - GetTestSyncInternalsUI()->HandleJsMessageReply( - "testMessage", JsArgList(&args)); + test_sync_internals_ui_->OnWebUISend(GURL(), name, args); } -TEST_F(SyncInternalsUITest, OnWebUISendBasic) { - ExpectSetupTeardownCalls(); +// Tests with NULL ProfileSyncService. +class SyncInternalsUITestWithoutService : public RenderViewHostTestHarness { + protected: + SyncInternalsUITestWithoutService() {} + + virtual ~SyncInternalsUITestWithoutService() {} - std::string name = "testName"; - ListValue args; - args.Append(Value::CreateIntegerValue(10)); + virtual void SetUp() { + NiceMock<ProfileMock>* profile_mock = new NiceMock<ProfileMock>(); + EXPECT_CALL(*profile_mock, GetProfileSyncService()) + .WillOnce(Return(static_cast<ProfileSyncService*>(NULL))); + profile_.reset(profile_mock); - EXPECT_CALL(mock_js_backend_, - ProcessMessage(name, HasArgsAsList(args), - GetTestSyncInternalsUIAddress())); + RenderViewHostTestHarness::SetUp(); - ConstructTestSyncInternalsUI(); + { + // Needed by |test_sync_internals_ui_|'s constructor. The + // message loop is provided by RenderViewHostTestHarness. + BrowserThread ui_thread_(BrowserThread::UI, + MessageLoopForUI::current()); + // |test_sync_internals_ui_|'s constructor triggers all the + // expectations above. + test_sync_internals_ui_.reset(new TestSyncInternalsUI(contents())); + } + + Mock::VerifyAndClearExpectations(profile_mock); + } - GetTestSyncInternalsUI()->OnWebUISend(GURL(), name, args); + scoped_ptr<TestSyncInternalsUI> test_sync_internals_ui_; +}; + +TEST_F(SyncInternalsUITestWithoutService, HandleJsEvent) { + EXPECT_CALL(*test_sync_internals_ui_, + ExecuteJavascript( + ASCIIToUTF16("chrome.sync.testMessage.fire({});"))); + + test_sync_internals_ui_->HandleJsEvent("testMessage", JsEventDetails()); } -TEST_F(SyncInternalsUITest, OnWebUISendBasicNullService) { - ExpectSetupTeardownCallsNullService(); +TEST_F(SyncInternalsUITestWithoutService, HandleJsReply) { + EXPECT_CALL( + *test_sync_internals_ui_, + ExecuteJavascript( + ASCIIToUTF16("chrome.sync.testMessage.handleReply(5,true);"))); - ConstructTestSyncInternalsUI(); + ListValue args; + args.Append(Value::CreateIntegerValue(5)); + args.Append(Value::CreateBooleanValue(true)); + test_sync_internals_ui_->HandleJsReply( + "testMessage", JsArgList(&args)); +} - std::string name = "testName"; +TEST_F(SyncInternalsUITestWithoutService, OnWebUISendBasic) { + const std::string& name = "testName"; ListValue args; args.Append(Value::CreateIntegerValue(5)); // Should drop the message. - GetTestSyncInternalsUI()->OnWebUISend(GURL(), name, args); + test_sync_internals_ui_->OnWebUISend(GURL(), name, args); } -namespace { -const char kAboutInfoCall[] = - "chrome.sync.getAboutInfo.handleReply({\"summary\":\"SYNC DISABLED\"});"; -} // namespace - -// TODO(lipalani) - add a test case to test about:sync with a non null service. -TEST_F(SyncInternalsUITest, OnWebUISendGetAboutInfoNullService) { - ExpectSetupTeardownCallsNullService(); - - ConstructTestSyncInternalsUI(); - - EXPECT_CALL(*GetTestSyncInternalsUI(), +// TODO(lipalani) - add a test case to test about:sync with a non null +// service. +TEST_F(SyncInternalsUITestWithoutService, OnWebUISendGetAboutInfo) { + const char kAboutInfoCall[] = + "chrome.sync.getAboutInfo.handleReply({\"summary\":\"SYNC DISABLED\"});"; + EXPECT_CALL(*test_sync_internals_ui_, ExecuteJavascript(ASCIIToUTF16(kAboutInfoCall))); ListValue args; - GetTestSyncInternalsUI()->OnWebUISend(GURL(), "getAboutInfo", args); + test_sync_internals_ui_->OnWebUISend(GURL(), "getAboutInfo", args); } } // namespace diff --git a/chrome/chrome.gyp b/chrome/chrome.gyp index e6004ec..871edaf 100644 --- a/chrome/chrome.gyp +++ b/chrome/chrome.gyp @@ -655,17 +655,15 @@ 'browser/sync/js_arg_list.cc', 'browser/sync/js_arg_list.h', 'browser/sync/js_backend.h', - 'browser/sync/js_transaction_observer.cc', - 'browser/sync/js_transaction_observer.h', + 'browser/sync/js_controller.h', 'browser/sync/js_event_details.cc', 'browser/sync/js_event_details.h', 'browser/sync/js_event_handler.h', - 'browser/sync/js_event_handler_list.cc', - 'browser/sync/js_event_handler_list.h', - 'browser/sync/js_event_router.h', - 'browser/sync/js_frontend.h', + 'browser/sync/js_reply_handler.h', 'browser/sync/js_sync_manager_observer.cc', 'browser/sync/js_sync_manager_observer.h', + 'browser/sync/js_transaction_observer.cc', + 'browser/sync/js_transaction_observer.h', 'browser/sync/protocol/proto_enum_conversions.cc', 'browser/sync/protocol/proto_enum_conversions.h', 'browser/sync/protocol/proto_value_conversions.cc', @@ -682,6 +680,8 @@ 'browser/sync/sessions/sync_session_context.cc', 'browser/sync/sessions/sync_session_context.h', 'browser/sync/shared_value.h', + 'browser/sync/sync_js_controller.cc', + 'browser/sync/sync_js_controller.h', 'browser/sync/syncable/blob.h', 'browser/sync/syncable/dir_open_result.h', 'browser/sync/syncable/directory_backing_store.cc', diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index d73ec05..38bcab0 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -3330,7 +3330,6 @@ 'browser/sync/engine/verify_updates_command_unittest.cc', 'browser/sync/js_arg_list_unittest.cc', 'browser/sync/js_event_details_unittest.cc', - 'browser/sync/js_event_handler_list_unittest.cc', 'browser/sync/js_sync_manager_observer_unittest.cc', 'browser/sync/notifier/cache_invalidation_packet_handler_unittest.cc', 'browser/sync/notifier/chrome_invalidation_client_unittest.cc', @@ -3347,6 +3346,7 @@ 'browser/sync/sessions/sync_session_unittest.cc', 'browser/sync/sessions/test_util.cc', 'browser/sync/sessions/test_util.h', + 'browser/sync/sync_js_controller_unittest.cc', 'browser/sync/syncable/directory_backing_store_unittest.cc', 'browser/sync/syncable/model_type_payload_map_unittest.cc', 'browser/sync/syncable/model_type_unittest.cc', |