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

Publish an event from DrmEngine if the CDM accepts the license. #1049

Merged

Conversation

chrisfillmore
Copy link
Contributor

Resolves #1035

@@ -46,6 +46,8 @@ describe('DrmEngine', function() {
var onKeyStatusSpy;
/** @type {!jasmine.Spy} */
var onExpirationSpy;
/** @type {!jasmine.Spy} */
var onEventSpy;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created this spy but don't actually use it for anything. Suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we need a separate integration test for this, but you could always expect that onEvent was called at the appropriate time in the "basic flow" test below.

Note that DRM integration tests are only run if you use ./build/test.py --drm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've left this as is. The new behaviour is already covered by a unit test, and I noticed that onExpirationSpy is similarly unused in drm_engine_integration. I'll leave it to you guys to decide whether you need these spies.

Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

Looks good otherwise. Thanks!

.then(function(response) {
if (this.destroyed_) return Promise.reject();

// Request succeeded, now pass the response to the CDM.
return session.update(response.data).then(function() {
var event = new shaka.util.FakeEvent('drmsessionupdate');
Copy link
Member

Choose a reason for hiding this comment

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

Please document this event type in lib/player.js. You'll see several examples toward the top of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -46,6 +46,8 @@ describe('DrmEngine', function() {
var onKeyStatusSpy;
/** @type {!jasmine.Spy} */
var onExpirationSpy;
/** @type {!jasmine.Spy} */
var onEventSpy;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we need a separate integration test for this, but you could always expect that onEvent was called at the appropriate time in the "basic flow" test below.

Note that DRM integration tests are only run if you use ./build/test.py --drm.

@joeyparrish joeyparrish self-assigned this Oct 4, 2017
@joeyparrish joeyparrish added the type: enhancement New feature or request label Oct 4, 2017
@joeyparrish joeyparrish added this to the v2.3.0 milestone Oct 4, 2017
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

Docs look good to me. Thanks!

Are you planning to change the integration tests to incorporate onEvent? If so, I'll wait to do the build bot pass.

@chrisfillmore
Copy link
Contributor Author

No, I don't think I need to add new integration tests.

@shaka-bot
Copy link
Collaborator

All tests passed!

@joeyparrish joeyparrish merged commit d1ddd72 into shaka-project:master Oct 4, 2017
@joeyparrish
Copy link
Member

Thanks, Chris!

@joeyparrish
Copy link
Member

Cherry-picked to v2.2.3.

@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants