[Android] youtube fullscreen in landscape and pip #23933

tapanmodh · 2024-05-31T12:51:54Z

Resolves brave/brave-browser#37267

~~Resolves brave/brave-browser#32846~~ (Closed in favor of brave/brave-browser#37267)
~~Resolves brave/brave-browser#26670~~ (Closed in favor of brave/brave-browser#37267)

⚠️ Note: superseded by #26355

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/1607
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:

Brave should launch the full screen youtube player when it detects a user has rotated their device.
Brave should launch the pip mode when youtube video is playing

browser/android/preferences/background_video_playback_tab_helper.cc

stoletheminerals · 2024-05-31T17:00:30Z

android/java/org/chromium/chrome/browser/app/BraveActivity.java

+
+        String pattern =
+                "^(?:https?:)?(?:\\/\\/)?(?:youtu\\.be\\/|(?:www\\.|m\\.)?youtube\\.com\\/(?:watch|v|embed)(?:\\.php)?(?:\\?.*v=|\\/))([a-zA-Z0-9\\_-]{7,15})(?:[\\?&][a-zA-Z0-9\\_-]+=[a-zA-Z0-9\\_-]+)*(?:[&\\/\\#].*)?$";
+        Pattern compiledPattern = Pattern.compile(pattern);


Android Chromium has url/android/java/src/org/chromium/url/GURL.java, I think we should use that instead of regexing. youtu.be links are redirected to youtube.com, that eTLD+1 is all we need to check regarding the domain. Please check this native implementation of YT video URL matching here https://github.com/brave/brave-core/pull/20730/files#diff-21b9f0db07f562d309edd861da6dea61508a287c552ac00f08bb70aa121c5b09R34

thank you @stoletheminerals. I'll update it

android/java/org/chromium/chrome/browser/app/BraveActivity.java

SergeyZhukovsky

++

deeppandya · 2024-06-10T09:20:15Z

android/java/org/chromium/chrome/browser/app/BraveActivity.java

+        }
+    }
+
+    private boolean isYTVideoUrl(GURL url) {


@tapanmodh can we place this function into some util class ? i think we are already using similar function at other places

we are using this function in two places in BraveActivity class. we can place into util class but it is not being used by any another class. I think we should move after it being used by another class.

we need some kind of test for this so would it be easier to unit test if we put it someplace else? Or actually maybe it's better to pass the url and isInPictureInPictureMode to BackgroundVideoPlaybackTabHelper and change it to maybeToggleFullscreen so we can test it in a normal browser test and also encapsulate the logic for handling this in a single place.

deeppandya · 2024-06-10T09:23:35Z

android/java/org/chromium/chrome/browser/BackgroundVideoPlaybackTabHelper.java

+    interface Natives {
+        void sendOrientationChangeEvent(WebContents webContents, boolean isFullScreen);
+
+        boolean isPlayingMedia(WebContents webContents);


Do we need to expose this function ? can we check idf the media is being played before we inject the script ?

we are not injecting the script in this case. we are checking only when user press the home button. we have to display PIP on portrait mode if video is playing.

doesn't FullscreenVideoPictureInPictureController already handle this? Also isPictureInPictureAllowedForFullscreenVideo in webcontents?

We have to display PIP from portrait mode and in that case FullscreenVideoPictureInPictureController is not handling this.

deeppandya

LGTM

deeppandya · 2024-06-12T18:23:36Z

@stoletheminerals do you have any concerns with changes ?

stoletheminerals

lgtm

bridiver · 2024-06-13T17:27:55Z

browser/android/preferences/background_video_playback_tab_helper.cc

+  std::string script;
+  if (is_full_screen) {
+    script =
+        "if(!document.fullscreenElement) {"


If YT updates their site and these classes/ids no longer match then Brave is going to look broken.

also please assign this to constexpr const char16_t and the if/else will just decide which one to run

use uR"js( ... )js" for multiline scripts in general, but I think we need to source this script from a component to handle YT site changes

actually do we even need this script at all? Can't we just call Browser::EnterFullscreenModeForTab?

sorry, can't use Browser here for android. On desktop setting it to fullscreen isn't the same as changing the video to fullscreen so not sure if we can use native methods here or not, but we definitely can below to exit

looks like FullscreenManager is the equivalent in java, but not sure if it has the desired behavior here or not

FullscreenManager does not have any function for desired behavior.

bridiver · 2024-06-13T17:28:27Z

browser/android/preferences/background_video_playback_tab_helper.cc

+  rfh->GetRemoteAssociatedInterfaces()->GetInterface(&script_injector_remote);
+  return script_injector_remote;
+}
+void JNI_BackgroundVideoPlaybackTabHelper_SendOrientationChangeEvent(


this method name doesn't make sense to me, it's not sending an orientation change, it's toggling YT fullscreen

also why are we putting this inside background_video_playback_tab_helper.cc?

We are already doing background audio playback for youtube videos in this class. I'll change the class name.

bridiver · 2024-06-13T17:30:09Z

browser/android/preferences/background_video_playback_tab_helper.cc

+        "}";
+  }
+  GetRemote(web_contents->GetPrimaryMainFrame())
+      ->RequestAsyncExecuteScript(


the use of RequestAsyncExecuteScript is not needed here because we're not executing anything that returns a result. web_contents()->GetPrimaryMainFrame()->ExecuteJavaScriptInIsolatedWorld is fine here

web_contents()->GetPrimaryMainFrame()->ExecuteJavaScriptInIsolatedWorld gives Failed to execute 'requestFullscreen' on 'Element': API can only be initiated by a user gesture. error. Can we execute script with user gesture in ExecuteJavaScriptInIsolatedWorld?

bridiver · 2024-06-13T17:32:15Z

browser/android/preferences/background_video_playback_tab_helper.cc

+        "     if (moviePlayer) {"
+        "         moviePlayer.click();"
+        "     }"
+        "     setTimeout(() => {"


why does this use setTimeout?

Youtube is not loading controllers initially for this usecase and because of that we are using setTimeout.

bridiver · 2024-06-13T17:40:20Z

browser/android/preferences/background_video_playback_tab_helper.cc

+namespace chrome {
+namespace android {
+
+mojo::AssociatedRemote<script_injector::mojom::ScriptInjector> GetRemote(


helper methods should go in anonymous namespaces

bridiver · 2024-06-13T17:42:04Z

This needs tests. Please use a simple test page with a media element with a fullscreen button

bridiver · 2024-06-13T17:45:32Z

browser/android/preferences/background_video_playback_tab_helper.cc

+        "   }"
+        "}";
+  } else {
+    script =


WebContents::HasActiveEffectivelyFullscreenVideo
WebContents::ExitFullscreen

bridiver · 2024-06-13T17:52:11Z

browser/android/preferences/background_video_playback_tab_helper.cc

-    "                document._addEventListener(a,b,c);"
-    "            }"
-    "        };"
+    "           if(a != 'visibilitychange') {"


what is this script even doing?

It is handling background audio playback if we put app in background while video is playing

I don't see why you need to keep a reference to the original function in document._addEventListener and it could potentially cause namespace collision problems. Also the timing of calling it in DidFinishNavigation does not guarantee that it will happen before any visibility change so you could end up in a place where it gets permanently set to not visible. This needs to be injected in the renderer and should not keep the original in document._addEventListener. You don't need to worry about calling it more than once if you inject it only at the right time.

also there should be comments explaining what this is doing

bridiver · 2024-06-13T18:21:11Z

browser/android/preferences/background_video_playback_tab_helper.cc

+    const base::android::JavaParamRef<jobject>& jweb_contents) {
+  content::WebContents* web_contents =
+      content::WebContents::FromJavaWebContents(jweb_contents);
+  content::MediaSessionImpl* media_session_impl =


you cannot access MediaSessionImpl outside of content. The only reason this compiles is because #include "content/browser/web_contents/web_contents_impl.h" is already incorrectly included and should be changed to #include "content/public/browser/web_contents.h" and check_includes = false should be removed from the target

the correct way to get the status of media playing is to implement WebContentsObserver::MediaStartedPlaying/WebContentsObserver::MediaStoppedPlaying` with a tab helper, but as mentioned in the comment above it doesn't seem like this method is even necessary

diff --git a/browser/android/preferences/BUILD.gn b/browser/android/preferences/BUILD.gn index 17a3651be37..20369a29f0c 100644 --- a/browser/android/preferences/BUILD.gn +++ b/browser/android/preferences/BUILD.gn @@ -7,8 +7,6 @@ import("//brave/components/ai_chat/core/common/buildflags/buildflags.gn> import("//build/config/android/rules.gni") source_set("preferences") { - # Remove when https://github.com/brave/brave-browser/issues/10657 is resolved - check_includes = false sources = [ "background_video_playback_tab_helper.cc", "background_video_playback_tab_helper.h", diff --git a/browser/android/preferences/background_video_playback_tab_helper.cc b/brow> index fdbdcd95c11..e2a15155ab2 100644 --- a/browser/android/preferences/background_video_playback_tab_helper.cc +++ b/browser/android/preferences/background_video_playback_tab_helper.cc @@ -7,14 +7,10 @@ #include <string> -#include "base/strings/utf_string_conversions.h" #include "brave/browser/android/preferences/features.h" #include "brave/components/brave_shields/content/browser/brave_shields_util.h" -#include "brave/components/constants/pref_names.h" -#include "chrome/browser/content_settings/host_content_settings_map_factory.h" #include "chrome/browser/profiles/profile.h" #include "components/prefs/pref_service.h" -#include "content/browser/web_contents/web_contents_impl.h" #include "content/public/browser/navigation_controller.h" #include "content/public/browser/navigation_entry.h" #include "content/public/browser/navigation_handle.h"

I have changed it to implement WebContentsObserver::MediaStartedPlaying/WebContentsObserver::MediaStoppedPlaying

github-actions · 2024-06-24T08:31:21Z

browser/android/preferences/background_video_playback_tab_helper.cc

+        ->RequestAsyncExecuteScript(
+            ISOLATED_WORLD_ID_BRAVE_INTERNAL, script,
+            blink::mojom::UserActivationOption::kActivate,
+            blink::mojom::PromiseResultOption::kAwait, base::NullCallback());


_{reported by reviewdog 🐶}
[semgrep] RequestAsyncExecuteScript usages should be vet by the security-team.

References:
- https://github.com/brave/brave-browser/wiki/Security-reviews (point 13)

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

Cc @thypon @diracdeltas @bridiver

github-actions · 2024-06-24T08:31:21Z

browser/android/preferences/background_video_playback_tab_helper.cc

+        } )js";
+    GetRemote(web_contents->GetPrimaryMainFrame())
+        ->RequestAsyncExecuteScript(
+            ISOLATED_WORLD_ID_BRAVE_INTERNAL, script,


_{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

kjozwiak · 2024-07-24T03:55:34Z

Removing from the 1.68.x milestone as the above wasn't even merged into master yet.

bsclifton · 2024-12-19T22:46:54Z

Closing in favor of #26355

tapanmodh added this to the 1.68.x - Nightly milestone May 31, 2024

tapanmodh requested review from deeppandya, SergeyZhukovsky, AlexeyBarabash and samartnik May 31, 2024 12:51

tapanmodh self-assigned this May 31, 2024

github-actions bot added the needs-security-review label May 31, 2024

github-actions bot assigned bridiver, diracdeltas and thypon May 31, 2024

tapanmodh requested a review from a team as a code owner May 31, 2024 12:57

diracdeltas requested a review from stoletheminerals May 31, 2024 16:34

diracdeltas removed their assignment May 31, 2024

stoletheminerals reviewed May 31, 2024

View reviewed changes

SergeyZhukovsky reviewed Jun 3, 2024

View reviewed changes

android/java/org/chromium/chrome/browser/app/BraveActivity.java Outdated Show resolved Hide resolved

tapanmodh force-pushed the youtube_fullscreen_landscape branch from 32e7eb9 to 7c66332 Compare June 3, 2024 19:23

SergeyZhukovsky approved these changes Jun 4, 2024

View reviewed changes

deeppandya reviewed Jun 10, 2024

View reviewed changes

deeppandya approved these changes Jun 12, 2024

View reviewed changes

stoletheminerals approved these changes Jun 13, 2024

View reviewed changes

stoletheminerals removed the needs-security-review label Jun 13, 2024

bridiver reviewed Jun 13, 2024

View reviewed changes

tapanmodh added 5 commits June 24, 2024 10:40

youtube fullscreen in landscape

c63c162

update deps file

2dcfeff

update youtube video url check

7b7a1bd

fix component build error

e6613e4

address PR comments

14a4d00

tapanmodh force-pushed the youtube_fullscreen_landscape branch from 7c66332 to 14a4d00 Compare June 24, 2024 08:21

github-actions bot reviewed Jun 24, 2024

View reviewed changes

github-actions bot added the needs-security-review label Jun 24, 2024

github-actions bot assigned diracdeltas Jun 24, 2024

kjozwiak removed this from the 1.68.x - Release milestone Jul 24, 2024

simoarpe mentioned this pull request Nov 4, 2024

Youtube fullscreen and picture in picture feature #26355

Draft

24 tasks

bsclifton closed this Dec 19, 2024

[Android] youtube fullscreen in landscape and pip #23933

[Android] youtube fullscreen in landscape and pip #23933

Conversation

tapanmodh commented May 31, 2024 • edited by simoarpe Loading

Resolves brave/brave-browser#37267

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Aug 5, 2024 • edited Loading

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

deeppandya left a comment

Choose a reason for hiding this comment

deeppandya commented Jun 12, 2024

stoletheminerals 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

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

bridiver commented Jun 13, 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

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

github-actions bot Jun 24, 2024

Choose a reason for hiding this comment

github-actions bot Jun 24, 2024

Choose a reason for hiding this comment

kjozwiak commented Jul 24, 2024

bsclifton commented Dec 19, 2024

tapanmodh commented May 31, 2024 •

edited by simoarpe

Loading

bridiver Aug 5, 2024 •

edited

Loading