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.

Sign up for GitHub

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

Jump to bottom

fix: several source setting inconsistencies and slight refactor #5054

Merged
merged 1 commit into from
May 9, 2018
Merged

fix: several source setting inconsistencies and slight refactor #5054

merged 1 commit into from
May 9, 2018

Conversation

brandonocasey
Copy link
Contributor

@brandonocasey brandonocasey commented Mar 26, 2018 •
edited
Loading

Description

  • load should always fire sourceset
    • because mediaEl.src -> mediaEl.removeAttribute('src') -> load() should fire a sourceset
  • initial sourceset should fire the correct source
  • handle empty string sources
  • sourceset should always fire an absolute source
  • player cache should be absolute urls
  • reset souceset before dispose and init sourceset after initial sourceset

TODO

  • probably one more pr as some browsers support what I am calling "error recovery", which means that they will start watching for a source append after an error if there is currently no source.

gkatsev
gkatsev reviewed Mar 29, 2018
View reviewed changes
src/js/tech/html5.js Outdated
@@ -46,7 +46,7 @@ class Html5 extends Tech {
// 1) Check if the source is new (if not, we want to keep the original so playback isn't interrupted)
// 2) Check to see if the network state of the tag was failed at init, and if so, reset the source
// anyway so the error gets fired.
if (source && (this.el_.currentSrc !== source.src || (options.tag && options.tag.initNetworkState_ === 3))) {
Copy link
Member

Choose a reason for hiding this comment

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

I think removing the initNetworkState_ check breaks if not, we want to keep the original so playback isn't interrupted

Copy link
Member

Choose a reason for hiding this comment

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

maybe we don't need a sourceset to happen during late init?

@brandonocasey brandonocasey force-pushed the fix/sourceset-append-source branch from daab41d to 64d2f7b Compare April 2, 2018 19:45
@brandonocasey brandonocasey changed the base branch from fix/sourceset-append-source to master April 2, 2018 20:11
@brandonocasey brandonocasey reopened this Apr 2, 2018
@brandonocasey brandonocasey force-pushed the fix/source-inconsistencies branch from c081e0b to c2ce2ef Compare April 5, 2018 16:48
@brandonocasey brandonocasey changed the base branch from master to feat/sourceset-update-cache April 5, 2018 16:49
@brandonocasey brandonocasey changed the title WIP - fix: several source setting inconsistencies with the browser fix: several source setting inconsistencies and slight refactor Apr 5, 2018
gkatsev
gkatsev reviewed Apr 30, 2018
View reviewed changes
src/js/player.js Outdated
@@ -2612,8 +2612,6 @@ class Player extends Component {
}

if (!titleCaseEquals(sourceTech.tech, this.techName_)) {
this.changingSrc_ = true;
Copy link
Member

Choose a reason for hiding this comment

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

why remove this? Aren't we changing sources 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.

I changed this because there were some cases where it would cause us to be stuck in changingSrc_ = true. I think the better change though is to keep this line and then wait for the tech we loaded to be ready and then set changingSrc_ = false.

src/js/tech/setup-sourceset.js Outdated
const getSrc = (el) => {
// We use the attribute to check if a source is set because when
// the source for the element is set to a blank string. The attribute will
// return '' and the property will return window.location.href.
Copy link
Member

Choose a reason for hiding this comment

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

however, this is the actual url that the browser tried to load, why hide it?

src/js/utils/filter-source.js Outdated
const mimetype = getMimeType(src.src);

if (!src.type && mimetype) {
src.type = mimetype;
}

src.src = getAbsoluteURL(src.src);
Copy link
Member

Choose a reason for hiding this comment

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

why change the source to be an absolute url?

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 don't think we should, I think this was leftover from some testing I was doing with absolute vs relative sources.

@brandonocasey brandonocasey force-pushed the feat/sourceset-update-cache branch 2 times, most recently from caac13a to cd5072b Compare May 7, 2018 17:51
@brandonocasey brandonocasey force-pushed the fix/source-inconsistencies branch 2 times, most recently from a8635bb to 1af2545 Compare May 7, 2018 18:16
@brandonocasey brandonocasey force-pushed the feat/sourceset-update-cache branch from cd5072b to 167f431 Compare May 7, 2018 19:07
@brandonocasey brandonocasey force-pushed the fix/source-inconsistencies branch from 1af2545 to b191398 Compare May 7, 2018 19:17
@gkatsev
Copy link
Member

gkatsev commented May 8, 2018

Since this merges into #5040, we should wait until that one is approved before merging this in.

@brandonocasey brandonocasey force-pushed the fix/source-inconsistencies branch from c44adf5 to a6ebacd Compare May 9, 2018 01:11
@brandonocasey brandonocasey changed the base branch from feat/sourceset-update-cache to master May 9, 2018 14:47
@brandonocasey brandonocasey force-pushed the fix/source-inconsistencies branch from a6ebacd to 428a904 Compare May 9, 2018 14:51
@brandonocasey brandonocasey merged commit 6147e5f into master May 9, 2018
@brandonocasey brandonocasey deleted the fix/source-inconsistencies branch May 9, 2018 19:52
brandonocasey added a commit that referenced this pull request May 9, 2018
@brandonocasey
fix: sourceset and browser behavior inconsistencies (#5054)
3609e10 
  * We now trigger `sourceset` any time a `<source>` element is appended to a mediaEl with no source.
  * `load` should always fire a `sourceset`
  * `sourceset` should always be the absolute url.
@brandonocasey brandonocasey mentioned this pull request May 9, 2018
Merged
brandonocasey added a commit that referenced this pull request May 11, 2018
@brandonocasey
fix: sourceset and browser behavior inconsistencies from (#5054) as (…
e1d26d8 
…#5162)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gkatsev gkatsev gkatsev approved these changes

Assignees
No one assigned
Labels
confirmed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@brandonocasey @gkatsev