[webkit-reviews] review denied: [Bug 62459] Local files cannot load icons. : [Attachment 97048] Changed pageCanHaveIcon to allow icons to pages whose url is not empty and not "about:blank"

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied 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 97048: Changed pageCanHaveIcon to allow icons to pages whose url is
not empty and not "about:blank"
https://bugs.webkit.org/attachment.cgi?id=97048&action=review

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

We definitely need a test for this change.  Thanks for looking into writing a
test.  When you have some more time, find me on IRC again and I'll give you
some more concrete help in finding an example test.

In the meantime, you can look at how the tests in LayoutTests/fast/preloader
detect otherwise invisible loads.  Another option is to try calling
layoutTestController.dumpResourceLoadCallbacks() to see if the icon load shows
up there.

> Source/WebCore/loader/icon/IconDatabase.cpp:105
>  static inline bool pageCanHaveIcon(const String& pageURL)
>  {
> -    return protocolIsInHTTPFamily(pageURL);
> +    return !pageURL.isEmpty() && pageURL != blankURL();
>  }

Comparison against blankURL is almost always wrong.  I'd check that the URL is
non-empty (and I'd get rid of this function and just make that check wherever
this function is called.