compositing: Fix a couple of bugs that prevented iframes from painting after navigation. #9285

pcwalton · 2016-01-13T05:00:03Z

The first bug was that iframes were not reflowed in their parent DOM when the child page navigated. This is fixed by simply having the constellation notify the appropriate script thread when navigation occurs.

The second bug was that the compositor was unable to adjust the pipeline for existing iframe layers, only new ones. This patch adds logic to do that.

Closes #8081.

r? @jdm
cc @metajack

highfive · 2016-01-13T05:00:08Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

jdm · 2016-01-13T06:26:49Z

The script and constellation changes look fine to me (thanks!), but the compositor changes are outside the area of my expertise. r? @glennw for that.

jdm · 2016-01-13T06:27:58Z

@pcwalton Can we test this using a reftest with reftest-wait and an iframe onload handler?

pcwalton · 2016-01-13T06:48:38Z

Is there documentation for that?

glennw · 2016-01-13T07:00:53Z

This is what I based the reftest-wait implementation on - http://testthewebforward.org/docs/reftests.html. There should be a few examples in our wpt/mozilla tests that make use of it.

glennw · 2016-01-13T20:10:51Z

Reviewed 1 of 6 files at r1.
Review status: 1 of 6 files reviewed at latest revision, 1 unresolved discussion.

components/compositing/compositor.rs, line 797 [r1] (raw file):
Can this create extra root layers for a subpage in the case of an update on a layer reisize (for example)?

Comments from the review on Reviewable.io

glennw · 2016-01-13T20:11:08Z

Reviewed 1 of 6 files at r1.
Review status: 2 of 6 files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

pcwalton · 2016-01-14T01:33:39Z

@jdm I don't know how to write the reftest in a way that works around Servo's bugs with the same origin policy. Here's my attempt:

<!DOCTYPE html>
<html class="reftest-wait">
<body>
<iframe src="../bogus"></iframe>
<script>
setTimeout(function() {
    var iframe = document.getElementsByTagName('iframe')[0];
    ...
    iframe.contentDocument.location.href = "data:text/html,Hello%20world";
    ...
}, 16);
</script>
</body>
</html>

contentDocument is sometimes null because if the 404 page successfully loads it's no longer same origin. If instead of ../bogus, a file:/about:/data: URL are used, the same origin policy always prevents me from accessing contentDocument. Likewise if no src attribute is specified at all. This works in other browsers.

pcwalton · 2016-01-14T01:34:40Z

Updated per review comments.

jdm · 2016-01-14T04:05:20Z

@pcwalton I was going to say that I don't understand the limitations here, but I realized that maybe you ran into #7826? Otherwise, did you try setting the src attribute instead of using iframe.contentDocument.location?

glennw · 2016-01-14T05:18:45Z

The compositor code looks good to me once the test issue from @jdm is resolved.

paulrouget · 2016-01-19T17:04:57Z

Any update?

pcwalton · 2016-01-22T20:09:17Z

I still have yet to write the test that was requested.

pcwalton · 2016-01-23T02:14:39Z

@jdm I can navigate the iframe using iframe.src = ..., but the same origin policy prevents me from calling iframe.contentDocument.addEventListener('load', ...), without which I don't know when to remove the reftest-wait class. So I'm stuck.

pcwalton · 2016-01-23T02:35:26Z

OK, the way I committed it seems to work.

r? @jdm

jdm · 2016-01-23T16:06:20Z

@pcwalton I meant adding a load listener on the iframe element, not on the framed document. That would work much more reliably than the current setup.

pcwalton · 2016-01-23T23:33:33Z

@jdm I tried that. The load event doesn't fire, even when it's placed on the iframe element. I think data URLs may be messing it up.

pcwalton · 2016-01-25T18:48:21Z

It also didn't fire with file URLs, BTW.


        compositing: Fix a couple of bugs that prevented iframes from painting

after navigation. The first bug was that iframes were not reflowed in their parent DOM when the child page navigated. This is fixed by simply having the constellation notify the appropriate script thread when navigation occurs. The second bug was that the compositor was unable to adjust the pipeline for existing iframe layers, only new ones. This patch adds logic to do that. Closes #8081.

pcwalton · 2016-01-25T19:14:42Z

@jdm So the test case in the updated patch times out as written. If you uncomment the console.log lines, the test does not time out (!) and they successfully write to the output. I'm at a loss here.

jdm · 2016-01-25T20:10:04Z

#9421.


        Auto merge of #9421 - jdm:iframe-painting-after-navigation-redux, r=p…

…cwalton compositing: Fix a couple of bugs that prevented iframes from painting after navigation. The first bug was that iframes were not reflowed in their parent DOM when the child page navigated. This is fixed by simply having the constellation notify the appropriate script thread when navigation occurs. The second bug was that the compositor was unable to adjust the pipeline for existing iframe layers, only new ones. This patch adds logic to do that. The third bug was that we have ad-hoc reflow calls throughout script/, and we didn't trigger any reflow from the code that dispatches the `load` event for the iframe so the test for the first two issues would always time out. The second commit adds another reflow call to do that, and also bites the bullet and adds a catch-all reflow (which does nothing if there's no dirty nodes in the document) at the return to the event loop. Closes #8081. Extension of #9285.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9421)


        Auto merge of #9421 - jdm:iframe-painting-after-navigation-redux, r=jdm

compositing: Fix a couple of bugs that prevented iframes from painting after navigation. The first bug was that iframes were not reflowed in their parent DOM when the child page navigated. This is fixed by simply having the constellation notify the appropriate script thread when navigation occurs. The second bug was that the compositor was unable to adjust the pipeline for existing iframe layers, only new ones. This patch adds logic to do that. The third bug was that we have ad-hoc reflow calls throughout script/, and we didn't trigger any reflow from the code that dispatches the `load` event for the iframe so the test for the first two issues would always time out. The second commit adds another reflow call to do that, and also bites the bullet and adds a catch-all reflow (which does nothing if there's no dirty nodes in the document) at the return to the event loop. Closes #8081. Extension of #9285.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9421)


        Auto merge of #9421 - jdm:iframe-painting-after-navigation-redux, r=jdm

compositing: Fix a couple of bugs that prevented iframes from painting after navigation. The first bug was that iframes were not reflowed in their parent DOM when the child page navigated. This is fixed by simply having the constellation notify the appropriate script thread when navigation occurs. The second bug was that the compositor was unable to adjust the pipeline for existing iframe layers, only new ones. This patch adds logic to do that. The third bug was that we have ad-hoc reflow calls throughout script/, and we didn't trigger any reflow from the code that dispatches the `load` event for the iframe so the test for the first two issues would always time out. The second commit adds another reflow call to do that, and also bites the bullet and adds a catch-all reflow (which does nothing if there's no dirty nodes in the document) at the return to the event loop. Closes #8081. Extension of #9285.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9421)


        Auto merge of #9421 - jdm:iframe-painting-after-navigation-redux, r=jdm

compositing: Fix a couple of bugs that prevented iframes from painting after navigation. The first bug was that iframes were not reflowed in their parent DOM when the child page navigated. This is fixed by simply having the constellation notify the appropriate script thread when navigation occurs. The second bug was that the compositor was unable to adjust the pipeline for existing iframe layers, only new ones. This patch adds logic to do that. The third bug was that we have ad-hoc reflow calls throughout script/, and we didn't trigger any reflow from the code that dispatches the `load` event for the iframe so the test for the first two issues would always time out. The second commit adds another reflow call to do that, and also bites the bullet and adds a catch-all reflow (which does nothing if there's no dirty nodes in the document) at the return to the event loop. Closes #8081. Extension of #9285.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9421)


        Auto merge of #9421 - jdm:iframe-painting-after-navigation-redux, r=jdm

compositing: Fix a couple of bugs that prevented iframes from painting after navigation. The first bug was that iframes were not reflowed in their parent DOM when the child page navigated. This is fixed by simply having the constellation notify the appropriate script thread when navigation occurs. The second bug was that the compositor was unable to adjust the pipeline for existing iframe layers, only new ones. This patch adds logic to do that. The third bug was that we have ad-hoc reflow calls throughout script/, and we didn't trigger any reflow from the code that dispatches the `load` event for the iframe so the test for the first two issues would always time out. The second commit adds another reflow call to do that, and also bites the bullet and adds a catch-all reflow (which does nothing if there's no dirty nodes in the document) at the return to the event loop. Closes #8081. Extension of #9285.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9421)


        Auto merge of #9421 - jdm:iframe-painting-after-navigation-redux, r=jdm

compositing: Fix a couple of bugs that prevented iframes from painting after navigation. The first bug was that iframes were not reflowed in their parent DOM when the child page navigated. This is fixed by simply having the constellation notify the appropriate script thread when navigation occurs. The second bug was that the compositor was unable to adjust the pipeline for existing iframe layers, only new ones. This patch adds logic to do that. The third bug was that we have ad-hoc reflow calls throughout script/, and we didn't trigger any reflow from the code that dispatches the `load` event for the iframe so the test for the first two issues would always time out. The second commit adds another reflow call to do that, and also bites the bullet and adds a catch-all reflow (which does nothing if there's no dirty nodes in the document) at the return to the event loop. Closes #8081. Extension of #9285.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9421)

highfive added the S-awaiting-review label Jan 13, 2016

pcwalton mentioned this pull request Jan 13, 2016

When an iframe loads a new page, dirty and reflow it so that the layout fragment will pick up the new subpage ID. #8662

Closed

highfive assigned glennw Jan 13, 2016

pcwalton force-pushed the pcwalton:iframe-painting-after-navigation-redux branch from 1856960 to c951157 Jan 14, 2016

pcwalton force-pushed the pcwalton:iframe-painting-after-navigation-redux branch from c951157 to bf4b255 Jan 23, 2016

highfive assigned jdm and unassigned glennw Jan 23, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Jan 23, 2016

pcwalton added S-awaiting-review and removed S-needs-code-changes labels Jan 23, 2016

pcwalton force-pushed the pcwalton:iframe-painting-after-navigation-redux branch from bf4b255 to 8d508df Jan 25, 2016

jdm mentioned this pull request Jan 25, 2016

compositing: Fix a couple of bugs that prevented iframes from painting after navigation. #9421

Merged

jdm closed this Jan 25, 2016

servo / servo

compositing: Fix a couple of bugs that prevented iframes from painting after navigation. #9285

compositing: Fix a couple of bugs that prevented iframes from painting after navigation. #9285

pcwalton commented Jan 13, 2016

highfive commented Jan 13, 2016

jdm commented Jan 13, 2016

jdm commented Jan 13, 2016

pcwalton commented Jan 13, 2016

glennw commented Jan 13, 2016

glennw commented Jan 13, 2016

glennw commented Jan 13, 2016

pcwalton commented Jan 14, 2016

pcwalton commented Jan 14, 2016

jdm commented Jan 14, 2016

glennw commented Jan 14, 2016

paulrouget commented Jan 19, 2016

pcwalton commented Jan 22, 2016

pcwalton commented Jan 23, 2016

pcwalton commented Jan 23, 2016

jdm commented Jan 23, 2016

pcwalton commented Jan 23, 2016

pcwalton commented Jan 25, 2016

pcwalton commented Jan 25, 2016

jdm commented Jan 25, 2016

servo / servo

Join GitHub today

compositing: Fix a couple of bugs that prevented iframes from painting after navigation. #9285

compositing: Fix a couple of bugs that prevented iframes from painting after navigation. #9285

Conversation

pcwalton commented Jan 13, 2016

highfive commented Jan 13, 2016

jdm commented Jan 13, 2016

jdm commented Jan 13, 2016

pcwalton commented Jan 13, 2016

glennw commented Jan 13, 2016

glennw commented Jan 13, 2016

glennw commented Jan 13, 2016

pcwalton commented Jan 14, 2016

pcwalton commented Jan 14, 2016

jdm commented Jan 14, 2016

glennw commented Jan 14, 2016

paulrouget commented Jan 19, 2016

pcwalton commented Jan 22, 2016

pcwalton commented Jan 23, 2016

pcwalton commented Jan 23, 2016

jdm commented Jan 23, 2016

pcwalton commented Jan 23, 2016

pcwalton commented Jan 25, 2016

pcwalton commented Jan 25, 2016

jdm commented Jan 25, 2016