[webkit-reviews] review granted: [Bug 180524] HTML-page with <object type="image/svg+xml" data="foo.svg"> often is blank : [Attachment 328928] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 180524: HTML-page with <object type="image/svg+xml" data="foo.svg"> often
is blank
https://bugs.webkit.org/show_bug.cgi?id=180524

Attachment 328928: Patch

https://bugs.webkit.org/attachment.cgi?id=328928&action=review




--- Comment #12 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 328928
  --> https://bugs.webkit.org/attachment.cgi?id=328928
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328928&action=review

> LayoutTests/fast/dom/iframe-inner-size-scaling.html:20
> +	       scrollbarWidth = UIHelper.isIOS() ? 0 : 15;

Are we not able to compute this? Either directly using the CSSOM or by exposing
a window.internals functions? I know scroll bar width computations is not a new
problem. We have other tests thats that need to know the scrollbar width. If
they hardcode the width then exposing a windows.internals function would be
useful.

On another note, I thought we had mock scroll bars. I guess we don’t use the
mock scroll bar code on iOS.

> LayoutTests/fast/dom/iframe-innerWidth-expected.txt:8
> +TEST COMPLETE

Please fix this test to not emit this until after all the PASS message as
emitting this twice is an indicator that this test can be flaky and depends on
testrunner.notifyDone() not being a blocking call that terminates the test.

> LayoutTests/fast/dom/iframe-innerWidth.html:2
> +    <script src="../../resources/js-test-pre.js"></script>

The indentation of the code starting at this line looks weird. I suggest
in-indenting it all.

Also, the current trend is to include js-test.js and forgo
js-test-pre/js-test-post.js

> LayoutTests/fast/dom/iframe-innerWidth.html:4
> +	   description("This tests that innerWidth/innerHeight on an frame
window are valid in the load event handler.");

You need to set window.jsTestIsAsync = true. Otherwise, finishJSTest() is
called twice and this test is flaky. See above remark in the expected.txt file.

> LayoutTests/fast/dom/iframe-innerWidth.html:6
> +	       frame = document.getElementById('iframe');

This is OK as-is. Would be good to pick one quote style and stick with it
throughout this test.