[Webkit-unassigned] [Bug 88665] Favicon URL list sent with favicon updated message contains out of date icon URLs

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=88665


Kent Tamura <tkent at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #149353|review?                     |review-
               Flag|                            |




--- Comment #53 from Kent Tamura <tkent at chromium.org>  2012-06-25 17:58:48 PST ---
(From update of attachment 149353)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.