-
Notifications
You must be signed in to change notification settings - Fork 425
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
Faster iframe api based player extraction. #694
Conversation
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.
Do you have some examples to back up the 1/50 size claim?
.../src/main/java/org/schabi/newpipe/extractor/services/youtube/YoutubeJavaScriptExtractor.java
Outdated
Show resolved
Hide resolved
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.
Can you think of a way to somehow automatically test if your new implementation is actually used and it does not just always go to the catch
block with the old implementation?
I can't think of anything right away, besides making everything public.
Edit: Reasoning being that the current tests either only cover your new implementation or the old one and not both
What would you like to happen if the new implementation fails? Printing an error? I could move them over to two methods so that they can be individually tested. |
Normally i would say log it, but because there is no logging setup => do nothing
Yeah that's probably the easiest thing |
8d2fccb
to
c2cce97
Compare
Done! Another thing I noticed is that the current tests do not account for the fact that the |
Can you also add tests for the 2 methods
You can create an issue for it. I have a rough idea on how to solve it, but finding the time to do it may prove hard |
Done! |
.../test/java/org/schabi/newpipe/extractor/services/youtube/YoutubeJavaScriptExtractorTest.java
Outdated
Show resolved
Hide resolved
Uses the IFrame API to reduce the required download to less than 1/50 of the size.
6296183
to
bb95168
Compare
bb95168
to
557140c
Compare
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.
Good enough, since i can't think of anything else either. As in better tests/rewriting for better testability
Uses the IFrame API to reduce the required download to less than 1/50 of the size.
Closes #693?