Skip to content

/ servo

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

Add contentDocument support for HTMLIFrameElement. Fixes #3808. #3953

Closed
wants to merge 2 commits into from
Closed

Add contentDocument support for HTMLIFrameElement. Fixes #3808. #3953

wants to merge 2 commits into from

Conversation

@neojski
Copy link
Contributor

neojski commented Nov 10, 2014

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>
  <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><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 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. In this PR I just added same origin check.
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Nov 10, 2014

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 git rebase -i or git commit --amend) when updating this pull request.
@neojski neojski force-pushed the neojski:implement-HTMLIFrameElement.contentDocument branch 2 times, most recently from 8bd59d7 to 1d85cd5 Nov 13, 2014
@neojski
Copy link
Contributor Author

neojski commented Nov 13, 2014

Rebased. After recent iframe changes (although I didn't check whether it's really #3951 that fixed it but sounds possible) the test seem to work. What do you think @jdm ?
@jdm
Copy link
Member

jdm commented Nov 14, 2014

I would be exceedingly surprised if the test suddenly started to work. The fundamental problem identified in #2122 should still be present.
@jdm
Copy link
Member

jdm commented Nov 14, 2014

Worth a try, anyways!
bors-servo pushed a commit that referenced this pull request Nov 14, 2014
bors-servo
auto merge of #3953 : neojski/servo/implement-HTMLIFrameElement.conte…
Loading status checks…
ea8db1a 
…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.
neojski added 2 commits Nov 10, 2014
@neojski
Add contentDocument support for HTMLIFrameElement. Fixes #3808.
af30484
@neojski
Iframe resource file moved to resources dir so it's not picked up by …
Loading status checks…
fb8a45c 
…test harness (and does not fail).
@neojski neojski force-pushed the neojski:implement-HTMLIFrameElement.contentDocument branch from 8d168f3 to fb8a45c Nov 14, 2014
@neojski
Copy link
Contributor Author

neojski commented Nov 14, 2014

@jdm, there were some failures, can we try again?
@Manishearth

This comment has been minimized.

Sign in to view
Copy link

Manishearth commented on fb8a45c Nov 14, 2014

r=jdm
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented on fb8a45c Nov 14, 2014

saw approval from jdm
at neojski@fb8a45c

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Nov 14, 2014

merging neojski/servo/implement-HTMLIFrameElement.contentDocument = fb8a45c into auto

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Nov 14, 2014

neojski/servo/implement-HTMLIFrameElement.contentDocument = fb8a45c merged ok, testing candidate = 85a2f0b

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Nov 14, 2014

all tests pass:
success: http://build.servo.org/builders/linux/builds/619
success: http://build.servo.org/builders/mac1/builds/161
success: http://build.servo.org/builders/mac2/builds/160

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Nov 14, 2014

fast-forwarding master to auto = 85a2f0b
bors-servo pushed a commit that referenced this pull request Nov 14, 2014
bors-servo
auto merge of #3953 : neojski/servo/implement-HTMLIFrameElement.conte…
Loading status checks…
85a2f0b 
…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.
@bors-servo bors-servo closed this Nov 14, 2014
@neojski
Copy link
Contributor Author

neojski commented Nov 14, 2014

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.
@Manishearth
Copy link
Member

Manishearth commented Nov 14, 2014

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
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@neojski @hoppipolla-critic-bot @jdm @Manishearth @bors-servo
You can’t perform that action at this time.