Add contentDocument support for HTMLIFrameElement. Fixes #3808. #3953
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Critic review: https://critic.hoppipolla.co.uk/r/3144 This is an external review system which you may optionally use for the code review of your pull request. In order to help critic track your changes, please do not make in-place history rewrites (e.g. via |
neojski
force-pushed
the
implement-HTMLIFrameElement.contentDocument
branch
2 times, most recently
from
8bd59d7
to
1d85cd5
Compare
|
I would be exceedingly surprised if the test suddenly started to work. The fundamental problem identified in #2122 should still be present. |
|
Worth a try, anyways! |
bors-servo
pushed a commit
that referenced
this pull request
Nov 14, 2014
…ntDocument, r=jdm Because of #2122 I cannot write test for this right now because it will be failing randomly due to that iframe issue. However, if it doesn't fail due to that issue a test like this: ```html <html> <head> <meta charset="utf8" /> <script src="harness.js"></script> <title>Iframe contentDocument test.</title> </head> <body> <iframe src="test_iframe_contentDocument_inner.html" id="iframe"></iframe> <script> waitForExplicitFinish(); var timeout = 100; var iframe = document.getElementById('iframe'); function test_contentWindow() { if (!iframe.contentWindow) { // Iframe not loaded yet, try again. // No load event for iframe, insert bug number here. setTimeout(test_contentWindow, timeout); return; } is(iframe.contentDocument.getElementById('test').textContent, 'value'); finish(); } test_contentWindow(); </script> </body> </html> ``` where inner is simply: ```html <html><body><div id="test">value</div></body></html> ``` passes. I have added `SameOrigin` method to the `UrlHelper`. I wanted to reuse it in [`constellation.rs` same_script check](https://github.com/servo/servo/blob/f0184a2d011e12845258a242d2d2f6b8b504a28d/components/compositing/constellation.rs#L625) but I it didn't want to compile saying ``` error: unresolved import `dom::urlhelper::UrlHelper`. Maybe a missing `extern crate dom`? ``` So I didn't include it in this PR for now. There is more discussion about the cross origin iframes in [another issue](#3939). In this PR I just added same origin check.
…test harness (and does not fail).
neojski
force-pushed
the
implement-HTMLIFrameElement.contentDocument
branch
from
8d168f3
to
fb8a45c
Compare
|
@jdm, there were some failures, can we try again? |
bors-servo
pushed a commit
that referenced
this pull request
Nov 14, 2014
…ntDocument, r=jdm Because of #2122 I cannot write test for this right now because it will be failing randomly due to that iframe issue. However, if it doesn't fail due to that issue a test like this: ```html <html> <head> <meta charset="utf8" /> <script src="harness.js"></script> <title>Iframe contentDocument test.</title> </head> <body> <iframe src="test_iframe_contentDocument_inner.html" id="iframe"></iframe> <script> waitForExplicitFinish(); var timeout = 100; var iframe = document.getElementById('iframe'); function test_contentWindow() { if (!iframe.contentWindow) { // Iframe not loaded yet, try again. // No load event for iframe, insert bug number here. setTimeout(test_contentWindow, timeout); return; } is(iframe.contentDocument.getElementById('test').textContent, 'value'); finish(); } test_contentWindow(); </script> </body> </html> ``` where inner is simply: ```html <html><body><div id="test">value</div></body></html> ``` passes. I have added `SameOrigin` method to the `UrlHelper`. I wanted to reuse it in [`constellation.rs` same_script check](https://github.com/servo/servo/blob/f0184a2d011e12845258a242d2d2f6b8b504a28d/components/compositing/constellation.rs#L625) but I it didn't want to compile saying ``` error: unresolved import `dom::urlhelper::UrlHelper`. Maybe a missing `extern crate dom`? ``` So I didn't include it in this PR for now. There is more discussion about the cross origin iframes in [another issue](#3939). In this PR I just added same origin check.
|
I'm pretty sure we did sth wrong here because I wasn't asked to squash before it got merged and so this silly change has two commits. I'm sorry about that. |
|
I just re-r+d since Josh had already done so, after quickly cross checking. Yeah, you could have squashed; but it's not necessary :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Because of #2122 I cannot write test for this right now because it will be failing randomly due to that iframe issue. However, if it doesn't fail due to that issue a test like this:
where inner is simply:
passes.
I have added
SameOriginmethod to theUrlHelper. I wanted to reuse it inconstellation.rssame_script check but I it didn't want to compile sayingSo I didn't include it in this PR for now.
There is more discussion about the cross origin iframes in another issue. In this PR I just added same origin check.