[Webkit-unassigned] [Bug 60247] Handle the touch icon in WebKit/Chromium

[bugzilla-daemon at webkit.org]

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





--- Comment #2 from David Levin <levin at chromium.org>  2011-05-05 18:09:50 PST ---
(From update of attachment 92469)
View in context: https://bugs.webkit.org/attachment.cgi?id=92469&action=review

Here's a few comments. Since this involves the chromium api though Darin should really review it. He may also say things that change what I suggested a little bit in some places so I won't mark this r- so he'll look it over.

> Source/WebKit/chromium/ChangeLog:8
> +        * WebKit.gyp:

Typically you should include a small note for each function or file about why a change was done there (or minimally what was done).

> Source/WebKit/chromium/public/WebIconURL.h:40
> +    Favicon = 1,

Inconsistent casing seems like it should be FavIcon.

Also why not make this "= 1 << 0" for consistency as well.

> Source/WebKit/chromium/public/WebIconURL.h:57
> +    WebIconType m_iconType;

Typical WebKit style is to make a class with private member variables and m_ prefixes or a struct with public member variables and no m_ prefix.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:533
> +            webIconURLs[i] = (WebIconURL(iconURLs[i].m_iconURL, WebIconTypeUtilities::ToWebIconType(iconURLs[i].m_iconType)));

Why have the outer parens?

> Source/WebKit/chromium/src/WebIconTypeUtilities.cpp:50
> +    default:

I would recommend not having a default and handling every case explicitly so that the compiler will catch missing enums in this switch if any are added.

In addition, just return the type directly from the case and then after the switch statement have an ASSERT_NOT_REACHED();

> Source/WebKit/chromium/src/WebIconTypeUtilities.cpp:69
> +    default:

Ditto.

> Source/WebKit/chromium/src/WebIconTypeUtilities.h:39
> +class WebIconTypeUtilities {

Is Utilities the common name for this in type of functionality in this directory? It looks like a Converter more than Utilities.

Also, I wonder if we can make the value align exactly and then do the conversion using a cast.

The fragileness of this approach would be overcome by using COMPILE_ASSERTs.

> Source/WebKit/chromium/src/WebIconTypeUtilities.h:40
> + public:

No indent on public:

> Source/WebKit/chromium/src/WebIconTypeUtilities.h:41
> +    static WebCore::IconType ToIconType(WebIconType);

Naming doesn't follow WebKit style -- should be toIconType.

> Source/WebKit/chromium/src/WebIconTypeUtilities.h:42
> +    static WebIconType ToWebIconType(WebCore::IconType);

Ditto.

> Source/WebKit/chromium/src/WebIconTypeUtilities.h:43
> +    static int ToIconTypes(int);

Ditto. Also there should be a variable name here. The variable name is only left out when it is obviously redundant with the information already on the line. This is not the case for the input "int".

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