Issue 4552: Restore features to correct state after disabling field trials #2471

jumde · 2019-05-21T19:17:05Z

Fix brave/brave-browser#4552

Submitter Checklist:

Test Plan:

Navigate to https://www.globalsign.com/en/ and verify if a lock is displayed instead of a Secure Chip

Secure Chip:

Lock Only:

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

simonhong · 2019-05-22T10:46:19Z

I think some of enabled/disabled features by us in BraveMainDelegate::BasicStartupComplete can be deleted.(ex, kLookalikeUrlNavigationSuggestionsUI is disabled by default but enabled by field trials)
FYI, BraveMainDelegateBrowserTest.DisabledFeatures would be useful for testing this change.
Also, it would be nice to add test for enabled features we override.

simonhong · 2019-05-22T10:48:07Z

How about splitting this into two PRs(or commits)?
One is disabling field trials and the other is for ev certs.

jumde · 2019-05-22T16:53:19Z

How about splitting this into two PRs(or commits)?
One is disabling field trials and the other is for ev certs.

@simonhong - HttpsIndicatorParameterBothToLock is the current behavior of the HTTPS indicator in the release channels. Disabling Field Trials was setting this back to EvToSecure which is not desired per the product requirements, so I merged the change into a single PR to avoid regressions from disabling field trials.

jumde · 2019-05-22T20:55:10Z

app/brave_main_delegate.cc

  };

  // Disabled features.
  const std::unordered_set<const char*> disabled_features = {
      autofill::features::kAutofillSaveCardSignInAfterLocalSave.name,
      autofill::features::kAutofillServerCommunication.name,
-      features::kAudioServiceOutOfProcess.name,


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 brave_main_delegate_browsertest.cc to alert if this feature gets enabled in future. cc: @darkdh @mkarolin

jumde · 2019-05-22T20:58:31Z

app/brave_main_delegate.cc

  };

  // Disabled features.
  const std::unordered_set<const char*> disabled_features = {
      autofill::features::kAutofillSaveCardSignInAfterLocalSave.name,
      autofill::features::kAutofillServerCommunication.name,
-      features::kAudioServiceOutOfProcess.name,
-      features::kDefaultEnableOopRasterization.name,


Same as: https://github.com/brave/brave-core/pull/2471/files#r286683268

Feature is disabled here: https://cs.chromium.org/chromium/src/gpu/config/gpu_finch_features.cc?l=58

cc: @mkarolin, @bbondy

jumde · 2019-05-22T20:59:40Z

app/brave_main_delegate.cc

      network::features::kNetworkService.name,
      unified_consent::kUnifiedConsent.name,
-      features::kLookalikeUrlNavigationSuggestionsUI.name,


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

simonhong · 2019-05-23T07:05:55Z

patches/components-variations-service-BUILD.gn.patch

+   # TODO(thakis): It's strange this is called "_like_official_build" but then
+   # checks is_chrome_branded, not is_official_build.
+-  fieldtrial_testing_like_official_build = is_chrome_branded
+  fieldtrial_testing_like_official_build = true


Instead of modifying args here, overriding it from config.js would be better.

simonhong · 2019-05-23T07:06:01Z

chromium_src/chrome/browser/ui/views/location_bar/location_bar_view_browsertest.cc

+IN_PROC_BROWSER_TEST_F(SecurityIndicatorTest, CheckIndicatorText) {
+  const GURL kMockNonsecureURL =
+      embedded_test_server()->GetURL("example.test", "/");
+  const base::string16 kEmptyString = base::string16();


fyi: there is base::EmptyString16().

diracdeltas · 2019-05-23T18:01:17Z

note for release managers: this needs to be in the same release as brave/brave-browser#4551

bridiver · 2019-05-24T19:07:45Z

chromium_src/components/variations/service/field_trial_unittest.cc

+namespace {
+typedef testing::Test FieldTrialsTest;
+
+TEST_F(FieldTrialsTest, FieldTrialsTestingEnabled) {


can we also verify that field trial downloads are disabled?

@bridiver

…rials auditors: @bridiver, @bbondy, @diracdeltas, @bsclifton, @simonhong

2. Using config.js instead of patch to disable field trials

…DownloadTest.VerifyChromiumPDFExntension

Make sure brave/brave-browser#4551 and #2471 landed first

bsclifton · 2019-05-29T21:48:53Z

Updated this PR, brave/brave-browser#4551, and both issues (brave/brave-browser#4283 and brave/brave-browser#4552) to have the 0.68.x milestone 😄👍

Also added release-notes/include tag to them (if we don't want to note that in release notes, feel free to change to exclude, @jumde)

Make sure brave/brave-browser#4551 and #2471 landed first

jumde self-assigned this May 21, 2019

jumde force-pushed the disable_field_trials branch from ce050d4 to 8a0447f Compare May 21, 2019 21:28

jumde changed the title ~~WIP - Issue 4283: Disable field trials~~ Issue 4283: Disable field trials May 21, 2019

jumde mentioned this pull request May 21, 2019

Disable field trials brave/brave-browser#4283

Closed

jumde changed the title ~~Issue 4283: Disable field trials~~ Issue 4283: Disable field trials + Display lock for all HTTPS sites (including sites with EV certs) May 21, 2019

jumde requested review from bbondy, bsclifton, bridiver and diracdeltas May 21, 2019 21:54

simonhong self-requested a review May 22, 2019 11:01

jumde force-pushed the disable_field_trials branch from 8a0447f to 522b68b Compare May 22, 2019 20:49

jumde commented May 22, 2019

View reviewed changes

jumde force-pushed the disable_field_trials branch 2 times, most recently from 1488acd to df516a6 Compare May 23, 2019 05:12

simonhong reviewed May 23, 2019

View reviewed changes

jumde mentioned this pull request May 23, 2019

Issue 4283: Disabling field trials brave/brave-browser#4551

Merged

25 tasks

jumde changed the title ~~Issue 4283: Disable field trials + Display lock for all HTTPS sites (including sites with EV certs)~~ Issue 4552: Restore features to correct state after disabling field trials May 23, 2019

jumde mentioned this pull request May 23, 2019

Revert features to correct state after disabling field trials brave/brave-browser#4552

Closed

3 tasks

jumde force-pushed the disable_field_trials branch from 3b8dd20 to 1fadff3 Compare May 23, 2019 15:35

jumde force-pushed the disable_field_trials branch 5 times, most recently from 3f1896c to dc56bfd Compare May 24, 2019 15:19

bridiver reviewed May 24, 2019

View reviewed changes

jumde added 5 commits May 24, 2019 13:25

Issue 4552: Restore features to correct state after disabling field t…

78c8700

…rials auditors: @bridiver, @bbondy, @diracdeltas, @bsclifton, @simonhong

fix lint

d29d4e3

1. Using base::EmptyString16 instead of std::string

afafa0b

2. Using config.js instead of patch to disable field trials

Setting kExtensionsInstallVerification to enforce-strict for BravePDF…

b6e88ef

…DownloadTest.VerifyChromiumPDFExntension

lint

58a37c5

jumde force-pushed the disable_field_trials branch from dc56bfd to 58a37c5 Compare May 24, 2019 20:27

bridiver mentioned this pull request May 28, 2019

Brave syncer #2010

Merged

19 tasks

darkdh added a commit that referenced this pull request May 28, 2019

Remove manually disable field trail feature

2cb26c9

Make sure brave/brave-browser#4551 and #2471 landed first

bridiver approved these changes May 29, 2019

View reviewed changes

jumde merged commit cda823c into master May 29, 2019

bsclifton deleted the disable_field_trials branch May 29, 2019 21:45

bsclifton added this to the 0.68.x - Nightly milestone May 29, 2019

darkdh added a commit that referenced this pull request May 30, 2019

Remove manually disable field trail feature

0c6b9b4

Make sure brave/brave-browser#4551 and #2471 landed first

darkdh added a commit that referenced this pull request May 30, 2019

Remove manually disable field trail feature

19333c7

Make sure brave/brave-browser#4551 and #2471 landed first

darkdh added a commit that referenced this pull request May 31, 2019

Remove manually disable field trail feature

26b1d96

Make sure brave/brave-browser#4551 and #2471 landed first

darkdh added a commit that referenced this pull request Jun 18, 2019

Remove manually disable field trail feature

e8e3417

Make sure brave/brave-browser#4551 and #2471 landed first

darkdh added a commit that referenced this pull request Jul 16, 2019

Remove manually disable field trail feature

7413857

Make sure brave/brave-browser#4551 and #2471 landed first

darkdh added a commit that referenced this pull request Jul 18, 2019

Remove manually disable field trail feature

0758c67

Make sure brave/brave-browser#4551 and #2471 landed first

darkdh added a commit that referenced this pull request Jul 23, 2019

Remove manually disable field trail feature

c193e25

Make sure brave/brave-browser#4551 and #2471 landed first

darkdh added a commit that referenced this pull request Aug 9, 2019

Remove manually disable field trail feature

d251beb

Make sure brave/brave-browser#4551 and #2471 landed first

darkdh added a commit that referenced this pull request Aug 9, 2019

Remove manually disable field trail feature

ddb80a5

Make sure brave/brave-browser#4551 and #2471 landed first

darkdh added a commit that referenced this pull request Aug 15, 2019

Remove manually disable field trail feature

e182e55

Make sure brave/brave-browser#4551 and #2471 landed first

darkdh added a commit that referenced this pull request Aug 19, 2019

Remove manually disable field trail feature

c5fbe1f

Make sure brave/brave-browser#4551 and #2471 landed first

esjarc mentioned this pull request Mar 25, 2021

[Desktop] Unable to install self-signed or third-party CRX extensions brave/brave-browser#2457

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 4552: Restore features to correct state after disabling field trials #2471

Issue 4552: Restore features to correct state after disabling field trials #2471

jumde commented May 21, 2019 •

edited

Loading

simonhong commented May 22, 2019 •

edited

Loading

simonhong commented May 22, 2019 •

edited

Loading

jumde commented May 22, 2019 •

edited

Loading

jumde May 22, 2019

jumde May 22, 2019

jumde May 22, 2019

simonhong May 23, 2019

simonhong May 23, 2019 •

edited

Loading

diracdeltas commented May 23, 2019

bridiver May 24, 2019

bsclifton commented May 29, 2019 •

edited

Loading

Issue 4552: Restore features to correct state after disabling field trials #2471

Issue 4552: Restore features to correct state after disabling field trials #2471

Conversation

jumde commented May 21, 2019 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

simonhong commented May 22, 2019 • edited Loading

simonhong commented May 22, 2019 • edited Loading

jumde commented May 22, 2019 • edited Loading

jumde May 22, 2019

Choose a reason for hiding this comment

jumde May 22, 2019

Choose a reason for hiding this comment

jumde May 22, 2019

Choose a reason for hiding this comment

simonhong May 23, 2019

Choose a reason for hiding this comment

simonhong May 23, 2019 • edited Loading

Choose a reason for hiding this comment

diracdeltas commented May 23, 2019

bridiver May 24, 2019

Choose a reason for hiding this comment

bsclifton commented May 29, 2019 • edited Loading

jumde commented May 21, 2019 •

edited

Loading

simonhong commented May 22, 2019 •

edited

Loading

simonhong commented May 22, 2019 •

edited

Loading

jumde commented May 22, 2019 •

edited

Loading

simonhong May 23, 2019 •

edited

Loading

bsclifton commented May 29, 2019 •

edited

Loading