Bug 85251

Created attachment 139556 [details] Patch

Reviewers interested in evaluating the total test coverage can compare the list of tests here, to the list of seamless features here: http://www.whatwg.org/specs/web-apps/current-work/#attr-iframe-seamless Seamless breaks down into 5 key pieces: 1. Inheriting stylesheets from the parent 2. inheriting styles from the iframe element to the documentElemetn of the child (For WebKit, we use the style on the Document node instead) 3. Navigating the parent context instead of the child context (for links, forms, etc.) 4. Setting the width/height of the parent iframe based on the child document's width/height 5. respecting the seamless sandbox flag. We've implemented all 5 of these. The implementations of each will be posted in follow-up patches tonight/tomorrow, etc.

Comment on attachment 139556 [details] Patch Attachment 139556 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12590427 New failing tests: fast/frames/seamless/seamless-inline.html

Created attachment 139570 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Mind blown. There is no reason that result should differ for Chromium. Will investigate.

Comment on attachment 139556 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139556&action=review Just some overall high-level comments. I also like adding all the tests at the beginning. Mike did that with calc, and I found it really helpful in understanding what each iterative patch fixed to see tests go from failing to passing, where the new result was clearly correct. > LayoutTests/ChangeLog:75 > + * fast/frames/seamless/resources/css-cascade-child.html: Added. > + * fast/frames/seamless/resources/done.html: Added. > + * fast/frames/seamless/resources/nested-seamless.html: Added. > + * fast/frames/seamless/resources/quirks-square.html: Added. > + * fast/frames/seamless/resources/square.html: Added. > + * fast/frames/seamless/resources/two-inline-blocks.html: Added. > + * fast/frames/seamless/seamless-basic-expected.txt: Added. > + * fast/frames/seamless/seamless-basic.html: Added. > + * fast/frames/seamless/seamless-css-cascade-expected.txt: Added. > + * fast/frames/seamless/seamless-css-cascade.html: Added. > + * fast/frames/seamless/seamless-designMode-expected.txt: Added. > + * fast/frames/seamless/seamless-designMode.html: Added. > + * fast/frames/seamless/seamless-float-expected.txt: Added. > + * fast/frames/seamless/seamless-float.html: Added. > + * fast/frames/seamless/seamless-form-get-expected.txt: Added. > + * fast/frames/seamless/seamless-form-get-named-expected.txt: Added. > + * fast/frames/seamless/seamless-form-get-named.html: Added. > + * fast/frames/seamless/seamless-form-get-override-expected.txt: Added. > + * fast/frames/seamless/seamless-form-get-override.html: Added. > + * fast/frames/seamless/seamless-form-get.html: Added. > + * fast/frames/seamless/seamless-form-post-expected.txt: Added. > + * fast/frames/seamless/seamless-form-post-named-expected.txt: Added. > + * fast/frames/seamless/seamless-form-post-named.html: Added. > + * fast/frames/seamless/seamless-form-post-override-expected.txt: Added. > + * fast/frames/seamless/seamless-form-post-override.html: Added. > + * fast/frames/seamless/seamless-form-post.html: Added. > + * fast/frames/seamless/seamless-hyperlink-expected.txt: Added. > + * fast/frames/seamless/seamless-hyperlink-named-expected.txt: Added. > + * fast/frames/seamless/seamless-hyperlink-named.html: Added. > + * fast/frames/seamless/seamless-hyperlink-override-expected.txt: Added. > + * fast/frames/seamless/seamless-hyperlink-override.html: Added. > + * fast/frames/seamless/seamless-hyperlink.html: Added. > + * fast/frames/seamless/seamless-inline-expected.txt: Added. > + * fast/frames/seamless/seamless-inline.html: Added. > + * fast/frames/seamless/seamless-min-max-expected.txt: Added. > + * fast/frames/seamless/seamless-min-max.html: Added. > + * fast/frames/seamless/seamless-nested-expected.txt: Added. > + * fast/frames/seamless/seamless-nested.html: Added. > + * fast/frames/seamless/seamless-quirks-expected.txt: Added. > + * fast/frames/seamless/seamless-quirks.html: Added. > + * fast/frames/seamless/seamless-sandbox-flag-expected.txt: Added. > + * fast/frames/seamless/seamless-sandbox-flag.html: Added. > + * fast/frames/seamless/seamless-sandbox-srcdoc-expected.txt: Added. > + * fast/frames/seamless/seamless-sandbox-srcdoc.html: Added. > + * fast/frames/seamless/seamless-window-location-expected.txt: Added. > + * fast/frames/seamless/seamless-window-location-href-expected.txt: Added. > + * fast/frames/seamless/seamless-window-location-href.html: Added. > + * fast/frames/seamless/seamless-window-location-replace-expected.txt: Added. > + * fast/frames/seamless/seamless-window-location-replace.html: Added. > + * fast/frames/seamless/seamless-window-location-sandbox-expected.txt: Added. > + * fast/frames/seamless/seamless-window-location-sandbox.html: Added. > + * fast/frames/seamless/seamless-window-location.html: Added. > + * fast/frames/seamless/seamless-window-open-expected.txt: Added. > + * fast/frames/seamless/seamless-window-open-override-expected.txt: Added. > + * fast/frames/seamless/seamless-window-open-override.html: Added. > + * fast/frames/seamless/seamless-window-open.html: Added. > + * http/tests/security/seamless/resources/square.html: Added. > + * http/tests/security/seamless/seamless-cross-origin-expected.txt: Added. > + * http/tests/security/seamless/seamless-cross-origin.html: Added. > + * http/tests/security/seamless/seamless-sandbox-srcdoc-expected.txt: Added. > + * http/tests/security/seamless/seamless-sandbox-srcdoc.html: Added. Can we remove the seamless prefix on the filenames? It's redundant to have them in a seamless directory *and* prefix every filename with seamless. > LayoutTests/fast/frames/seamless/seamless-basic.html:2 > +<script src="../../js/resources/js-test-pre.js"></script> Here and below. You still need js-test-post.js to be loaded after your script. There were some complications incorporating it's functionality into js-test-pre that we never took the time to address. For example, you're missing the TEST COMPLETE line. > LayoutTests/fast/frames/seamless/seamless-form-get-named.html:17 > + window.onload=function() { Nit: (here and in other tests) spaces around the = > LayoutTests/fast/frames/seamless/seamless-form-get-named.html:24 > + srcdoc='<form target=tg method=GET action=resources/done.html> > + <input type=submit> > + </from> Mind-blown. I didn't realize you don't need to escape '<' and newlines!

Do you have any substantive comments? These nits don't even seem worth changing, honestly... Thank you for looking.

(In reply to comment #6) > (From update of attachment 139556 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=139556&action=review > Can we remove the seamless prefix on the filenames? It's redundant to have them in a seamless directory *and* prefix every filename with seamless. I agree it's somewhat redundant, I can remove them if it maters to you. :) > > LayoutTests/fast/frames/seamless/seamless-basic.html:2 > > +<script src="../../js/resources/js-test-pre.js"></script> > > Here and below. > > You still need js-test-post.js to be loaded after your script. There were some complications incorporating it's functionality into js-test-pre that we never took the time to address. > > For example, you're missing the TEST COMPLETE line. I don't actually care too much about the TEST COMPLETE, I just want the shouldBe, etc. Most of these tests don't use waitUntilDone, so I could include js-test-post, but more and more I've seen tests just not bother to include js-test-post, so I didn't bother here. :) > > LayoutTests/fast/frames/seamless/seamless-form-get-named.html:17 > > + window.onload=function() { > > Nit: (here and in other tests) spaces around the = Very possible to fix, but without anything else to change seems needless to upload a new patch. > > LayoutTests/fast/frames/seamless/seamless-form-get-named.html:24 > > + srcdoc='<form target=tg method=GET action=resources/done.html> > > + <input type=submit> > > + </from> > > Mind-blown. I didn't realize you don't need to escape '<' and newlines! Yup. :) srcdoc is very useful!

run-webkit-tests fast/frames/seamless --chromium --release does not reproduce the seamless-inline.html failure for me locally. :( Perhaps its a linux-only failure. I'll look at the test and see if it's doing anything odd.

I tried adding js-test-post, but it puts the TEST COMPLET in the wrong place: Test that floated seamless iframes 'shrink-wrap' their contents, as floated divs would. On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". PASS successfullyParsed is true TEST COMPLETE FAIL window.getComputedStyle(iframe1).width should be 150px. Was 300px. FAIL window.getComputedStyle(iframe1).height should be 50px. Was 150px. FAIL window.getComputedStyle(iframe2).width should be 100px. Was 300px. FAIL window.getComputedStyle(iframe2).height should be 100px. Was 150px. So I'm removing them again.

After staring at it more, I've decided I really like the seamless-* prefix, as it makes the tests independent of their location. We could drop it... but unless you feel strongly, I'd like to keep it.

I can see how lack of js-test-post could be confusing. In a somewhat last-minute change I added "description" calls to all of these tests which is when the instructions about "TEST COMPLETE" started appearing at the top of the output. Previously I didn't have description() and was just using shouldBe. However, given that js-test-post is kinda broken for onload-based tests, i'd rather leave it out for now. Again, if you feel strongly I could add it, but this seems like one of the lesser important aspects of this patch series and I'd rather have your review energy spent on the more interesting code changes to follow. :)

Created attachment 139600 [details] Patch

The cr-linux failure was actually super helpful! It tracked down a problem I had been having with seamless-inline layout. Turns out my layout code was fine... just my test had some whitespace in it which was causing 4px layout issues. :) With the whitespace removed, the layout should be platform independent (I suspect cr-linux has a different default font than cr-mac? -- which is slightly concerning!)

I believe I have addressed all the nits and the cr-linux failures. I hope to land this in the morning and post my code changes tomorrow afternoon (in various patches). Those code changes will of course include updates to these test expectations. :)

(In reply to comment #11) > After staring at it more, I've decided I really like the seamless-* prefix, as it makes the tests independent of their location. We could drop it... but unless you feel strongly, I'd like to keep it. Optimizing for moving tests around seems like optimizing the wrong thing. In practice, we basically never move tests. It's not a big deal, but it's not a pattern I'd want to see repeated everywhere, and people cargo-cult all the time. (In reply to comment #12) > I can see how lack of js-test-post could be confusing. In a somewhat last-minute change I added "description" calls to all of these tests which is when the instructions about "TEST COMPLETE" started appearing at the top of the output. Previously I didn't have description() and was just using shouldBe. > > However, given that js-test-post is kinda broken for onload-based tests, i'd rather leave it out for now. Again, if you feel strongly I could add it, but this seems like one of the lesser important aspects of this patch series and I'd rather have your review energy spent on the more interesting code changes to follow. :) At some point, someone will try to merge js-test-post into js-test-pre. Leaving it out in a bunch of tests might make that task harder because it will be harder to evaluate whether all the cases where tests are failing due to added "TEST COMPLETE" lines are accidental changes in behavior or tests that leave out the post.js script. Again, not a big deal, but consistency with the rest of the tests in the tree seems like something that will lead to lower maintenance burden and cognitive overhead. BTW, the way you're supposed to use js-test for async tests is to: 1. set window.jsTestIsAsync = true instead of calling waitUntilDone. 2. call finishJSTest() instead of calling notifyDone. Then everything works right. At one point, Arv was going to override layoutTestController.notifyDone/waitUntilDone to just do the right thing, but he never got around to it. In fact, I think it was doing this that blocked getting rid of js-test-post.

(In reply to comment #16) > (In reply to comment #12) > > I can see how lack of js-test-post could be confusing. In a somewhat last-minute change I added "description" calls to all of these tests which is when the instructions about "TEST COMPLETE" started appearing at the top of the output. Previously I didn't have description() and was just using shouldBe. > > > > However, given that js-test-post is kinda broken for onload-based tests, i'd rather leave it out for now. Again, if you feel strongly I could add it, but this seems like one of the lesser important aspects of this patch series and I'd rather have your review energy spent on the more interesting code changes to follow. :) > > At some point, someone will try to merge js-test-post into js-test-pre. Leaving it out in a bunch of tests might make that task harder because it will be harder to evaluate whether all the cases where tests are failing due to added "TEST COMPLETE" lines are accidental changes in behavior or tests that leave out the post.js script. > > Again, not a big deal, but consistency with the rest of the tests in the tree seems like something that will lead to lower maintenance burden and cognitive overhead. % grep -l -r js-test-pre . | grep -v ChangeLog | wc 5576 5576 270260 % grep -l -r js-test-post . | grep -v ChangeLog | wc 4519 4519 209969 It appears their job may be hard already, since a full 20% of our script-tests are missing js-test-post. :)

Created attachment 139644 [details] Replace description calls with debug to remove confusing TEST COMPLETE message

Comment on attachment 139644 [details] Replace description calls with debug to remove confusing TEST COMPLETE message View in context: https://bugs.webkit.org/attachment.cgi?id=139644&action=review Just some high-level comments, Ojan commented on the TEST COMPLETE, I agree with him as it would make the output more readable to some unfamiliar with the test but don't feel strongly about it (especially since it requires some boiler plate code). I really think we should prefix the attribute as the spec seems to be in-flux but there seems to be some strong argument towards non-prefixing and I may miss on that. Apart from that, the test are fine. > LayoutTests/fast/frames/seamless/resources/css-cascade-child.html:1 > +<style> Do we need a DOCTYPE? > LayoutTests/fast/frames/seamless/seamless-css-cascade.html:6 > +#one { color: red; } Usually red is for failures. > LayoutTests/fast/frames/seamless/seamless-hyperlink-named.html:22 > + <iframe name='tg' seamless > + srcdoc='<a target=tg href=resources/done.html>Click me</a>'></iframe> Are seamless iframe supposed to create entries in the history? Would it be worth dump the history to ensure that? > LayoutTests/fast/frames/seamless/seamless-hyperlink.html:14 > +<iframe srcdoc=" Shouldn't the outer iframe be seamless too? > LayoutTests/fast/frames/seamless/seamless-inline.html:10 > +<div id="parent1" class="parent"><iframe id="iframe1" seamless style="display: inline" src="resources/two-inline-blocks.html"></iframe><div style="background-color: purple; display: inline-block; width: 25px; height: 50px;"></div></div> The sibling style could be also factored into our <style> tag above: .sibling { background-color: purple; display: inline-block; width: 25px; height: 50px; } > LayoutTests/fast/frames/seamless/seamless-inline.html:33 > + shouldBeEqualToString("window.getComputedStyle(iframe1).width", "150px"); > + shouldBeEqualToString("window.getComputedStyle(iframe1).height", "50px"); > + shouldBeEqualToString("window.getComputedStyle(parent1).height", "50px"); > + > + shouldBeEqualToString("window.getComputedStyle(iframe2).width", "100px"); > + shouldBeEqualToString("window.getComputedStyle(iframe2).height", "100px"); > + shouldBeEqualToString("window.getComputedStyle(parent2).height", "150px"); I would expect those 2 iframes to be equally sized but I may be missing something here.

> > LayoutTests/fast/frames/seamless/seamless-hyperlink.html:14 > > +<iframe srcdoc=" > > Shouldn't the outer iframe be seamless too? Nope. The test runs inside the first iframe.

IMHO, the test names aren't worth worrying too much about.

(In reply to comment #19) > (From update of attachment 139644 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=139644&action=review > > LayoutTests/fast/frames/seamless/resources/css-cascade-child.html:1 > > +<style> > > Do we need a DOCTYPE? This doc is never used for metrics, only css cascade, but I added one anyway. > > LayoutTests/fast/frames/seamless/seamless-css-cascade.html:6 > > +#one { color: red; } > > Usually red is for failures. Never displayed, but changed to yellow anyway. > > LayoutTests/fast/frames/seamless/seamless-hyperlink-named.html:22 > > + <iframe name='tg' seamless > > + srcdoc='<a target=tg href=resources/done.html>Click me</a>'></iframe> > > Are seamless iframe supposed to create entries in the history? Would it be worth dump the history to ensure that? The seamless iframe doesn't, no. I'll ask abarth, as he wrote the navigation code. We can add one if necessary when landing those parts. > > LayoutTests/fast/frames/seamless/seamless-hyperlink.html:14 > > +<iframe srcdoc=" > > Shouldn't the outer iframe be seamless too? No, it's just a container for the test here. We definitely don't want to cause the parent doc to navigate, hence enclosing the test content in this non-seamless iframe. > > LayoutTests/fast/frames/seamless/seamless-inline.html:10 > > +<div id="parent1" class="parent"><iframe id="iframe1" seamless style="display: inline" src="resources/two-inline-blocks.html"></iframe><div style="background-color: purple; display: inline-block; width: 25px; height: 50px;"></div></div> > > The sibling style could be also factored into our <style> tag above: > .sibling { background-color: purple; display: inline-block; width: 25px; height: 50px; } Done. > > LayoutTests/fast/frames/seamless/seamless-inline.html:33 > > + shouldBeEqualToString("window.getComputedStyle(iframe1).width", "150px"); > > + shouldBeEqualToString("window.getComputedStyle(iframe1).height", "50px"); > > + shouldBeEqualToString("window.getComputedStyle(parent1).height", "50px"); > > + > > + shouldBeEqualToString("window.getComputedStyle(iframe2).width", "100px"); > > + shouldBeEqualToString("window.getComputedStyle(iframe2).height", "100px"); > > + shouldBeEqualToString("window.getComputedStyle(parent2).height", "150px"); > > I would expect those 2 iframes to be equally sized but I may be missing something here. Sorry, this was an error introduced when I made previous suggested cleanups! The parents are not supposed to be the same width! https://github.com/eseidel/webkit/compare/master...seamless#diff-35 Fixed.

Created attachment 139649 [details] Patch

Comment on attachment 139649 [details] Patch Sounds like these tests are ready to land.

> I really think we should prefix the attribute as the spec seems to be in-flux but there seems to be some strong argument towards non-prefixing and I may miss on that. Discussing that with Adam, it looks like we don't prefix small HTML5 features (e.g. sanbox) and let Hixie standardization takes into account web-compatibility. I haven't seen any mention of that on webkit-dev so sorry this is news to me. Eric mentioned that Hixie was fine with no prefixing which means you can remove this objection.

Comment on attachment 139649 [details] Patch Clearing flags on attachment: 139649 Committed r115742: <http://trac.webkit.org/changeset/115742>

All reviewed patches have been landed. Closing bug.