[webkit-reviews] review denied: [Bug 29240] iframes keep getting scrollbars with scrolling="no" : [Attachment 69872] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Mike Lawther
<mikelawther at chromium.org>'s request for review:
Bug 29240: iframes keep getting scrollbars with scrolling="no"
https://bugs.webkit.org/show_bug.cgi?id=29240

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=69872&action=review

r- due to the layout test output. I don't really know the C++ side very well.
It looks reasonable to me, but I'm not 100% it's correct.

> LayoutTests/fast/frames/iframe-scrolling-attribute-overflowscroll.html:10
> +<script src="../../resources/dump-as-markup.js"></script>

I don't think this test benefits from being dump-as-markup. dumpAsText would be
fine and would make the output less bulky. Also, just spitting out the
dimensions makes it harder to figure out what the correct behavior is. It would
be better if it did something like:

if (expected != actual)
    output.innerHTML = "PASS";
else
    output.innerHTML = "FAIL: expected clientWidth " + expected + " actual
clientWidth " + actual;

Where expected is the iframe's width (i.e. 200), ideally grabbed via JS
(iframe.clientWidth should do) instead of hardcoded and actual is the body
element's clientWidth.

Also, you can move these checks into the top-level document. I think that will
make the test easier to follow than having them in the subframes. You can dig
into the frames by doing iframe.contentWindow.document.body.clientWidth.

> LayoutTests/fast/frames/iframe-scrolling-attribute-overflowscroll.html:15
> +<p>This page tests the behavior of the <tt>scrolling=no</tt> attribute on
> +<tt>&lt;iframe&gt;</tt> elements which contain a page large enough to need
to
> +be scrolled, and which have overflow:scroll set on the html or body
tags.</p>

I'd change this to explicitly say what the correct behavior is. Something like:

This page tests that there are no scrollbars with iframe elements which have
scrolling=no, contain a page large enough to need to be scrolled and have
overflow:scroll set on the html or body elements. If the page doesn't have a
scrollbar, then the iframe's body's clientWidth should be equal to the iframe's
clientWidth.

> LayoutTests/fast/frames/iframe-scrolling-attribute-overflowscroll.html:23
> +<iframe src="resources/big-page-htmlscroll.html" scrolling="no"></iframe>
> +<iframe src="resources/big-page-bodyscroll.html" scrolling="no"></iframe>

Nit: Using resource files is fine. You could also inject the contents of the
iframes on the iframes load event, i.e.
onload="this.contentWindow.document.write(...)". and keep the whole test in one
file. I find it easier to understand tests that don't require navigating
between files, but it's not a big deal