Bug 62459 – Local files cannot load icons.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Rafael Brandao]

Created attachment 96745 [details]
You cannot see this page's icon if you load it locally.

WebKit currently defines that only pages whose protocol is in HTTP family (HTTP or HTTPS) can have icons. So if you're designing a page like the one I've attached and want to preview it offline, you won't see any icon displayed. If you upload it to somewhere else, like dropbox, you'll be able to see it correctly.

This behavior looks unintuitive for me, so I would like to discuss why local files can't load icons too, as it can load anything else. Before looking into the code, I've searched examples with working icons because my first thought was that I could be using the icon keyword in the wrong way. I was puzzled because I couldn't reproduce it with my local file, but every page I've visited had an icon displayed. I didn't see anything explaining this on HTML5 specs. The closest thing I've found there was this:

"In the absence of a link with the icon keyword, for Documents obtained over HTTP or HTTPS, user agents may instead attempt to fetch and use an icon with the absolute URL obtained by resolving the URL "/favicon.ico" against the document's address, as if the page had declared that icon using the icon keyword."

Please share your thoughts about it. :)

[Ademar Reis]

FYI, I know that at least firefox (3.6) does load favicons from local addresses. Would be nice to see what other popular browsers do (IE, Opera, Firefox4).

[Adam Barth]

Seems reasonable.

[Rafael Brandao]

Firefox4: icon is loaded and displayed correctly.
Opera 11.10: idem.
IE 8.0.7600.16385: doesn't work. It may crash or display this warning: "To help protect your security, Internet Explorer has restricted this webpage from running scripts or ActiveX controls that could access your computer."

Do we really have any protocol that we should not allow loading icons? I may be wrong but I don't see any problem to allow any page to load its own icon. Perhaps "about:blank" (as it should work just like an empty url)?

[Adam Barth]

Yeah, only the "/favicon.ico" part should be HTTP/HTTPS specific.

[Rafael Brandao]

Created attachment 97048 [details]
Changed pageCanHaveIcon to allow icons to pages whose url is not empty and not "about:blank"

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

[Adam Barth]

Comment on attachment 97048 [details]
Changed pageCanHaveIcon to allow icons to pages whose url is not empty and not "about:blank"

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.

[Darin Adler]

(In reply to comment #6)
> > 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.

Checking against blankURL specifically is a bad idea no matter where we put the code. For one thing the code here is a case sensitive check, and would not work for "ABOUT:BLANK". If we want to limit this and allow protocols such as "file" then I could imagine checking for the "about" scheme with the protocolIs function. This should check the protocol, not the entire URL.

Getting rid of this function and doing the check wherever this function is called is a bad idea. The point of this function is to make clear the purpose of the check. Inside the function is the appropriate place to add comments and explain the logic. Inlining the check at call sites would be a significant step backwards in clarity.

[Adam Barth]

> Getting rid of this function and doing the check wherever this function is called is a bad idea. The point of this function is to make clear the purpose of the check. Inside the function is the appropriate place to add comments and explain the logic. Inlining the check at call sites would be a significant step backwards in clarity.

Fair enough. :)

[Rafael Brandao]

I was also afraid of that blankURL() I've added, thanks for the feedback. I've decided to follow Darin suggestion and check for the protocol instead. I was also concerned about removing those parts of code, but it felt right for me to remove something that is not being used anymore. If there's a problem let me know.

About the tests, I've finally got something running, and then I've realized "layoutTestController.dumpResourceLoadCallbacks()" doesn't detect icon load. It might be related to the fact that icons are loaded after the load of main resources.

On those tests, they use "layoutTestController.dumpResourceResponseMIMETypes();" to display MIME types of loaded resources, and it also fails for icons. :(

I'll be poking you again soon. Thanks for the all the help. :)

[Rafael Brandao]

I was running a layout test with the following settings:

    layoutTestController.dumpAsText();
    layoutTestController.dumpResourceResponseMIMETypes();
    layoutTestController.dumpResourceLoadCallbacks();
    layoutTestController.waitUntilDone();
    setTimeout("layoutTestController.notifyDone()",10000);

Even with this waiting time of 10 seconds, I couldn't track the icon being loaded.

[Rafael Brandao]

I've realized that icons are being loaded in any case. The only problem is to decide whether they'll be stored or not into the IconDatabase. So when someone wants to make sure the icon loaded was properly stored there and it's available for use, then he should wait for the FrameLoaderClient::dispatchDidReceiveIcon. This might be a waste in the process, as it invests processing time to download a resource that won't be stored and won't be used. So when I was doing my layout test using dumpResourceLoadCallbacks, I could see the icon being loaded in any case.

So I have two proposals to solve this, one of them is to add the check pageCanHaveIcon right at FrameLoader::startIconLoader and it would avoid to proceed in case we can't have any icon for that page. That would probably make the resource to not be downloaded, thus failing at my layout test. The second option is to modify FrameLoaderClient::dispatchDidReceiveIcon to print anything meaningful about this event whenever we've activated dumpFrameLoadCallbacks. This last option would probably require a port-specific change to every FrameLoaderClient implementation, and I was trying to avoid to create port-specific layout tests.

What you guys think?

[Darin Adler]

I think it makes sense for IconController::startLoader or IconLoader::startLoading or both to decide not to load an icon based on the same kind of pageCanHaveIcon logic that's currently in IconDatabase.cpp. To do that we'd have to make that function accessible somewhere they could share it.

[Darin Adler]

But I don’t think it’s really all that valuable. This rule affects very few loads, so there’s no reason to spread it out in even more code. It’s not great in theory to do a load and then throw away the result, but the actual case is rare.

[Rafael Brandao]

Created attachment 98076 [details]
Modified constraint to what has been suggested, moved it to IconDatabase class, and added that check to IconController

I've moved it to IconDatabase class because it is mainly used there, so the code wouldn't need to change. This move make it visible for IconController, so I added the pageCanHaveIcon there, and in case it can't, I just don't download any resource. With this change, if we ever change the constraint again and make local files to not have their favicons, this new layout test won't get the resource callback, thus failing.

I'm looking forward for feedback on this. Thanks!

[WebKit Review Bot]

Comment on attachment 98076 [details]
Modified constraint to what has been suggested, moved it to IconDatabase class, and added that check to IconController

Attachment 98076 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8924091

[Antonio Gomes]

Comment on attachment 98076 [details]
Modified constraint to what has been suggested, moved it to IconDatabase class, and added that check to IconController

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

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

is not it too high value? and flaky-prone?

> Source/WebCore/loader/icon/IconDatabase.h:141
> +    static inline bool pageCanHaveIcon(const String& pageURL)

not sure if 'inline' is needed here.

> Source/WebCore/platform/KURL.cpp:1973
> -bool protocolIsInHTTPFamily(const String& url)
> -{
> -    unsigned length = url.length();
> -    const UChar* characters = url.characters();
> -    return length > 4
> -        && isLetterMatchIgnoringCase(characters[0], 'h')
> -        && isLetterMatchIgnoringCase(characters[1], 't')
> -        && isLetterMatchIgnoringCase(characters[2], 't')
> -        && isLetterMatchIgnoringCase(characters[3], 'p')
> -        && (characters[4] == ':'
> -            || (isLetterMatchIgnoringCase(characters[4], 's') && length > 5 && characters[5] == ':'));
> -}
> -
>  }

Unrelated change, and should go to a separeted patch.

[Rafael Brandao]

It may be too high indeed, it was more relevant when I was testing some LayoutTestController features. In "http/tests/misc/favicon-loads-with-images-disabled.html", I see the use of "queueReload" after everything was setup, maybe this one would be better; as I don't know exactly the minimum amount of time I should wait for the icon (and if I don't it won't show up), I'll follow what I've seen in other tests.

So instead of changing KURL right here, should I create a bug report as soon as this patch lands and remove that unused function?

[Rafael Brandao]

> > Source/WebCore/platform/KURL.cpp:1973
> > -bool protocolIsInHTTPFamily(const String& url)
> > -{
> > -    unsigned length = url.length();
> > -    const UChar* characters = url.characters();
> > -    return length > 4
> > -        && isLetterMatchIgnoringCase(characters[0], 'h')
> > -        && isLetterMatchIgnoringCase(characters[1], 't')
> > -        && isLetterMatchIgnoringCase(characters[2], 't')
> > -        && isLetterMatchIgnoringCase(characters[3], 'p')
> > -        && (characters[4] == ':'
> > -            || (isLetterMatchIgnoringCase(characters[4], 's') && length > 5 && characters[5] == ':'));
> > -}
> > -
> >  }
> 
> Unrelated change, and should go to a separeted patch.

As I've removed this function protocolIsInHTTPFamily from pageCanHaveIcon, it became dead code, so this would make this change related to this patch, doesn't?

[Rafael Brandao]

Created attachment 98206 [details]
Modified layout test to use queueReload instead, and did not change KURL in this patch.

I've modified as you've suggested, and for now KURL is not changed, but it's important to remember that protocolIsInHTTPFamily became dead code (and could be easily fixed as soon as this lands).

[WebKit Review Bot]

Comment on attachment 98206 [details]
Modified layout test to use queueReload instead, and did not change KURL in this patch.

Attachment 98206 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8918691

[Antonio Gomes]

> I've modified as you've suggested, and for now KURL is not changed, but it's important to remember that protocolIsInHTTPFamily became dead code (and could be easily fixed as soon as this lands).

I trust you to remove it afterwards:)

[Rafael Brandao]

(In reply to comment #21)
> > I've modified as you've suggested, and for now KURL is not changed, but it's important to remember that protocolIsInHTTPFamily became dead code (and could be easily fixed as soon as this lands).
> 
> I trust you to remove it afterwards:)

:)

Do you know why it is failing on chromium-ews? It looks like it's using another IconDatabase.h, I don't get it.

[Adam Barth]

Comment on attachment 98206 [details]
Modified layout test to use queueReload instead, and did not change KURL in this patch.

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

> Source/WebCore/loader/icon/IconController.cpp:118
> +    String pageURL = m_frame->document()->url();
> +    if (!IconDatabase::pageCanHaveIcon(pageURL))
> +        return;

This is not the pageURL.  It's the document's URL.  In any case, I'd just remove the temporary.

> Source/WebCore/loader/icon/IconDatabase.h:141
> +    static bool pageCanHaveIcon(const String& pageURL)

Does this really need to be the page's URL, or is the document URL sufficient?  I'm not sure.

[Darin Adler]

(In reply to comment #23)
> > Source/WebCore/loader/icon/IconDatabase.h:141
> > +    static bool pageCanHaveIcon(const String& pageURL)
> 
> Does this really need to be the page's URL, or is the document URL sufficient?  I'm not sure.

What is “the page’s URL”?

[Adam Barth]

> What is “the page’s URL”?

I'm not sure.  Maybe the URL of the document of the main frame of the page?

[Rafael Brandao]

(In reply to comment #23)
> > Source/WebCore/loader/icon/IconDatabase.h:141
> > +    static bool pageCanHaveIcon(const String& pageURL)
> 
> Does this really need to be the page's URL, or is the document URL sufficient?  I'm not sure.

When no favicon is specified, according to the specs, it'll look for the absolute URL obtained by resolving the URL "/favicon.ico" against the document's address (when its protocol is HTTP or HTTPS). At least at that point, the document's URL is sufficient.

Now that you've pointed that out, I've seen another URL that is constantly used with the document's URL: the initial request's url. You can see it being used, for example, in IconController::commitToDatabase. The same iconURL is attached to both of them, so if we start to evaluate both urls at the same time at the pageCanHaveIcon check, what would be the preferred behavior? To not load an icon if any of them cannot have one, or to load an icon if any of them can have it?

[Adam Barth]

> When no favicon is specified, according to the specs, it'll look for the absolute URL obtained by resolving the URL "/favicon.ico" against the document's address (when its protocol is HTTP or HTTPS). At least at that point, the document's URL is sufficient.

The question is whether we're interested in the main frame's document or whether any frame is sufficient.  Presumably we only care about the icon for the main frame.

As Darin points out, there is not such thing as a "pageURL".  Pages don't have URLs.  We probably should get rid of all references to that name.  (This is likely just a naming issue, not an actually observable bug.)

> Now that you've pointed that out, I've seen another URL that is constantly used with the document's URL: the initial request's url. You can see it being used, for example, in IconController::commitToDatabase. The same iconURL is attached to both of them, so if we start to evaluate both urls at the same time at the pageCanHaveIcon check, what would be the preferred behavior? To not load an icon if any of them cannot have one, or to load an icon if any of them can have it?

The initial request URL is really dangerous to use.  That probably lets an attacker inject a favicon into someone else's web site!  That's somewhat orthogonal to what you're trying to do here, but you probably won't want to do very much with that URl.

[Rafael Brandao]

Created attachment 98823 [details]
Removing temporary variable and changing variable's name from pageURL to documentURL

I don't know if there's much to add about this, it's quite the same as the last one. I've built this on linux and it's working. If chromium-ews fails again and everything is ok, as I don't know why it is failing to build there, I think we should take the risk.

[WebKit Review Bot]

Comment on attachment 98823 [details]
Removing temporary variable and changing variable's name from pageURL to documentURL

Attachment 98823 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8945585

[Adam Barth]

Comment on attachment 98823 [details]
Removing temporary variable and changing variable's name from pageURL to documentURL

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

> Source/WebCore/loader/icon/IconDatabase.h:141
> +    static bool pageCanHaveIcon(const String& documentURL)

Why is this called "pageCanHaveIcon" when it takes a documentURL?  Maybe it should be called documentCanHaveIcon?  Also, why is this function inline?  It probably should be out-of-line.

[Adam Barth]

Created attachment 99329 [details]
Patch with ???

[Adam Barth]

To capture the discussion with Rafael:

1) The build failure was real.
2) I've uploaded a new patch that fixes the problem.
3) There's a ??? in the patch where I wasn't sure what to do.
4) I've added an ASSERT that we're always dealing with the main frame (so that the frame's document's url really is the URL of the page).  I'm not 100% sure the ASSERT is correct, but the tests will tell us.

I've put the ball back into Rafael's court.

[Rafael Brandao]

(In reply to comment #32)
> To capture the discussion with Rafael:
> 
> 1) The build failure was real.
Thanks Adam for showing me why it was not building. I'll pay more attention next time. :)

> 3) There's a ??? in the patch where I wasn't sure what to do.
I believe that if we don't have IconDatabase available to store icons, it seems pointless to allow pages to have icons as we won't be able to keep them anyway. But, by returning false for all of them, I wouldn't be able to keep the same layout test for every platform as I wanted, because now downloading or not that resource will depend on IconDatabase being enabled or not.

If I keep the same behavior for pageCanHaveIcon, we would waste time downloading the icon and wouldn't be able to use it, but at least we should see the same result everywhere. I feel more inclined to try this option, because I don't understand much why IconDatabase would be kept disabled (is there any other way to store icons that I'm missing?).

[Rafael Brandao]

(In reply to comment #27)
> As Darin points out, there is not such thing as a "pageURL".  Pages don't have URLs.  We probably should get rid of all references to that name.  (This is likely just a naming issue, not an actually observable bug.)

Can't we assume that pageURL is a synonym for documentURL? I'm suggesting this because many functions on IconDatabase, IconDatabaseBase and IconDatabaseClient use this "PageURL", and to change all of them would require changing stuff on many different ports at once. If we don't change the function names, but only the variable names, it will look awkward, like pageCanHaveIcon(String documentURL). Is this naming issue that much valuable? Maybe a comment in the code could make "pageURL" more clear, and we could keep them for now.

[Rafael Brandao]

Created attachment 101240 [details]
Layout test should be skipped for platforms with IconDatabase disabled, and using the test with timeout.

The problem with chromium-ews has shown to me that it would be impossible to make a single layout test for all platforms. So I gave up, and for now I've marked this test as skipped on chromium (and hopefully did this correctly). I've decided to use the timeout version of the test because it seems more stable right now. I've done many tests with the previous setting and I've noticed one test case that failed among many (flaky test). I've done many tests with this new one too but didn't see it failing anytime yet (so it doesn't mean it's 100% safe neither). The problem with favicons instability in tests is probably related to the fact that the icon may be requested at a time that IconDatabase's thread can't read from I/O to find out for sure if it already has the icon data or not, so it leaves the favicon load decision for "later". I think 400 ms for timeout is reasonably good. Another reason for this change is that the previous test setting is not working anymore for some reason.

Hopefully we'll find a good solution for this. It seems more reasonable to leave the test running for all platforms that I don't know yet the output so I can see them failing and then fixing it later.

[WebKit Review Bot]

Attachment 101240 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/loader/icon/IconDatabase.cpp:576:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/loader/icon/IconDatabase.cpp:578:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: FAILURES FOR <lucid, x86_64, release, cpu>
ERROR: Line:3833 More specific entry on line 2360 overrides line 3833 fast/blockflow/broken-ideograph-small-caps.html
LayoutTests/platform/chromium/test_expectations.txt:3833:  More specific entry on line 2360 overrides line 3833 fast/blockflow/broken-ideograph-small-caps.html  [test/expectations] [5]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 101240 [details]
Layout test should be skipped for platforms with IconDatabase disabled, and using the test with timeout.

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

[Rafael Brandao]

(In reply to comment #37)
> (From update of attachment 101240 [details])
> 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.

I didn't understand how bad it was until I actually see what a manual-test is... ugh. I don't want that. :/

I'm quite surprised how in some platforms tests like favicon-loads-with-icon-loading-override are not marked as flaky. I've tried something exactly like that, but got another flaky test as well.The whole problem is the decision IconLoadUnknown which brings instability.

For testing purposes, I guess we could get rid of that decision - so I'm planning to add a new function to LayoutTestController to ignore it and then we would safely detect at least the load request of favicon in layout tests. What do you think of it?

Another option would be to add event listener on those HTMLLinkElements so we could detect "load", as I've seen in other kinds of layout tests. But those target web developers, and I don't see how this event would benefit them, so it already sounds like a bad idea.

Any thoughts on that approach? I'm running out of ideas by now. :(
I'll soon submit a patch to show how I'm planning to use it.

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

Sure. Favicon tests are usually inside a misc directory, maybe I should create a misc folder here as well. Do you have any suggestion on that?

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

I should have marked it as fail instead, as chromium doesn't have IconDatabase enabled. I did that because another icon test, just beside that one, had the same description.

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

Actually I think that's quite relevant! :D

[Rafael Brandao]

Just did more tests, and then figured out it is more complicated than I've thought. Every test I did, I've got to decide what to do with IconLoad twice, one in the very beginning, and one in the very end. There's two different behaviors happening here: the most usual is to get initially an IconLoadUnknown, and then an IconLoadYes (and we get track of it starting to be loaded). The other is to get IconLoadNo initially (because we don't have the icon URL yet), and then IconLoadUnknown. Even if my approach does work, I'll get myself into another problem: the order of the dump resource callbacks will not be fixed, so we still get a flaky test when we rely on a text diff. I could stop dumping resource callbacks, and also add to LayoutTestController something like dumpIconLoadCallbacks so we would only print them... seriously, is there any better way to do it? :/

Is this at least a good enough idea worth trying? :P

If not, then I believe it's time for a manual-test (or no test). :(

[Adam Barth]

Sigh.  Thanks for looking into the testing side of this bug.  As much as it makes me sad, I think we should go with a manual test for this bug.  :(

[Rafael Brandao]

Created attachment 101620 [details]
Just added the manual-test and removed the unrelated comments on the code.

[WebKit Review Bot]

Comment on attachment 101620 [details]
Just added the manual-test and removed the unrelated comments on the code.

Clearing flags on attachment: 101620

Committed r91540: <http://trac.webkit.org/changeset/91540>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This appears to have broken favicon loading completely, see bug 65692.