-
Notifications
You must be signed in to change notification settings - Fork 900
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue 4552: Restore features to correct state after disabling field trials #2471
Changes from all commits
78c8700
d29d4e3
afafa0b
b6e88ef
58a37c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,11 +26,10 @@ | |
#include "chrome/common/chrome_switches.h" | ||
#include "components/autofill/core/common/autofill_features.h" | ||
#include "components/autofill/core/common/autofill_payments_features.h" | ||
#include "components/omnibox/common/omnibox_features.h" | ||
#include "components/password_manager/core/common/password_manager_features.h" | ||
#include "components/unified_consent/feature.h" | ||
#include "content/public/common/content_features.h" | ||
#include "extensions/common/extension_features.h" | ||
#include "gpu/config/gpu_finch_features.h" | ||
#include "services/network/public/cpp/features.h" | ||
#include "third_party/widevine/cdm/buildflags.h" | ||
#include "ui/base/ui_base_features.h" | ||
|
@@ -132,24 +131,24 @@ bool BraveMainDelegate::BasicStartupComplete(int* exit_code) { | |
command_line.AppendSwitch(switches::kDisableDomainReliability); | ||
command_line.AppendSwitch(switches::kDisableChromeGoogleURLTrackingClient); | ||
command_line.AppendSwitch(switches::kNoPings); | ||
command_line.AppendSwitchASCII(switches::kExtensionsInstallVerification, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
switches::kExtensionContentVerificationEnforceStrict); | ||
|
||
// Enabled features. | ||
const std::unordered_set<const char*> enabled_features = { | ||
#if BUILDFLAG(ENABLE_EXTENSIONS) | ||
extensions_features::kNewExtensionUpdaterService.name, | ||
#endif | ||
features::kDesktopPWAWindowing.name, | ||
omnibox::kSimplifyHttpsIndicator.name, | ||
}; | ||
|
||
// Disabled features. | ||
const std::unordered_set<const char*> disabled_features = { | ||
autofill::features::kAutofillSaveCardSignInAfterLocalSave.name, | ||
autofill::features::kAutofillServerCommunication.name, | ||
features::kAudioServiceOutOfProcess.name, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing since this feature is disabled by default. https://cs.chromium.org/chromium/src/content/public/common/content_features.cc?l=51 The test in to check if this feature is disabled is still available in |
||
features::kDefaultEnableOopRasterization.name, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
network::features::kNetworkService.name, | ||
unified_consent::kUnifiedConsent.name, | ||
features::kLookalikeUrlNavigationSuggestionsUI.name, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as: https://github.com/brave/brave-core/pull/2471/files#r286683268 Feature is disabled here: https://cs.chromium.org/chromium/src/chrome/common/chrome_features.cc?l=332 cc: @simonhong |
||
}; | ||
|
||
command_line.AppendFeatures(enabled_features, disabled_features); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,118 @@ | ||
/* Copyright (c) 2019 The Brave Authors. All rights reserved. | ||
* This Source Code Form is subject to the terms of the Mozilla Public | ||
* License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
* You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
||
#include "chrome/browser/ui/views/location_bar/location_bar_view.h" | ||
|
||
#include "base/strings/string_util.h" | ||
#include "chrome/browser/ssl/security_state_tab_helper.h" | ||
#include "chrome/browser/ui/views/frame/browser_view.h" | ||
#include "chrome/test/base/in_process_browser_test.h" | ||
#include "chrome/test/base/ui_test_utils.h" | ||
#include "components/omnibox/browser/location_bar_model_impl.h" | ||
#include "components/omnibox/common/omnibox_features.h" | ||
#include "content/public/test/url_loader_interceptor.h" | ||
#include "net/cert/ct_policy_status.h" | ||
#include "net/ssl/ssl_info.h" | ||
#include "net/test/cert_test_util.h" | ||
#include "net/test/test_data_directory.h" | ||
|
||
const char kMockSecureHostname[] = "example-secure.test"; | ||
const GURL kMockSecureURL = GURL("https://example-secure.test"); | ||
|
||
class SecurityIndicatorTest : public InProcessBrowserTest { | ||
public: | ||
SecurityIndicatorTest() : InProcessBrowserTest(), cert_(nullptr) {} | ||
|
||
void SetUpInProcessBrowserTestFixture() override { | ||
cert_ = | ||
net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"); | ||
ASSERT_TRUE(cert_); | ||
ASSERT_TRUE(embedded_test_server()->Start()); | ||
} | ||
|
||
LocationBarView* GetLocationBarView() { | ||
BrowserView* browser_view = | ||
BrowserView::GetBrowserViewForBrowser(browser()); | ||
return browser_view->GetLocationBarView(); | ||
} | ||
|
||
void SetUpInterceptor(net::CertStatus cert_status) { | ||
url_loader_interceptor_ = std::make_unique<content::URLLoaderInterceptor>( | ||
base::BindRepeating(&SecurityIndicatorTest::InterceptURLLoad, | ||
base::Unretained(this), cert_status)); | ||
} | ||
|
||
void ResetInterceptor() { url_loader_interceptor_.reset(); } | ||
|
||
bool InterceptURLLoad(net::CertStatus cert_status, | ||
content::URLLoaderInterceptor::RequestParams* params) { | ||
if (params->url_request.url.host() != kMockSecureHostname) | ||
return false; | ||
net::SSLInfo ssl_info; | ||
ssl_info.cert = cert_; | ||
ssl_info.cert_status = cert_status; | ||
ssl_info.ct_policy_compliance = | ||
net::ct::CTPolicyCompliance::CT_POLICY_COMPLIES_VIA_SCTS; | ||
network::ResourceResponseHead resource_response; | ||
resource_response.mime_type = "text/html"; | ||
resource_response.ssl_info = ssl_info; | ||
params->client->OnReceiveResponse(resource_response); | ||
// Send an empty response's body. This pipe is not filled with data. | ||
mojo::DataPipe pipe; | ||
params->client->OnStartLoadingResponseBody(std::move(pipe.consumer_handle)); | ||
network::URLLoaderCompletionStatus completion_status; | ||
completion_status.ssl_info = ssl_info; | ||
params->client->OnComplete(completion_status); | ||
return true; | ||
} | ||
|
||
private: | ||
scoped_refptr<net::X509Certificate> cert_; | ||
std::unique_ptr<content::URLLoaderInterceptor> url_loader_interceptor_; | ||
|
||
DISALLOW_COPY_AND_ASSIGN(SecurityIndicatorTest); | ||
}; | ||
|
||
IN_PROC_BROWSER_TEST_F(SecurityIndicatorTest, CheckIndicatorText) { | ||
const GURL kMockNonsecureURL = | ||
embedded_test_server()->GetURL("example.test", "/"); | ||
const base::string16 kEmptyString = base::EmptyString16(); | ||
|
||
const struct { | ||
GURL url; | ||
net::CertStatus cert_status; | ||
security_state::SecurityLevel security_level; | ||
bool should_show_text; | ||
base::string16 indicator_text; | ||
} cases[]{// Default | ||
{kMockSecureURL, net::CERT_STATUS_IS_EV, security_state::EV_SECURE, | ||
false, kEmptyString}, | ||
{kMockSecureURL, 0, security_state::SECURE, false, kEmptyString}, | ||
{kMockNonsecureURL, 0, security_state::NONE, false, kEmptyString}}; | ||
|
||
content::WebContents* tab = | ||
browser()->tab_strip_model()->GetActiveWebContents(); | ||
ASSERT_TRUE(tab); | ||
|
||
// After SetUpInterceptor() is called, requests to this hostname will be | ||
// mocked and use specified certificate validation results. This allows tests | ||
// to mock Extended Validation (EV) certificate connections. | ||
SecurityStateTabHelper* helper = SecurityStateTabHelper::FromWebContents(tab); | ||
ASSERT_TRUE(helper); | ||
LocationBarView* location_bar_view = GetLocationBarView(); | ||
|
||
for (const auto& c : cases) { | ||
base::test::ScopedFeatureList scoped_feature_list; | ||
scoped_feature_list.InitAndEnableFeature(omnibox::kSimplifyHttpsIndicator); | ||
SetUpInterceptor(c.cert_status); | ||
ui_test_utils::NavigateToURL(browser(), c.url); | ||
EXPECT_EQ(c.security_level, helper->GetSecurityLevel()); | ||
EXPECT_EQ(c.should_show_text, | ||
location_bar_view->location_icon_view()->ShouldShowLabel()); | ||
EXPECT_EQ(c.indicator_text, | ||
location_bar_view->location_icon_view()->GetText()); | ||
ResetInterceptor(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
/* Copyright (c) 2019 The Brave Authors. All rights reserved. | ||
* This Source Code Form is subject to the terms of the Mozilla Public | ||
* License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
* You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
||
#include "components/omnibox/browser/location_bar_model_impl.h" | ||
|
||
#include "components/omnibox/browser/omnibox_field_trial.h" | ||
|
||
#define GetFieldTrialParamValueByFeature \ | ||
GetFieldTrialParamValueByFeature_ChromiumImpl | ||
|
||
namespace base { | ||
std::string GetFieldTrialParamValueByFeature(const base::Feature& feature, | ||
const std::string& param_name) { | ||
return OmniboxFieldTrial::kSimplifyHttpsIndicatorParameterBothToLock; | ||
} | ||
} // namespace base | ||
|
||
#include "../../../../../../../components/omnibox/browser/location_bar_model_impl.cc" // NOLINT | ||
|
||
#undef GetFieldTrialParamValueByFeature |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
/* Copyright (c) 2019 The Brave Authors. All rights reserved. | ||
* This Source Code Form is subject to the terms of the Mozilla Public | ||
* License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
* You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
||
#include "components/variations/service/buildflags.h" | ||
#include "testing/gtest/include/gtest/gtest.h" | ||
|
||
namespace { | ||
typedef testing::Test FieldTrialsTest; | ||
|
||
TEST_F(FieldTrialsTest, FieldTrialsTestingEnabled) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we also verify that field trial downloads are disabled? |
||
bool enabled = false; | ||
#if BUILDFLAG(FIELDTRIAL_TESTING_ENABLED) | ||
enabled = true | ||
#endif // BUILDFLAG(FIELDTRIAL_TESTING_ENABLED) | ||
EXPECT_FALSE(enabled); | ||
} | ||
|
||
} // namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lint indent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved - clang-format ftw 😄