Skip to content
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

Add support to "Cromite" flags #538

Merged
merged 12 commits into from
Nov 29, 2023
Merged

Add support to "Cromite" flags #538

merged 12 commits into from
Nov 29, 2023

Conversation

uazo
Copy link
Owner

@uazo uazo commented Nov 18, 2023

Description

Add SET_CROMITE_FEATURE_ENABLED*, SET_CROMITE_FEATURE_DISABLED* and CROMITE_FEATURE macros, logic has been adapted from that found in brave.
Allows flags to be defined in separate files.
Activates a new cromite tab in chrome://flags with only the flags added and changed. In android added chrome://flags/cromite in the setting ui.
see #403

All submissions

  • [*] there are no other open Pull Requests for the same update/change
  • [*] Bromite can be built with these changes
  • [*] I have tested that the new change works as intended (AVD or physical device will do)

Format

  • [*] patch subject and filename match (e.g. Subject: Alternative cache (NIK-based) -> Alternative-cache-NIK-based.patch)
  • [*] patch description contains explanation of changes
  • [*] no unnecessary whitespace or unrelated changes

Add SET_CROMITE_FEATURE_ENABLED*, SET_CROMITE_FEATURE_DISABLED*
and CROMITE_FEATURE macros, logic has been adapted from that found
in brave.
Allows flags to be defined in separate files.
Activates a new cromite tab in chrome://flags with only the flags
added and changed. In android added chrome://flags/cromite
in the setting ui.
Currently in wip
@uazo
Copy link
Owner Author

uazo commented Nov 18, 2023

I plan to edit all patches that modify or add flags to cromite, excluding switches for now, the idea is to continue simplifying the rebase process.
please note that the changes will be NOT compatible with older versions and will require this patch and the bromite-build-utils.patch
changes will have to be applied to each individual patch, which makes the adoption process rather lengthy and will therefore be done as we go along.
the aim is also to eliminate the big problem of wiggle which, during rebase, replaces the flag name with a random one: if I don't notice it, these things happen: #487
I would also like to try to minimise the patch-for-patch rebase that I do manually with each upgrade, trying to automate it as much as possible, and delegating the compiler to warn me of anomalies, such as the absence of flags.
with the occasion some modifications to the ui chrome://flags:

  • displaying the default status of all flags (active or inactive)
  • separated the flags modified by cromite into a new tab
  • allowed, through the use of gn cpp_bromite_include(), already used for content settings patches, the separation of flag state definitions in different files

in android I have added direct access to a special version (chrome://flags/cromite) which allows the selection of a restricted range of flags, which will probably have to be modified once I start using it and understand the requirements.

Some examples of modifications already made will follow to explain the details

@uazo
Copy link
Owner Author

uazo commented Nov 18, 2023

in 3046dcc first example of use:

 BASE_FEATURE(kRestrictGamepadAccess,
-             "RestrictGamepadAccess",
-             base::FEATURE_DISABLED_BY_DEFAULT);
+             "RestrictGamepadAccess",             // enabled
+             base::FEATURE_ENABLED_BY_DEFAULT);   // in bromite

become

+SET_CROMITE_FEATURE_ENABLED(kRestrictGamepadAccess);

leaving the chromium code unchanged.
the idea is that of brave, used for example in brave/brave-core@27535eb (ref #469) is to generate an array of flags + status to be used as an override at application start-up, when populating the feature list: the difference between my code and brave's is at the time of the override (FeatureList::FinalizeInitialization()).

There are new define:

@uazo
Copy link
Owner Author

uazo commented Nov 18, 2023

in c64a775 simply an example of the use of CROMITE_FEATURE
I introduce it mainly for the ui, which is now chrome://flags/cromite, currently sketched but working; in android will probably have to be transformed into java settings, we shall see. It depends on whether I find it easier to activate the nativeui settings (the ones on the desktop for instance), but that's another matter.

@uazo
Copy link
Owner Author

uazo commented Nov 18, 2023

in 6acf8d8 an example of the use of the chromite_flags folder so structured but if necessary I will add more folders:

Details

image

with the various entry points in the specific files (example, example) this way, each patch will have its own override file.

separate note for the folder chromite_flags/chrome/browser/about_flags_cc, about_flags.cc has two edit points, the parameters and the kFeatureEntries list. for now I decided to reuse the same file with defines identifying the section: so 6acf8d8#diff-36022052f5651cc967d7f98fc0dde92075cbc05b3b670f26e9ec0832ae30a210R158 verification of FEATURE_PARAM_SECTION and FLAG_SECTION is required

Git apply result for new-method-change-flags branch
@uazo
Copy link
Owner Author

uazo commented Nov 18, 2023

6c36a08 <-- this should make you understand the problem I am trying to solve

@uazo
Copy link
Owner Author

uazo commented Nov 18, 2023

@chirayudesai @romain-hunault breaking change coming
let me know what you think

@PF4Public
Copy link

breaking change coming
let me know what you think

Will those changes prevent usage of your patches separately?

@uazo
Copy link
Owner Author

uazo commented Nov 19, 2023

Will those changes prevent usage of your patches separately?

yes, without this patch the others cannot be used.
unless I split the logic from the ui, which I would do if asked.

@PF4Public
Copy link

PF4Public commented Nov 19, 2023

yes, without this patch the others cannot be used.

If this patch applies cleanly without depending on yet another one, then it shouldn't be a problem.

@uazo
Copy link
Owner Author

uazo commented Nov 19, 2023

If this patch applies cleanly without depending on yet another one

ehm.. this patch needs bromite-build-utils.patch
I realise that I am changing a lot from what we did in bromite, but from a developmental point of view, mine seems the best solution.

@uazo
Copy link
Owner Author

uazo commented Nov 19, 2023

next step, these patches in this order:

 60 Add flag to configure maximum connections per host
 65 Add an always incognito mode
 63 Add bookmark import export actions
 76 Add flag to disable IPv6 probes
 86 Enable StrictOriginIsolation and SitePerProcess
 91 Restore Search Ready Omnibox flag
101 Revert flags remove disable pull to refresh effect
118 Restore Simplified NTP launch
120 Disable text fragments by default
123 Revert flags remove num raster threads
127 Re introduce modal dialog flag to close all tabs
131 Add AllowUserCertificates flag
132 Add IsCleartextPermitted flag
133 Add flag for omnibox autocomplete filtering
134 Revert Delete block external form redirects
135 Add flag to disable external intent requests
136 Enable share intent
138 Add flag to disable vibration
143 Experimental user scripts support
149 Restore offline indicator v2 flag
160 Add flag for save data header
168 Disable TLS resumption
169 Move navigation bar to bottom
171 Add site engagement flag
172 Enable Certificate Transparency
205 AudioBuffer AnalyserNode fp mitigations
218 Fonts fingerprinting mitigation
222 Warning message for unsupported hardware aes
224 Add setting to invert tap and long tap
251 Viewport Protection flag
254 Disable speechSynthesis getVoices API
256 Log dangling attributes in some html elements
278 Add support to jxl

add also chrome/browser/flags/android/java/src/org/chromium/chrome/browser/flags/ChromeFeatureList.java
drop

  • "first_run: deactivate autoupdate globally"
  • "Offer builtin autocomplete for chrome://flags"
  • "00Temp-Disable-kAutomaticLazyFrameLoadingToEmbeds.patch"

@uazo
Copy link
Owner Author

uazo commented Nov 29, 2023

I'll end up like this for now.
I already have other improvements in mind for the future.

@uazo uazo marked this pull request as ready for review November 29, 2023 11:43
@uazo uazo merged commit 49a7e0f into master Nov 29, 2023
1 check passed
@uazo uazo deleted the new-method-change-flags branch November 29, 2023 12:09
@PF4Public
Copy link

FYI while building Cromite on Linux from 91419aa I get following error:

ninja -v -j8 -l25 -m90 -C out/Release chrome
ninja: Entering directory `out/Release'
ninja: error: '../../cromite_flags/third_party/blink/common/features_cc/placeholder.txt', needed by 'gen/cromite_flags/third_party_blink_common_features_cc.inc', missing and no known rule to make it

@uazo
Copy link
Owner Author

uazo commented Nov 30, 2023

strange, it is probably a problem with the order of the deps.
can you confirm that that file //cromite_flags/third_party/blink/common/features_cc/placeholder.txt exists?
I could have the contents of out/xxx/gen/cromite_flags/third_party_blink_common_features_cc.inc?

@PF4Public
Copy link

Oh, I see your builder had finished without errors.

Sorry for the noise, that was an issue with build/linux/unbundle/remove_bundled_libraries.py it is cleaning everything that has third_party in path. Just added cromite_flags/third_party into exclusion list, should be good now.

uazo added a commit that referenced this pull request Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants