[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