-
Notifications
You must be signed in to change notification settings - Fork 893
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
Youtube fullscreen and picture in picture feature #26355
base: master
Are you sure you want to change the base?
Conversation
32add61
to
2213fcd
Compare
b419b06
to
5c2fc60
Compare
3ac8cd7
to
36bb32d
Compare
Chromium major version is behind target branch (131.0.6778.85 vs 132.0.6834.15). Please rebase. |
f33ba86
to
a3c42b1
Compare
FORWARD_DECLARE_TEST(YouTubeTabHelperBrowserTest, RuleMatchTestScriptFalse); | ||
FORWARD_DECLARE_TEST(YouTubeTabHelperBrowserTest, RuleMatchTestScriptTrue); | ||
|
||
// This class loads and stores the rules from the youtube.json file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could have more documentation here. What are some example rules? Is the JSON part of this PR? If not, maybe an example ruleset could be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I think I have a partial idea of what this is supposed to do. This should be genericizing and centralizing any scripting that we want to do for YouTube, right? For example, we previously had logic in background_video_playback_tab_helper.cc and this PR is removing that file. The background play is one example script that gets loaded.
Where does it get loaded from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK seems to get loaded from component updater 😄 I see the comments in youtube_component_installer.cc
which has the example directory structure and then youtube_json.h
has an example of the JSON.
After I figured it out, maybe the ask I have is just adding a comment like:
// - See `youtube_json.h` for an example of the JSON this class loads
// - See `youtube_component_installer.cc` for example of the component layout.
// This is where the location where the scripts are loaded from.
(unless I didn't understand it correctly!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be genericizing and centralizing any scripting that we want to do for YouTube, right?
Correct! That's the idea. But I haven't created the extension (yet) I've temporarily used extension ID (lhhcaamjbmbijmjbnnodjaknblkiagon) PSST. I'm going over https://github.com/brave/brave-core-crx-packager now. Probably I won't be able to complete the full process as presumably I don't have full access. I'll post an update soon.
Maybe we could have more documentation here. What are some example rules? Is the JSON part of this PR? If not, maybe an example ruleset could be useful.
The idea that seems the best compromize to me would be to have some tests that contain the JS we are injecting.
Same as done by Shivan here https://github.com/brave/brave-core/tree/master/test/data/psst-component-data
I'm also adding the comments you suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improved code comments 👉 ae6fac2
(#26355)
// |_ scripts/ | ||
// |_ keep-playing-audio.js | ||
// |_ fullscreen.js | ||
// See youtube_json.cc for the format of youtube.json. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to locate where the component would be. I saw some branches in
- brave/brave-core-crx-packager@master...youtube_fullscreen_landscape
- brave/brave-core-crx-packager@master...youtube_quality_android
But they didn't seem to be packaging a component. Looking at this extension ID (lhhcaamjbmbijmjbnnodjaknblkiagon
), it seems we would be piggybacking off the Brave Privacy Settings Selection for Sites Tool (PSST) Files
component. That seems to be guarded by the brave://flags/#brave-psst
flag (default OFF).
@ShivanKaul does it make sense to club these YouTube specific scripts into the PSST package? For reference, PSST added here.
I also couldn't find any packaging for PSST. Usually, you'd see something in https://github.com/brave/brave-core-crx-packager which would include an npm run
command (ex: npm run data-files-local-data-files
) then upload to go-update. I checked the job list in ci.brave.com and didn't see anything for PSST either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll include a better and more elaborate answer tomorrow: I've just picked a temporary extension ID for testing. Next step will be creating a new one specifically for the youtube injector component before marking this PR as ready to review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have created a new component ID ninjlighifanhiflenpeafpnanjemako
and a PEM key, everything has been uploaded to our 1Password vault, search for YouTube Script Injector Component
.
You won't be able to find any packaging for the PSST component as it didn't go into production eventually. I've used it as a starting point because the structure for what we need was similar. It's now time to fully diverge.
That seems to be guarded by the brave://flags/#brave-psst flag (default OFF).
That's something I was forced to use because I was using the PSST implementation, but it's now changing.
The new logic can be found in components/youtube_script_injector/common/features.cc
where the kBraveYouTubeScriptInjector
will be enabled by default and probably behind griffin.
So to answer your question:
does it make sense to club these YouTube specific scripts into the PSST package?
No, I think it does not make any sense, and I'm going to commit the new component ID now.
I also couldn't find any packaging for PSST
Correct, as PSST packaging didn't go into production.
Injects the script in `DidFinishNavigation` instead of waiting for `DocumentOnLoadCompletedInPrimaryMainFrame` as when playing YT video the latter may not be called on time.
Perform refactoring and remove unneeded test script parameters
1845ee5
to
2b3ab59
Compare
6f799f8
to
ae6fac2
Compare
❌ DRAFT PR NOT READY TO REVIEW
Resolves brave/brave-browser#37267
🎨 [Under discussion] Figma design (option 4): https://www.figma.com/design/eQclVYFfDpTgztKDCq9ROk/Youtube-Improvements?node-id=541-3325&node-type=section&m=dev
This Android component is solely and fully focused on pages containing YouTube videos and provide some enhancements by injecting two scripts:
keep-playing-audio.js
this script is something we are already injecting in our stable Brave for Android (check here). It seems to work good, and its only goal is to keep playing the audio when the app goes in background. Because it's battle tested I refrained from touching except for a few comments.fullscreen.js
This script instead takes care of triggering the fullscreen mode. It's injected only when one of these two events happen: 1.device rotation in landscape, 2. entering PIP mode. When the script is injected it plays the video if paused, and switch into fullscreen mode.👇
keep-playing-audio.js
👇
fullscreen.js
New Component Info
Component name:
YouTube Script Injector
ID:
ninjlighifanhiflenpeafpnanjemako
Public Key:
MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAnwhDV1BTzTeDNsi+DxnyxrKqugj/a/8dKQE8usNweMkpORdxFxflbJkA5IErDyW3xca63pdMYhArPjHr0OUBACYKBfzEYF/Yo0kNxxJLWylr7FKwZxOKosmlZXUW8J2wtFu8ua6oQB2lEOk7y2jxwUAka/a0JPxeDsU9ByadmAdVZ7aaiCEGsJgEtHrpPha/2AJi8xerYLeB4bFfFc2bdXKEsN4UzjVLYpCfsKAuxEtO2QC2+Rylz60Vq3ANjjnlXINDJrl3jsSwF6GXHEE4Qzl7Qvu142N9G2rsSxMyPlunD8EGrEjK9fGRc6RPGQy1LbyY3imXpiDp0vXicCaYuwIDAQAB
Key.pem available in 1Password vault. Search for
YouTube Script Injector Component
.Example of
manifest.json
Example of
youtube.json
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: