Migrate FeedbackUploader to SimpleURLLoader.
This is a copy of https://crrev.com/c/1067424 but with tests fixed.
Bug: 844932
Change-Id: I77239bf76a673b33d56b8b7fe584a78ec319bc24
Reviewed-on: https://chromium-review.googlesource.com/1114253
Commit-Queue: Robbie McElrath <[email protected]>
Reviewed-by: Matt Menke <[email protected]>
Reviewed-by: Ahmed Fakhry <[email protected]>
Reviewed-by: Trent Apted <[email protected]>
Cr-Commit-Position: refs/heads/master@{#573467}diff --git a/components/feedback/BUILD.gn b/components/feedback/BUILD.gn
index a3fa1d10..9278c2fb 100644
--- a/components/feedback/BUILD.gn
+++ b/components/feedback/BUILD.gn
@@ -16,8 +16,6 @@
"feedback_switches.h",
"feedback_uploader.cc",
"feedback_uploader.h",
- "feedback_uploader_delegate.cc",
- "feedback_uploader_delegate.h",
"feedback_uploader_factory.cc",
"feedback_uploader_factory.h",
"feedback_util.cc",
@@ -40,6 +38,7 @@
"//content/public/browser",
"//content/public/common",
"//net",
+ "//services/network/public/cpp",
"//third_party/re2",
"//third_party/zlib/google:zip",
]
@@ -65,6 +64,8 @@
"//components/variations/net",
"//content/test:test_support",
"//net:test_support",
+ "//services/network:test_support",
+ "//services/network/public/cpp",
"//testing/gmock",
"//testing/gtest",
]
diff --git a/components/feedback/DEPS b/components/feedback/DEPS
index 2d72d20..5d02e2060 100644
--- a/components/feedback/DEPS
+++ b/components/feedback/DEPS
@@ -11,6 +11,9 @@
"+net/base",
"+net/traffic_annotation",
"+net/url_request",
+ "+net/http",
+ "+services/network/public/cpp",
+ "+services/network/test",
"+third_party/re2",
"+third_party/zlib/google",
]
diff --git a/components/feedback/feedback_data_unittest.cc b/components/feedback/feedback_data_unittest.cc
index 06fb1ff..d5e7baf 100644
--- a/components/feedback/feedback_data_unittest.cc
+++ b/components/feedback/feedback_data_unittest.cc
@@ -14,6 +14,9 @@
#include "components/prefs/testing_pref_service.h"
#include "content/public/test/test_browser_context.h"
#include "content/public/test/test_browser_thread_bundle.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
+#include "services/network/test/test_url_loader_factory.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace feedback {
@@ -26,18 +29,21 @@
class MockUploader : public FeedbackUploader {
public:
- MockUploader(content::BrowserContext* context,
- const base::Closure& on_report_sent)
- : FeedbackUploader(context,
+ MockUploader(
+ scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
+ content::BrowserContext* context,
+ base::OnceClosure on_report_sent)
+ : FeedbackUploader(url_loader_factory,
+ context,
FeedbackUploaderFactory::CreateUploaderTaskRunner()),
- on_report_sent_(on_report_sent) {}
+ on_report_sent_(std::move(on_report_sent)) {}
~MockUploader() override {}
// feedback::FeedbackUploader:
- void StartDispatchingReport() override { on_report_sent_.Run(); }
+ void StartDispatchingReport() override { std::move(on_report_sent_).Run(); }
private:
- base::Closure on_report_sent_;
+ base::OnceClosure on_report_sent_;
DISALLOW_COPY_AND_ASSIGN(MockUploader);
};
@@ -51,9 +57,13 @@
class FeedbackDataTest : public testing::Test {
protected:
FeedbackDataTest()
- : uploader_(&context_,
- base::Bind(&FeedbackDataTest::set_send_report_callback,
- base::Unretained(this))),
+ : test_shared_loader_factory_(
+ base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
+ &test_url_loader_factory_)),
+ uploader_(test_shared_loader_factory_,
+ &context_,
+ base::BindOnce(&FeedbackDataTest::set_send_report_callback,
+ base::Unretained(this))),
data_(base::MakeRefCounted<FeedbackData>(&uploader_)) {}
~FeedbackDataTest() override = default;
@@ -79,6 +89,8 @@
base::Closure quit_closure_;
std::unique_ptr<base::RunLoop> run_loop_;
content::TestBrowserThreadBundle test_browser_thread_bundle_;
+ network::TestURLLoaderFactory test_url_loader_factory_;
+ scoped_refptr<network::SharedURLLoaderFactory> test_shared_loader_factory_;
content::TestBrowserContext context_;
MockUploader uploader_;
scoped_refptr<FeedbackData> data_;
diff --git a/components/feedback/feedback_uploader.cc b/components/feedback/feedback_uploader.cc
index 6b70f3a..77bae900 100644
--- a/components/feedback/feedback_uploader.cc
+++ b/components/feedback/feedback_uploader.cc
@@ -9,12 +9,14 @@
#include "components/data_use_measurement/core/data_use_user_data.h"
#include "components/feedback/feedback_report.h"
#include "components/feedback/feedback_switches.h"
-#include "components/feedback/feedback_uploader_delegate.h"
#include "components/variations/net/variations_http_headers.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/storage_partition.h"
#include "net/base/load_flags.h"
#include "net/url_request/url_fetcher.h"
+#include "services/network/public/cpp/resource_request.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/simple_url_loader.h"
namespace feedback {
@@ -28,6 +30,11 @@
constexpr char kProtoBufMimeType[] = "application/x-protobuf";
+constexpr int kHttpPostSuccessNoContent = 204;
+constexpr int kHttpPostFailNoConnection = -1;
+constexpr int kHttpPostFailClientError = 400;
+constexpr int kHttpPostFailServerError = 500;
+
// The minimum time to wait before uploading reports are retried. Exponential
// backoff delay is applied on successive failures.
// This value can be overriden by tests by calling
@@ -53,9 +60,11 @@
} // namespace
FeedbackUploader::FeedbackUploader(
+ scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
content::BrowserContext* context,
scoped_refptr<base::SingleThreadTaskRunner> task_runner)
- : context_(context),
+ : url_loader_factory_(std::move(url_loader_factory)),
+ context_(context),
feedback_reports_path_(GetPathFromContext(context)),
task_runner_(task_runner),
feedback_post_url_(GetFeedbackPostGURL()),
@@ -116,7 +125,7 @@
}
void FeedbackUploader::AppendExtraHeadersToUploadRequest(
- net::URLFetcher* fetcher) {}
+ network::ResourceRequest* resource_request) {}
void FeedbackUploader::DispatchReport() {
net::NetworkTrafficAnnotationTag traffic_annotation =
@@ -148,39 +157,80 @@
"by direct user request."
policy_exception_justification: "Not implemented."
})");
- // Note: FeedbackUploaderDelegate deletes itself and the fetcher.
- net::URLFetcher* fetcher =
- net::URLFetcher::Create(
- feedback_post_url_, net::URLFetcher::POST,
- new FeedbackUploaderDelegate(
- base::Bind(&FeedbackUploader::OnReportUploadSuccess, AsWeakPtr()),
- base::Bind(&FeedbackUploader::OnReportUploadFailure,
- AsWeakPtr())),
- traffic_annotation)
- .release();
- data_use_measurement::DataUseUserData::AttachToFetcher(
- fetcher, data_use_measurement::DataUseUserData::FEEDBACK_UPLOADER);
+ auto resource_request = std::make_unique<network::ResourceRequest>();
+ resource_request->url = feedback_post_url_;
+ resource_request->load_flags =
+ net::LOAD_DO_NOT_SAVE_COOKIES | net::LOAD_DO_NOT_SEND_COOKIES;
+ resource_request->method = "POST";
+
// Tell feedback server about the variation state of this install.
- net::HttpRequestHeaders headers;
// Note: It's OK to pass SignedIn::kNo if it's unknown, as it does not affect
// transmission of experiments coming from the variations server.
- variations::AppendVariationHeaders(fetcher->GetOriginalURL(),
- context_->IsOffTheRecord()
- ? variations::InIncognito::kYes
- : variations::InIncognito::kNo,
- variations::SignedIn::kNo, &headers);
- fetcher->SetExtraRequestHeaders(headers.ToString());
+ variations::AppendVariationHeaders(
+ feedback_post_url_,
+ context_->IsOffTheRecord() ? variations::InIncognito::kYes
+ : variations::InIncognito::kNo,
+ variations::SignedIn::kNo, &resource_request->headers);
- fetcher->SetUploadData(kProtoBufMimeType, report_being_dispatched_->data());
- fetcher->SetRequestContext(
- content::BrowserContext::GetDefaultStoragePartition(context_)
- ->GetURLRequestContext());
+ AppendExtraHeadersToUploadRequest(resource_request.get());
- AppendExtraHeadersToUploadRequest(fetcher);
+ std::unique_ptr<network::SimpleURLLoader> simple_url_loader =
+ network::SimpleURLLoader::Create(std::move(resource_request),
+ traffic_annotation);
+ network::SimpleURLLoader* simple_url_loader_ptr = simple_url_loader.get();
+ simple_url_loader->AttachStringForUpload(report_being_dispatched_->data(),
+ kProtoBufMimeType);
+ auto it = uploads_in_progress_.insert(uploads_in_progress_.begin(),
+ std::move(simple_url_loader));
+ // TODO(https://crbug.com/808498): Re-add data use measurement once
+ // SimpleURLLoader supports it.
+ // ID=data_use_measurement::DataUseUserData::FEEDBACK_UPLOADER
+ simple_url_loader_ptr->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+ url_loader_factory_.get(),
+ base::BindOnce(&FeedbackUploader::OnDispatchComplete,
+ base::Unretained(this), std::move(it)));
+}
- fetcher->SetLoadFlags(net::LOAD_DO_NOT_SAVE_COOKIES |
- net::LOAD_DO_NOT_SEND_COOKIES);
- fetcher->Start();
+void FeedbackUploader::OnDispatchComplete(
+ UrlLoaderList::iterator it,
+ std::unique_ptr<std::string> response_body) {
+ std::stringstream error_stream;
+ network::SimpleURLLoader* simple_url_loader = it->get();
+ int response_code = kHttpPostFailNoConnection;
+ if (simple_url_loader->ResponseInfo() &&
+ simple_url_loader->ResponseInfo()->headers) {
+ response_code = simple_url_loader->ResponseInfo()->headers->response_code();
+ }
+ if (response_code == kHttpPostSuccessNoContent) {
+ error_stream << "Success";
+ OnReportUploadSuccess();
+ } else {
+ bool should_retry = true;
+ // Process the error for debug output
+ if (response_code == kHttpPostFailNoConnection) {
+ error_stream << "No connection to server.";
+ } else if ((response_code >= kHttpPostFailClientError) &&
+ (response_code < kHttpPostFailServerError)) {
+ // Client errors mean that the server failed to parse the proto that was
+ // sent, or that some requirements weren't met by the server side
+ // validation, and hence we should NOT retry sending this corrupt report
+ // and give up.
+ should_retry = false;
+
+ error_stream << "Client error: HTTP response code " << response_code;
+ } else if (response_code >= kHttpPostFailServerError) {
+ error_stream << "Server error: HTTP response code " << response_code;
+ } else {
+ error_stream << "Unknown error: HTTP response code " << response_code;
+ }
+
+ OnReportUploadFailure(should_retry);
+ }
+
+ LOG(WARNING) << "FEEDBACK: Submission to feedback server ("
+ << simple_url_loader->GetFinalURL()
+ << ") status: " << error_stream.str();
+ uploads_in_progress_.erase(it);
}
void FeedbackUploader::UpdateUploadTimer() {
diff --git a/components/feedback/feedback_uploader.h b/components/feedback/feedback_uploader.h
index 1c500fe..816fa25 100644
--- a/components/feedback/feedback_uploader.h
+++ b/components/feedback/feedback_uploader.h
@@ -5,6 +5,7 @@
#ifndef COMPONENTS_FEEDBACK_FEEDBACK_UPLOADER_H_
#define COMPONENTS_FEEDBACK_FEEDBACK_UPLOADER_H_
+#include <list>
#include <queue>
#include <vector>
@@ -22,9 +23,11 @@
class BrowserContext;
} // namespace content
-namespace net {
-class URLFetcher;
-} // namespace net
+namespace network {
+struct ResourceRequest;
+class SimpleURLLoader;
+class SharedURLLoaderFactory;
+} // namespace network
namespace feedback {
@@ -36,8 +39,10 @@
class FeedbackUploader : public KeyedService,
public base::SupportsWeakPtr<FeedbackUploader> {
public:
- FeedbackUploader(content::BrowserContext* context,
- scoped_refptr<base::SingleThreadTaskRunner> task_runner);
+ FeedbackUploader(
+ scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
+ content::BrowserContext* context,
+ scoped_refptr<base::SingleThreadTaskRunner> task_runner);
~FeedbackUploader() override;
static void SetMinimumRetryDelayForTesting(base::TimeDelta delay);
@@ -84,6 +89,9 @@
private:
friend class FeedbackUploaderTest;
+ // This is a std::list so that iterators remain valid during modifications.
+ using UrlLoaderList = std::list<std::unique_ptr<network::SimpleURLLoader>>;
+
struct ReportsUploadTimeComparator {
bool operator()(const scoped_refptr<FeedbackReport>& a,
const scoped_refptr<FeedbackReport>& b) const;
@@ -91,19 +99,26 @@
// Called from DispatchReport() to give implementers a chance to add extra
// headers to the upload request before it's sent.
- virtual void AppendExtraHeadersToUploadRequest(net::URLFetcher* fetcher);
+ virtual void AppendExtraHeadersToUploadRequest(
+ network::ResourceRequest* resource_request);
// Uploads the |report_being_dispatched_| to be uploaded. It must
// call either OnReportUploadSuccess() or OnReportUploadFailure() so that
// dispatching reports can progress.
void DispatchReport();
+ void OnDispatchComplete(UrlLoaderList::iterator it,
+ std::unique_ptr<std::string> response_body);
+
// Update our timer for uploading the next report.
void UpdateUploadTimer();
void QueueReportWithDelay(std::unique_ptr<std::string> data,
base::TimeDelta delay);
+ // URLLoaderFactory used for network requests.
+ scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
+
// Browser context this uploader was created for.
content::BrowserContext* context_;
@@ -132,6 +147,8 @@
// at-a-time should be dispatched.
bool is_dispatching_;
+ UrlLoaderList uploads_in_progress_;
+
DISALLOW_COPY_AND_ASSIGN(FeedbackUploader);
};
diff --git a/components/feedback/feedback_uploader_delegate.cc b/components/feedback/feedback_uploader_delegate.cc
deleted file mode 100644
index fd7e475a..0000000
--- a/components/feedback/feedback_uploader_delegate.cc
+++ /dev/null
@@ -1,71 +0,0 @@
-// Copyright 2014 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "components/feedback/feedback_uploader_delegate.h"
-
-#include <memory>
-#include <sstream>
-
-#include "base/logging.h"
-#include "components/feedback/feedback_report.h"
-#include "net/url_request/url_fetcher.h"
-
-namespace feedback {
-
-namespace {
-
-constexpr int kHttpPostSuccessNoContent = 204;
-constexpr int kHttpPostFailNoConnection = -1;
-constexpr int kHttpPostFailClientError = 400;
-constexpr int kHttpPostFailServerError = 500;
-
-} // namespace
-
-FeedbackUploaderDelegate::FeedbackUploaderDelegate(
- const base::Closure& success_callback,
- const ReportFailureCallback& error_callback)
- : success_callback_(success_callback), error_callback_(error_callback) {}
-
-FeedbackUploaderDelegate::~FeedbackUploaderDelegate() {}
-
-void FeedbackUploaderDelegate::OnURLFetchComplete(
- const net::URLFetcher* source) {
- std::unique_ptr<const net::URLFetcher> source_scoper(source);
-
- std::stringstream error_stream;
- int response_code = source->GetResponseCode();
- if (response_code == kHttpPostSuccessNoContent) {
- error_stream << "Success";
- success_callback_.Run();
- } else {
- bool should_retry = true;
- // Process the error for debug output
- if (response_code == kHttpPostFailNoConnection) {
- error_stream << "No connection to server.";
- } else if ((response_code >= kHttpPostFailClientError) &&
- (response_code < kHttpPostFailServerError)) {
- // Client errors mean that the server failed to parse the proto that was
- // sent, or that some requirements weren't met by the server side
- // validation, and hence we should NOT retry sending this corrupt report
- // and give up.
- should_retry = false;
-
- error_stream << "Client error: HTTP response code " << response_code;
- } else if (response_code >= kHttpPostFailServerError) {
- error_stream << "Server error: HTTP response code " << response_code;
- } else {
- error_stream << "Unknown error: HTTP response code " << response_code;
- }
-
- error_callback_.Run(should_retry);
- }
-
- LOG(WARNING) << "FEEDBACK: Submission to feedback server ("
- << source->GetURL() << ") status: " << error_stream.str();
-
- // This instance won't be used for anything else, delete us.
- delete this;
-}
-
-} // namespace feedback
diff --git a/components/feedback/feedback_uploader_delegate.h b/components/feedback/feedback_uploader_delegate.h
deleted file mode 100644
index 817b53f..0000000
--- a/components/feedback/feedback_uploader_delegate.h
+++ /dev/null
@@ -1,43 +0,0 @@
-// Copyright 2014 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef COMPONENTS_FEEDBACK_FEEDBACK_UPLOADER_DELEGATE_H_
-#define COMPONENTS_FEEDBACK_FEEDBACK_UPLOADER_DELEGATE_H_
-
-#include "base/callback.h"
-#include "base/macros.h"
-#include "base/memory/ref_counted.h"
-#include "components/feedback/feedback_uploader.h"
-#include "net/url_request/url_fetcher_delegate.h"
-
-namespace feedback {
-
-// Type of the callback that gets invoked when uploading a feedback report
-// fails. |should_retry| is set to true when it's OK to retry sending the
-// report; e.g. when the failure is not a client error and retries is likely to
-// fail again.
-using ReportFailureCallback = base::Callback<void(bool should_retry)>;
-
-// FeedbackUploaderDelegate is a simple HTTP uploader for a feedback report.
-// When finished, it runs the appropriate callback passed in via the
-// constructor, and then deletes itself.
-class FeedbackUploaderDelegate : public net::URLFetcherDelegate {
- public:
- FeedbackUploaderDelegate(const base::Closure& success_callback,
- const ReportFailureCallback& error_callback);
- ~FeedbackUploaderDelegate() override;
-
- private:
- // Overridden from net::URLFetcherDelegate.
- void OnURLFetchComplete(const net::URLFetcher* source) override;
-
- base::Closure success_callback_;
- ReportFailureCallback error_callback_;
-
- DISALLOW_COPY_AND_ASSIGN(FeedbackUploaderDelegate);
-};
-
-} // namespace feedback
-
-#endif // COMPONENTS_FEEDBACK_FEEDBACK_UPLOADER_DELEGATE_H_
diff --git a/components/feedback/feedback_uploader_dispatch_unittest.cc b/components/feedback/feedback_uploader_dispatch_unittest.cc
index 300d3d70..351a07c 100644
--- a/components/feedback/feedback_uploader_dispatch_unittest.cc
+++ b/components/feedback/feedback_uploader_dispatch_unittest.cc
@@ -11,13 +11,16 @@
#include "base/run_loop.h"
#include "base/task_scheduler/post_task.h"
#include "base/task_scheduler/task_traits.h"
+#include "base/test/bind_test_util.h"
#include "components/feedback/feedback_uploader_factory.h"
#include "components/variations/variations_associated_data.h"
#include "components/variations/variations_http_header_provider.h"
#include "content/public/test/test_browser_context.h"
#include "content/public/test/test_browser_thread_bundle.h"
-#include "net/url_request/test_url_fetcher_factory.h"
-#include "net/url_request/url_fetcher_delegate.h"
+#include "net/http/http_util.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
+#include "services/network/test/test_url_loader_factory.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace feedback {
@@ -27,9 +30,8 @@
constexpr base::TimeDelta kTestRetryDelay =
base::TimeDelta::FromMilliseconds(1);
-constexpr int kHttpPostSuccessNoContent = 204;
-constexpr int kHttpPostFailClientError = 400;
-constexpr int kHttpPostFailServerError = 500;
+constexpr char kFeedbackPostUrl[] =
+ "https://www.google.com/tools/feedback/chrome/__submit";
void QueueReport(FeedbackUploader* uploader, const std::string& report_data) {
uploader->QueueReport(std::make_unique<std::string>(report_data));
@@ -40,7 +42,10 @@
class FeedbackUploaderDispatchTest : public ::testing::Test {
protected:
FeedbackUploaderDispatchTest()
- : browser_thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP) {}
+ : browser_thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
+ shared_url_loader_factory_(
+ base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
+ &test_url_loader_factory_)) {}
~FeedbackUploaderDispatchTest() override {
// Clean up registered ids.
@@ -58,11 +63,21 @@
base::FieldTrialList::CreateFieldTrial(trial_name, group_name)->group();
}
+ scoped_refptr<network::SharedURLLoaderFactory> shared_url_loader_factory() {
+ return shared_url_loader_factory_;
+ }
+
+ network::TestURLLoaderFactory* test_url_loader_factory() {
+ return &test_url_loader_factory_;
+ }
+
content::BrowserContext* context() { return &context_; }
private:
content::TestBrowserThreadBundle browser_thread_bundle_;
content::TestBrowserContext context_;
+ network::TestURLLoaderFactory test_url_loader_factory_;
+ scoped_refptr<network::SharedURLLoaderFactory> shared_url_loader_factory_;
DISALLOW_COPY_AND_ASSIGN(FeedbackUploaderDispatchTest);
};
@@ -76,54 +91,89 @@
CreateFieldTrialWithId("Test", "Group1", 123);
FeedbackUploader uploader(
- context(), FeedbackUploaderFactory::CreateUploaderTaskRunner());
+ shared_url_loader_factory(), context(),
+ FeedbackUploaderFactory::CreateUploaderTaskRunner());
- net::TestURLFetcherFactory factory;
- QueueReport(&uploader, "test");
-
- net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
net::HttpRequestHeaders headers;
- fetcher->GetExtraRequestHeaders(&headers);
+ test_url_loader_factory()->SetInterceptor(
+ base::BindLambdaForTesting([&](const network::ResourceRequest& request) {
+ headers = request.headers;
+ }));
+
+ QueueReport(&uploader, "test");
+ base::RunLoop().RunUntilIdle();
+
std::string value;
EXPECT_TRUE(headers.GetHeader("X-Client-Data", &value));
EXPECT_FALSE(value.empty());
- // The fetcher's delegate is responsible for freeing the fetcher (and itself).
- fetcher->delegate()->OnURLFetchComplete(fetcher);
variations::VariationsHttpHeaderProvider::GetInstance()->ResetForTesting();
}
-TEST_F(FeedbackUploaderDispatchTest, TestVariousServerResponses) {
+TEST_F(FeedbackUploaderDispatchTest, 204Response) {
FeedbackUploader::SetMinimumRetryDelayForTesting(kTestRetryDelay);
FeedbackUploader uploader(
- context(), FeedbackUploaderFactory::CreateUploaderTaskRunner());
+ shared_url_loader_factory(), context(),
+ FeedbackUploaderFactory::CreateUploaderTaskRunner());
EXPECT_EQ(kTestRetryDelay, uploader.retry_delay());
// Successful reports should not introduce any retries, and should not
// increase the backoff delay.
- net::TestURLFetcherFactory factory;
- factory.set_remove_fetcher_on_delete(true);
+ network::ResourceResponseHead head;
+ std::string headers("HTTP/1.1 204 No Content\n\n");
+ head.headers = base::MakeRefCounted<net::HttpResponseHeaders>(
+ net::HttpUtil::AssembleRawHeaders(headers.c_str(), headers.size()));
+ network::URLLoaderCompletionStatus status;
+ test_url_loader_factory()->AddResponse(GURL(kFeedbackPostUrl), head, "",
+ status);
QueueReport(&uploader, "Successful report");
- net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
- fetcher->set_response_code(kHttpPostSuccessNoContent);
- fetcher->delegate()->OnURLFetchComplete(fetcher);
+ base::RunLoop().RunUntilIdle();
+
EXPECT_EQ(kTestRetryDelay, uploader.retry_delay());
EXPECT_TRUE(uploader.QueueEmpty());
+}
+TEST_F(FeedbackUploaderDispatchTest, 400Response) {
+ FeedbackUploader::SetMinimumRetryDelayForTesting(kTestRetryDelay);
+ FeedbackUploader uploader(
+ shared_url_loader_factory(), context(),
+ FeedbackUploaderFactory::CreateUploaderTaskRunner());
+
+ EXPECT_EQ(kTestRetryDelay, uploader.retry_delay());
// Failed reports due to client errors are not retried. No backoff delay
// should be doubled.
+ network::ResourceResponseHead head;
+ std::string headers("HTTP/1.1 400 Bad Request\n\n");
+ head.headers = base::MakeRefCounted<net::HttpResponseHeaders>(
+ net::HttpUtil::AssembleRawHeaders(headers.c_str(), headers.size()));
+ network::URLLoaderCompletionStatus status;
+ test_url_loader_factory()->AddResponse(GURL(kFeedbackPostUrl), head, "",
+ status);
QueueReport(&uploader, "Client error failed report");
- fetcher = factory.GetFetcherByID(0);
- fetcher->set_response_code(kHttpPostFailClientError);
- fetcher->delegate()->OnURLFetchComplete(fetcher);
+ base::RunLoop().RunUntilIdle();
+
EXPECT_EQ(kTestRetryDelay, uploader.retry_delay());
EXPECT_TRUE(uploader.QueueEmpty());
+}
+TEST_F(FeedbackUploaderDispatchTest, 500Response) {
+ FeedbackUploader::SetMinimumRetryDelayForTesting(kTestRetryDelay);
+ FeedbackUploader uploader(
+ shared_url_loader_factory(), context(),
+ FeedbackUploaderFactory::CreateUploaderTaskRunner());
+
+ EXPECT_EQ(kTestRetryDelay, uploader.retry_delay());
// Failed reports due to server errors are retried.
+ network::ResourceResponseHead head;
+ std::string headers("HTTP/1.1 500 Server Error\n\n");
+ head.headers = base::MakeRefCounted<net::HttpResponseHeaders>(
+ net::HttpUtil::AssembleRawHeaders(headers.c_str(), headers.size()));
+ network::URLLoaderCompletionStatus status;
+ test_url_loader_factory()->AddResponse(GURL(kFeedbackPostUrl), head, "",
+ status);
QueueReport(&uploader, "Server error failed report");
- fetcher = factory.GetFetcherByID(0);
- fetcher->set_response_code(kHttpPostFailServerError);
- fetcher->delegate()->OnURLFetchComplete(fetcher);
+ base::RunLoop().RunUntilIdle();
+
EXPECT_EQ(kTestRetryDelay * 2, uploader.retry_delay());
EXPECT_FALSE(uploader.QueueEmpty());
}
diff --git a/components/feedback/feedback_uploader_factory.cc b/components/feedback/feedback_uploader_factory.cc
index 5445e1d..f446d03 100644
--- a/components/feedback/feedback_uploader_factory.cc
+++ b/components/feedback/feedback_uploader_factory.cc
@@ -10,6 +10,8 @@
#include "base/task_scheduler/task_traits.h"
#include "components/feedback/feedback_uploader.h"
#include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/storage_partition.h"
namespace feedback {
@@ -51,7 +53,10 @@
KeyedService* FeedbackUploaderFactory::BuildServiceInstanceFor(
content::BrowserContext* context) const {
- return new FeedbackUploader(context, task_runner_);
+ return new FeedbackUploader(
+ content::BrowserContext::GetDefaultStoragePartition(context)
+ ->GetURLLoaderFactoryForBrowserProcess(),
+ context, task_runner_);
}
content::BrowserContext* FeedbackUploaderFactory::GetBrowserContextToUse(
diff --git a/components/feedback/feedback_uploader_unittest.cc b/components/feedback/feedback_uploader_unittest.cc
index 6c93fa02..356229e 100644
--- a/components/feedback/feedback_uploader_unittest.cc
+++ b/components/feedback/feedback_uploader_unittest.cc
@@ -18,6 +18,9 @@
#include "components/feedback/feedback_uploader_factory.h"
#include "content/public/test/test_browser_context.h"
#include "content/public/test/test_browser_thread_bundle.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
+#include "services/network/test/test_url_loader_factory.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace feedback {
@@ -35,8 +38,11 @@
class MockFeedbackUploader : public FeedbackUploader {
public:
- MockFeedbackUploader(content::BrowserContext* context)
- : FeedbackUploader(context,
+ MockFeedbackUploader(
+ scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
+ content::BrowserContext* context)
+ : FeedbackUploader(url_loader_factory,
+ context,
FeedbackUploaderFactory::CreateUploaderTaskRunner()) {}
~MockFeedbackUploader() override {}
@@ -112,13 +118,17 @@
public:
FeedbackUploaderTest() {
FeedbackUploader::SetMinimumRetryDelayForTesting(kRetryDelayForTest);
+ test_shared_loader_factory_ =
+ base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
+ &test_url_loader_factory_);
RecreateUploader();
}
~FeedbackUploaderTest() override = default;
void RecreateUploader() {
- uploader_ = std::make_unique<MockFeedbackUploader>(&context_);
+ uploader_ = std::make_unique<MockFeedbackUploader>(
+ test_shared_loader_factory_, &context_);
}
void QueueReport(const std::string& data) {
@@ -128,6 +138,8 @@
MockFeedbackUploader* uploader() const { return uploader_.get(); }
private:
+ network::TestURLLoaderFactory test_url_loader_factory_;
+ scoped_refptr<network::SharedURLLoaderFactory> test_shared_loader_factory_;
content::TestBrowserThreadBundle test_browser_thread_bundle_;
content::TestBrowserContext context_;
std::unique_ptr<MockFeedbackUploader> uploader_;
