Warning text if the user has currently selected Block all cookies #27090

vadimstruts · 2024-12-24T02:17:47Z

Resolves brave/brave-browser#42295
Resolves brave/brave-browser#42062

added cookie policy setting and script blocking to the webcompat report
added a warning text with learn more link under the Block all cookies toggle (for Desktop, Android iOS).

Platform	Screenshot
Desktop
Android
iOS

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

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, for instance:

Test Plan:

cookie policy setting and script blocking fields in the webcompat report

send webcompat report
check the attached JSON for the new fields:
"block_scripts" - should contain value "true" or "false" depends from the settings
"cookie_policy" - should contain next possible values: "block" or "block_third_party" or "allow" (For Desktop or Android), or as for IOS it could contain value "block" otherwise it should not be present

DJAndries · 2025-01-02T22:44:45Z

components/webcompat_reporter/browser/fields.h

@@ -21,6 +21,8 @@ inline constexpr char kLanguageFarblingField[] = "languageFarblingEnabled";
 inline constexpr char kBraveVPNEnabledField[] = "braveVPNEnabled";
 inline constexpr char kChannelField[] = "channel";
 inline constexpr char kVersionField[] = "version";
+inline constexpr char kCookiePolicyField[] = "cookie_policy";
+inline constexpr char kBlockScriptsField[] = "block_scripts";


I will add these fields on the server-side. Please remember to update https://github.com/brave/brave-browser/wiki/Web-compatibility-reports

components/webcompat_reporter/browser/webcompat_report_uploader.cc

deeppandya

LGTM

github-actions · 2025-01-05T13:08:45Z

The security team is monitoring all repositories for certain keywords. This PR includes the word(s) "policy" and so security team members have been added as reviewers to take a look.

No need to request a full security review at this stage, the security team will take a look shortly and either clear the label or request more information/changes.

Notifications have already been sent, but if this is blocking your merge feel free to reach out directly to the security team on Slack so that we can expedite this check.

DJAndries

looks great, thanks

browser/webcompat_reporter/webcompat_reporter_service_delegate.cc

mkarolin · 2025-01-07T14:31:31Z

app/brave_settings_strings.grdp

+  <message name="IDS_SETTINGS_BRAVE_SHIELDS_COOKIE_CONTROL_BLOCK_ALL_DEPRECATED_LABEL" desc="Block all cookies deprecation warning">
+    Block all cookies option is deprecated. To force-enable, 
+  </message>
+  <message name="IDS_SETTINGS_BRAVE_SHIELDS_COOKIE_CONTROL_BLOCK_ALL_DEPRECATED_LINK_LABEL" desc="Block all cookies learn more link text">
+    Learn more
+  </message>


The To force-enable, Learn more part doesn't look right to me. Also, because this warning is shown with the setting itself, I don't think repeating Block all cookies is necessary. I would suggest perhaps:

This option is deprecated. <link>Learn how to force-enable</link>

cc: @rmcfadden3 WDYT?

@mkarolin how about The "Block all Cookies" option has been deprecated. To learn how to force-enable this option, visit [our help page](https://support.brave.com/).

Note that I just used the general support.brave.com link as a placeholder on the href. We should obviously embed the exact URL once we have it.

If the goal is to send someone to Flags or something, rather than help center, LMK and I can tweak the copy accordingly.

Thanks @rmcfadden3! Sounds good to me.

mkarolin · 2025-01-07T14:32:28Z

browser/ui/android/strings/android_brave_strings.grd

+      <message name="IDS_BLOCK_COOKIES_DEPRECATED_LABEL" desc="Block all cookies deprecation warning">
+        Block all cookies option is deprecated. To force-enable,
+      </message>
+      <message name="IDS_BLOCK_COOKIES_DEPRECATED_LINK_LABEL" desc="Block all cookies deprecation warning link">
+        <ph name="BEGIN_LINK_1">&lt;LINK_1&gt;</ph> learn more<ph name="END_LINK_1">&lt;/LINK_1&gt;</ph>
+      </message>


Same as the desktop.

I updated strings & screenshots above

mkarolin · 2025-01-08T16:49:30Z

app/brave_settings_strings.grdp

+  <message name="IDS_SETTINGS_BRAVE_SHIELDS_COOKIE_CONTROL_BLOCK_ALL_DEPRECATED_LABEL" desc="Block all cookies deprecation warning">
+    The "Block all Cookies" option has been deprecated. To learn how to force-enable this option, visit 
+  </message>
+  <message name="IDS_SETTINGS_BRAVE_SHIELDS_COOKIE_CONTROL_BLOCK_ALL_DEPRECATED_LINK_LABEL" desc="Block all cookies learn more link text">
+    our help page
+  </message>


It is not great for localization when a sentence is split up between strings. It is better to use placeholders like you have in the Android file, but it should be all in one string (both here and in the Android file).

Signed-off-by: Vadym Struts <[email protected]>

mkarolin · 2025-01-08T19:42:37Z

browser/ui/android/strings/android_brave_strings.grd

+      <message name="IDS_BLOCK_COOKIES_DEPRECATED_LABEL" desc="Block all cookies deprecation warning">
+        The "Block all Cookies" option has been deprecated. To learn how to force-enable this option, visit
+      </message>
+      <message name="IDS_BLOCK_COOKIES_DEPRECATED_LINK_LABEL" desc="Block all cookies deprecation warning link">
+        <ph name="BEGIN_LINK_1">&lt;LINK_1&gt;</ph> our help page<ph name="END_LINK_1">&lt;/LINK_1&gt;</ph>
+      </message>


Sorry, @vadimstruts but can you merge these into one as well?

Yeah, thanks, I missed it. Done

Signed-off-by: Vadym Struts <[email protected]>

mkarolin

strings++ Thanks!

brave-builds · 2025-01-10T16:07:51Z

Released in v1.76.6

vadimstruts linked an issue Dec 24, 2024 that may be closed by this pull request

Warning text if the user has currently selected Block all cookies brave/brave-browser#42295

Closed

github-actions bot assigned vadimstruts Dec 24, 2024

github-actions bot added the CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) label Dec 24, 2024

vadimstruts requested review from DJAndries and StephenHeaps December 30, 2024 21:49

vadimstruts marked this pull request as ready for review December 30, 2024 22:44

vadimstruts requested review from deeppandya and a team as code owners December 30, 2024 22:44

Brandon-T approved these changes Dec 30, 2024

View reviewed changes

DJAndries reviewed Jan 2, 2025

View reviewed changes

DJAndries requested changes Jan 3, 2025

View reviewed changes

components/webcompat_reporter/browser/webcompat_report_uploader.cc Outdated Show resolved Hide resolved

deeppandya approved these changes Jan 3, 2025

View reviewed changes

github-actions bot assigned kdenhartog and thypon Jan 5, 2025

DJAndries approved these changes Jan 6, 2025

View reviewed changes

DJAndries reviewed Jan 6, 2025

View reviewed changes

browser/webcompat_reporter/webcompat_reporter_service_delegate.cc Show resolved Hide resolved

mkarolin reviewed Jan 7, 2025

View reviewed changes

StephenHeaps approved these changes Jan 7, 2025

View reviewed changes

mkarolin reviewed Jan 8, 2025

View reviewed changes

vadimstruts added 8 commits January 8, 2025 19:15

added warning for desktop

86c1fbf

Signed-off-by: Vadym Struts <[email protected]>

added warning for android

153f6e9

Signed-off-by: Vadym Struts <[email protected]>

format

98f2f33

Signed-off-by: Vadym Struts <[email protected]>

fix test

38c7744

Signed-off-by: Vadym Struts <[email protected]>

working on adding new fields for webcompat report

e363373

Signed-off-by: Vadym Struts <[email protected]>

working on adding new fields for webcompat report

2ad2785

Signed-off-by: Vadym Struts <[email protected]>

working on adding new fields for webcompat report

07e28c3

Signed-off-by: Vadym Struts <[email protected]>

working on adding new fields for webcompat report

64487cd

Signed-off-by: Vadym Struts <[email protected]>

vadimstruts added 3 commits January 8, 2025 19:15

fix unit tests + format

25db351

Signed-off-by: Vadym Struts <[email protected]>

fix review suggestions

8571751

Signed-off-by: Vadym Struts <[email protected]>

fix strings

f859e52

Signed-off-by: Vadym Struts <[email protected]>

vadimstruts force-pushed the 42295-warning-text-if-the-user-has-currently-selected-block-all-cookies branch from 11cccab to f859e52 Compare January 8, 2025 18:16

fix strings

d478494

Signed-off-by: Vadym Struts <[email protected]>

mkarolin reviewed Jan 8, 2025

View reviewed changes

fix strings

665778f

Signed-off-by: Vadym Struts <[email protected]>

mkarolin approved these changes Jan 8, 2025

View reviewed changes

vadimstruts merged commit 53b345a into master Jan 10, 2025
18 checks passed

vadimstruts deleted the 42295-warning-text-if-the-user-has-currently-selected-block-all-cookies branch January 10, 2025 12:49

github-actions bot added this to the 1.76.x - Nightly milestone Jan 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warning text if the user has currently selected Block all cookies #27090

Warning text if the user has currently selected Block all cookies #27090

vadimstruts commented Dec 24, 2024 •

edited

Loading

DJAndries Jan 2, 2025

deeppandya left a comment

github-actions bot commented Jan 5, 2025

DJAndries left a comment

mkarolin Jan 7, 2025

rmcfadden3 Jan 7, 2025

mkarolin Jan 7, 2025

mkarolin Jan 7, 2025

vadimstruts Jan 8, 2025

mkarolin Jan 8, 2025

vadimstruts Jan 8, 2025

mkarolin Jan 8, 2025

vadimstruts Jan 8, 2025

mkarolin left a comment

brave-builds commented Jan 10, 2025

Warning text if the user has currently selected Block all cookies #27090

Warning text if the user has currently selected Block all cookies #27090

Conversation

vadimstruts commented Dec 24, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

cookie policy setting and script blocking fields in the webcompat report

Choose a reason for hiding this comment

deeppandya left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 5, 2025

DJAndries left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

brave-builds commented Jan 10, 2025

vadimstruts commented Dec 24, 2024 •

edited

Loading