[webkit-reviews] review denied: [Bug 59143] Need populate touch-icon url to FrameLoaderClient : [Attachment 91114] Initial implementation

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied
michaelbai at chromium.org's request for review:
Bug 59143: Need populate touch-icon url to FrameLoaderClient
https://bugs.webkit.org/show_bug.cgi?id=59143

Attachment 91114: Initial implementation
https://bugs.webkit.org/attachment.cgi?id=91114&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=91114&action=review

r- to correct issues noted above.

Also, this change seems like it will load all icons on all page loads in all
browsers.  This isn't correct since Desktop browsers (at least Safari) is not
going to want to try to load the "touch" icon variants on every page load. 
MobileSafari on iOS currently only loads the "touch" icons when the user saves
the URL (or web app) to the Home screen, not on every page load.  You need a
way to enable/disable certain icons loading (and have "touch" icons off by
default) before you land this.

Also, please make sure WebKit still compiles with ENABLE(ICON_DATABASE)
disabled.  Thanks!

> Source/WebCore/dom/Document.cpp:355
> +	   default:
> +	       return -1;

There should not be a default case here.  Having one defeats the compiler from
warning you when you don't have a case defined.  This return statement should
be moved outside of the switch statement.

This should also be an (inline) method in IconURL.{h|cpp} so that you can share
code between Document.cpp and DocumentLoader.cpp.

This method should really return size_t (which is used for array indexes), and
then default to Favicon if 'type' is unknown and ASSERT_NOT_REACHED() before
returning a default value.

> Source/WebCore/dom/Document.cpp:4384
> +    if (index != -1)
> +	   return m_iconURLs[index];
> +    else
> +	   return IconURL();

Should use early return here:

    if (index == -1)
	return IconURL();
    return m_iconURLs[index];

> Source/WebCore/dom/Document.h:1338
> +    IconURL m_iconURLs[3];

"3" should not be hard-coded here.  You should use the IconType enum to define
this, or add some kind of compiler assertion, or define a const value in
IconURL.h that is next to the IconType enum.

> Source/WebCore/loader/DocumentLoader.cpp:92
> +static int convertIconTypeToIndex(IconType type) {
> +    switch(type) {
> +	   case Favicon:
> +	       return 0;
> +	   case TouchPrecomposedIcon:
> +	       return 1;
> +	   case TouchIcon:
> +	       return 2;
> +	   default:
> +	       return -1;
> +    }
> +}

Duplicate code.  Please use the method added to IconURL.h.

> Source/WebCore/loader/DocumentLoader.cpp:684
> +    if (index != -1) {
> +	   return m_iconURLs[index];
> +    } else {
> +	   return IconURL();
> +    }

Please use early return.  Unnecessary curly braces.  (Please run
check-webkit-style on your changed files.)

> Source/WebCore/loader/DocumentLoader.h:304
> +	   IconURL m_iconURLs[3];

"3" should be a constant defined in IconURL.h (as noted for Document.cpp).

> Source/WebCore/loader/FrameLoader.cpp:525
> -    url.setPath("/favicon.ico");
> -    return url;
> +    if (iconType == Favicon) {
> +	   url.setPath("/TouchPrecomposedIcon.ico");

Why did this change from "/favicon.ico" to "/TouchPrecomposedIcon.ico"?  I
don't think this is correct.