[Android] youtube quality settings #24164

tapanmodh · 2024-06-12T17:49:21Z

Resolves brave/brave-browser#32842

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
https://github.com/brave/reviews/issues/1720
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:

HD Definition Playback On
Go to Settings -> Media -> High Definition Playback -> Select On
Restart brave
Play any youtube video which has 720p resolution then it will play video with 720p resolution
HD Definition Playback Allow over Wi-Fi
Go to Settings -> Media -> High Definition Playback -> Select Allow over Wi-Fi
Restart brave
Mobile data: Youtube video play with default resolution decided by youtube.com player
On Wi-Fi: Play any youtube video which has 720p resolution then it will play video with 720p resolution
HD Definition Playback Off
Youtube video play with default resolution decided by youtube.com player

SergeyZhukovsky · 2024-06-25T18:40:18Z

android/java/org/chromium/chrome/browser/settings/BraveMediaYTQualityPreferences.java

+    @Override
+    public void setDefaultQuality(@YTVideoQuality int defaultQuality) {
+        UserPrefs.get(getProfile()).setInteger(BravePref.YT_VIDEO_QUALITY_PREF, defaultQuality);
+        BraveRelaunchUtils.askForRelaunch(getActivity());


do we really need to ask for relaunch? I think as a max we should reload a current tab or even don't do anything at all.

SergeyZhukovsky · 2024-06-25T18:41:42Z

android/java/org/chromium/chrome/browser/settings/BraveMediaRadioButtonYTQualityPreference.java

+            rbAllowWifi.setChecked(true);
+        } else if (mQuality == YTVideoQuality.OFF) {
+            rbOff.setChecked(true);
+        }


it's probably a good idea to setChecked to false for all 3 before the if or make default values in xml

SergeyZhukovsky

++

deeppandya

LGTM

mkarolin

strings++

stoletheminerals

security lgtm. cc @bridiver for the JS inject

tapanmodh · 2024-08-06T20:20:49Z

@stoletheminerals implementation changed so I have created new security/privacy review: https://github.com/brave/reviews/issues/1720

bridiver · 2024-08-08T01:24:30Z

browser/profiles/brave_renderer_updater.cc

+    PrefService* pref_service) {
+#if BUILDFLAG(IS_ANDROID)
+  int qualityPref = pref_service->GetInteger(kYTVideoQualityPref);
+  return qualityPref == static_cast<int>(settings::YTVideoQuality::kOn) ||


this is not something BraveRendererUpdater should have to understand. You should send the raw pref value and also this is not going to correctly update when the connection type changes

we have to send value based on preference set by user in the settings and connection type and for that we need connection type only when site starts loading. we are not listening for connection type changes.

bridiver · 2024-08-08T01:31:25Z

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc

@@ -47,7 +47,7 @@ const char kObservingScriptletEntryPoint[] =

 const char kScriptletInitScript[] =
    R"((function() {
-          let text = '(function() {\nconst scriptletGlobals = (() => {\nconst forwardedMapMethods = ["has", "get", "set"];\nconst handler = {\nget(target, prop) { if (forwardedMapMethods.includes(prop)) { return Map.prototype[prop].bind(target) } return target.get(prop); },\nset(target, prop, value) { if (!forwardedMapMethods.includes(prop)) { target.set(prop, value); } }\n};\nreturn new Proxy(new Map(%s), handler);\n})();\nlet deAmpEnabled = %s;\n' + %s + '})()';
+          let text = '(function() {\nconst scriptletGlobals = (() => {\nconst forwardedMapMethods = ["has", "get", "set"];\nconst handler = {\nget(target, prop) { if (forwardedMapMethods.includes(prop)) { return Map.prototype[prop].bind(target) } return target.get(prop); },\nset(target, prop, value) { if (!forwardedMapMethods.includes(prop)) { target.set(prop, value); } }\n};\nreturn new Proxy(new Map(%s), handler);\n})();\nlet deAmpEnabled = %s;\nlet isYtHdQualityPlaybackEnabled = %s;\n' + %s + '})()';


I don't think this makes sense as part of cosmetic filter or adblock. Either this class should be made into a more generic handler and renamed or we should create something new here. wdyt @antonok-edm

bridiver · 2024-08-08T01:43:52Z

browser/profiles/brave_renderer_updater.cc

@@ -9,6 +9,7 @@

 #include "base/check_is_test.h"
 #include "base/functional/bind.h"
+#include "brave/browser/android/constants.h"


missing android guard

bridiver · 2024-08-08T01:44:19Z

browser/android/constants.h

+namespace settings {
+
+// GENERATED_JAVA_ENUM_PACKAGE: org.chromium.chrome.browser.settings
+enum class YTVideoQuality { kOn, kAllowOverWifi, kOff };


youtube_constants.h or youtube/constants.h and also please don't abbreviate. Use Youtube/youtube instead of YT in general for clarity

bridiver · 2024-08-08T01:48:17Z

android/BUILD.gn

@@ -14,6 +14,7 @@ declare_args() {

 java_cpp_enum("brave_android_java_enums_srcjar") {
  sources = [
+    "//brave/browser/android/constants.h",


this file should have it's own java_cpp_enum in https://github.com/brave/brave-core/pull/24164/files#diff-c528ac841614c7466a1d915247b27506c4318d8bea37e40f0c63b186e24b28a0

The decentralized_dns one should also be fixed in a follow-up.

bridiver · 2024-08-08T01:49:45Z

components/constants/pref_names.h

@@ -115,6 +115,7 @@ inline constexpr char kPlayYTVideoInBrowserEnabled[] =
    "brave.play_yt_video_in_browser_enabled";
 inline constexpr char kBackgroundVideoPlaybackEnabled[] =
    "brave.background_video_playback";
+inline constexpr char kYTVideoQualityPref[] = "brave.yt_video_quality";


please move this into pref_names.h in the same directory as the enum along with its own java_cpp_strings target

The existing java_pref_names_srcjar has the same problem as https://github.com/brave/brave-core/pull/24164/files#r1708392864 above

stoletheminerals · 2024-08-08T13:56:30Z

android/java/org/chromium/chrome/browser/settings/BraveMediaYTQualityPreferences.java

+        if (chromeTabbedActivity != null && chromeTabbedActivity.getActivityTab() != null) {
+            Tab currentTab = chromeTabbedActivity.getActivityTab();
+            if (currentTab.getUrl().domainIs(BraveConstants.YOUTUBE_DOMAIN)) {
+                currentTab.reload();


why do we need to reload the tab?

github-actions · 2024-08-10T13:34:52Z

renderer/brave_content_renderer_client.cc

    new cosmetic_filters::CosmeticFiltersJsRenderFrameObserver(
-        render_frame, ISOLATED_WORLD_ID_BRAVE_INTERNAL, dynamic_params_closure);
+        render_frame, ISOLATED_WORLD_ID_BRAVE_INTERNAL, dynamic_params_closure,


_{reported by reviewdog 🐶}
[semgrep] Security hotspot found (ISOLATED_WORLD). A security-team member should analyze the code security for possible vulnerabilities.

Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/brave-isolated-world.yaml

Cc @thypon @diracdeltas @bridiver

github-actions · 2024-08-12T15:57:58Z

[puLL-Merge] - brave/brave-core@24164

Description

This PR introduces a new feature for YouTube video quality settings on Android devices. It adds a preference to control HD video playback on YouTube, allowing users to choose between always enabling HD quality, enabling it only on Wi-Fi, or disabling it.

Changes

Changes

android/brave_java_resources.gni and android/brave_java_sources.gni:
- Added new XML resource files and Java source files for the YouTube quality preferences.
BraveMediaRadioButtonYoutubeQualityPreference.java and BraveMediaYoutubeQualityPreferences.java:
- New Java classes to handle the UI and logic for YouTube quality preferences.
brave_media_radio_button_youtube_quality_preference.xml and brave_media_youtube_quality_preferences.xml:
- New XML layout files for the YouTube quality preference UI.
media_preferences.xml:
- Added a new preference item for YouTube high definition playback.
browser/android/youtube/:
- New directory with constants.h and pref_names.h for YouTube-related constants and preferences.
browser/brave_profile_prefs.cc:
- Registered the new YouTube video quality preference.
browser/profiles/brave_renderer_updater.cc:
- Updated to include the new YouTube HD quality playback setting in the renderer configuration.
browser/ui/android/strings/android_brave_strings.grd:
- Added new string resources for the YouTube quality settings.
common/brave_renderer_configuration.mojom:
- Added a new field is_youtube_hd_quality_playback_enabled to the DynamicParams struct.
components/cosmetic_filters/renderer/:
- Updated to pass the YouTube HD quality setting to the JavaScript handler.
renderer/brave_content_renderer_client.cc:
- Modified to include the YouTube HD quality setting when creating the cosmetic filters observer.

Possible Issues

The implementation assumes that the YouTube domain remains constant. If YouTube changes its domain structure, the reload functionality in BraveMediaYoutubeQualityPreferences.java might need to be updated.

Security Hotspots

No significant security hotspots were identified in this change. The new feature primarily deals with user preferences and does not introduce new attack surfaces or handle sensitive data.

bridiver · 2024-08-19T21:34:12Z

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.h

@@ -39,7 +39,8 @@ class CosmeticFiltersJSHandler {
  // filtering is enabled, and returns whether or not to proceed with cosmetic
  // filtering.
  bool ProcessURL(const GURL& url, std::optional<base::OnceClosure> callback);
-  void ApplyRules(bool de_amp_enabled);
+  void ApplyRules(bool de_amp_enabled,
+                  bool is_youtube_hd_quality_playback_enabled);


this is definitely not what we want to do here, we should selectively enable these scripts on the browser side by using different data sources, but also right now this is going to be disabled when adblock is disabled which is clearly not what we want here

bridiver

see https://github.com/brave/brave-core/pull/24164/files#r1722372784

kjozwiak · 2024-09-17T06:06:30Z

Removing from 1.70.x as the above needs to go into master first which is currently on 1.72.x. Once the above is merged, the milestone will be added automatically.

github-actions bot assigned tapanmodh Jun 12, 2024

tapanmodh force-pushed the youtube_quality_settings branch from 7ded4f2 to 739ad8f Compare June 25, 2024 11:41

tapanmodh added this to the 1.69.x - Nightly milestone Jun 25, 2024

tapanmodh requested review from SergeyZhukovsky, AlexeyBarabash and samartnik June 25, 2024 13:06

tapanmodh marked this pull request as ready for review June 25, 2024 13:06

tapanmodh requested review from deeppandya and a team as code owners June 25, 2024 13:06

SergeyZhukovsky reviewed Jun 25, 2024

View reviewed changes

SergeyZhukovsky approved these changes Jun 27, 2024

View reviewed changes

deeppandya approved these changes Jun 27, 2024

View reviewed changes

mkarolin approved these changes Jun 28, 2024

View reviewed changes

stoletheminerals requested a review from bridiver July 1, 2024 14:12

stoletheminerals approved these changes Jul 1, 2024

View reviewed changes

tapanmodh added 3 commits August 5, 2024 19:39

youtube quality settings

c8243be

youtube quality settings

06bc821

address PR comments

a348acd

tapanmodh marked this pull request as draft August 5, 2024 14:51

tapanmodh force-pushed the youtube_quality_settings branch from 4cb41ff to 10e92bd Compare August 5, 2024 14:51

youtube quality from components

1e219a2

tapanmodh force-pushed the youtube_quality_settings branch from 10e92bd to 1e219a2 Compare August 6, 2024 05:14

antonok-edm approved these changes Aug 6, 2024

View reviewed changes

This was referenced Aug 6, 2024

youtube quality android brave/adblock-resources#196

Open

youtube quality android brave/adblock-lists#1944

Open

tapanmodh marked this pull request as ready for review August 6, 2024 20:03

tapanmodh requested a review from a team as a code owner August 6, 2024 20:03

tapanmodh requested a review from deeppandya August 6, 2024 20:20

tapanmodh requested review from SergeyZhukovsky, stoletheminerals, antonok-edm and mkarolin August 6, 2024 20:20

tapanmodh modified the milestones: 1.69.x - Beta, 1.70.x - Nightly Aug 6, 2024

bridiver reviewed Aug 8, 2024

View reviewed changes

stoletheminerals reviewed Aug 8, 2024

View reviewed changes

github-actions bot added the puLL-Merge label Aug 10, 2024

github-actions bot reviewed Aug 10, 2024

View reviewed changes

github-actions bot added the needs-security-review label Aug 10, 2024

github-actions bot assigned bridiver, diracdeltas and thypon Aug 10, 2024

tapanmodh force-pushed the youtube_quality_settings branch from 9bbe6f4 to 108cabb Compare August 10, 2024 14:35

address PR comments

c136310

tapanmodh force-pushed the youtube_quality_settings branch from 108cabb to c136310 Compare August 12, 2024 15:57

bridiver reviewed Aug 19, 2024

View reviewed changes

bridiver requested changes Aug 19, 2024

View reviewed changes

kjozwiak removed this from the 1.70.x - Release milestone Sep 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Android] youtube quality settings #24164

[Android] youtube quality settings #24164

tapanmodh commented Jun 12, 2024 •

edited

Loading

SergeyZhukovsky Jun 25, 2024

SergeyZhukovsky Jun 25, 2024

SergeyZhukovsky left a comment

deeppandya left a comment

mkarolin left a comment

stoletheminerals left a comment

tapanmodh commented Aug 6, 2024

bridiver Aug 8, 2024

tapanmodh Aug 9, 2024

bridiver Aug 8, 2024

bridiver Aug 8, 2024

bridiver Aug 8, 2024 •

edited

Loading

bridiver Aug 8, 2024

bridiver Aug 8, 2024 •

edited

Loading

stoletheminerals Aug 8, 2024

github-actions bot Aug 10, 2024

github-actions bot commented Aug 12, 2024

Changes

bridiver Aug 19, 2024

bridiver left a comment

kjozwiak commented Sep 17, 2024

[Android] youtube quality settings #24164

Are you sure you want to change the base?

[Android] youtube quality settings #24164

Conversation

tapanmodh commented Jun 12, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeyZhukovsky left a comment

Choose a reason for hiding this comment

deeppandya left a comment

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

stoletheminerals left a comment

Choose a reason for hiding this comment

tapanmodh commented Aug 6, 2024

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

bridiver Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Aug 10, 2024

Choose a reason for hiding this comment

github-actions bot commented Aug 12, 2024

Description

Changes

Possible Issues

Security Hotspots

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

kjozwiak commented Sep 17, 2024

tapanmodh commented Jun 12, 2024 •

edited

Loading

bridiver Aug 8, 2024 •

edited

Loading

bridiver Aug 8, 2024 •

edited

Loading