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

Update processing model to most recent spec + add processing model tests #196

Merged
merged 4 commits into from
Dec 16, 2013

Conversation

RickEyre
Copy link
Contributor

@RickEyre RickEyre commented Dec 9, 2013

Depends on #194 #187, #198.

Fixes #173, #148, #110.

This isn't ready to land yet as we need to land the others first and I still need to give it a once over to make sure that the tests are correct.

Some of the CSS settings we set don't currently work in PhantomJS for some reason, as well as document.createProcessingInstruction. I'm currently trying to figure out how to get PhantomJS to support it as it's in the source and Chrome and Safari have it, so I would think PhantomJS would be able to have it as well. I've disabled processing model tests on time stamp tests for now for this reason.

I don't think we should block on these things though and just fix them as we either get PhantomJS to support it or move to Testling. See #158 for more discussion.

@RickEyre
Copy link
Contributor Author

It's failing because Travis is getting a different computed heights and width. Difference of like ~1px. I wonder if we could do something like have an acceptable range for computed styles or something... ?

@RickEyre
Copy link
Contributor Author

I'm going to disable tests that use computed styles in them for now because we need a more robust way of dealing with different computed styles on different systems. Later when we ass in the overlap avoidance there are going to be a lot of discrepancies like this.

@RickEyre
Copy link
Contributor Author

Switched to just doing parsing tests instead of disabling the tests completely...

@@ -11,7 +11,7 @@ module.exports = function( grunt ) {
curly: true,
eqeqeq: true,
newcap: true,
unused: true,
// Add this back once process algo is fixed // unused: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

File a bug on this, link to it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed as #203.

@humphd
Copy link
Contributor

humphd commented Dec 13, 2013

A few comments and questions. I think this is generally fine. r=me when you've sorted this stuff out.

@humphd
Copy link
Contributor

humphd commented Dec 13, 2013

Also, you should update the README to reflect the addition of overlay to some functions.

@RickEyre
Copy link
Contributor Author

Thanks for review Dave. So looking into the _reset it looks like we can't expose that on VTTCue in Firefox as the WebIDLs reserve _ prefixed names. So either we can rename _reset to reset in the polyfill or we can do something like var reset = cue._reset || cue.reset? Any preference @humphd ?

@RickEyre
Copy link
Contributor Author

I'll open up a follow up bug for the reset name change as it's not completely related to this.

@RickEyre
Copy link
Contributor Author

Follow up issue filed as #204.

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

Successfully merging this pull request may close these issues.

Update the processing model to the reflect the changes in the spec
2 participants