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

use https for all embeds #4903

Merged
merged 1 commit into from Jan 28, 2014
Merged

use https for all embeds #4903

merged 1 commit into from Jan 28, 2014

Conversation

minrk
Copy link
Member

@minrk minrk commented Jan 28, 2014

YouTube, Vimeo, Scribd

I can't think of a reason not to, and http embeds won't work on https pages.

@Carreau
Copy link
Member

Carreau commented Jan 28, 2014

+1

I think we should consider adding it for IFrame too. But some site can't be accessed through https...

YouTube, Vimeo, Scribd
@minrk
Copy link
Member Author

minrk commented Jan 28, 2014

I think we should consider adding it for IFrame too.

IFrame takes a whole url, we don't specify the protocol anywhere in the base IFrame, so there's nothing to change.

@Carreau
Copy link
Member

Carreau commented Jan 28, 2014

IFrame takes a whole url, we don't specify the protocol anywhere in the base IFrame, so there's nothing to change.

But people embeding http:// url got nothing on https:// notebooks. We should ad least put a warning when using http:// don't you think ?

@minrk
Copy link
Member Author

minrk commented Jan 28, 2014

don't you think?

I don't. The browser does its own warning about untrusted content on a secure page, we don't need to double up, especially since we don't know anything about the page when the object is constructed in the Kernel.

@Carreau
Copy link
Member

Carreau commented Jan 28, 2014

I don't. The browser does its own warning about untrusted content on a secure page.

Which most of time for IFrame is "just do nothing and show a blank area".

@takluyver
Copy link
Member

I'd leave it up to users to write IFrames correctly for now. If it becomes a common problem, we'll think about how we might help people get it right, but I don't think we need to fix it pre-emptively.

@jasongrout
Copy link
Member

#4904 I think is a subset of this PR.

@fperez
Copy link
Member

fperez commented Jan 28, 2014

+1 for merging this one in its current form.

takluyver added a commit that referenced this pull request Jan 28, 2014
@takluyver takluyver merged commit 1fb6096 into ipython:master Jan 28, 2014
@minrk minrk deleted the https-youtube branch January 28, 2014 19:43
@rgbkrk
Copy link
Member

rgbkrk commented Jan 28, 2014

This is kind of moot at this point, but we could use the '//' format: //www.youtube.com/watch?v=foo. It's part of RFC #1808. That allows the browser to resolve the protocol according to the page its on.

@jasongrout
Copy link
Member

Min pointed out that relative protocol urls (//some.address/) will cause problems with static html exports (where those urls then become file://...).

minrk added a commit that referenced this pull request Jan 29, 2014
YouTube, Vimeo, Scribd

I can't think of a reason not to, and http embeds won't work on https pages.
@Carreau
Copy link
Member

Carreau commented Jan 29, 2014

I would have vote to merge @fperez one (for once) and rebase this one but too late :-)

(Ugly new github UI, confusing)

pankajp pushed a commit to pankajp/ipython that referenced this pull request Feb 19, 2014
YouTube, Vimeo, Scribd

I can't think of a reason not to, and http embeds won't work on https pages.
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
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.

None yet

6 participants