[webkit-reviews] review granted: [Bug 62459] Local files cannot load icons. : [Attachment 101240] Layout test should be skipped for platforms with IconDatabase disabled, and using the test with timeout.

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Rafael Brandao
<rafael.lobo at openbossa.org>'s request for review:
Bug 62459: Local files cannot load icons.
https://bugs.webkit.org/show_bug.cgi?id=62459

Attachment 101240: Layout test should be skipped for platforms with
IconDatabase disabled, and using the test with timeout.
https://bugs.webkit.org/attachment.cgi?id=101240&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=101240&action=review


This looks fine except for the test.  I don't have any great suggestions for
how to improve the test.  :(

Another option is to make this a manual-test, but that's somewhat like defeat.

> LayoutTests/fast/preloader/favicon.html:4
> +<link type="image/png" href="resources/basic.png" sizes="64x64" rel="icon"
/>

This test probably shouldn't be in the preloader directory.  It's not about the
preloader.  It's about icon loading, right?

> LayoutTests/fast/preloader/favicon.html:12
> +  setTimeout("layoutTestController.notifyDone()",400);

setTimeout in situations like this is frowns.  It leads to slow, flaky tests. 
I'm not sure how else to write this test, though, given that there's no event
that's generated when the icon load completes.

> LayoutTests/platform/chromium/test_expectations.txt:2989
> +BUGWK62459 : fast/preloader/favicon.html = FAIL PASS

So, it's already being marked as flaky?  That's not super promising.

>> Source/WebCore/loader/icon/IconDatabase.cpp:576
>> +	//printf("setIconURLForPageURL %s - %s\n",
iconURLOriginal.utf8().data(), pageURLOriginal.utf8().data());
> 
> Should have a space between // and comment  [whitespace/comments] [4]

This code should probably be removed before landing.  :)