Bug 10907 – REGRESSION: New Icon Loaders don't handle certain non-server-root URLs correctly

• Component: Page Loading
• Status: RESOLVED FIXED

[Brady Eidson]

Since switching over the WebCore Icon Loader, certain URLs not at the root of a server don't get their default icon figured out correctly.

To reproduce -
1. Wipe your ~/Library/Safari/Icons directly to start fresh
2. Open ToT
3. Go to www.google.com
4. Note the icon is downloaded and displayed
5. Search for "hello world"
6. You're taking to the URL "http://www.google.com/search?hl=en&q=hello+world&btnG=Google+Search"
7. Note the search results page doesn't have the Google favicon

I can confirm by inspecting an icon.db file with sqlite3 that the reason this occurs is because the search result page doesn't have an icon <link> tag but rather relies on the default favicon.ico.  Problem here is we don't properly construct the default favicon URL in Frame::iconURL().  The constructed URL, as found in the icon.db file, is -
http://www.google.com/favicon.ico?hl=en&q=hello+world&btnG=Google+Search

It seems the post data remains appended to the URL, but there could also be other artifacts that remain in other cases.

Fix is coming shortly...

[Brady Eidson]

Here's what I found -
- Constructing the iconURL by copying the Frame's Doc URL then replacing the path component alone wasn't working due to intricacies in KURL.  Found a better way to do this
- The first site you visited on a server would get its icon.  The 2nd and later wouldn't.  We never got to the stage where we'd map the pageURL to the iconURL unless we kicked off an icon load.  Change it so we make this mapping no matter what once a frame is finished loading.  Indeed, this was the behavior in the old WebKit icon database as well

[Brady Eidson]

Created attachment 10622 [details]
Small changes go a long way

[John Sullivan]

Comment on attachment 10622 [details]
Small changes go a long way

typo in comment: "it's url(s)" should be "its"

+    // FIXME - Need to be able to do the following platform independently
+#if PLATFORM(MAC)
+    FrameMac* frameMac = Mac(this);
+    iconDatabase->setIconURLForPageURL(icon.url(), frameMac->originalRequestURL().url());
+#endif
+}

I think the right way to do this is to override this method in FrameMac and call through to super before handling the frameMac->originalRequestURL case. Then other platforms can do the same thing if they are also keeping track of the original request URL some other way.

Please either make this change or explain why it isn't the right approach. The rest of the patch looks fine.

[Brady Eidson]

Created attachment 10629 [details]
This is alot better, I think

[John Sullivan]

Comment on attachment 10629 [details]
This is alot better, I think

Much nicer, r=me.

[Brady Eidson]

Commited in r16423