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

[Android] youtube fullscreen in landscape and pip #23933

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions android/brave_java_sources.gni
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import("//brave/components/signin/public/android/java_sources.gni")
import("//components/feed/features.gni")

brave_java_sources = [
"../../brave/android/java/org/chromium/chrome/browser/BackgroundVideoPlaybackTabHelper.java",
"../../brave/android/java/org/chromium/chrome/browser/BraveAdFreeCalloutDialogFragment.java",
"../../brave/android/java/org/chromium/chrome/browser/BraveAppHooks.java",
"../../brave/android/java/org/chromium/chrome/browser/BraveApplicationImplBase.java",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/* Copyright (c) 2024 The Brave Authors. All rights reserved.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at https://mozilla.org/MPL/2.0/. */

package org.chromium.chrome.browser;

import org.jni_zero.JNINamespace;
import org.jni_zero.NativeMethods;

import org.chromium.content_public.browser.WebContents;

@JNINamespace("chrome::android")
public class BackgroundVideoPlaybackTabHelper {

public static void toggleFullscreen(WebContents webContents, boolean isFullScreen) {
BackgroundVideoPlaybackTabHelperJni.get().toggleFullscreen(webContents, isFullScreen);
}

public static boolean isPlayingMedia(WebContents webContents) {
return BackgroundVideoPlaybackTabHelperJni.get().isPlayingMedia(webContents);
}

@NativeMethods
interface Natives {
void toggleFullscreen(WebContents webContents, boolean isFullScreen);

boolean isPlayingMedia(WebContents webContents);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
73 changes: 64 additions & 9 deletions android/java/org/chromium/chrome/browser/app/BraveActivity.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import android.app.Activity;
import android.app.NotificationChannel;
import android.app.NotificationManager;
import android.app.PictureInPictureParams;
import android.content.Context;
import android.content.Intent;
import android.content.IntentSender;
Expand Down Expand Up @@ -83,6 +84,7 @@
import org.chromium.brave_wallet.mojom.TxService;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.ApplicationLifetime;
import org.chromium.chrome.browser.BackgroundVideoPlaybackTabHelper;
import org.chromium.chrome.browser.BraveAdFreeCalloutDialogFragment;
import org.chromium.chrome.browser.BraveFeatureUtil;
import org.chromium.chrome.browser.BraveHelper;
Expand Down Expand Up @@ -210,6 +212,7 @@
import org.chromium.mojo.bindings.ConnectionErrorHandler;
import org.chromium.mojo.system.MojoException;
import org.chromium.ui.widget.Toast;
import org.chromium.url.GURL;

import java.util.Arrays;
import java.util.Calendar;
Expand All @@ -218,17 +221,18 @@
import java.util.Locale;
import java.util.concurrent.CopyOnWriteArrayList;

/**
* Brave's extension for ChromeActivity
*/
/** Brave's extension for ChromeActivity */
@JNINamespace("chrome::android")
public abstract class BraveActivity extends ChromeActivity
implements BrowsingDataBridge.OnClearBrowsingDataListener, BraveVpnObserver,
OnBraveSetDefaultBrowserListener, ConnectionErrorHandler, PrefObserver,
BraveSafeBrowsingApiHandler.BraveSafeBrowsingApiHandlerDelegate,
BraveNewsConnectionErrorHandler.BraveNewsConnectionErrorHandlerDelegate,
MiscAndroidMetricsConnectionErrorHandler
.MiscAndroidMetricsConnectionErrorHandlerDelegate {
implements BrowsingDataBridge.OnClearBrowsingDataListener,
BraveVpnObserver,
OnBraveSetDefaultBrowserListener,
ConnectionErrorHandler,
PrefObserver,
BraveSafeBrowsingApiHandler.BraveSafeBrowsingApiHandlerDelegate,
BraveNewsConnectionErrorHandler.BraveNewsConnectionErrorHandlerDelegate,
MiscAndroidMetricsConnectionErrorHandler
.MiscAndroidMetricsConnectionErrorHandlerDelegate {
public static final String BRAVE_WALLET_HOST = "wallet";
public static final String BRAVE_WALLET_URL = "brave://wallet/crypto/portfolio/assets";
public static final String BRAVE_BUY_URL = "brave://wallet/crypto/fund-wallet";
Expand Down Expand Up @@ -261,6 +265,8 @@ public abstract class BraveActivity extends ChromeActivity

public static final int APP_OPEN_COUNT_FOR_WIDGET_PROMO = 25;

private static final String TAG = "BraveActivity";

private static final boolean ENABLE_IN_APP_UPDATE =
Build.VERSION.SDK_INT < Build.VERSION_CODES.UPSIDE_DOWN_CAKE;

Expand Down Expand Up @@ -366,9 +372,30 @@ public void onPauseWithNative() {
if (BraveVpnUtils.isVpnFeatureSupported(BraveActivity.this)) {
BraveVpnNativeWorker.getInstance().removeObserver(this);
}

super.onPauseWithNative();
}

@Override
public void onUserLeaveHint() {
super.onUserLeaveHint();
if (isActivityFinishingOrDestroyed()) return;
Tab currentTab = getActivityTab();
if (currentTab != null
&& currentTab.getUrl() != null
&& isYTVideoUrl(currentTab.getUrl())
&& !isInPictureInPictureMode()
&& BackgroundVideoPlaybackTabHelper.isPlayingMedia(currentTab.getWebContents())) {
BackgroundVideoPlaybackTabHelper.toggleFullscreen(currentTab.getWebContents(), true);
try {
enterPictureInPictureMode(new PictureInPictureParams.Builder().build());
} catch (IllegalStateException | IllegalArgumentException e) {
Log.e(TAG, "Error entering PiP: " + e);
return;
}
}
}

@Override
public boolean onMenuOrKeyboardAction(int id, boolean fromMenu) {
final Tab currentTab = getActivityTab();
Expand Down Expand Up @@ -1378,6 +1405,34 @@ public void initBraveNewsController() {
}
}

@Override
public void performOnConfigurationChanged(Configuration newConfig) {
super.performOnConfigurationChanged(newConfig);

Tab currentTab = getActivityTab();
if (currentTab != null
&& currentTab.getUrl() != null
&& isYTVideoUrl(currentTab.getUrl())
&& !isInPictureInPictureMode()) {
BackgroundVideoPlaybackTabHelper.toggleFullscreen(
currentTab.getWebContents(),
newConfig.orientation == Configuration.ORIENTATION_LANDSCAPE);
}
}

private boolean isYTVideoUrl(GURL url) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@bridiver bridiver Aug 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (!GURL.isEmptyOrInvalid(url)
&& url.domainIs(BraveConstants.YOUTUBE_DOMAIN)
&& url.getPath() != null
&& url.getPath().equalsIgnoreCase("/watch")
&& url.getQuery() != null) {
String videoId = UrlUtilities.getValueForKeyInQuery(url, "v");
return videoId != null && videoId.trim().length() > 0;
}
return false;
}

private void migrateBgPlaybackToFeature() {
if (ChromeSharedPreferences.getInstance()
.readBoolean(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,13 @@
import java.util.function.BooleanSupplier;

public abstract class BraveToolbarLayoutImpl extends ToolbarLayout
implements BraveToolbarLayout, OnClickListener, View.OnLongClickListener,
BraveRewardsObserver, BraveRewardsNativeWorker.PublisherObserver,
ConnectionErrorHandler, PlaylistServiceObserverImplDelegate {
implements BraveToolbarLayout,
OnClickListener,
View.OnLongClickListener,
BraveRewardsObserver,
BraveRewardsNativeWorker.PublisherObserver,
ConnectionErrorHandler,
PlaylistServiceObserverImplDelegate {
private static final String TAG = "BraveToolbar";

private static final List<String> BRAVE_SEARCH_ENGINE_DEFAULT_REGIONS =
Expand Down
3 changes: 1 addition & 2 deletions browser/android/preferences/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ import("//brave/components/ai_chat/core/common/buildflags/buildflags.gni")
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",
Expand All @@ -26,6 +24,7 @@ source_set("preferences") {
"//brave/components/brave_shields/core/common",
"//brave/components/brave_sync",
"//brave/components/p3a",
"//brave/components/script_injector/common/mojom",
"//components/content_settings/core/browser",
"//components/prefs",
"//content/public/browser",
Expand Down
1 change: 1 addition & 0 deletions browser/android/preferences/DEPS
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
specific_include_rules = {
"background_video_playback_tab_helper.cc": [
"+content/browser/media/session/media_session_impl.h",
"+content/browser/web_contents/web_contents_impl.h",
],
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,31 +7,35 @@

#include <string>

#include "base/strings/utf_string_conversions.h"
#include "brave/browser/android/preferences/features.h"
#include "brave/build/android/jni_headers/BackgroundVideoPlaybackTabHelper_jni.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 "brave/components/script_injector/common/mojom/script_injector.mojom.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/common/chrome_isolated_world_ids.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"
#include "content/public/browser/web_contents.h"
#include "mojo/public/cpp/bindings/associated_remote.h"
#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
#include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
#include "url/gurl.h"

namespace {
bool is_media_playing_ = false;

const char16_t k_youtube_background_playback_script[] =
u"(function() {"
" if (document._addEventListener === undefined) {"
" document._addEventListener = document.addEventListener;"
" document.addEventListener = function(a,b,c) {"
" if(a != 'visibilitychange') {"
" document._addEventListener(a,b,c);"
" }"
" };"
" if(a != 'visibilitychange') {"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this script even doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also there should be comments explaining what this is doing

" document._addEventListener(a,b,c);"
" }"
" };"
" }"
"}());";

Expand All @@ -58,6 +62,14 @@ bool IsBackgroundVideoPlaybackEnabled(content::WebContents* contents) {

return true;
}

mojo::AssociatedRemote<script_injector::mojom::ScriptInjector> GetRemote(
content::RenderFrameHost* rfh) {
mojo::AssociatedRemote<script_injector::mojom::ScriptInjector>
script_injector_remote;
rfh->GetRemoteAssociatedInterfaces()->GetInterface(&script_injector_remote);
return script_injector_remote;
}
} // namespace

BackgroundVideoPlaybackTabHelper::BackgroundVideoPlaybackTabHelper(
Expand All @@ -80,4 +92,68 @@ void BackgroundVideoPlaybackTabHelper::DidFinishNavigation(
}
}

void BackgroundVideoPlaybackTabHelper::MediaStartedPlaying(
const MediaPlayerInfo& /*video_type*/,
const content::MediaPlayerId& id) {
is_media_playing_ = true;
}

void BackgroundVideoPlaybackTabHelper::MediaStoppedPlaying(
const MediaPlayerInfo& /*video_type*/,
const content::MediaPlayerId& id,
WebContentsObserver::MediaStoppedReason /*reason*/) {
is_media_playing_ = false;
}

namespace chrome {
namespace android {

void JNI_BackgroundVideoPlaybackTabHelper_ToggleFullscreen(
JNIEnv* env,
const base::android::JavaParamRef<jobject>& jweb_contents,
jboolean is_full_screen) {
content::WebContents* web_contents =
content::WebContents::FromJavaWebContents(jweb_contents);
// Injecting this script to make youtube video fullscreen on landscape mode
// and exit fullscreen on portrait mode.
if (is_full_screen) {
constexpr const char16_t script[] =
uR"js(if(!document.fullscreenElement) {
var fullscreenBtn =
document.getElementsByClassName('fullscreen-icon');
if(fullscreenBtn && fullscreenBtn.length > 0) {
fullscreenBtn[0].click();
} else {
var moviePlayer = document.getElementById('movie_player');
if (moviePlayer) {
moviePlayer.click();
}
setTimeout(() => {
var fullscreenBtn =
document.getElementsByClassName('fullscreen-icon');
if(fullscreenBtn && fullscreenBtn.length > 0) {
fullscreenBtn[0].click();
}
}, 50);
}
} )js";
GetRemote(web_contents->GetPrimaryMainFrame())
->RequestAsyncExecuteScript(
ISOLATED_WORLD_ID_BRAVE_INTERNAL, script,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

blink::mojom::UserActivationOption::kActivate,
blink::mojom::PromiseResultOption::kAwait, base::NullCallback());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
if (web_contents->HasActiveEffectivelyFullscreenVideo()) {
web_contents->ExitFullscreen(true);
}
}
}

jboolean JNI_BackgroundVideoPlaybackTabHelper_IsPlayingMedia(
JNIEnv* env,
const base::android::JavaParamRef<jobject>& jweb_contents) {
return is_media_playing_;
}
} // namespace android
} // namespace chrome
WEB_CONTENTS_USER_DATA_KEY_IMPL(BackgroundVideoPlaybackTabHelper);
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@ class BackgroundVideoPlaybackTabHelper
void DidFinishNavigation(
content::NavigationHandle* navigation_handle) override;

void MediaStartedPlaying(const MediaPlayerInfo& video_type,
const content::MediaPlayerId& id) override;
void MediaStoppedPlaying(
const MediaPlayerInfo& video_type,
const content::MediaPlayerId& id,
WebContentsObserver::MediaStoppedReason reason) override;

WEB_CONTENTS_USER_DATA_KEY_DECL();
};

Expand Down
1 change: 1 addition & 0 deletions build/android/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ brave_xml_preprocessor("brave_java_xml_preprocess_resources") {

generate_jni("jni_headers") {
sources = [
"//brave/android/java/org/chromium/chrome/browser/BackgroundVideoPlaybackTabHelper.java",
"//brave/android/java/org/chromium/chrome/browser/BraveFeatureUtil.java",
"//brave/android/java/org/chromium/chrome/browser/BraveLocalState.java",
"//brave/android/java/org/chromium/chrome/browser/BraveRelaunchUtils.java",
Expand Down
1 change: 1 addition & 0 deletions build/android/config.gni
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ playcore_target =
"//brave/third_party/android_deps:com_google_android_play_core_java"

brave_jni_headers_sources = [
"//brave/android/java/org/chromium/chrome/browser/BackgroundVideoPlaybackTabHelper.java",
"//brave/android/java/org/chromium/chrome/browser/BraveFeatureUtil.java",
"//brave/android/java/org/chromium/chrome/browser/BraveLocalState.java",
"//brave/android/java/org/chromium/chrome/browser/BraveRelaunchUtils.java",
Expand Down
Loading