Tech 2.0 additional fixes #2166
Conversation
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 1766e2c (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: e3c0285 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 5d19240 (Please note that this is a fully automated comment.) |
| bufferedDuration += end - start; | ||
| } | ||
| buffered() { | ||
| return null; |
There was a problem hiding this comment.
should this return null? Or should it just return a 0, 0 time range?
There was a problem hiding this comment.
This is never called outside the unit tests. null to me seems more appropriate because their is no buffer at all.
There was a problem hiding this comment.
If this isn't used outside of tests do we need to define it here? Should we change something about the test or whatever relies on buffered?
Otherwise I think we should follow the spec.
The buffered attribute must return a new static normalised TimeRanges object that represents the ranges of the media resource, if any, that the user agent has buffered, at the time the attribute is evaluated. Users agents must accurately determine the ranges available, even for media streams where this can only be determined by tedious inspection.
http://www.w3.org/html/wg/drafts/html/master/semantics.html#dom-media-buffered
|
a few minor comments, otherwise, LGTM. |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 6f455f4 (Please note that this is a fully automated comment.) |
|
I don't know if saucelabs is supposed to work but locally the tests are fine. However, we have this error on the CI:
|
|
@pam retry |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 6f455f4 (Please note that this is a fully automated comment.) |
|
I have a feeling like this whole SauceLabs experiment is going to get killed pretty soon. These "random" failures are infuriating. |
|
@forbesjo was attempting to switch to grunt-sauce(something), which bootstrap uses and seems to be having better luck with, but Joe still ran into a bunch of random browser errors. Joe can you make a PR with however far you got, incase someone else wants to try to get farther? |
|
The PR I have open should skip the problematic browsers and have all the
|
|
Ah right, sorry. Forgot that was already a PR. #2149 |
|
Do I need to fix something so that the CI doesn't fail? |
|
Just tested locally and the tests pass for me, so I think we can move forward. Reviewing the PR myself now. |
| 'autoplay': this.options_.autoplay, | ||
| 'preload': this.options_.preload, | ||
| 'loop': this.options_.loop, | ||
| 'muted': this.options_.muted |
There was a problem hiding this comment.
Very nice. For #2033 I want to try to move away from using options like this and instead use the player properties (e.g. this.autoplay()), but there's still more work to do to make that even possible. Not something for this PR but just wanted to call it out as something that may change in the near future.
|
Aside from the |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: aa998c9 (Please note that this is a fully automated comment.) |
|
I though at first it would be better to fix the unit tests as well. However, the problem that we have is |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 956d77a (Please note that this is a fully automated comment.) |
|
Object does not support "defineProperty properties or methods for lte ie9 video.js, line:153 char:142 |
|
lgtm! merging in. @xiaocen2050 I think that's part of an issue @mmcc found, that we're not currently using loose mode for the babel process. |
|
@eXon FYI, since this from your master branch you're gonna need to rebase your master since we auto-squash multi-commit PRs. |
Just went over everything on #2126. Let me know if everything is good :)