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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri May 6 09:57:15 PDT 2011


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





--- Comment #3 from michaelbai at chromium.org  2011-05-06 09:57:15 PST ---
(In reply to comment #2)
> (From update of attachment 92469 [details])
> 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).
> 

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.
>

I think Favicon is a term now. It is also used in IconType.h and chromium.

> > 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.
> 

Done

> > 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?
> 

Done

> > 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();
> 

Done

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

Done

> > 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.
>

I would prefer use the switch ... case, it prevents the missing match of icon type definition. It might also the reason we have WebXXX leayer.

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

Done

> > 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.
> 

Done

> > 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".

Done, How do I suppress the style check error, I used to have the variable name here.

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



More information about the webkit-unassigned mailing list