[Webkit-unassigned] [Bug 59143] Need populate touch-icon url to FrameLoaderClient

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from michaelbai at chromium.org  2011-04-26 10:25:17 PST ---
Hi David,

Thanks very much for the quick response.

I have some issues want to be discussed before I make another patch.

a. Is it OK that using 'SUPPORT_TOUCH_ICON' to disable the touch icon loading?

b. Please refer to my inline reply for convertIconTypeToIndex.


Tao

(In reply to comment #2)
> (From update of attachment 91114 [details])
> 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.

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