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

Patch Chromium files to fix PIP behavior #27849

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

simoarpe
Copy link
Collaborator

@simoarpe simoarpe commented Feb 27, 2025

Converting patches into bytecode. Draft PR - Not ready to review

Resolves brave/brave-browser#44287

Demo (with Sound)

2Screen.Recording.2025-02-28.at.6.28.09.PM.mov

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:

Test Plan:

  • Open Brave Browser and navigate to youtube.com
  • Search for a random video and play it
  • Rotate the device in landscape mode
  • Navigate to the home screen using the swiping gestures
  • Observe the activity playing the video entering into picture in picture mode
  • Lock the screen
  • Observe the audio keeps playing
  • Unlock the screen
  • Observe the activity is still correctly playing the video in picture in picture mode

@simoarpe simoarpe added CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS CI/skip-windows-x64 Do not run CI builds for Windows x64 CI/skip-macos-arm64 Do not run CI builds for macOS arm64 labels Feb 27, 2025
@simoarpe simoarpe self-assigned this Feb 27, 2025
@simoarpe simoarpe requested a review from a team as a code owner February 27, 2025 16:31
@simoarpe simoarpe force-pushed the simone/fix-pip-behavior branch from 9334715 to 8ff8f83 Compare February 28, 2025 09:50
@samartnik
Copy link
Contributor

Why we need direct patches instead of bytecode ones? Are we planning to upstream it?

@simoarpe
Copy link
Collaborator Author

Happy to switch to bytecode approach. Moving this PR into a draft state.

@simoarpe simoarpe marked this pull request as draft February 28, 2025 16:14

@Override
public void exitPersistentFullscreenMode() {
+ if (mActivity.isInPictureInPictureMode() && !mTabHiddenByChangedTabs) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

And one more quiestion, shouldn't it be apllied only for onHidden case? It feels like in other cases of calling exitPersistentFullscreenMode we shouldn't care of mTabHiddenByChangedTabs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

exitPersistentFullscreenMode method is called also by onActivityStateChange, so acting only on onHidden case would not prevent the activity from being reset into normal mode.

brave-browser_–_FullscreenHtmlApiHandlerBase_java__brave-browser__all_main_

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-ios Do not run CI builds for iOS CI/skip-macos-arm64 Do not run CI builds for macOS arm64 CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows-x64 Do not run CI builds for Windows x64
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Picture in picture stops working when device screen is turned off
2 participants