[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 #9 from michaelbai at chromium.org  2011-05-06 14:46:03 PST ---
(In reply to comment #7)
> (From update of attachment 92594 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92594&action=review
> 
> > Source/WebKit/chromium/public/WebFrame.h:135
> > +    virtual WebVector<WebIconURL> favIconURL(int) const = 0;
> 
> what is this magic int parameter?
> 

It is the combination of IconTypes, added a comment

> > Source/WebKit/chromium/public/WebIconURL.h:38
> > +enum WebIconType {
> 
> please create a separate header file for each toplevel type: class, struct or enum.
> 
> please also follow the naming conventions for enums.  enum Foo { FooA, FooB, ... };
> 

I moved the WebIconType into WebIconURL as it will only used by WebIconURL. I didn't add the prefix as to access the emun value has to be used WebIconURL::. Hope it is fine.

> > Source/WebKit/chromium/public/WebIconURL.h:57
> > +    WebIconType iconType;
> 
> do these fields need to be mutable?  maybe RIIA would be sufficient?
> 

Sorry, I don't know RIIA stand for, I guess you want to use 
const WebIconTyp iconType.

If so, I can not assign the WebIconURL to WebVector in WebFrameImpl::favIcon(int).

> > Source/WebKit/chromium/src/WebIconTypeUtilities.h:39
> > +class WebIconTypeUtilities {
> 
> utilities classes tend to become dumping grounds.  can you instead give this a more specific name?  it seems to be all about type conversion.
> 
> have you considered making the api types have exactly the same values as the webcore ones?  that could simplify conversions.  see AssertMatchingEnums.cpp.

Removed WebIconTypeUtilities, Used AssertMatchingEnums

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