[webkit-reviews] review denied: [Bug 88665] Favicon URL list sent with favicon updated message contains out of date icon URLs : [Attachment 149353] Patch with a proposed fix for the bug (with layout tests, more recently synced, no extra files, better style, fixed CR comments)

[bugzilla-daemon at webkit.org]

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

Attachment 149353: Patch with a proposed fix for the bug (with layout tests,
more recently synced, no extra files, better style, fixed CR comments)
https://bugs.webkit.org/attachment.cgi?id=149353&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
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.