Skip to content

Commit

Permalink
Fix variation id transmission with feedback reports.
Browse files Browse the repository at this point in the history
This was originally added by this CL:
https://chromiumcodereview.appspot.com/15714003

But later broke when the code was componentized here:
https://codereview.chromium.org/116863002

This CL restores the original functionality and adds
a test to hopefully prevent it from regressing in the
future.

BUG=460604

Review URL: https://codereview.chromium.org/942863003

Cr-Commit-Position: refs/heads/master@{#317655}
  • Loading branch information
asvitkine-chromium authored and Commit bot committed Feb 23, 2015
1 parent d52ca40 commit e6f7a58
Show file tree
Hide file tree
Showing 6 changed files with 81 additions and 1 deletion.
2 changes: 2 additions & 0 deletions components/components_tests.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@
'enhanced_bookmarks/item_position_unittest.cc',
'feedback/feedback_common_unittest.cc',
'feedback/feedback_data_unittest.cc',
'feedback/feedback_uploader_chrome_unittest.cc',
'feedback/feedback_uploader_unittest.cc',
'gcm_driver/gcm_account_mapper_unittest.cc',
'gcm_driver/gcm_channel_status_request_unittest.cc',
Expand Down Expand Up @@ -640,6 +641,7 @@
'sources!': [
'feedback/feedback_common_unittest.cc',
'feedback/feedback_data_unittest.cc',
'feedback/feedback_uploader_chrome_unittest.cc',
'feedback/feedback_uploader_unittest.cc',
'gcm_driver/gcm_account_mapper_unittest.cc',
'gcm_driver/gcm_channel_status_request_unittest.cc',
Expand Down
1 change: 1 addition & 0 deletions components/feedback.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
'../third_party/zlib/google/zip.gyp:zip',
'keyed_service_core',
'feedback_proto',
'components.gyp:variations_http_provider',
],
'include_dirs': [
'..',
Expand Down
1 change: 1 addition & 0 deletions components/feedback/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ static_library("feedback") {
"//base",
"//components/keyed_service/core:core",
"//components/feedback/proto",
"//components/variations/net",
"//content/public/common",
"//net",
"//third_party/zlib:zip",
Expand Down
1 change: 1 addition & 0 deletions components/feedback/DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ include_rules = [
"-content",
"+components/keyed_service",
"+components/user_prefs",
"+components/variations",
"+content/public/browser",
"+content/public/test",
"+net/base",
Expand Down
12 changes: 11 additions & 1 deletion components/feedback/feedback_uploader_chrome.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#include "components/feedback/feedback_uploader_chrome.h"

#include <string>

#include "base/callback.h"
#include "base/command_line.h"
#include "base/files/file_path.h"
Expand All @@ -12,6 +14,7 @@
#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_header_provider.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/browser_thread.h"
#include "net/base/load_flags.h"
Expand Down Expand Up @@ -42,14 +45,21 @@ FeedbackUploaderChrome::FeedbackUploaderChrome(
void FeedbackUploaderChrome::DispatchReport(const std::string& data) {
GURL post_url(url_);

// Note: FeedbackUploaderDelegate deletes itself and the fetcher.
net::URLFetcher* fetcher = net::URLFetcher::Create(
post_url, net::URLFetcher::POST,
new FeedbackUploaderDelegate(
data,
base::Bind(&FeedbackUploaderChrome::UpdateUploadTimer, AsWeakPtr()),
base::Bind(&FeedbackUploaderChrome::RetryReport, AsWeakPtr())));

fetcher->SetUploadData(std::string(kProtoBufMimeType), data);
// Tell feedback server about the variation state of this install.
net::HttpRequestHeaders headers;
variations::VariationsHttpHeaderProvider::GetInstance()->AppendHeaders(
fetcher->GetOriginalURL(), context_->IsOffTheRecord(), false, &headers);
fetcher->SetExtraRequestHeaders(headers.ToString());

fetcher->SetUploadData(kProtoBufMimeType, data);
fetcher->SetRequestContext(context_->GetRequestContext());
fetcher->SetLoadFlags(net::LOAD_DO_NOT_SAVE_COOKIES |
net::LOAD_DO_NOT_SEND_COOKIES);
Expand Down
65 changes: 65 additions & 0 deletions components/feedback/feedback_uploader_chrome_unittest.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
// Copyright 2015 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_chrome.h"

#include "base/message_loop/message_loop.h"
#include "base/metrics/field_trial.h"
#include "components/variations/variations_associated_data.h"
#include "content/public/test/test_browser_context.h"
#include "net/url_request/test_url_fetcher_factory.h"
#include "net/url_request/url_fetcher_delegate.h"
#include "testing/gtest/include/gtest/gtest.h"

namespace feedback {

class FeedbackUploaderChromeTest : public ::testing::Test {
protected:
FeedbackUploaderChromeTest() {}

~FeedbackUploaderChromeTest() override {
// Clean up registered ids.
variations::testing::ClearAllVariationIDs();
}

// Registers a field trial with the specified name and group and an associated
// google web property variation id.
void CreateFieldTrialWithId(const std::string& trial_name,
const std::string& group_name,
int variation_id) {
variations::AssociateGoogleVariationID(
variations::GOOGLE_WEB_PROPERTIES, trial_name, group_name,
static_cast<variations::VariationID>(variation_id));
base::FieldTrialList::CreateFieldTrial(trial_name, group_name)->group();
}

private:
base::MessageLoopForUI message_loop_;

DISALLOW_COPY_AND_ASSIGN(FeedbackUploaderChromeTest);
};

TEST_F(FeedbackUploaderChromeTest, VariationHeaders) {
// Register a trial and variation id, so that there is data in variations
// headers.
base::FieldTrialList field_trial_list_(NULL);
CreateFieldTrialWithId("Test", "Group1", 123);

content::TestBrowserContext context;
FeedbackUploaderChrome uploader(&context);

net::TestURLFetcherFactory factory;
uploader.DispatchReport("test");

net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
net::HttpRequestHeaders headers;
fetcher->GetExtraRequestHeaders(&headers);
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);
}

} // namespace feedback

0 comments on commit e6f7a58

Please sign in to comment.