youtube quality android #196
Conversation
tapanmodh
requested review from
ShivanKaul,
szilardszaloki,
bcaller and
antonok-edm
resources/brave-youtube-quality.js
Outdated
| function handlePlaybackVideoQuality() { | ||
| let attemptCount = 0; | ||
| const maxAttempts = 5; | ||
| clearInterval(intervalId); |
There was a problem hiding this comment.
wouldn't it be better to use a mutation observer here?
| @@ -0,0 +1,56 @@ | |||
| /// brave-youtube-quality.js | |||
| (function() { | |||
| const IS_MOBILE_YOUTUBE = window.location.hostname == 'm.youtube.com'; | |||
There was a problem hiding this comment.
The hostname could be www.youtube.com
There was a problem hiding this comment.
I think we need to include both as we need m.youtube.com for mobile sites and www.youtube.com for desktop sites option
| /// brave-youtube-quality.js | ||
| (function() { | ||
| const IS_MOBILE_YOUTUBE = window.location.hostname == 'm.youtube.com'; | ||
| const IS_ANDROID = window.navigator.userAgent.indexOf('Android') > -1; |
There was a problem hiding this comment.
why don't you decide whether to inject the script or not in native (java or cpp)?
There was a problem hiding this comment.
@stoletheminerals it's not possible if we're using this via adblock scriptlet injections; those can be selectively injected per-domain, but not according to any other criteria. If you want to move checks to native code it'll have to be a completely separate injection mechanism.
There was a problem hiding this comment.
@antonok-edm thanks, yeah that makes sense. I'm just not sure why we decided to move this code to adblock scriptlets. To me it's weird to inject first and then check if we injected it on Android, when we could decide before the injection, but I understand that we can't do it in the current implementation. Anyway, it's not a security concern.
tapanmodh
force-pushed
the
youtube_quality_android
branch
from
fe6a3ae
to
ccf5518
Compare
| @@ -0,0 +1,30 @@ | |||
| /// brave-youtube-quality.js | |||
| (function() { | |||
There was a problem hiding this comment.
@tapanmodh do you know if the same script works for desktop ? I think It should. We can create an issue for the desktop if we want to use the same feature on the desktop.
There was a problem hiding this comment.
yes, same script works for desktop
filter_lists/list_catalog.json
Outdated
| @@ -117,7 +117,7 @@ | |||
| "support_url": "https://github.com/brave/adblock-lists" | |||
| }, | |||
| { | |||
| "url": "https://raw.githubusercontent.com/brave/adblock-lists/master/brave-lists/brave-specific.txt", | |||
| "url": "https://raw.githubusercontent.com/brave/adblock-lists/d2ab11d0241c5e135315652de96c52fa975dd0a0/brave-lists/brave-specific.txt", | |||
There was a problem hiding this comment.
It was for testing. I'll revert this to master
Related to issue: brave/brave-browser#32842
Brave-core PR: brave/brave-core#24164
Adblock-lists PR: brave/adblock-lists#1944