[webkit-reviews] review requested: [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]

Rafael Brandao <rafael.lobo at openbossa.org> has asked  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 Rafael Brandao <rafael.lobo at openbossa.org>
I had to eliminate the code that checks whether it was part of HTTP family as
it wasn't used anywhere else. I didn't add any test to webkit for now because I
currently don't know very well how to do it. I was checking the changes on
IconDatabase.cpp and I couldn't find any patch there that changed or added new
tests. It seems it has been "ok" to not add new tests.

When I was examining the current layout tests, I've found many of them checking
if icons could be loaded as images, but it wouldn't help in this patch. The
most promising layout test I've found was
"/http/tests/misc/favicon-loads-with-images-disabled.html".

I'm currently having some trouble trying to make http layout tests run here. As
far as I've read on
http://www.chromium.org/developers/testing/webkit-layout-tests, files are
accessed via http on those tests, so adding a new test there probably won't
help to catch this bug.

As I will be quite busy with college the next few days, I won't be able to make
any progress for a while. So I'm uploading my current patch to get any feedback
about it. I would appreciate some help to get into layout tests and create a
good one for this, if a new test is needed. :)