Favicon URL list sent with favicon updated message contains out of date icon URLs

• Component: Images
• Status: RESOLVED FIXED

[Pete Williamson]

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.

[Pete Williamson]

Created attachment 146608 [details]
Patch with a proposed fix for the bug

Proposed implementation for the second solution approach.

[WebKit Review Bot]

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.

[Brady Eidson]

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.

[Pete Williamson]

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.

[Pete Williamson]

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.

[Pete Williamson]

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

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

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.

[Pete Williamson]

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

[WebKit Review Bot]

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.

[Pete Williamson]

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

[WebKit Review Bot]

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.

[Pete Williamson]

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.

[Pete Williamson]

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.

[Kent Tamura]

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

[Pete Williamson]

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.

[Pete Williamson]

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.

[WebKit Review Bot]

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.

[Pete Williamson]

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

[Build Bot]

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

[Build Bot]

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

[Pete Williamson]

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.

[WebKit Review Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Hajime Morrita]

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.

[Pete Williamson]

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

[Build Bot]

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

[Gustavo Noronha (kov)]

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

[Build Bot]

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

[Pete Williamson]

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

[Pete Williamson]

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.

[Pete Williamson]

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.

[WebKit Review Bot]

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.

[Pete Williamson]

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.

[Build Bot]

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

[Gustavo Noronha (kov)]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Pete Williamson]

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

[Gustavo Noronha (kov)]

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

[Build Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Pete Williamson]

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

[Build Bot]

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

[Pete Williamson]

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.

[Build Bot]

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

[Early Warning System Bot]

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

[Pete Williamson]

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

[Pete Williamson]

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

[Pete Williamson]

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.

[Kent Tamura]

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.

[Pete Williamson]

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

[Pete Williamson]

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

[Kent Tamura]

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.

[Pete Williamson]

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

[Pete Williamson]

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.

[Build Bot]

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

[Build Bot]

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

[Pete Williamson]

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.

[Pete Williamson]

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.

[Pete Williamson]

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

[Pete Williamson]

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

[Kent Tamura]

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.

[Pete Williamson]

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

[Pete Williamson]

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

[Kent Tamura]

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.

[Pete Williamson]

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)

[Pete Williamson]

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

[Kent Tamura]

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.

[WebKit Review Bot]

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

[Kent Tamura]

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
> [phone]  Pete Williamson  <[e-mail]>
> +        Export the iconURL list to make it available to the Internals class for testing

Please add a blank line before the description.

[Pete Williamson]

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*

[WebKit Review Bot]

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.

[e-mail] 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.

[Pete Williamson]

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 '+'

[Dmitry Titov]

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

[WebKit Review Bot]

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>

[Csaba Osztrogonác]

(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

[Csaba Osztrogonác]

(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)

[Pete Williamson]

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

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This change caused bug 120784.