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

[cookies] Add --cookies-from-browser support for Whale #9649

Merged
merged 1 commit into from
May 17, 2024

Conversation

roeniss
Copy link
Contributor

@roeniss roeniss commented Apr 8, 2024

Description of your pull request and other information

Currently, yt-dlp doens't support --cookies-from-browser Naver Whale browser because the browser uses its own directory in Windows, macOS, and Linux.

Luckily, Naver Whale is based on Chromium, so thanksfully I could just add a little bit of code.

Fixes #9307

Before submitting a pull request make sure you have:

In order to be accepted and merged into yt-dlp each piece of code must be in public domain or released under Unlicense. Check all of the following options that apply:

  • I am the original author of this code and I am willing to release it under Unlicense
  • I am not the original author of this code but it is in public domain or released under Unlicense (provide reliable evidence)

What is the purpose of your pull request?

edit: Add a mention for dog-fooding

@bashonly bashonly added the enhancement New feature or request label Apr 8, 2024
@bashonly bashonly changed the title [ie/youtube] Add cookie support for Whale browser [cookies] Add --cookies-from-browser support for Whale Apr 8, 2024
@pukkandan
Copy link
Member

#2903

@bashonly
Copy link
Member

bashonly commented Apr 8, 2024

@pukkandan see #9307. Whale uses a different keyring name on linux and can't be supported as chromium/chrome

@pukkandan
Copy link
Member

I see. Is this a common thing? Is #7805 similar issue? If so, it would be more beneficial to support passing keyring name in the CLI than adding just whale.

@bashonly
Copy link
Member

bashonly commented Apr 8, 2024

Is #7805 similar issue?

Looks like possibly yes, but really there's no way to tell from the limited info in that issue.

If so, it would be more beneficial to support passing keyring name in the CLI than adding just whale.

Maybe? But cramming more into the --cookies-from-browser syntax would not be great IMO. Would the user even necessarily know what the keyring name is? I think it might be better to add proper support for these browsers, as the alternative is involuntarily maintaining some sort of knowledge bank of keyring names in our tracker that we direct users to every time we get a dupe issue opened about one

@pukkandan
Copy link
Member

I am concerned the list will quickly grow too large, but I see your point. We can merge this for now and think of generalization if this keeps coming up

@roeniss
Copy link
Contributor Author

roeniss commented Apr 9, 2024

Wait please, I'll review my PR within 24 hours from now. Some fix seems not merged.

@coletdjnz
Copy link
Member

Is #7805 similar issue?

Looks like possibly yes, but really there's no way to tell from the limited info in that issue.

If so, it would be more beneficial to support passing keyring name in the CLI than adding just whale.

Maybe? But cramming more into the --cookies-from-browser syntax would not be great IMO. Would the user even necessarily know what the keyring name is? I think it might be better to add proper support for these browsers, as the alternative is involuntarily maintaining some sort of knowledge bank of keyring names in our tracker that we direct users to every time we get a dupe issue opened about one

I think at this point we should support more browsers for convenience of the user..

Large list of supported browsers doesn't matter imo. You could throw the full list into a section of the README, with only the main few listed on the option help string

@roeniss
Copy link
Contributor Author

roeniss commented Apr 9, 2024

code check is done.

I think there must be serveral ways instead of adding one by one. We just temporarily need more browsers example to decide which way to go. This is one of them.

@roeniss
Copy link
Contributor Author

roeniss commented May 14, 2024

Should I do something else to be merged?

@roeniss
Copy link
Contributor Author

roeniss commented May 16, 2024

@pukkandan gentle ping

@bashonly bashonly merged commit dd9ad97 into yt-dlp:master May 17, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for whale on --cookies-from-browser
4 participants