[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


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #92594|review?                     |review-
               Flag|                            |




--- Comment #7 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2011-05-06 11:58:37 PST ---
(From update of attachment 92594)
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?

> 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, ... };

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

do these fields need to be mutable?  maybe RIIA would be sufficient?

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

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