Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[video_player] Add cookie support for network resources #2882

Conversation

jaysephjw
Copy link

@jaysephjw jaysephjw commented Jul 16, 2020

Description

I Broke this change up in to 3 parts, #3136 #3137 and #3138. Review each step individually.

Add cookie support for video_player network resources.

Avoids the problem of relying on "AVURLAssetHTTPHeaderFieldsKey" (undocumented ios API) seen in #953 and #897 by only supporting cookies.

Implementation

Dart:
Optionally include a List when calling VideoPlayerController.network
Pigeon these over to hosts via List, which each cookie as a "Set-Cookie" value strings.

iOS:
Parse the strings into NSHTTPCookies and attach them using AVURLAssetHTTPCookiesKey

Android:
Parse the strings into HttpCookies so we can then break name=value pairs back out, and build a "Cookie" header for EXOPlayer

Web:
Set the cookies onto the document with the document.cookie setter, which supports full "Set-Cooke" value strings. Note: Haven't tested web yet. Are these still there after the player disposes? Can we cleanup?

Related Issues

Testing done

Tested on iOS and Android with an HLS server that requires an auth cookie.

Breaking Change

  • No, this is not a breaking change.

Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I updated/added relevant documentation (doc comments with ///).
  • I read and followed the [Flutter Style Guide].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.
  • I have tested on Android
  • I have tested on iOS

TODO:

  • Test on web
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.

@jaysephjw
Copy link
Author

jaysephjw commented Jul 16, 2020

Realized I need to be using the pigeon tool (neat!) to update the courier types instead of by hand (oops!).

Currently blocked on that since I need List, but can't get List to work flutter/flutter#61660

@jaysephjw jaysephjw force-pushed the jaysephjw/video_player/add_cookie_support branch from b3b65eb to 5b2fce7 Compare August 31, 2020 19:26
@rohitsharma1113
Copy link

rohitsharma1113 commented Oct 11, 2020

Expecting this to get merged anytime soon @jaysephjw ?
Needed the cookies functionality for serving content securely via CloudFront.

@jaysephjw
Copy link
Author

@rohitsharma1113 I am hoping to! Updating this today. But have not gotten any review yet.

From looking at previous PRs and feedback from the build system, it looks like I may need to separate PRs out so as to bump the platform_interface first. I'll do that now.

@jaysephjw
Copy link
Author

@iskakaushik @cyanglaz can anyone take a look?

@@ -19,6 +23,7 @@
## 0.10.12+3

* Avoiding uses or overrides a deprecated API in `VideoPlayerPlugin` class.
>>>>>>> master
Copy link
Member

Choose a reason for hiding this comment

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

Leftovers of a merge here

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Can I close this PR then in favour of the other 3; #3136 #3137 and #3138?

Copy link
Author

@jaysephjw jaysephjw Jan 10, 2021

Choose a reason for hiding this comment

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

@Salakar If that's best then yes, whatever makes the most sense to avoid confusion. As a start I updated this PR to have 0 diffs so its obvious the changes are happening elsewhere.

@rohitsharma1113
Copy link

Any update on this ?

@jaysephjw jaysephjw changed the title [video_player] Add cookie support for video_player network resources [video_player] Add cookie support for network resources Jan 10, 2021
@jaysephjw jaysephjw closed this Jan 10, 2021
@jaysephjw jaysephjw force-pushed the jaysephjw/video_player/add_cookie_support branch from ee694d3 to 34faaaf Compare January 10, 2021 18:05
@jaysephjw
Copy link
Author

I may wait for #3281, which updates the pigeon version, before landing this. I'll then push new diffs from the new pigeon version.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants