[webkit-reviews] review denied: [Bug 60247] Handle the touch icon in WebKit/Chromium : [Attachment 92594] Address all comments

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied
michaelbai at chromium.org's request for review:
Bug 60247: Handle the touch icon in WebKit/Chromium
https://bugs.webkit.org/show_bug.cgi?id=60247

Attachment 92594: Address all comments
https://bugs.webkit.org/attachment.cgi?id=92594&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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.