Skip to content

Commit

Permalink
Make experiment labels conditional on general Google Update support.
Browse files Browse the repository at this point in the history
Previously it was conditional on the BrowserDistribution. This was
needed when some distros (e.g., the Chrome Binaries) should not have
experiment labels. Nowadays, however, it's purely conditional on whether
or not the brand supports integration with the updater.

This is another step in chopping down BrowserDistribution.

BUG=373987

Review-Url: https://codereview.chromium.org/2777933002
Cr-Commit-Position: refs/heads/master@{#460675}
  • Loading branch information
GregTho authored and Commit bot committed Mar 30, 2017
1 parent e5064a0 commit 1a21f83
Show file tree
Hide file tree
Showing 9 changed files with 27 additions and 49 deletions.
4 changes: 0 additions & 4 deletions chrome/installer/util/browser_distribution.cc
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,3 @@ void BrowserDistribution::UpdateInstallStatus(bool system_install,
installer::ArchiveType archive_type,
installer::InstallStatus install_status) {
}

bool BrowserDistribution::ShouldSetExperimentLabels() {
return false;
}
4 changes: 0 additions & 4 deletions chrome/installer/util/browser_distribution.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,6 @@ class BrowserDistribution {
installer::ArchiveType archive_type,
installer::InstallStatus install_status);

// Returns true if this distribution should set the Omaha experiment_labels
// registry value.
virtual bool ShouldSetExperimentLabels();

protected:
explicit BrowserDistribution(
std::unique_ptr<AppRegistrationData> app_reg_data);
Expand Down
4 changes: 0 additions & 4 deletions chrome/installer/util/google_chrome_distribution.cc
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,3 @@ void GoogleChromeDistribution::UpdateInstallStatus(bool system_install,
InstallUtil::GetInstallReturnCode(install_status),
install_static::GetAppGuid());
}

bool GoogleChromeDistribution::ShouldSetExperimentLabels() {
return true;
}
2 changes: 0 additions & 2 deletions chrome/installer/util/google_chrome_distribution.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@ class GoogleChromeDistribution : public BrowserDistribution {
installer::ArchiveType archive_type,
installer::InstallStatus install_status) override;

bool ShouldSetExperimentLabels() override;

protected:
// Disallow construction from others.
GoogleChromeDistribution();
Expand Down
4 changes: 0 additions & 4 deletions chrome/installer/util/google_chrome_distribution_dummy.cc
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,3 @@ void GoogleChromeDistribution::UpdateInstallStatus(bool system_install,
installer::ArchiveType archive_type,
installer::InstallStatus install_status) {
}

bool GoogleChromeDistribution::ShouldSetExperimentLabels() {
return false;
}
4 changes: 0 additions & 4 deletions chrome/installer/util/google_chrome_sxs_distribution.cc
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,3 @@ base::string16 GoogleChromeSxSDistribution::GetStartMenuShortcutSubfolder(
return GetShortcutName();
}
}

bool GoogleChromeSxSDistribution::ShouldSetExperimentLabels() {
return true;
}
1 change: 0 additions & 1 deletion chrome/installer/util/google_chrome_sxs_distribution.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ class GoogleChromeSxSDistribution : public GoogleChromeDistribution {
base::string16 GetShortcutName() override;
base::string16 GetStartMenuShortcutSubfolder(
Subfolder subfolder_type) override;
bool ShouldSetExperimentLabels() override;

private:
friend class BrowserDistribution;
Expand Down
48 changes: 26 additions & 22 deletions chrome/installer/util/google_update_settings.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "base/win/registry.h"
#include "base/win/win_util.h"
#include "chrome/common/chrome_switches.h"
#include "chrome/install_static/install_modes.h"
#include "chrome/install_static/install_util.h"
#include "chrome/installer/util/app_registration_data.h"
#include "chrome/installer/util/browser_distribution.h"
Expand Down Expand Up @@ -850,28 +851,32 @@ bool GoogleUpdateSettings::GetUpdateDetail(ProductData* data) {
bool GoogleUpdateSettings::SetExperimentLabels(
bool system_install,
const base::string16& experiment_labels) {
// There is nothing to do if this brand does not support integration with
// Google Update.
if (!install_static::kUseGoogleUpdateIntegration)
return false;

HKEY reg_root = system_install ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER;

// Use the browser distribution and install level to write to the correct
// client state/app guid key.
bool success = false;
BrowserDistribution* dist = BrowserDistribution::GetDistribution();
if (dist->ShouldSetExperimentLabels()) {
base::string16 client_state_path(
system_install ? dist->GetStateMediumKey() : dist->GetStateKey());
RegKey client_state(
reg_root, client_state_path.c_str(), KEY_SET_VALUE | KEY_WOW64_32KEY);
// It is possible that the registry keys do not yet exist or have not yet
// been ACLed by Google Update to be user writable.
if (!client_state.Valid())
return false;
if (experiment_labels.empty()) {
success = client_state.DeleteValue(google_update::kExperimentLabels)
== ERROR_SUCCESS;
} else {
success = client_state.WriteValue(google_update::kExperimentLabels,
experiment_labels.c_str()) == ERROR_SUCCESS;
}
base::string16 client_state_path(system_install ? dist->GetStateMediumKey()
: dist->GetStateKey());
RegKey client_state(reg_root, client_state_path.c_str(),
KEY_SET_VALUE | KEY_WOW64_32KEY);
// It is possible that the registry keys do not yet exist or have not yet
// been ACLed by Google Update to be user writable.
if (!client_state.Valid())
return false;
if (experiment_labels.empty()) {
success = client_state.DeleteValue(google_update::kExperimentLabels) ==
ERROR_SUCCESS;
} else {
success =
client_state.WriteValue(google_update::kExperimentLabels,
experiment_labels.c_str()) == ERROR_SUCCESS;
}

return success;
Expand All @@ -880,14 +885,13 @@ bool GoogleUpdateSettings::SetExperimentLabels(
bool GoogleUpdateSettings::ReadExperimentLabels(
bool system_install,
base::string16* experiment_labels) {
HKEY reg_root = system_install ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER;

// If this distribution does not set the experiment labels, don't bother
// reading.
BrowserDistribution* dist = BrowserDistribution::GetDistribution();
if (!dist->ShouldSetExperimentLabels())
// There is nothing to do if this brand does not support integration with
// Google Update.
if (!install_static::kUseGoogleUpdateIntegration)
return false;

HKEY reg_root = system_install ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER;
BrowserDistribution* dist = BrowserDistribution::GetDistribution();
base::string16 client_state_path(
system_install ? dist->GetStateMediumKey() : dist->GetStateKey());

Expand Down
5 changes: 1 addition & 4 deletions chrome/installer/util/google_update_settings_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,8 @@ class GoogleUpdateSettingsTest : public testing::Test {
// Install a basic InstallDetails instance.
install_static::ScopedInstallDetails details(install == SYSTEM_INSTALL);

BrowserDistribution* chrome = BrowserDistribution::GetDistribution();
base::string16 value;
#if defined(GOOGLE_CHROME_BUILD)
EXPECT_TRUE(chrome->ShouldSetExperimentLabels());

// Before anything is set, ReadExperimentLabels should succeed but return
// an empty string.
EXPECT_TRUE(GoogleUpdateSettings::ReadExperimentLabels(
Expand All @@ -88,6 +85,7 @@ class GoogleUpdateSettingsTest : public testing::Test {
RegKey key;
HKEY root = install == SYSTEM_INSTALL ?
HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER;
BrowserDistribution* chrome = BrowserDistribution::GetDistribution();
base::string16 state_key = install == SYSTEM_INSTALL ?
chrome->GetStateMediumKey() : chrome->GetStateKey();

Expand All @@ -114,7 +112,6 @@ class GoogleUpdateSettingsTest : public testing::Test {
EXPECT_EQ(base::string16(), value);
key.Close();
#else
EXPECT_FALSE(chrome->ShouldSetExperimentLabels());
EXPECT_FALSE(GoogleUpdateSettings::ReadExperimentLabels(
install == SYSTEM_INSTALL, &value));
#endif // GOOGLE_CHROME_BUILD
Expand Down

0 comments on commit 1a21f83

Please sign in to comment.