Skip to content

Commit

Permalink
Migrate FinancialPing to SimpleURLLoader
Browse files Browse the repository at this point in the history
URLFetcher will stop working with advent of Network Service, and
SimpleURLLoader is the replacement API for most clients.
This CL migrates RLZTracker and FinancialPing away from URLFetcher.

Bug=773295,844989

Change-Id: I07fc8d09b359727ad8649e38355dd6af2bc60b02
Reviewed-on: https://chromium-review.googlesource.com/1142749
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Carlos Pizano <cpu@chromium.org>
Reviewed-by: Roger Tawa <rogerta@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582698}
  • Loading branch information
tonikitoo authored and Commit Bot committed Aug 13, 2018
1 parent cfb4c38 commit a739bf4
Show file tree
Hide file tree
Showing 16 changed files with 189 additions and 129 deletions.
6 changes: 4 additions & 2 deletions chrome/browser/rlz/chrome_rlz_tracker_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include "content/public/browser/notification_source.h"
#include "content/public/common/content_switches.h"
#include "rlz/buildflags/buildflags.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"

#if defined(OS_WIN)
#include "chrome/installer/util/google_update_settings.h"
Expand Down Expand Up @@ -115,8 +116,9 @@ bool ChromeRLZTrackerDelegate::IsOnUIThread() {
return content::BrowserThread::CurrentlyOn(content::BrowserThread::UI);
}

net::URLRequestContextGetter* ChromeRLZTrackerDelegate::GetRequestContext() {
return g_browser_process->system_request_context();
scoped_refptr<network::SharedURLLoaderFactory>
ChromeRLZTrackerDelegate::GetURLLoaderFactory() {
return g_browser_process->shared_url_loader_factory();
}

bool ChromeRLZTrackerDelegate::GetBrand(std::string* brand) {
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/rlz/chrome_rlz_tracker_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class ChromeRLZTrackerDelegate : public rlz::RLZTrackerDelegate,
// RLZTrackerDelegate implementation.
void Cleanup() override;
bool IsOnUIThread() override;
net::URLRequestContextGetter* GetRequestContext() override;
scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory() override;
bool GetBrand(std::string* brand) override;
bool IsBrandOrganic(const std::string& brand) override;
bool GetReactivationBrand(std::string* brand) override;
Expand Down
2 changes: 2 additions & 0 deletions components/rlz/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ static_library("rlz") {
"//components/google/core/browser",
"//net",
"//rlz:rlz_lib",
"//services/network/public/cpp:cpp",
]

if (is_ios) {
Expand All @@ -35,6 +36,7 @@ source_set("unit_tests") {
":rlz",
"//net:test_support",
"//rlz:test_support",
"//services/network/public/cpp:cpp",
"//ui/base",
]

Expand Down
1 change: 1 addition & 0 deletions components/rlz/DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ include_rules = [
"+net",
"+rlz",
"+ui/base",
"+services/network/public",

# rlz is used on iOS.
"-content",
Expand Down
63 changes: 55 additions & 8 deletions components/rlz/rlz_tracker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "base/trace_event/trace_event.h"
#include "build/build_config.h"
#include "components/rlz/rlz_tracker_delegate.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"

#if defined(OS_CHROMEOS)
#include "base/syslog_logging.h"
Expand Down Expand Up @@ -151,16 +152,60 @@ bool SendFinancialPing(const std::string& brand,
#else
product_signature = "chrome";
#endif
return rlz_lib::SendFinancialPing(rlz_lib::CHROME, points,
product_signature.c_str(),
brand.c_str(), referral_ascii.c_str(),
lang_ascii.c_str(), false, true);
return rlz_lib::SendFinancialPing(
rlz_lib::CHROME, points, product_signature.c_str(), brand.c_str(),
referral_ascii.c_str(), lang_ascii.c_str(), false, true);
}

} // namespace

RLZTracker* RLZTracker::tracker_ = nullptr;

// WrapperURLLoaderFactory subclasses mojom::URLLoaderFactory as non-mojo, cross
// thread class. It basically posts ::CreateLoaderAndStart calls over to the UI
// thread, to call them on the real mojo object.
class RLZTracker::WrapperURLLoaderFactory
: public network::mojom::URLLoaderFactory {
public:
explicit WrapperURLLoaderFactory(
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
: url_loader_factory_(std::move(url_loader_factory)),
main_thread_task_runner_(base::SequencedTaskRunnerHandle::Get()) {}

void CreateLoaderAndStart(network::mojom::URLLoaderRequest loader,
int32_t routing_id,
int32_t request_id,
uint32_t options,
const network::ResourceRequest& request,
network::mojom::URLLoaderClientPtr client,
const net::MutableNetworkTrafficAnnotationTag&
traffic_annotation) override {
if (main_thread_task_runner_->RunsTasksInCurrentSequence()) {
url_loader_factory_->CreateLoaderAndStart(
std::move(loader), routing_id, request_id, options, request,
std::move(client), traffic_annotation);
} else {
main_thread_task_runner_->PostTask(
FROM_HERE,
base::BindOnce(&WrapperURLLoaderFactory::CreateLoaderAndStart,
base::Unretained(this), std::move(loader), routing_id,
request_id, options, request, std::move(client),
traffic_annotation));
}
}
void Clone(network::mojom::URLLoaderFactoryRequest factory) override {
NOTIMPLEMENTED();
}

private:
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;

// Runner for RLZ main thread tasks.
scoped_refptr<base::SequencedTaskRunner> main_thread_task_runner_;

DISALLOW_COPY_AND_ASSIGN(WrapperURLLoaderFactory);
};

// static
RLZTracker* RLZTracker::GetInstance() {
return tracker_ ? tracker_ : base::Singleton<RLZTracker>::get();
Expand Down Expand Up @@ -261,9 +306,11 @@ bool RLZTracker::Init(bool first_run,
#endif

// Could be null; don't run if so. RLZ will try again next restart.
net::URLRequestContextGetter* context_getter = delegate_->GetRequestContext();
if (context_getter) {
rlz_lib::SetURLRequestContext(context_getter);
auto shared_url_loader_factory = delegate_->GetURLLoaderFactory();
if (shared_url_loader_factory) {
custom_url_loader_factory_ =
std::make_unique<WrapperURLLoaderFactory>(shared_url_loader_factory);
rlz_lib::SetURLLoaderFactory(custom_url_loader_factory_.get());
ScheduleDelayedInit(delay);
}

Expand Down Expand Up @@ -571,7 +618,7 @@ bool RLZTracker::ScheduleClearRlzState() {
// static
void RLZTracker::CleanupRlz() {
GetInstance()->Cleanup();
rlz_lib::SetURLRequestContext(nullptr);
rlz_lib::SetURLLoaderFactory(nullptr);
}

// static
Expand Down
5 changes: 4 additions & 1 deletion components/rlz/rlz_tracker.h
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,10 @@ class RLZTracker {
// Minimum delay before sending financial ping after initialization.
base::TimeDelta min_init_delay_;

// Runner for RLZ background tasks. The checked is used to verify operations
class WrapperURLLoaderFactory;
std::unique_ptr<WrapperURLLoaderFactory> custom_url_loader_factory_;

// Runner for RLZ background tasks. The checker is used to verify operations
// occur in the correct sequence, especially in tests.
scoped_refptr<base::SequencedTaskRunner> background_task_runner_;
SEQUENCE_CHECKER(sequence_checker_);
Expand Down
11 changes: 6 additions & 5 deletions components/rlz/rlz_tracker_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@
#include "base/macros.h"
#include "base/strings/string16.h"

namespace net {
class URLRequestContextGetter;
}
namespace network {
class SharedURLLoaderFactory;
} // namespace network

namespace rlz {

Expand All @@ -30,8 +30,9 @@ class RLZTrackerDelegate {
// Returns whether the current thread is the UI thread.
virtual bool IsOnUIThread() = 0;

// Returns the URLRequestContextGetter to use for network connections.
virtual net::URLRequestContextGetter* GetRequestContext() = 0;
// Returns the SharedURLLoaderFactory to use for network connections.
virtual scoped_refptr<network::SharedURLLoaderFactory>
GetURLLoaderFactory() = 0;

// Returns the brand code for the installation of Chrome in |brand| and a
// boolean indicating whether the operation was a success or not.
Expand Down
7 changes: 5 additions & 2 deletions components/rlz/rlz_tracker_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "components/rlz/rlz_tracker_delegate.h"
#include "net/url_request/url_request_test_util.h"
#include "rlz/test/rlz_test_helpers.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"
#include "testing/gtest/include/gtest/gtest.h"

#if defined(OS_IOS)
Expand Down Expand Up @@ -71,8 +72,10 @@ class TestRLZTrackerDelegate : public RLZTrackerDelegate {

bool IsOnUIThread() override { return true; }

net::URLRequestContextGetter* GetRequestContext() override {
return request_context_getter_.get();
scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory()
override {
NOTIMPLEMENTED() << "If this is called, it needs an implementation.";
return nullptr;
}

bool GetBrand(std::string* brand) override {
Expand Down
5 changes: 5 additions & 0 deletions rlz/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ if (!is_android) {
"//base",
"//base/third_party/dynamic_annotations",
"//net",
"//services/network/public/cpp:cpp",
"//services/network/public/mojom",
"//url",
]

Expand Down Expand Up @@ -121,6 +123,7 @@ if (!is_android) {
":rlz_lib",
"//base",
"//base/test:test_support",
"//services/network/public/cpp:cpp",
"//testing/gtest",
]
if (is_chromeos) {
Expand Down Expand Up @@ -149,7 +152,9 @@ if (!is_android) {
":rlz_utils",
":test_support",
"//base",
"//mojo/core/embedder",
"//net:test_support",
"//services/network:test_support",
"//testing/gmock",
"//testing/gtest",
"//third_party/zlib",
Expand Down
3 changes: 3 additions & 0 deletions rlz/DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,11 @@ include_rules = [
"+build",
"+chromeos/dbus",
"+chromeos/system",
"+mojo/core/embedder",
"+net", # This is only used when force_rlz_use_chrome_net=1 is passed to gyp.
"+third_party/zlib",
"+services/network/public",
"+services/network/test",
]

hooks = [
Expand Down
Loading

0 comments on commit a739bf4

Please sign in to comment.