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

Bugfix for issue #3100 #4673

Merged
merged 2 commits into from
Oct 31, 2017
Merged

Conversation

mmodrow
Copy link
Contributor

@mmodrow mmodrow commented Oct 18, 2017

Description

url.js parseUrl drops the protocol from the return object on IE if the url starts with "//:". Chrome returns the current window.location.protocol value in that case. This causes the test mentioned in #3100 to fail on IE, but not on Chrome or FF.

Specific Changes proposed

I added an if-clause to url.js‘s parseUrl to add the current window.location.protocol if the protocol of the given url is empty (or //).

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

@gkatsev
Copy link
Member

gkatsev commented Oct 19, 2017

This is probably fine. I'll have to take a look and see if there might be any issues with this.

if (details.protocol === '') {
details.protocol = window.location.protocol;
}

Copy link
Member

Choose a reason for hiding this comment

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

Overally, seems like a good change/fix. I wonder if it ought to be simply testing !details.protocol instead?

Also, maybe it should be moved above the if (addToBody) {} block to keep the details.protocol conditions together?

Copy link
Contributor Author

@mmodrow mmodrow Oct 23, 2017

Choose a reason for hiding this comment

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

There should be no protocol that is as well legal as falsy. So !details.protocol sounds good.

You probably refer to the 2nd if (addToBody) in your second suggestion? That should be fine as well.

That would result in a result like:

  if (details.protocol === 'https:') {
    details.host = details.host.replace(/:443$/, '');
  }

  if (!details.protocol) {
    details.protocol = window.location.protocol;
  }

  if (addToBody) {
    document.body.removeChild(div);
  }

  return details;
};

Copy link
Member

Choose a reason for hiding this comment

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

Yep, exactly. That'd be great, thanks!

…ionals and expanded from (details.protocol === '') to (!details.protocol)

Both implement suggestions from @misteroneill .
@mmodrow
Copy link
Contributor Author

mmodrow commented Oct 25, 2017

Here you are. Thanks for the constructive feedback!

Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Thanks @marcamo!

@mmodrow
Copy link
Contributor Author

mmodrow commented Oct 29, 2017

Did I miss some needed action here? As far as I understood everything is fine. Is "needs:testing" still applicable? At the end of the day this fixed the existing tests' failing. So I don't think that Unit Tests updated or fixed, Docs/guides updated or Example created (starter template on JSBin) is applicable, is it?

Or are we simply waiting for a 2nd core contributor approval?

@gkatsev gkatsev merged commit bebca9c into videojs:master Oct 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants