[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 bugzilla-daemon at webkit.org
Sat Dec 9 18:31:33 PST 2017

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

Attachment 328928: Patch


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

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

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

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

> 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.

More information about the webkit-reviews mailing list