diff options
20 files changed, 150 insertions, 181 deletions
diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc index 37824cd..5bf4489 100644 --- a/chrome/browser/sync/glue/sync_backend_host.cc +++ b/chrome/browser/sync/glue/sync_backend_host.cc @@ -26,7 +26,6 @@ #include "chrome/browser/sync/glue/bridged_sync_notifier.h" #include "chrome/browser/sync/glue/change_processor.h" #include "chrome/browser/sync/glue/chrome_encryptor.h" -#include "chrome/browser/sync/glue/http_bridge.h" #include "chrome/browser/sync/glue/sync_backend_registrar.h" #include "chrome/browser/sync/invalidations/invalidator_storage.h" #include "chrome/browser/sync/sync_prefs.h" @@ -43,6 +42,7 @@ #include "net/url_request/url_request_context_getter.h" #include "sync/internal_api/public/base_transaction.h" #include "sync/internal_api/public/engine/model_safe_worker.h" +#include "sync/internal_api/public/http_bridge.h" #include "sync/internal_api/public/read_transaction.h" #include "sync/internal_api/public/util/experiments.h" #include "sync/notifier/sync_notifier.h" @@ -323,9 +323,41 @@ SyncBackendHost::~SyncBackendHost() { namespace { +// Helper to construct a user agent string (ASCII) suitable for use by +// the syncapi for any HTTP communication. This string is used by the sync +// backend for classifying client types when calculating statistics. +std::string MakeUserAgentForSyncApi() { + std::string user_agent; + user_agent = "Chrome "; +#if defined(OS_WIN) + user_agent += "WIN "; +#elif defined(OS_CHROMEOS) + user_agent += "CROS "; +#elif defined(OS_LINUX) + user_agent += "LINUX "; +#elif defined(OS_FREEBSD) + user_agent += "FREEBSD "; +#elif defined(OS_OPENBSD) + user_agent += "OPENBSD "; +#elif defined(OS_MACOSX) + user_agent += "MAC "; +#endif + chrome::VersionInfo version_info; + if (!version_info.is_valid()) { + DLOG(ERROR) << "Unable to create chrome::VersionInfo object"; + return user_agent; + } + + user_agent += version_info.Version(); + user_agent += " (" + version_info.LastChange() + ")"; + if (!version_info.IsOfficialBuild()) + user_agent += "-devel"; + return user_agent; +} + csync::HttpPostProviderFactory* MakeHttpBridgeFactory( const scoped_refptr<net::URLRequestContextGetter>& getter) { - return new HttpBridgeFactory(getter); + return new HttpBridgeFactory(getter, MakeUserAgentForSyncApi()); } } // namespace @@ -1012,38 +1044,6 @@ void SyncBackendHost::Core::OnActionableError( sync_error); } -// Helper to construct a user agent string (ASCII) suitable for use by -// the syncapi for any HTTP communication. This string is used by the sync -// backend for classifying client types when calculating statistics. -std::string MakeUserAgentForSyncApi() { - std::string user_agent; - user_agent = "Chrome "; -#if defined(OS_WIN) - user_agent += "WIN "; -#elif defined(OS_CHROMEOS) - user_agent += "CROS "; -#elif defined(OS_LINUX) - user_agent += "LINUX "; -#elif defined(OS_FREEBSD) - user_agent += "FREEBSD "; -#elif defined(OS_OPENBSD) - user_agent += "OPENBSD "; -#elif defined(OS_MACOSX) - user_agent += "MAC "; -#endif - chrome::VersionInfo version_info; - if (!version_info.is_valid()) { - DLOG(ERROR) << "Unable to create chrome::VersionInfo object"; - return user_agent; - } - - user_agent += version_info.Version(); - user_agent += " (" + version_info.LastChange() + ")"; - if (!version_info.IsOfficialBuild()) - user_agent += "-devel"; - return user_agent; -} - void SyncBackendHost::Core::DoInitialize(const DoInitializeOptions& options) { DCHECK(!sync_loop_); sync_loop_ = options.sync_loop; @@ -1078,7 +1078,6 @@ void SyncBackendHost::Core::DoInitialize(const DoInitializeOptions& options) { options.workers, options.extensions_activity_monitor, options.registrar /* as SyncManager::ChangeDelegate */, - MakeUserAgentForSyncApi(), options.credentials, new BridgedSyncNotifier( options.chrome_sync_notification_bridge, diff --git a/chrome/browser/sync/test/test_http_bridge_factory.h b/chrome/browser/sync/test/test_http_bridge_factory.h index d78c029..4c91127 100644 --- a/chrome/browser/sync/test/test_http_bridge_factory.h +++ b/chrome/browser/sync/test/test_http_bridge_factory.h @@ -15,8 +15,6 @@ namespace browser_sync { class TestHttpBridge : public csync::HttpPostProviderInterface { public: // Begin csync::HttpPostProviderInterface implementation: - virtual void SetUserAgent(const char* user_agent) OVERRIDE {} - virtual void SetExtraRequestHeaders(const char * headers) OVERRIDE {} virtual void SetURL(const char* url, int port) OVERRIDE {} diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index af8ed82..4d17b2a 100644 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -2152,8 +2152,6 @@ 'browser/sync/glue/generic_change_processor.h', 'browser/sync/glue/history_model_worker.cc', 'browser/sync/glue/history_model_worker.h', - 'browser/sync/glue/http_bridge.cc', - 'browser/sync/glue/http_bridge.h', 'browser/sync/glue/model_association_manager.cc', 'browser/sync/glue/model_association_manager.h', 'browser/sync/glue/model_associator.h', diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index eb4383e..401f788 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -1549,7 +1549,6 @@ 'browser/sync/glue/frontend_data_type_controller_mock.cc', 'browser/sync/glue/frontend_data_type_controller_mock.h', 'browser/sync/glue/frontend_data_type_controller_unittest.cc', - 'browser/sync/glue/http_bridge_unittest.cc', 'browser/sync/glue/model_association_manager_unittest.cc', 'browser/sync/glue/model_associator_mock.cc', 'browser/sync/glue/model_associator_mock.h', diff --git a/sync/engine/net/server_connection_manager.cc b/sync/engine/net/server_connection_manager.cc index 92f083e..152223a 100644 --- a/sync/engine/net/server_connection_manager.cc +++ b/sync/engine/net/server_connection_manager.cc @@ -168,11 +168,9 @@ ScopedServerStatusWatcher::~ScopedServerStatusWatcher() { ServerConnectionManager::ServerConnectionManager( const string& server, int port, - bool use_ssl, - const string& user_agent) + bool use_ssl) : sync_server_(server), sync_server_port_(port), - user_agent_(user_agent), use_ssl_(use_ssl), proto_sync_path_(kSyncServerSyncPath), get_time_path_(kSyncServerGetTimePath), diff --git a/sync/engine/net/server_connection_manager.h b/sync/engine/net/server_connection_manager.h index 4083ba2..c61352e 100644 --- a/sync/engine/net/server_connection_manager.h +++ b/sync/engine/net/server_connection_manager.h @@ -176,8 +176,7 @@ class ServerConnectionManager { ServerConnectionManager(const std::string& server, int port, - bool use_ssl, - const std::string& user_agent); + bool use_ssl); virtual ~ServerConnectionManager(); @@ -191,8 +190,6 @@ class ServerConnectionManager { void AddListener(ServerConnectionEventListener* listener); void RemoveListener(ServerConnectionEventListener* listener); - inline std::string user_agent() const { return user_agent_; } - inline HttpResponse::ServerConnectionCode server_status() const { DCHECK(thread_checker_.CalledOnValidThread()); return server_status_; @@ -287,9 +284,6 @@ class ServerConnectionManager { // The unique id of the user's client. std::string client_id_; - // The user-agent string for HTTP. - std::string user_agent_; - // Indicates whether or not requests should be made using HTTPS. bool use_ssl_; diff --git a/sync/engine/syncer_proto_util_unittest.cc b/sync/engine/syncer_proto_util_unittest.cc index 753e490..b0c9f6e 100644 --- a/sync/engine/syncer_proto_util_unittest.cc +++ b/sync/engine/syncer_proto_util_unittest.cc @@ -206,7 +206,7 @@ TEST_F(SyncerProtoUtilTest, AddRequestBirthday) { class DummyConnectionManager : public csync::ServerConnectionManager { public: DummyConnectionManager() - : ServerConnectionManager("unused", 0, false, "version"), + : ServerConnectionManager("unused", 0, false), send_error_(false), access_denied_(false) {} diff --git a/sync/internal_api/DEPS b/sync/internal_api/DEPS index 955b31e..ab23f90 100644 --- a/sync/internal_api/DEPS +++ b/sync/internal_api/DEPS @@ -1,8 +1,6 @@ include_rules = [ "+googleurl", - "+net/base/net_errors.h", - "+net/base/network_change_notifier.h", - "+net/http/http_status_code.h", + "+net", "+sync/engine", "+sync/js", "+sync/notifier", diff --git a/chrome/browser/sync/glue/http_bridge.cc b/sync/internal_api/http_bridge.cc index dfaf81c..6aa6c4a 100644 --- a/chrome/browser/sync/glue/http_bridge.cc +++ b/sync/internal_api/http_bridge.cc @@ -2,13 +2,11 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "chrome/browser/sync/glue/http_bridge.h" +#include "sync/internal_api/public/http_bridge.h" #include "base/message_loop.h" #include "base/message_loop_proxy.h" #include "base/string_number_conversions.h" -#include "content/public/browser/browser_thread.h" -#include "content/public/common/content_client.h" #include "net/base/host_resolver.h" #include "net/base/load_flags.h" #include "net/base/net_errors.h" @@ -21,13 +19,18 @@ #include "net/url_request/url_request_context.h" #include "net/url_request/url_request_status.h" -using content::BrowserThread; - namespace browser_sync { HttpBridge::RequestContextGetter::RequestContextGetter( - net::URLRequestContextGetter* baseline_context_getter) - : baseline_context_getter_(baseline_context_getter) { + net::URLRequestContextGetter* baseline_context_getter, + const std::string& user_agent) + : baseline_context_getter_(baseline_context_getter), + network_task_runner_( + baseline_context_getter_->GetNetworkTaskRunner()), + user_agent_(user_agent) { + DCHECK(baseline_context_getter_); + DCHECK(network_task_runner_); + DCHECK(!user_agent_.empty()); } HttpBridge::RequestContextGetter::~RequestContextGetter() {} @@ -38,28 +41,26 @@ HttpBridge::RequestContextGetter::GetURLRequestContext() { if (!context_.get()) { net::URLRequestContext* baseline_context = baseline_context_getter_->GetURLRequestContext(); - context_.reset(new RequestContext(baseline_context)); + context_.reset( + new RequestContext(baseline_context, GetNetworkTaskRunner(), + user_agent_)); baseline_context_getter_ = NULL; } - // Apply the user agent which was set earlier. - if (is_user_agent_set()) - context_->set_user_agent(user_agent_); - return context_.get(); } scoped_refptr<base::SingleThreadTaskRunner> HttpBridge::RequestContextGetter::GetNetworkTaskRunner() const { - return BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO); + return network_task_runner_; } HttpBridgeFactory::HttpBridgeFactory( - net::URLRequestContextGetter* baseline_context_getter) { - DCHECK(baseline_context_getter != NULL); - request_context_getter_ = - new HttpBridge::RequestContextGetter(baseline_context_getter); -} + net::URLRequestContextGetter* baseline_context_getter, + const std::string& user_agent) + : request_context_getter_( + new HttpBridge::RequestContextGetter( + baseline_context_getter, user_agent)) {} HttpBridgeFactory::~HttpBridgeFactory() { } @@ -75,8 +76,14 @@ void HttpBridgeFactory::Destroy(csync::HttpPostProviderInterface* http) { } HttpBridge::RequestContext::RequestContext( - net::URLRequestContext* baseline_context) - : baseline_context_(baseline_context) { + net::URLRequestContext* baseline_context, + const scoped_refptr<base::SingleThreadTaskRunner>& + network_task_runner, + const std::string& user_agent) + : baseline_context_(baseline_context), + network_task_runner_(network_task_runner), + user_agent_(user_agent) { + DCHECK(!user_agent_.empty()); // Create empty, in-memory cookie store. set_cookie_store(new net::CookieMonster(NULL, NULL)); @@ -105,18 +112,19 @@ HttpBridge::RequestContext::RequestContext( set_accept_language(baseline_context->accept_language()); set_accept_charset(baseline_context->accept_charset()); - // We default to the browser's user agent. This can (and should) be overridden - // with set_user_agent. - set_user_agent(content::GetUserAgent(GURL())); - set_net_log(baseline_context->net_log()); } HttpBridge::RequestContext::~RequestContext() { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); + DCHECK(network_task_runner_->BelongsToCurrentThread()); delete http_transaction_factory(); } +const std::string& HttpBridge::RequestContext::GetUserAgent( + const GURL& url) const { + return user_agent_; +} + HttpBridge::URLFetchState::URLFetchState() : url_poster(NULL), aborted(false), request_completed(false), @@ -127,6 +135,8 @@ HttpBridge::URLFetchState::~URLFetchState() {} HttpBridge::HttpBridge(HttpBridge::RequestContextGetter* context_getter) : context_getter_for_request_(context_getter), + network_task_runner_( + context_getter_for_request_->GetNetworkTaskRunner()), created_on_loop_(MessageLoop::current()), http_post_completed_(false, false) { } @@ -134,15 +144,6 @@ HttpBridge::HttpBridge(HttpBridge::RequestContextGetter* context_getter) HttpBridge::~HttpBridge() { } -void HttpBridge::SetUserAgent(const char* user_agent) { - DCHECK_EQ(MessageLoop::current(), created_on_loop_); - if (DCHECK_IS_ON()) { - base::AutoLock lock(fetch_state_lock_); - DCHECK(!fetch_state_.request_completed); - } - context_getter_for_request_->set_user_agent(user_agent); -} - void HttpBridge::SetExtraRequestHeaders(const char * headers) { DCHECK(extra_headers_.empty()) << "HttpBridge::SetExtraRequestHeaders called twice."; @@ -195,8 +196,8 @@ bool HttpBridge::MakeSynchronousPost(int* error_code, int* response_code) { DCHECK(url_for_request_.is_valid()) << "Invalid URL for request"; DCHECK(!content_type_.empty()) << "Payload not set"; - if (!BrowserThread::PostTask( - BrowserThread::IO, FROM_HERE, + if (!network_task_runner_->PostTask( + FROM_HERE, base::Bind(&HttpBridge::CallMakeAsynchronousPost, this))) { // This usually happens when we're in a unit test. LOG(WARNING) << "Could not post CallMakeAsynchronousPost task"; @@ -215,7 +216,7 @@ bool HttpBridge::MakeSynchronousPost(int* error_code, int* response_code) { } void HttpBridge::MakeAsynchronousPost() { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); + DCHECK(network_task_runner_->BelongsToCurrentThread()); base::AutoLock lock(fetch_state_lock_); DCHECK(!fetch_state_.request_completed); if (fetch_state_.aborted) @@ -263,8 +264,8 @@ void HttpBridge::Abort() { return; fetch_state_.aborted = true; - if (!BrowserThread::PostTask( - BrowserThread::IO, FROM_HERE, + if (!network_task_runner_->PostTask( + FROM_HERE, base::Bind(&HttpBridge::DestroyURLFetcherOnIOThread, this, fetch_state_.url_poster))) { // Madness ensues. @@ -277,12 +278,12 @@ void HttpBridge::Abort() { } void HttpBridge::DestroyURLFetcherOnIOThread(net::URLFetcher* fetcher) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); + DCHECK(network_task_runner_->BelongsToCurrentThread()); delete fetcher; } void HttpBridge::OnURLFetchComplete(const net::URLFetcher* source) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); + DCHECK(network_task_runner_->BelongsToCurrentThread()); base::AutoLock lock(fetch_state_lock_); if (fetch_state_.aborted) return; diff --git a/chrome/browser/sync/glue/http_bridge_unittest.cc b/sync/internal_api/http_bridge_unittest.cc index 745350d..a4a4ceb 100644 --- a/chrome/browser/sync/glue/http_bridge_unittest.cc +++ b/sync/internal_api/http_bridge_unittest.cc @@ -5,16 +5,14 @@ #include "base/message_loop_proxy.h" #include "base/synchronization/waitable_event.h" #include "base/threading/thread.h" -#include "chrome/browser/sync/glue/http_bridge.h" -#include "content/public/test/test_browser_thread.h" #include "net/test/test_server.h" #include "net/url_request/test_url_fetcher_factory.h" #include "net/url_request/url_fetcher_delegate.h" #include "net/url_request/url_request_test_util.h" +#include "sync/internal_api/public/http_bridge.h" #include "testing/gtest/include/gtest/gtest.h" using browser_sync::HttpBridge; -using content::BrowserThread; namespace { // TODO(timsteele): Should use PathService here. See Chromium Issue 3113. @@ -29,11 +27,13 @@ class SyncHttpBridgeTest : public testing::Test { FilePath(kDocRoot)), fake_default_request_context_getter_(NULL), bridge_for_race_test_(NULL), - io_thread_(BrowserThread::IO) { + io_thread_("IO thread") { } virtual void SetUp() { - io_thread_.StartIOThread(); + base::Thread::Options options; + options.message_loop_type = MessageLoop::TYPE_IO; + io_thread_.StartWithOptions(options); } virtual void TearDown() { @@ -48,13 +48,13 @@ class SyncHttpBridgeTest : public testing::Test { HttpBridge* BuildBridge() { if (!fake_default_request_context_getter_) { fake_default_request_context_getter_ = - new TestURLRequestContextGetter( - BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO)); + new TestURLRequestContextGetter(io_thread_.message_loop_proxy()); fake_default_request_context_getter_->AddRef(); } HttpBridge* bridge = new HttpBridge( new HttpBridge::RequestContextGetter( - fake_default_request_context_getter_)); + fake_default_request_context_getter_, + "user agent")); return bridge; } @@ -82,7 +82,7 @@ class SyncHttpBridgeTest : public testing::Test { } MessageLoop* GetIOThreadLoop() { - return io_thread_.DeprecatedGetThreadObject()->message_loop(); + return io_thread_.message_loop(); } // Note this is lazy created, so don't call this before your bridge. @@ -92,7 +92,7 @@ class SyncHttpBridgeTest : public testing::Test { net::TestServer test_server_; - content::TestBrowserThread* io_thread() { return &io_thread_; } + base::Thread* io_thread() { return &io_thread_; } HttpBridge* bridge_for_race_test() { return bridge_for_race_test_; } @@ -104,7 +104,7 @@ class SyncHttpBridgeTest : public testing::Test { HttpBridge* bridge_for_race_test_; // Separate thread for IO used by the HttpBridge. - content::TestBrowserThread io_thread_; + base::Thread io_thread_; MessageLoop loop_; }; @@ -118,9 +118,10 @@ class ShuntedHttpBridge : public HttpBridge { // returns. ShuntedHttpBridge(net::URLRequestContextGetter* baseline_context_getter, SyncHttpBridgeTest* test, bool never_finishes) - : HttpBridge(new HttpBridge::RequestContextGetter( - baseline_context_getter)), - test_(test), never_finishes_(never_finishes) { } + : HttpBridge( + new HttpBridge::RequestContextGetter( + baseline_context_getter, "user agent")), + test_(test), never_finishes_(never_finishes) { } protected: virtual void MakeAsynchronousPost() { ASSERT_TRUE(MessageLoop::current() == test_->GetIOThreadLoop()); @@ -156,12 +157,10 @@ void SyncHttpBridgeTest::RunSyncThreadBridgeUseTest( base::WaitableEvent* signal_when_created, base::WaitableEvent* signal_when_released) { scoped_refptr<net::URLRequestContextGetter> ctx_getter( - new TestURLRequestContextGetter( - BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO))); + new TestURLRequestContextGetter(io_thread_.message_loop_proxy())); { scoped_refptr<ShuntedHttpBridge> bridge(new ShuntedHttpBridge( ctx_getter, this, true)); - bridge->SetUserAgent("bob"); bridge->SetURL("http://www.google.com", 9999); bridge->SetPostPayload("text/plain", 2, " "); bridge_for_race_test_ = bridge; @@ -178,8 +177,8 @@ void SyncHttpBridgeTest::RunSyncThreadBridgeUseTest( TEST_F(SyncHttpBridgeTest, TestUsesSameHttpNetworkSession) { // Run this test on the IO thread because we can only call // URLRequestContextGetter::GetURLRequestContext on the IO thread. - BrowserThread::PostTask( - BrowserThread::IO, FROM_HERE, + io_thread()->message_loop()->PostTask( + FROM_HERE, base::Bind(&SyncHttpBridgeTest::TestSameHttpNetworkSession, MessageLoop::current(), this)); MessageLoop::current()->Run(); @@ -188,11 +187,9 @@ TEST_F(SyncHttpBridgeTest, TestUsesSameHttpNetworkSession) { // Test the HttpBridge without actually making any network requests. TEST_F(SyncHttpBridgeTest, TestMakeSynchronousPostShunted) { scoped_refptr<net::URLRequestContextGetter> ctx_getter( - new TestURLRequestContextGetter( - BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO))); + new TestURLRequestContextGetter(io_thread()->message_loop_proxy())); scoped_refptr<HttpBridge> http_bridge(new ShuntedHttpBridge( ctx_getter, this, false)); - http_bridge->SetUserAgent("bob"); http_bridge->SetURL("http://www.google.com", 9999); http_bridge->SetPostPayload("text/plain", 2, " "); @@ -232,14 +229,13 @@ TEST_F(SyncHttpBridgeTest, TestMakeSynchronousPostLiveWithPayload) { EXPECT_EQ(payload, std::string(http_bridge->GetResponseContent())); } -// Full round-trip test of the HttpBridge, using custom UA string +// Full round-trip test of the HttpBridge. TEST_F(SyncHttpBridgeTest, TestMakeSynchronousPostLiveComprehensive) { ASSERT_TRUE(test_server_.Start()); scoped_refptr<HttpBridge> http_bridge(BuildBridge()); GURL echo_header = test_server_.GetURL("echoall"); - http_bridge->SetUserAgent("bob"); http_bridge->SetURL(echo_header.spec().c_str(), echo_header.IntPort()); std::string test_payload = "###TEST PAYLOAD###"; @@ -256,7 +252,7 @@ TEST_F(SyncHttpBridgeTest, TestMakeSynchronousPostLiveComprehensive) { std::string response(http_bridge->GetResponseContent(), http_bridge->GetResponseContentLength()); EXPECT_EQ(std::string::npos, response.find("Cookie:")); - EXPECT_NE(std::string::npos, response.find("User-Agent: bob")); + EXPECT_NE(std::string::npos, response.find("User-Agent: user agent")); EXPECT_NE(std::string::npos, response.find(test_payload.c_str())); } @@ -313,19 +309,18 @@ TEST_F(SyncHttpBridgeTest, TestResponseHeader) { TEST_F(SyncHttpBridgeTest, Abort) { scoped_refptr<net::URLRequestContextGetter> ctx_getter( - new TestURLRequestContextGetter( - BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO))); + new TestURLRequestContextGetter(io_thread()->message_loop_proxy())); scoped_refptr<ShuntedHttpBridge> http_bridge(new ShuntedHttpBridge( ctx_getter, this, true)); - http_bridge->SetUserAgent("bob"); http_bridge->SetURL("http://www.google.com", 9999); http_bridge->SetPostPayload("text/plain", 2, " "); int os_error = 0; int response_code = 0; - BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, - base::Bind(&SyncHttpBridgeTest::Abort, http_bridge)); + io_thread()->message_loop_proxy()->PostTask( + FROM_HERE, + base::Bind(&SyncHttpBridgeTest::Abort, http_bridge)); bool success = http_bridge->MakeSynchronousPost(&os_error, &response_code); EXPECT_FALSE(success); EXPECT_EQ(net::ERR_ABORTED, os_error); @@ -333,11 +328,9 @@ TEST_F(SyncHttpBridgeTest, Abort) { TEST_F(SyncHttpBridgeTest, AbortLate) { scoped_refptr<net::URLRequestContextGetter> ctx_getter( - new TestURLRequestContextGetter( - BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO))); + new TestURLRequestContextGetter(io_thread()->message_loop_proxy())); scoped_refptr<ShuntedHttpBridge> http_bridge(new ShuntedHttpBridge( ctx_getter, this, false)); - http_bridge->SetUserAgent("bob"); http_bridge->SetURL("http://www.google.com", 9999); http_bridge->SetPostPayload("text/plain", 2, " "); @@ -369,8 +362,8 @@ TEST_F(SyncHttpBridgeTest, AbortAndReleaseBeforeFetchComplete) { // Stop IO so we can control order of operations. base::WaitableEvent io_waiter(false, false); - ASSERT_TRUE(BrowserThread::PostTask( - BrowserThread::IO, FROM_HERE, + ASSERT_TRUE(io_thread()->message_loop_proxy()->PostTask( + FROM_HERE, base::Bind(&base::WaitableEvent::Wait, base::Unretained(&io_waiter)))); signal_when_created.Wait(); // Wait till we have a bridge to abort. @@ -387,8 +380,8 @@ TEST_F(SyncHttpBridgeTest, AbortAndReleaseBeforeFetchComplete) { fetcher.set_response_code(200); fetcher.set_cookies(cookies); fetcher.SetResponseString(response_content); - ASSERT_TRUE(BrowserThread::PostTask( - BrowserThread::IO, FROM_HERE, + ASSERT_TRUE(io_thread()->message_loop_proxy()->PostTask( + FROM_HERE, base::Bind(&net::URLFetcherDelegate::OnURLFetchComplete, base::Unretained(delegate), &fetcher))); diff --git a/chrome/browser/sync/glue/http_bridge.h b/sync/internal_api/public/http_bridge.h index c932d10..cec5526 100644 --- a/chrome/browser/sync/glue/http_bridge.h +++ b/sync/internal_api/public/http_bridge.h @@ -2,8 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#ifndef CHROME_BROWSER_SYNC_GLUE_HTTP_BRIDGE_H_ -#define CHROME_BROWSER_SYNC_GLUE_HTTP_BRIDGE_H_ +#ifndef SYNC_INTERNAL_API_PUBLIC_HTTP_BRIDGE_H_ +#define SYNC_INTERNAL_API_PUBLIC_HTTP_BRIDGE_H_ #pragma once #include <string> @@ -50,25 +50,21 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>, // accept-charsets, and proxy service information for bridged requests. // Typically |baseline_context| should be the net::URLRequestContext of the // currently active profile. - explicit RequestContext(net::URLRequestContext* baseline_context); + RequestContext( + net::URLRequestContext* baseline_context, + const scoped_refptr<base::SingleThreadTaskRunner>& + network_task_runner, + const std::string& user_agent); // The destructor MUST be called on the IO thread. virtual ~RequestContext(); - // Set the user agent for requests using this context. The default is - // the browser's UA string. - void set_user_agent(const std::string& ua) { user_agent_ = ua; } - - virtual const std::string& GetUserAgent(const GURL& url) const OVERRIDE { - // If the user agent is set explicitly return that, otherwise call the - // base class method to return default value. - return user_agent_.empty() ? - net::URLRequestContext::GetUserAgent(url) : user_agent_; - } + virtual const std::string& GetUserAgent(const GURL& url) const OVERRIDE; private: - std::string user_agent_; - net::URLRequestContext* baseline_context_; + net::URLRequestContext* const baseline_context_; + const scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_; + const std::string user_agent_; DISALLOW_COPY_AND_ASSIGN(RequestContext); }; @@ -76,11 +72,9 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>, // Lazy-getter for RequestContext objects. class RequestContextGetter : public net::URLRequestContextGetter { public: - explicit RequestContextGetter( - net::URLRequestContextGetter* baseline_context_getter); - - void set_user_agent(const std::string& ua) { user_agent_ = ua; } - bool is_user_agent_set() const { return !user_agent_.empty(); } + RequestContextGetter( + net::URLRequestContextGetter* baseline_context_getter, + const std::string& user_agent); // net::URLRequestContextGetter implementation. virtual net::URLRequestContext* GetURLRequestContext() OVERRIDE; @@ -91,10 +85,10 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>, virtual ~RequestContextGetter(); private: - // User agent to apply to the net::URLRequestContext. - std::string user_agent_; - scoped_refptr<net::URLRequestContextGetter> baseline_context_getter_; + const scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_; + // User agent to apply to the net::URLRequestContext. + const std::string user_agent_; // Lazily initialized by GetURLRequestContext(). scoped_ptr<RequestContext> context_; @@ -105,7 +99,6 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>, explicit HttpBridge(RequestContextGetter* context); // csync::HttpPostProvider implementation. - 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, @@ -156,7 +149,9 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>, // Gets a customized net::URLRequestContext for bridged requests. See // RequestContext definition for details. - scoped_refptr<RequestContextGetter> context_getter_for_request_; + const scoped_refptr<RequestContextGetter> context_getter_for_request_; + + const scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_; // The message loop of the thread we were created on. This is the thread that // will block on MakeSynchronousPost while the IO thread fetches data from @@ -212,8 +207,9 @@ class HttpBridge : public base::RefCountedThreadSafe<HttpBridge>, class HttpBridgeFactory : public csync::HttpPostProviderFactory { public: - explicit HttpBridgeFactory( - net::URLRequestContextGetter* baseline_context_getter); + HttpBridgeFactory( + net::URLRequestContextGetter* baseline_context_getter, + const std::string& user_agent); virtual ~HttpBridgeFactory(); // csync::HttpPostProviderFactory: @@ -225,11 +221,12 @@ class HttpBridgeFactory : public csync::HttpPostProviderFactory { // common components. HttpBridge::RequestContextGetter* GetRequestContextGetter(); - scoped_refptr<HttpBridge::RequestContextGetter> request_context_getter_; + const scoped_refptr<HttpBridge::RequestContextGetter> + request_context_getter_; DISALLOW_COPY_AND_ASSIGN(HttpBridgeFactory); }; } // namespace browser_sync -#endif // CHROME_BROWSER_SYNC_GLUE_HTTP_BRIDGE_H_ +#endif // SYNC_INTERNAL_API_PUBLIC_HTTP_BRIDGE_H_ diff --git a/sync/internal_api/public/http_post_provider_interface.h b/sync/internal_api/public/http_post_provider_interface.h index adee0df..378e18f 100644 --- a/sync/internal_api/public/http_post_provider_interface.h +++ b/sync/internal_api/public/http_post_provider_interface.h @@ -16,10 +16,6 @@ namespace csync { // want to make a subsequent POST. class HttpPostProviderInterface { public: - // Use specified user agent string when POSTing. If not called a default UA - // may be used. - virtual void SetUserAgent(const char* user_agent) = 0; - // Add additional headers to the request. virtual void SetExtraRequestHeaders(const char* headers) = 0; diff --git a/sync/internal_api/public/sync_manager.h b/sync/internal_api/public/sync_manager.h index 86f2b6d..3a2cf2e 100644 --- a/sync/internal_api/public/sync_manager.h +++ b/sync/internal_api/public/sync_manager.h @@ -368,6 +368,9 @@ class SyncManager { // HTTP header. Used internally when collecting stats to classify clients. // |sync_notifier| is owned and used to listen for notifications. // |report_unrecoverable_error_function| may be NULL. + // + // TODO(akalin): Replace the |post_factory| parameter with a + // URLFetcher parameter. bool Init(const FilePath& database_location, const csync::WeakHandle<csync::JsEventHandler>& event_handler, @@ -381,7 +384,6 @@ class SyncManager { csync::ExtensionsActivityMonitor* extensions_activity_monitor, ChangeDelegate* change_delegate, - const std::string& user_agent, const SyncCredentials& credentials, csync::SyncNotifier* sync_notifier, const std::string& restored_key_for_bootstrapping, diff --git a/sync/internal_api/sync_manager.cc b/sync/internal_api/sync_manager.cc index 25eee06..eb5a02d 100644 --- a/sync/internal_api/sync_manager.cc +++ b/sync/internal_api/sync_manager.cc @@ -204,7 +204,6 @@ class SyncManager::SyncInternal csync::ExtensionsActivityMonitor* extensions_activity_monitor, ChangeDelegate* change_delegate, - const std::string& user_agent, const SyncCredentials& credentials, csync::SyncNotifier* sync_notifier, const std::string& restored_key_for_bootstrapping, @@ -720,7 +719,6 @@ bool SyncManager::Init( const std::vector<csync::ModelSafeWorker*>& workers, csync::ExtensionsActivityMonitor* extensions_activity_monitor, ChangeDelegate* change_delegate, - const std::string& user_agent, const SyncCredentials& credentials, csync::SyncNotifier* sync_notifier, const std::string& restored_key_for_bootstrapping, @@ -743,7 +741,6 @@ bool SyncManager::Init( workers, extensions_activity_monitor, change_delegate, - user_agent, credentials, sync_notifier, restored_key_for_bootstrapping, @@ -866,7 +863,6 @@ bool SyncManager::SyncInternal::Init( const std::vector<csync::ModelSafeWorker*>& workers, csync::ExtensionsActivityMonitor* extensions_activity_monitor, ChangeDelegate* change_delegate, - const std::string& user_agent, const SyncCredentials& credentials, csync::SyncNotifier* sync_notifier, const std::string& restored_key_for_bootstrapping, @@ -905,7 +901,7 @@ bool SyncManager::SyncInternal::Init( report_unrecoverable_error_function_)); connection_manager_.reset(new SyncAPIServerConnectionManager( - sync_server_and_path, port, use_ssl, user_agent, post_factory)); + sync_server_and_path, port, use_ssl, post_factory)); net::NetworkChangeNotifier::AddIPAddressObserver(this); observing_ip_address_changes_ = true; diff --git a/sync/internal_api/syncapi_server_connection_manager.cc b/sync/internal_api/syncapi_server_connection_manager.cc index 078c6fc..f2abcc9 100644 --- a/sync/internal_api/syncapi_server_connection_manager.cc +++ b/sync/internal_api/syncapi_server_connection_manager.cc @@ -37,7 +37,6 @@ bool SyncAPIBridgedConnection::Init(const char* path, std::string connection_url = MakeConnectionURL(sync_server, path, use_ssl); HttpPostProviderInterface* http = post_provider_; - http->SetUserAgent(scm_->user_agent().c_str()); http->SetURL(connection_url.c_str(), sync_server_port); if (!auth_token.empty()) { @@ -90,9 +89,8 @@ SyncAPIServerConnectionManager::SyncAPIServerConnectionManager( const std::string& server, int port, bool use_ssl, - const std::string& client_version, HttpPostProviderFactory* factory) - : ServerConnectionManager(server, port, use_ssl, client_version), + : ServerConnectionManager(server, port, use_ssl), post_provider_factory_(factory) { DCHECK(post_provider_factory_.get()); } diff --git a/sync/internal_api/syncapi_server_connection_manager.h b/sync/internal_api/syncapi_server_connection_manager.h index 5a3f952..9678045 100644 --- a/sync/internal_api/syncapi_server_connection_manager.h +++ b/sync/internal_api/syncapi_server_connection_manager.h @@ -55,7 +55,6 @@ class SyncAPIServerConnectionManager SyncAPIServerConnectionManager(const std::string& server, int port, bool use_ssl, - const std::string& client_version, HttpPostProviderFactory* factory); virtual ~SyncAPIServerConnectionManager(); diff --git a/sync/internal_api/syncapi_server_connection_manager_unittest.cc b/sync/internal_api/syncapi_server_connection_manager_unittest.cc index 67b3e25..a6e34d0 100644 --- a/sync/internal_api/syncapi_server_connection_manager_unittest.cc +++ b/sync/internal_api/syncapi_server_connection_manager_unittest.cc @@ -29,7 +29,6 @@ class BlockingHttpPost : public HttpPostProviderInterface { BlockingHttpPost() : wait_for_abort_(false, false) {} virtual ~BlockingHttpPost() {} - 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, @@ -74,7 +73,7 @@ class BlockingHttpPostFactory : public HttpPostProviderFactory { TEST(SyncAPIServerConnectionManagerTest, EarlyAbortPost) { SyncAPIServerConnectionManager server( - "server", 0, true, "1", new BlockingHttpPostFactory()); + "server", 0, true, new BlockingHttpPostFactory()); ServerConnectionManager::PostBufferParams params; ScopedServerStatusWatcher watcher(&server, ¶ms.response); @@ -90,7 +89,7 @@ TEST(SyncAPIServerConnectionManagerTest, EarlyAbortPost) { TEST(SyncAPIServerConnectionManagerTest, AbortPost) { SyncAPIServerConnectionManager server( - "server", 0, true, "1", new BlockingHttpPostFactory()); + "server", 0, true, new BlockingHttpPostFactory()); ServerConnectionManager::PostBufferParams params; ScopedServerStatusWatcher watcher(&server, ¶ms.response); diff --git a/sync/internal_api/syncapi_unittest.cc b/sync/internal_api/syncapi_unittest.cc index 8384de2..1d125ce 100644 --- a/sync/internal_api/syncapi_unittest.cc +++ b/sync/internal_api/syncapi_unittest.cc @@ -653,7 +653,6 @@ 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, @@ -785,7 +784,7 @@ class SyncManagerTest : public testing::Test, "bogus", 0, false, base::MessageLoopProxy::current(), new TestHttpPostProviderFactory(), routing_info, workers, - &extensions_activity_monitor_, this, "bogus", + &extensions_activity_monitor_, this, credentials, sync_notifier_mock_, "", csync::SyncManager::TEST_IN_MEMORY, diff --git a/sync/sync.gyp b/sync/sync.gyp index 7fca6b9..9d4cd1f 100644 --- a/sync/sync.gyp +++ b/sync/sync.gyp @@ -297,6 +297,7 @@ 'internal_api/public/base_transaction.h', 'internal_api/public/change_record.h', 'internal_api/public/configure_reason.h', + 'internal_api/public/http_bridge.h', 'internal_api/public/http_post_provider_factory.h', 'internal_api/public/http_post_provider_interface.h', 'internal_api/public/read_node.h', @@ -312,6 +313,7 @@ 'internal_api/change_reorder_buffer.h', 'internal_api/debug_info_event_listener.cc', 'internal_api/debug_info_event_listener.h', + 'internal_api/http_bridge.cc', 'internal_api/js_mutation_event_observer.cc', 'internal_api/js_mutation_event_observer.h', 'internal_api/js_sync_manager_observer.cc', @@ -639,6 +641,7 @@ 'dependencies': [ '../base/base.gyp:base', '../net/net.gyp:net', + '../net/net.gyp:net_test_support', '../testing/gmock.gyp:gmock', '../testing/gtest.gyp:gtest', 'protocol/sync_proto.gyp:sync_proto', @@ -652,6 +655,7 @@ 'export_dependent_settings': [ '../base/base.gyp:base', '../net/net.gyp:net', + '../net/net.gyp:net_test_support', '../testing/gmock.gyp:gmock', '../testing/gtest.gyp:gtest', 'protocol/sync_proto.gyp:sync_proto', @@ -668,6 +672,7 @@ 'sources': [ 'internal_api/public/change_record_unittest.cc', 'internal_api/debug_info_event_listener_unittest.cc', + 'internal_api/http_bridge_unittest.cc', 'internal_api/js_mutation_event_observer_unittest.cc', 'internal_api/js_sync_manager_observer_unittest.cc', 'internal_api/syncapi_server_connection_manager_unittest.cc', diff --git a/sync/test/engine/mock_connection_manager.cc b/sync/test/engine/mock_connection_manager.cc index 4bf97f5..ab29b9a 100644 --- a/sync/test/engine/mock_connection_manager.cc +++ b/sync/test/engine/mock_connection_manager.cc @@ -41,7 +41,7 @@ using syncable::WriteTransaction; static char kValidAuthToken[] = "AuthToken"; MockConnectionManager::MockConnectionManager(syncable::Directory* directory) - : ServerConnectionManager("unused", 0, false, "version"), + : ServerConnectionManager("unused", 0, false), server_reachable_(true), conflict_all_commits_(false), conflict_n_commits_(0), |