[webkit-reviews] review requested: [Bug 88665] Favicon URL list sent with favicon updated message contains out of date icon URLs : [Attachment 147382] Patch with a proposed fix for the bug (adds unit tests, fixes style issue)

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jun 13 12:00:26 PDT 2012


Pete Williamson <petewil at chromium.org> has asked  for review:
Bug 88665: Favicon URL list sent with favicon updated message contains out of
date icon URLs
https://bugs.webkit.org/show_bug.cgi?id=88665

Attachment 147382: Patch with a proposed fix for the bug (adds unit tests,
fixes style issue)
https://bugs.webkit.org/attachment.cgi?id=147382&action=review

------- Additional Comments from Pete Williamson <petewil at chromium.org>
This extends the base patch, which fixes a bug with stale icon URLs in the
document IconURL list.	Basically, if DOM manipulation replaced an icon URL
with a newer one, we were keeping the older icon URL around even though it was
no longer in the DOM.  There is also a fix for duplicates in the iconURL list,
which the code had been creating before.

This patch adds a bunch of unit testing around the favicons to address the
review comments that it has never before been a well tested feature.  

To do so, I had to extend the unit test framework.  First, I exposed two new
methods on WebDocument to allow the tests to get to the underlying URL list
maintained in the document class, and call the "reset" function.  If there is a
better way than exposing them in the WebDocument, I'm all ears.

Second, I added some new methods to the LayoutTestController to allow the unit
tests to request a dump of the iconURL list from the document.	I could add a
second method to expose the recalculate URL list function, but I was trying to
keep it minimal, let me know if anyone thinks it would be useful.

Then, I modified the TestShell class to dump the icon list (and call
recalculate) when requested.

With all this done, I brought back the existing test for the icon properties,
made it work, and removed it from the testExpectations file now that it passes
reliably.  I then added two new tests.	One tests a list of icon URLs in the
head element, the other tests changing the URL in the head element 3 times.


More information about the webkit-reviews mailing list