Bug 88665 - Favicon URL list sent with favicon updated message contains out of date icon URLs
Summary: Favicon URL list sent with favicon updated message contains out of date icon ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Minor
Assignee: Nobody
URL:
Keywords:
Depends on: 90854 90857
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-08 10:21 PDT by Pete Williamson
Modified: 2013-09-09 10:03 PDT (History)
17 users (show)

See Also:


Attachments
Patch with a proposed fix for the bug (4.60 KB, patch)
2012-06-08 10:59 PDT, Pete Williamson
beidson: review-
Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (adds unit tests, fixes style issue) (28.58 KB, patch)
2012-06-13 12:00 PDT, Pete Williamson
no flags Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with unit tests, more recently synced) (76.66 KB, patch)
2012-06-13 17:29 PDT, Pete Williamson
no flags Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files) (22.80 KB, patch)
2012-06-13 17:53 PDT, Pete Williamson
no flags Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style) (22.73 KB, patch)
2012-06-14 09:32 PDT, Pete Williamson
no flags Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style) (22.74 KB, patch)
2012-06-14 09:38 PDT, Pete Williamson
no flags Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style) (22.73 KB, patch)
2012-06-14 10:29 PDT, Pete Williamson
no flags Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style) (22.73 KB, patch)
2012-06-14 16:43 PDT, Pete Williamson
no flags Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style) (15.60 KB, patch)
2012-06-19 15:55 PDT, Pete Williamson
no flags Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style) (11.95 KB, patch)
2012-06-20 10:35 PDT, Pete Williamson
no flags Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style) (11.88 KB, patch)
2012-06-20 10:43 PDT, Pete Williamson
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style) (13.98 KB, patch)
2012-06-20 18:13 PDT, Pete Williamson
morrita: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments) (11.77 KB, patch)
2012-06-21 11:14 PDT, Pete Williamson
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments) (21.47 KB, patch)
2012-06-21 14:47 PDT, Pete Williamson
no flags Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments) (15.91 KB, patch)
2012-06-22 10:42 PDT, Pete Williamson
no flags Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments) (13.70 KB, patch)
2012-06-22 11:26 PDT, Pete Williamson
no flags Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments) (13.75 KB, patch)
2012-06-22 11:39 PDT, Pete Williamson
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (587.67 KB, application/zip)
2012-06-22 13:57 PDT, WebKit Review Bot
no flags Details
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments) (14.83 KB, patch)
2012-06-22 16:55 PDT, Pete Williamson
gns: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (666.47 KB, application/zip)
2012-06-22 19:55 PDT, WebKit Review Bot
no flags Details
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments) (9.82 KB, patch)
2012-06-25 10:07 PDT, Pete Williamson
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments) (11.37 KB, patch)
2012-06-25 10:37 PDT, Pete Williamson
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments) (16.27 KB, patch)
2012-06-25 11:21 PDT, Pete Williamson
no flags Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments) (16.27 KB, patch)
2012-06-25 14:03 PDT, Pete Williamson
tkent: review-
Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments) (16.92 KB, patch)
2012-06-26 11:02 PDT, Pete Williamson
no flags Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments) (16.25 KB, patch)
2012-06-27 09:49 PDT, Pete Williamson
no flags Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments) (16.17 KB, patch)
2012-06-27 18:05 PDT, Pete Williamson
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments) (deleted)
2012-06-27 18:53 PDT, Pete Williamson
no flags Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments) (20.59 KB, patch)
2012-06-27 19:03 PDT, Pete Williamson
no flags Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments) (20.59 KB, patch)
2012-06-27 19:46 PDT, Pete Williamson
tkent: review-
Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments) (14.48 KB, patch)
2012-06-28 10:47 PDT, Pete Williamson
no flags Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments) (19.84 KB, patch)
2012-06-29 09:44 PDT, Pete Williamson
tkent: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments) (19.80 KB, patch)
2012-07-09 15:17 PDT, Pete Williamson
no flags Details | Formatted Diff | Diff
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments, BrowserTests pass) (19.75 KB, patch)
2012-07-13 13:41 PDT, Pete Williamson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pete Williamson 2012-06-08 10:21:34 PDT
Overview:  If the favicon in the HTML header is changed via DOM manipulation, then webkit will send a notification that the favicon changed which contains a list of favicon URLs that still includes the previous favicon, even though it is no longer in the DOM.


Steps to Reproduce: 
1. Load a page with a favicon
2. Send an event to the page that causes its JS to change the favicon to a different one
3. Send a second event to the page to cause the JS to change the favicon again


Actual Results: 
The code handling the favicon change gets a list of icon URLs with the event which contains both the favicon from step 2 and the favicon from step 3

Expected Results: 
The code handling the favicon change should get a list of icon URLs without the favicon from step 2

Build Date & Platform:
Top of tree as of June 6, 2012.  Mac

Additional Builds and Platforms: 
The bug also occurs on PC and Linux


Additional Information: 
We build a list of URLs to pass back to handle multiple kinds of favicons, such as touch favicons for running in a touch screen environment, and regular favicons for a non touch environment.  It could potentially contain links to favicons of different sizes.  We keep a list of icon urls (m_iconURLs) and we add every new icon we see to the list, but we never remove icons for a particular page.

One potential approach we could take is to ensure that icons get removed when they are removed from the DOM, but this encounters a problem.  Presumably we will eliminate duplicates when creating the list.  Suppose we have two identical Icon URLs in the list, and we remove one when the DOM changes.  Without adding some kind of refcount scheme, we would end up removing both references.  This approach is workable, but complicated and difficult to maintain.

A second approach is to re-build the list by walking the DOM every time the list is requested.  (I have a sample patch showing this approach which I'll submit for review).  We have to consider that the second approach may be slower.

Also, while investigating I discovered that we end up with many copies of the same icon URL in the list (we add one every time the api to get them gets called).  My patch will also address this.

There is more discussion on this issue in chromium bug 125806 where we discuss the alternatives for fixing the bug in more detail.
Comment 1 Pete Williamson 2012-06-08 10:59:23 PDT
Created attachment 146608 [details]
Patch with a proposed fix for the bug

Proposed implementation for the second solution approach.
Comment 2 WebKit Review Bot 2012-06-08 11:04:08 PDT
Attachment 146608 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/dom/Document.cpp', u'Source..." exit_code: 1
Source/WebCore/dom/Document.cpp:4818:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Brady Eidson 2012-06-08 16:07:56 PDT
Comment on attachment 146608 [details]
Patch with a proposed fix for the bug

We historically have not supported dynamic changing of the icon very well, almost intentionally.

If we are going to start supporting this and expect to continue supporting it *consistently*, then I'm afraid you have the burden of being the first one to write regression tests that are capable of capturing the expectations.
Comment 4 Pete Williamson 2012-06-13 12:00:25 PDT
Created attachment 147382 [details]
Patch with a proposed fix for the bug (adds unit tests, fixes style issue)

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.
Comment 5 Pete Williamson 2012-06-13 17:29:41 PDT
Created attachment 147447 [details]
Patch with a proposed fix for the bug (with unit tests, more recently synced)

Last patch did not pass unit tests since it had not synced recently enough.  Here is the same patch with a more recent sync, and one whitespace issue fixed.
Comment 6 Pete Williamson 2012-06-13 17:53:27 PDT
Created attachment 147452 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files)

Trying again - this time without the extra files that slipped into the last patch
Comment 7 WebKit Review Bot 2012-06-13 17:55:59 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 8 WebKit Review Bot 2012-06-13 17:56:36 PDT
Attachment 147452 [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
Tools/DumpRenderTree/chromium/TestShell.cpp:61:  Alphabetical sorting problem.  [build/include_order] [4]
Tools/DumpRenderTree/chromium/TestShell.cpp:619:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebKit/chromium/src/WebDocument.cpp:228:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 3 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Pete Williamson 2012-06-14 09:32:23 PDT
Created attachment 147602 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style)

Changes to pass style checks
Comment 10 WebKit Review Bot 2012-06-14 09:34:53 PDT
Attachment 147602 [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
Tools/DumpRenderTree/chromium/TestShell.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Pete Williamson 2012-06-14 09:38:33 PDT
Created attachment 147604 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style)

Trying again to get past style checks
Comment 12 WebKit Review Bot 2012-06-14 09:42:28 PDT
Attachment 147604 [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
Tools/DumpRenderTree/chromium/TestShell.cpp:45:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Pete Williamson 2012-06-14 10:29:39 PDT
Created attachment 147610 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style)

Let's try again to pass style checks.  See previous patch comments for the long version about what this patch does.
Comment 14 Pete Williamson 2012-06-14 16:43:53 PDT
Created attachment 147678 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style)

Same patch, resubmitting due to failure of the build system trying to run the cr-linux test.
Comment 15 Kent Tamura 2012-06-14 21:42:02 PDT
Comment on attachment 147678 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style)

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

> Source/WebKit/chromium/public/WebDocument.h:108
> +    WEBKIT_EXPORT WebVector<WebIconURL> iconURLs() const;
> +    WEBKIT_EXPORT void recalculateIconURLs();

Do you add them only for DumpRenderTree?
If so, please don't do it.  We should use WebCore/testing/Internals.*.
Comment 16 Pete Williamson 2012-06-19 15:55:47 PDT
Created attachment 148447 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style)

Addresses Kent's concern about not adding methods to the public interface of WebDocument by using "Internals" instead.  Now instead of using a dump routine, we have an internals routine that provides a list of the current Icon URLs attached to the head of a document as a string, and the unit test checks that string.
Comment 17 Pete Williamson 2012-06-20 10:35:53 PDT
Created attachment 148598 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style)

redone with a fresh sync so that automated tools will merge properly.
Comment 18 WebKit Review Bot 2012-06-20 10:39:01 PDT
Attachment 148598 [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/testing/Internals.cpp:1134:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/testing/Internals.cpp:1134:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/ChangeLog:29:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:32:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 4 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Pete Williamson 2012-06-20 10:43:22 PDT
Created attachment 148601 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style)

Fix style nits
Comment 20 Build Bot 2012-06-20 12:01:38 PDT
Comment on attachment 148601 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style)

Attachment 148601 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12996322
Comment 21 Build Bot 2012-06-20 12:30:44 PDT
Comment on attachment 148601 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style)

Attachment 148601 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13003283
Comment 22 Pete Williamson 2012-06-20 18:13:18 PDT
Created attachment 148697 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style)

Slight changes to approach based on CR feedback.  
We now always calculate the URL list.
We changed the way we build the list of IconURLs to be a bit simpler(using the existing KURL instead of making a new one, less string manipulation)
We now use a list of URLs in the Internals method instead of a big concatenated string.
Comment 23 WebKit Review Bot 2012-06-20 18:16:31 PDT
Attachment 148697 [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/testing/Internals.cpp:1131:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Build Bot 2012-06-20 18:47:20 PDT
Comment on attachment 148697 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style)

Attachment 148697 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13013084
Comment 25 Build Bot 2012-06-20 18:56:03 PDT
Comment on attachment 148697 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style)

Attachment 148697 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13018064
Comment 26 Hajime Morrita 2012-06-20 21:36:06 PDT
Comment on attachment 148697 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style)

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

The code is over-commented. We can get rid of most of them.

On linker errors, you need to export some symbols which is referred from the Internals object.
See http://trac.webkit.org/changeset/120684 as an example.
WebCore.exp.in, WebKit2.def, symbols.fitter are such files.

> Source/WebCore/dom/Document.cpp:4877
> +    // Ensure URL is not already in the list before adding it.

This comment can be removed.

> Source/WebCore/testing/Internals.h:185
> +    PassRefPtr<DOMStringList> dumpIconURLs(Document*) const;

"dump" is redundant.

> Source/WebKit/chromium/public/WebFrame.h:-148
> -    virtual WebVector<WebIconURL> iconURLs(int iconTypes) const = 0;

I don't think we need to change this const-ness.
Comment 27 Pete Williamson 2012-06-21 11:14:53 PDT
Created attachment 148845 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

This is not ready for checkin yet, just testing to see if style is right and to get the compile errors from the buildbot
Comment 28 Build Bot 2012-06-21 11:52:52 PDT
Comment on attachment 148845 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Attachment 148845 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13036180
Comment 29 Gustavo Noronha (kov) 2012-06-21 13:19:49 PDT
Comment on attachment 148845 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Attachment 148845 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13029208
Comment 30 Build Bot 2012-06-21 14:24:03 PDT
Comment on attachment 148845 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Attachment 148845 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13031225
Comment 31 Pete Williamson 2012-06-21 14:47:54 PDT
Created attachment 148889 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Trying a fix to the build bot
Comment 32 Pete Williamson 2012-06-22 10:42:51 PDT
Created attachment 149055 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Last patch had an extra change, trying again.
Comment 33 Pete Williamson 2012-06-22 11:26:10 PDT
Created attachment 149070 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Try again to get the right set of files, but not include anything that is already committed.
Comment 34 WebKit Review Bot 2012-06-22 11:28:47 PDT
Attachment 149070 [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/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 35 Pete Williamson 2012-06-22 11:39:06 PDT
Created attachment 149074 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Adding a bug ID for the changelog for the newly added file.
Comment 36 Build Bot 2012-06-22 12:31:36 PDT
Comment on attachment 149074 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Attachment 149074 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13033473
Comment 37 Gustavo Noronha (kov) 2012-06-22 13:24:54 PDT
Comment on attachment 149074 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Attachment 149074 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13025467
Comment 38 WebKit Review Bot 2012-06-22 13:57:16 PDT
Comment on attachment 149074 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

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

New failing tests:
fast/dom/icon-url-property.html
Comment 39 WebKit Review Bot 2012-06-22 13:57:23 PDT
Created attachment 149097 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 40 Pete Williamson 2012-06-22 16:55:07 PDT
Created attachment 149142 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Adding symbols.filter change to see if that gets past the buildbots
Comment 41 Gustavo Noronha (kov) 2012-06-22 17:26:10 PDT
Comment on attachment 149142 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Attachment 149142 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13039514
Comment 42 Build Bot 2012-06-22 17:53:13 PDT
Comment on attachment 149142 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Attachment 149142 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13027587
Comment 43 WebKit Review Bot 2012-06-22 19:54:58 PDT
Comment on attachment 149142 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

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

New failing tests:
fast/dom/icon-url-property.html
Comment 44 WebKit Review Bot 2012-06-22 19:55:06 PDT
Created attachment 149159 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 45 Pete Williamson 2012-06-25 10:07:33 PDT
Created attachment 149312 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Now also changing symbols.filter
Comment 46 Build Bot 2012-06-25 10:14:54 PDT
Comment on attachment 149312 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Attachment 149312 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13101105
Comment 47 Pete Williamson 2012-06-25 10:37:02 PDT
Created attachment 149320 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

fix warnings due to removed variables.
Comment 48 Build Bot 2012-06-25 10:56:55 PDT
Comment on attachment 149320 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Attachment 149320 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13087224
Comment 49 Early Warning System Bot 2012-06-25 11:11:50 PDT
Comment on attachment 149320 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Attachment 149320 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13087231
Comment 50 Pete Williamson 2012-06-25 11:21:10 PDT
Created attachment 149331 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

fix build warnings about unused variables
Comment 51 Pete Williamson 2012-06-25 14:03:18 PDT
Created attachment 149353 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Fix Symbol Exporting for GTK for the internals change
Comment 52 Pete Williamson 2012-06-25 15:44:55 PDT
Comment on attachment 149353 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

OK, all comments addressed, it builds and tests pass, ready for review.
Comment 53 Kent Tamura 2012-06-25 17:58:48 PDT
Comment on attachment 149353 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

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

> ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=88665
> +        Exposing a test function in Internals

Please reverse the order of these lines.

  Exposing a test function in Internals
  https://bugs.webkit.org/show_bug.cgi?id=88665

> LayoutTests/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=88665
> +        Added new unit tests to support changing the favicon dynamically

ditto.

> LayoutTests/fast/dom/icon-url-property.html:44
> +    // dump the icon URL list in the document

This comment looks false. The code show PASS or FAIL.

> Source/WebCore/ChangeLog:10
> +        https://bugs.webkit.org/show_bug.cgi?id=88665
> +        Changed the behavior of iconURLs to always recalculate the list.  As it turns out, it can
> +        contain stale URLs in the case that some script manipulates the DOM, which breaks scripts
> +        trying to reset the favicon URL.  Also added a method in Internals to allow tests to get
> +        the list of icon
> +
> +        Reviewed by NOBODY (OOPS!).
> +

Our standard format of a ChangeLog entry is like:

        Changed the behavior of iconURLs to always recalculate the list.
        https://bugs.webkit.org/show_bug.cgi?id=88665

        Reviewed by NOBODY (OOPS!).

        As it turns out, it can contain stale URLs in the case that some script
        manipulates the DOM, which breaks scripts trying to reset the favicon
        URL. Also added a method in Internals to allow tests to get the list of
        icon

> Source/WebCore/ChangeLog:39
> +        * WebCore.exp.in:
> +        * dom/Document.cpp:
> +        (WebCore::Document::iconURLs):
> +        (WebCore):
> +        (WebCore::Document::recalculateIconURLs):
> +        * dom/Document.h:
> +        (Document):
> +        * html/HTMLLinkElement.cpp:
> +        (WebCore::HTMLLinkElement::process):
> +        (WebCore::HTMLLinkElement::iconType):
> +        (WebCore):
> +        (WebCore::HTMLLinkElement::iconSizes):
> +        * html/HTMLLinkElement.h:
> +        (HTMLLinkElement):
> +        * loader/LinkLoader.cpp:
> +        (WebCore::LinkLoader::loadLink):
> +        * loader/LinkLoader.h:
> +        (LinkLoader):
> +        * loader/icon/IconController.cpp:
> +        (WebCore::IconController::urlsForTypes):
> +        * testing/Internals.cpp:
> +        (WebCore::Internals::iconURLs):
> +        (WebCore):
> +        * testing/Internals.h:
> +        (Internals):
> +        * testing/Internals.idl:

Please add comments for each of files/functions about what you changed.

> Source/WebCore/dom/Document.cpp:4836
> +    recalculateIconURLs();
>      return m_iconURLs;

recalculateIconURLs() and Document::m_iconURLs are used only in iconURLS().  It seems we can remove m_iconURLs, and fold recalculateIconURLs() into iconURLs().

If you'd like to keep recalculateIconURLs() function, it should be private.

> Source/WebCore/dom/Document.cpp:4853
> +        if (!equalIgnoringCase(linkElement->type(), iconMIMEType)
> +            || !(linkElement->iconType() == Favicon))

You don't need to fold these lines.

!(linkElement->iconType() == Favicon) should be linkElement->iconType() != Favicon ?

> Source/WebCore/dom/Document.cpp:4862
> +        IconURL newURL(linkElement->href(),
> +                       linkElement->iconSizes(),
> +                       linkElement->type(),
> +                       linkElement->iconType());

You don't need to fold these lines.

> Source/WebCore/html/HTMLLinkElement.h:60
> +    String iconSizes() const;

Need a comment about how the sizes are represented in the resultant string.
Comment 54 Pete Williamson 2012-06-26 11:02:22 PDT
Created attachment 149563 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Address tkent cr comments
Comment 55 Pete Williamson 2012-06-26 13:05:15 PDT
Comment on attachment 149563 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

CR comments addressed, ready for re-review
Comment 56 Kent Tamura 2012-06-26 19:19:55 PDT
Comment on attachment 149563 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

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

> Source/WebCore/dom/Document.cpp:4847
> +        if (!equalIgnoringCase(linkElement->type(), iconMIMEType) || !(linkElement->iconType() == Favicon))

nit: !(linkElement->iconType() == Favicon) looks strange.  Why not linkElement->iconType() != Favicon ?

> Source/WebCore/loader/icon/IconController.cpp:89
> +    // Start by adding the favicon from the current document if we have one, if not, use the default favicon.

This change looks unnecessary.
Comment 57 Pete Williamson 2012-06-27 09:49:35 PDT
Created attachment 149766 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Minor CR changes
Comment 58 Pete Williamson 2012-06-27 18:05:52 PDT
Created attachment 149838 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

I removed a bit too much in my attempt to get rid of dead code, we still need the part of addIconURL that fires the event.
Comment 59 Build Bot 2012-06-27 18:27:05 PDT
Comment on attachment 149838 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Attachment 149838 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13101897
Comment 60 Build Bot 2012-06-27 18:50:57 PDT
Comment on attachment 149838 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Attachment 149838 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13098946
Comment 61 Pete Williamson 2012-06-27 18:53:01 PDT
Created attachment 149850 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Fix compile errors - If we bring back AddIconURLs, we need some supporting code.
Comment 62 Pete Williamson 2012-06-27 19:03:53 PDT
Created attachment 149851 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Some tests seem to be missing from the previous patch, resubmitting.
Comment 63 Pete Williamson 2012-06-27 19:46:03 PDT
Created attachment 149857 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Retrying the same patch since it failed to unpack for style checking
Comment 64 Pete Williamson 2012-06-27 21:23:34 PDT
Comment on attachment 149857 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Changes ready for CR
Comment 65 Kent Tamura 2012-06-27 21:31:48 PDT
Comment on attachment 149857 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

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

> LayoutTests/fast/dom/icon-url-change.html:12
> +function debugOutput(str) {
> +    text = document.createTextNode(str);
> +    debugDiv = document.getElementById('debugDiv');
> +    div = document.createElement ('div');
> +    div.appendChild(text);
> +    debugDiv.appendChild(div);
> +}

Please do not re-implement such function.  You had better use debug() in fast/js/resources/js-test-pre.js.

> LayoutTests/fast/dom/icon-url-change.html:23
> +    for (var i = 0; i < links.length; ++i) {
> +      var link = links[i];
> +      if (link.type=="image/x-icon" && link.rel=="shortcut icon") {
> +        docHead.removeChild(link);
> +        break; // Assuming only one match at most.
> +      }
> +    }

We usually use four-space indentation in JavaScript code.

> LayoutTests/fast/dom/icon-url-change.html:35
> +    iconURL = document.getElementsByTagName("head")[0].getElementsByTagName("link")[0].href;

document.getElementsByTagName("link")[0].href is enough?

> LayoutTests/fast/dom/icon-url-change.html:59
> +        debugOutput('PASS - URL list matches expected');
> +    else
> +        debugOutput('FAIL - URL list does not match expected');

You had better use testPassed() and testFailed() defined in fast/js/resources/js-test-pre.js

> LayoutTests/fast/dom/icon-url-list.html:14
> +function debugOutput(str) {
> +    text = document.createTextNode(str);
> +    debugDiv = document.getElementById('debugDiv');
> +    div = document.createElement ('div');
> +    div.appendChild(text);
> +    debugDiv.appendChild(div);
> +}

same comment as icon-url-change.html

> LayoutTests/fast/dom/icon-url-list.html:34
> +    for (var i = 0; i < links.length; ++i) {
> +      var oldLink = links[i];
> +      if (oldLink.type=="image/x-icon" && oldLink.rel=="shortcut icon") {
> +        // if we find the child, replace it with the new node.
> +	//debugOutput('replacing ' + oldLink.href + ' with ' + newLink.href);
> +        docHead.replaceChild(newLink, oldLink);
> +        return; // Assuming only one match at most.
> +      }
> +    }

ditto.
Also, please remove commented-out code.

> LayoutTests/fast/dom/icon-url-list.html:44
> +    iconURL = document.getElementsByTagName("head")[0].getElementsByTagName("link")[0].href;

ditto.

> LayoutTests/fast/dom/icon-url-list.html:51
> +    iconURL = document.getElementsByTagName("head")[0].getElementsByTagName("link")[0].href;

ditto.

> LayoutTests/fast/dom/icon-url-list.html:63
> +        debugOutput('PASS - URL list matches expected');
> +    else
> +        debugOutput('FAIL - URL list does not match expected');

ditto.
Comment 66 Pete Williamson 2012-06-28 10:47:59 PDT
Created attachment 149972 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Addressed CR comments for unit tests
Comment 67 Pete Williamson 2012-06-28 17:09:47 PDT
Comment on attachment 149972 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Ready for re-review
Comment 68 Kent Tamura 2012-06-28 17:54:47 PDT
Comment on attachment 149972 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

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

> LayoutTests/ChangeLog:11
> +        * fast/dom/icon-url-change-expected.txt: Added.
> +        * fast/dom/icon-url-change.html: Added a new test for changing the favicon dynamically
> +        * fast/dom/icon-url-list-expected.txt: Added.
> +        * fast/dom/icon-url-list.html: Added a new test for multiple favicons in the HTML header

The patch doesn't contain these files.
Comment 69 Pete Williamson 2012-06-29 09:44:13 PDT
Created attachment 150199 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Added back in the missing files (there was a flaw in my process for making a patch)
Comment 70 Pete Williamson 2012-06-29 14:24:25 PDT
Comment on attachment 150199 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Added in the missing files, ready for review
Comment 71 Kent Tamura 2012-07-09 00:29:32 PDT
Comment on attachment 150199 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Looks good.
Comment 72 WebKit Review Bot 2012-07-09 01:39:36 PDT
Comment on attachment 150199 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Rejecting attachment 150199 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
um/third_party/v8-i18n --revision 105 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
44>At revision 105.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...
Total errors found: 0 in 0 files

Full output: http://queues.webkit.org/results/13163209
Comment 73 Kent Tamura 2012-07-09 02:03:13 PDT
Comment on attachment 150199 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

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

> LayoutTests/ChangeLog:15
> +        Need a short description and bug URL (OOPS!)

Remove this line.

> Source/WebCore/ChangeLog:35
> +        Need a short description and bug URL (OOPS!)

Remove this line.

> Source/WebKit2/ChangeLog:2
> +2012-06-29  Pete Williamson  <petewil@google.com>
> +        Export the iconURL list to make it available to the Internals class for testing

Please add a blank line before the description.
Comment 74 Pete Williamson 2012-07-09 15:17:04 PDT
Created attachment 151330 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Very slight updates to the change log, and a logical merge to use RefPtr<HTMLCollection> instead of HTMLCollection*
Comment 75 WebKit Review Bot 2012-07-09 15:17:22 PDT
Comment on attachment 151330 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Rejecting attachment 151330 [details] from commit-queue.

petewil@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 76 Pete Williamson 2012-07-09 15:19:23 PDT
Comment on attachment 151330 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Oops, I don't have permission to set cq to '+'
Comment 77 Dmitry Titov 2012-07-09 17:45:32 PDT
Comment on attachment 151330 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

This is patch for landing, setting cq+ for Peter...
Comment 78 WebKit Review Bot 2012-07-09 18:37:54 PDT
Comment on attachment 151330 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

Clearing flags on attachment: 151330

Committed r122178: <http://trac.webkit.org/changeset/122178>
Comment 79 Csaba Osztrogonác 2012-07-10 01:00:47 PDT
(In reply to comment #78)
> (From update of attachment 151330 [details])
> Clearing flags on attachment: 151330
> 
> Committed r122178: <http://trac.webkit.org/changeset/122178>

It made 750 tests crash on Qt WK2 - https://bugs.webkit.org/show_bug.cgi?id=90854
Comment 80 Csaba Osztrogonác 2012-07-10 01:48:20 PDT
(In reply to comment #78)
> (From update of attachment 151330 [details])
> Clearing flags on attachment: 151330
> 
> Committed r122178: <http://trac.webkit.org/changeset/122178>

Rolled out by http://trac.webkit.org/changeset/122205 (https://bugs.webkit.org/show_bug.cgi?id=90857)
Comment 81 Pete Williamson 2012-07-13 13:41:30 PDT
Created attachment 152323 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments, BrowserTests pass)

The previous attempt passed the commit queue and built fine, but made some chromium tests fail, and some Qt tests fail.

We fixed the Qt test crashes by making sure the check the head() pointer for null in Document.cpp before de-referencing it.  We noticed the crash was in the call to children, which would crash if the document had no html HEAD tag, so we think this will fix the Qt crashes.

We fixed the browser tests by noticing that the previous check in Document.cpp in addIconURLs was too restrictive - we only accepted the node as a favicon if the mime type was set to image/x-icon, but it is legitimate for the mime type to not be set at all on a favicon, so we removed the check for image type.  The browser tests now all succeed.

Those are the only two changes since the last patch, both in Document::addIconURLs
Comment 82 WebKit Review Bot 2012-07-16 21:37:42 PDT
Comment on attachment 152323 [details]
Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments, BrowserTests pass)

Clearing flags on attachment: 152323

Committed r122806: <http://trac.webkit.org/changeset/122806>
Comment 83 WebKit Review Bot 2012-07-16 21:37:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 84 Alexey Proskuryakov 2013-09-09 10:03:49 PDT
This change caused bug 120784.