[webkit-reviews] review denied: [Bug 85251] Add seamless test cases (all of these will pass as we land the implementation patches) : [Attachment 139556] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Eric Seidel <eric at webkit.org>'s
request for review:
Bug 85251: Add seamless test cases (all of these will pass as we land the
implementation patches)
https://bugs.webkit.org/show_bug.cgi?id=85251

Attachment 139556: Patch
https://bugs.webkit.org/attachment.cgi?id=139556&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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!