Commit c6191389 authored by Dan Harrington's avatar Dan Harrington Committed by Commit Bot

feedv2: choose upload URL depending on channel

We need to pick the action upload endpoint based on
the Chrome channel. While I was here, I refactored some code
to hopefully make it more clear. There was one actual improvement:
we were needlessly copying the request body before gzipping it.

See b/156953207 for some additional information about the api endpoint.

Bug: 1044139
Change-Id: I7ddac54abca3471d68dfffb080ede929ad178848
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2222801Reviewed-by: default avatarIan Wells <iwells@chromium.org>
Commit-Queue: Dan H <harringtond@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774698}
parent 60f4f958
...@@ -41,20 +41,48 @@ ...@@ -41,20 +41,48 @@
namespace feed { namespace feed {
namespace { namespace {
constexpr char kAuthenticationScope[] =
"https://www.googleapis.com/auth/googlenow";
constexpr char kApplicationOctetStream[] = "application/octet-stream"; constexpr char kApplicationOctetStream[] = "application/octet-stream";
constexpr base::TimeDelta kNetworkTimeout = base::TimeDelta::FromSeconds(30); constexpr base::TimeDelta kNetworkTimeout = base::TimeDelta::FromSeconds(30);
// Add URLs for Bling when it is supported. signin::ScopeSet GetAuthScopes() {
constexpr char kFeedQueryUrl[] = return {"https://www.googleapis.com/auth/googlenow"};
"https://www.google.com/httpservice/retry/TrellisClankService/FeedQuery"; }
constexpr char kNextPageQueryUrl[] =
"https://www.google.com/httpservice/retry/TrellisClankService/" GURL GetFeedQueryURL(feedwire::FeedQuery::RequestReason reason) {
"NextPageQuery"; // Add URLs for Bling when it is supported.
constexpr char kBackgroundQueryUrl[] = switch (reason) {
case feedwire::FeedQuery::SCHEDULED_REFRESH:
case feedwire::FeedQuery::IN_PLACE_UPDATE:
return GURL(
"https://www.google.com/httpservice/noretry/TrellisClankService/" "https://www.google.com/httpservice/noretry/TrellisClankService/"
"FeedQuery"; "FeedQuery");
case feedwire::FeedQuery::NEXT_PAGE_SCROLL:
return GURL(
"https://www.google.com/httpservice/retry/TrellisClankService/"
"NextPageQuery");
case feedwire::FeedQuery::MANUAL_REFRESH:
return GURL(
"https://www.google.com/httpservice/retry/TrellisClankService/"
"FeedQuery");
default:
return GURL();
}
}
GURL GetUploadActionURL(version_info::Channel channel) {
switch (channel) {
case version_info::Channel::BETA:
return GURL(
"https://staging-discover-pa.sandbox.googleapis.com/v1/"
"actions:upload");
case version_info::Channel::STABLE:
return GURL("https://discover-pa.googleapis.com/v1/actions:upload");
default:
return GURL(
"https://autopush-discover-pa.sandbox.googleapis.com/v1/"
"actions:upload");
}
}
GURL GetUrlWithoutQuery(const GURL& url) { GURL GetUrlWithoutQuery(const GURL& url) {
GURL::Replacements replacements; GURL::Replacements replacements;
...@@ -102,12 +130,25 @@ void ParseAndForwardResponse(base::OnceCallback<void(RESULT)> result_callback, ...@@ -102,12 +130,25 @@ void ParseAndForwardResponse(base::OnceCallback<void(RESULT)> result_callback,
void AddMothershipPayloadQueryParams(bool is_post, void AddMothershipPayloadQueryParams(bool is_post,
const std::string& payload, const std::string& payload,
const std::string& language_tag, const std::string& language_tag,
GURL* url) { GURL& url) {
if (!is_post) if (!is_post)
*url = net::AppendQueryParameter(*url, "reqpld", payload); url = net::AppendQueryParameter(url, "reqpld", payload);
*url = net::AppendQueryParameter(*url, "fmt", "bin"); url = net::AppendQueryParameter(url, "fmt", "bin");
if (!language_tag.empty()) if (!language_tag.empty())
*url = net::AppendQueryParameter(*url, "hl", language_tag); url = net::AppendQueryParameter(url, "hl", language_tag);
}
// Compresses and attaches |request_body| for upload if it's not empty.
// Returns the compressed size of the request.
int PopulateRequestBody(const std::string& request_body,
network::SimpleURLLoader* loader) {
if (request_body.empty())
return 0;
std::string compressed_request_body;
compression::GzipCompress(request_body, &compressed_request_body);
loader->AttachStringForUpload(compressed_request_body,
kApplicationOctetStream);
return compressed_request_body.size();
} }
} // namespace } // namespace
...@@ -167,11 +208,10 @@ class FeedNetworkImpl::NetworkFetch { ...@@ -167,11 +208,10 @@ class FeedNetworkImpl::NetworkFetch {
private: private:
void StartAccessTokenFetch() { void StartAccessTokenFetch() {
signin::ScopeSet scopes{kAuthenticationScope};
// It's safe to pass base::Unretained(this) since deleting the token fetcher // It's safe to pass base::Unretained(this) since deleting the token fetcher
// will prevent the callback from being completed. // will prevent the callback from being completed.
token_fetcher_ = std::make_unique<signin::PrimaryAccountAccessTokenFetcher>( token_fetcher_ = std::make_unique<signin::PrimaryAccountAccessTokenFetcher>(
"feed", identity_manager_, scopes, "feed", identity_manager_, GetAuthScopes(),
base::BindOnce(&NetworkFetch::AccessTokenFetchFinished, base::BindOnce(&NetworkFetch::AccessTokenFetchFinished,
base::Unretained(this), tick_clock_->NowTicks()), base::Unretained(this), tick_clock_->NowTicks()),
signin::PrimaryAccountAccessTokenFetcher::Mode::kWaitUntilAvailable); signin::PrimaryAccountAccessTokenFetcher::Mode::kWaitUntilAvailable);
...@@ -250,53 +290,39 @@ class FeedNetworkImpl::NetworkFetch { ...@@ -250,53 +290,39 @@ class FeedNetworkImpl::NetworkFetch {
resource_request->site_for_cookies = net::SiteForCookies::FromUrl(url); resource_request->site_for_cookies = net::SiteForCookies::FromUrl(url);
} }
SetRequestHeaders(!request_body_.empty(), resource_request.get()); SetRequestHeaders(!request_body_.empty(), *resource_request);
auto simple_loader = network::SimpleURLLoader::Create( auto simple_loader = network::SimpleURLLoader::Create(
std::move(resource_request), traffic_annotation); std::move(resource_request), traffic_annotation);
simple_loader->SetAllowHttpErrorResults(true); simple_loader->SetAllowHttpErrorResults(true);
simple_loader->SetTimeoutDuration(kNetworkTimeout); simple_loader->SetTimeoutDuration(kNetworkTimeout);
PopulateRequestBody(simple_loader.get());
const int compressed_size =
PopulateRequestBody(request_body_, simple_loader.get());
UMA_HISTOGRAM_COUNTS_1M(
"ContentSuggestions.Feed.Network.RequestSizeKB.Compressed",
compressed_size / 1024);
return simple_loader; return simple_loader;
} }
void SetRequestHeaders(bool has_request_body, void SetRequestHeaders(bool has_request_body,
network::ResourceRequest* request) const { network::ResourceRequest& request) const {
if (has_request_body) { if (has_request_body) {
request->headers.SetHeader(net::HttpRequestHeaders::kContentType, request.headers.SetHeader(net::HttpRequestHeaders::kContentType,
kApplicationOctetStream); kApplicationOctetStream);
request->headers.SetHeader("Content-Encoding", "gzip"); request.headers.SetHeader("Content-Encoding", "gzip");
} }
variations::SignedIn signed_in_status = variations::SignedIn::kNo; variations::SignedIn signed_in_status = variations::SignedIn::kNo;
if (!access_token_.empty()) { if (!access_token_.empty()) {
request->headers.SetHeader(net::HttpRequestHeaders::kAuthorization, request.headers.SetHeader(net::HttpRequestHeaders::kAuthorization,
"Bearer " + access_token_); "Bearer " + access_token_);
signed_in_status = variations::SignedIn::kYes; signed_in_status = variations::SignedIn::kYes;
} }
// Add X-Client-Data header with experiment IDs from field trials. // Add X-Client-Data header with experiment IDs from field trials.
variations::AppendVariationsHeader(url_, variations::InIncognito::kNo, variations::AppendVariationsHeader(url_, variations::InIncognito::kNo,
signed_in_status, request); signed_in_status, &request);
}
void PopulateRequestBody(network::SimpleURLLoader* loader) {
std::string compressed_request_body;
if (!request_body_.empty()) {
std::string uncompressed_request_body(
reinterpret_cast<const char*>(request_body_.data()),
request_body_.size());
compression::GzipCompress(uncompressed_request_body,
&compressed_request_body);
loader->AttachStringForUpload(compressed_request_body,
kApplicationOctetStream);
}
UMA_HISTOGRAM_COUNTS_1M(
"ContentSuggestions.Feed.Network.RequestSizeKB.Compressed",
static_cast<int>(compressed_request_body.size() / 1024));
} }
void OnSimpleLoaderComplete(std::unique_ptr<std::string> response) { void OnSimpleLoaderComplete(std::unique_ptr<std::string> response) {
...@@ -334,11 +360,10 @@ class FeedNetworkImpl::NetworkFetch { ...@@ -334,11 +360,10 @@ class FeedNetworkImpl::NetworkFetch {
response_body = std::move(*response); response_body = std::move(*response);
if (response_info.status_code == net::HTTP_UNAUTHORIZED) { if (response_info.status_code == net::HTTP_UNAUTHORIZED) {
signin::ScopeSet scopes{kAuthenticationScope};
CoreAccountId account_id = identity_manager_->GetPrimaryAccountId(); CoreAccountId account_id = identity_manager_->GetPrimaryAccountId();
if (!account_id.empty()) { if (!account_id.empty()) {
identity_manager_->RemoveAccessTokenFromCache(account_id, scopes, identity_manager_->RemoveAccessTokenFromCache(
access_token_); account_id, GetAuthScopes(), access_token_);
} }
} }
} }
...@@ -394,10 +419,12 @@ FeedNetworkImpl::FeedNetworkImpl( ...@@ -394,10 +419,12 @@ FeedNetworkImpl::FeedNetworkImpl(
const std::string& api_key, const std::string& api_key,
scoped_refptr<network::SharedURLLoaderFactory> loader_factory, scoped_refptr<network::SharedURLLoaderFactory> loader_factory,
const base::TickClock* tick_clock, const base::TickClock* tick_clock,
PrefService* pref_service) PrefService* pref_service,
version_info::Channel chrome_channel)
: delegate_(delegate), : delegate_(delegate),
identity_manager_(identity_manager), identity_manager_(identity_manager),
api_key_(api_key), api_key_(api_key),
chrome_channel_(chrome_channel),
loader_factory_(loader_factory), loader_factory_(loader_factory),
tick_clock_(tick_clock), tick_clock_(tick_clock),
pref_service_(pref_service) {} pref_service_(pref_service) {}
...@@ -415,26 +442,13 @@ void FeedNetworkImpl::SendQueryRequest( ...@@ -415,26 +442,13 @@ void FeedNetworkImpl::SendQueryRequest(
// TODO(harringtond): Decide how we want to override these URLs for testing. // TODO(harringtond): Decide how we want to override these URLs for testing.
// Should probably add a command-line flag. // Should probably add a command-line flag.
GURL url; GURL url = GetFeedQueryURL(request.feed_request().feed_query().reason());
switch (request.feed_request().feed_query().reason()) { if (url.is_empty())
case feedwire::FeedQuery::SCHEDULED_REFRESH: return std::move(callback).Run({});
case feedwire::FeedQuery::IN_PLACE_UPDATE:
url = GURL(kBackgroundQueryUrl);
break;
case feedwire::FeedQuery::NEXT_PAGE_SCROLL:
url = GURL(kNextPageQueryUrl);
break;
case feedwire::FeedQuery::MANUAL_REFRESH:
url = GURL(kFeedQueryUrl);
break;
default:
std::move(callback).Run({});
return;
}
AddMothershipPayloadQueryParams(/*is_post=*/false, base64proto, AddMothershipPayloadQueryParams(/*is_post=*/false, base64proto,
delegate_->GetLanguageTag(), &url); delegate_->GetLanguageTag(), url);
Send(url, "GET", /*request_body=*/std::string(), Send(url, "GET", /*request_body=*/{},
base::BindOnce(&ParseAndForwardResponse<QueryRequestResult, base::BindOnce(&ParseAndForwardResponse<QueryRequestResult,
NetworkRequestType::kFeedQuery>, NetworkRequestType::kFeedQuery>,
std::move(callback))); std::move(callback)));
...@@ -446,11 +460,10 @@ void FeedNetworkImpl::SendActionRequest( ...@@ -446,11 +460,10 @@ void FeedNetworkImpl::SendActionRequest(
std::string binary_proto; std::string binary_proto;
request.SerializeToString(&binary_proto); request.SerializeToString(&binary_proto);
GURL url( GURL url = GetUploadActionURL(chrome_channel_);
"https://www.google.com/httpservice/retry/ClankActionUploadService/" AddMothershipPayloadQueryParams(/*is_post=*/true,
"ClankActionUpload"); /*payload=*/{}, delegate_->GetLanguageTag(),
AddMothershipPayloadQueryParams(/*is_post=*/true, /*payload=*/std::string(), url);
delegate_->GetLanguageTag(), &url);
Send(url, "POST", std::move(binary_proto), Send(url, "POST", std::move(binary_proto),
base::BindOnce( base::BindOnce(
&ParseAndForwardResponse<ActionRequestResult, &ParseAndForwardResponse<ActionRequestResult,
......
...@@ -11,6 +11,7 @@ ...@@ -11,6 +11,7 @@
#include "base/containers/unique_ptr_adapters.h" #include "base/containers/unique_ptr_adapters.h"
#include "base/memory/scoped_refptr.h" #include "base/memory/scoped_refptr.h"
#include "components/feed/core/v2/feed_network.h" #include "components/feed/core/v2/feed_network.h"
#include "components/version_info/channel.h"
#include "url/gurl.h" #include "url/gurl.h"
class PrefService; class PrefService;
...@@ -43,7 +44,8 @@ class FeedNetworkImpl : public FeedNetwork { ...@@ -43,7 +44,8 @@ class FeedNetworkImpl : public FeedNetwork {
const std::string& api_key, const std::string& api_key,
scoped_refptr<network::SharedURLLoaderFactory> loader_factory, scoped_refptr<network::SharedURLLoaderFactory> loader_factory,
const base::TickClock* tick_clock, const base::TickClock* tick_clock,
PrefService* pref_service); PrefService* pref_service,
version_info::Channel chrome_channel);
~FeedNetworkImpl() override; ~FeedNetworkImpl() override;
FeedNetworkImpl(const FeedNetworkImpl&) = delete; FeedNetworkImpl(const FeedNetworkImpl&) = delete;
FeedNetworkImpl& operator=(FeedNetworkImpl&) = delete; FeedNetworkImpl& operator=(FeedNetworkImpl&) = delete;
...@@ -79,6 +81,7 @@ class FeedNetworkImpl : public FeedNetwork { ...@@ -79,6 +81,7 @@ class FeedNetworkImpl : public FeedNetwork {
Delegate* delegate_; Delegate* delegate_;
signin::IdentityManager* identity_manager_; signin::IdentityManager* identity_manager_;
const std::string api_key_; const std::string api_key_;
const version_info::Channel chrome_channel_;
scoped_refptr<network::SharedURLLoaderFactory> loader_factory_; scoped_refptr<network::SharedURLLoaderFactory> loader_factory_;
const base::TickClock* tick_clock_; const base::TickClock* tick_clock_;
PrefService* pref_service_; PrefService* pref_service_;
......
...@@ -94,7 +94,7 @@ class FeedNetworkTest : public testing::Test { ...@@ -94,7 +94,7 @@ class FeedNetworkTest : public testing::Test {
feed_network_ = std::make_unique<FeedNetworkImpl>( feed_network_ = std::make_unique<FeedNetworkImpl>(
&delegate_, identity_test_env_.identity_manager(), "dummy_api_key", &delegate_, identity_test_env_.identity_manager(), "dummy_api_key",
shared_url_loader_factory_, task_environment_.GetMockTickClock(), shared_url_loader_factory_, task_environment_.GetMockTickClock(),
&profile_prefs_); &profile_prefs_, version_info::Channel::STABLE);
} }
FeedNetwork* feed_network() { return feed_network_.get(); } FeedNetwork* feed_network() { return feed_network_.get(); }
...@@ -410,8 +410,8 @@ TEST_F(FeedNetworkTest, SendActionRequestSendsValidRequest) { ...@@ -410,8 +410,8 @@ TEST_F(FeedNetworkTest, SendActionRequestSendsValidRequest) {
RespondToActionRequest(GetTestActionResponse(), net::HTTP_OK); RespondToActionRequest(GetTestActionResponse(), net::HTTP_OK);
EXPECT_EQ( EXPECT_EQ(
GURL("https://www.google.com/httpservice/retry/ClankActionUploadService/" GURL(
"ClankActionUpload?fmt=bin&hl=en"), "https://discover-pa.googleapis.com/v1/actions:upload?fmt=bin&hl=en"),
resource_request.url); resource_request.url);
EXPECT_EQ("POST", resource_request.method); EXPECT_EQ("POST", resource_request.method);
......
...@@ -145,7 +145,8 @@ FeedService::FeedService( ...@@ -145,7 +145,8 @@ FeedService::FeedService(
base::DefaultTickClock::GetInstance(), profile_prefs); base::DefaultTickClock::GetInstance(), profile_prefs);
feed_network_ = std::make_unique<FeedNetworkImpl>( feed_network_ = std::make_unique<FeedNetworkImpl>(
network_delegate_.get(), identity_manager, api_key, url_loader_factory, network_delegate_.get(), identity_manager, api_key, url_loader_factory,
base::DefaultTickClock::GetInstance(), profile_prefs); base::DefaultTickClock::GetInstance(), profile_prefs,
chrome_info.channel);
store_ = std::make_unique<FeedStore>(std::move(database)); store_ = std::make_unique<FeedStore>(std::move(database));
stream_ = std::make_unique<FeedStream>( stream_ = std::make_unique<FeedStream>(
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment