[webkit-reviews] review denied: [Bug 60247] Handle the touch icon in WebKit/Chromium : [Attachment 93022] Fix style

[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 93022: Fix style
https://bugs.webkit.org/attachment.cgi?id=93022&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=93022&action=review

> Source/WebKit/chromium/public/WebFrame.h:137
> +    // the document loaded in this frame. The iconTypes could be a bit-mask
of

Sorry for all the back-n-forth on this.  Being out on vacation has meant that I
haven't
had the time to give this the focus I should.  Thanks for putting up with all
the cycles.

nit: "The iconTypes [is] a bit-mask of WebIconURL::Type values, used to select
from
the available set of icon URLs."

nit: Since this method returns possibly many WebIconURL objects, please
pluralize the
method name.  It should be favIconURLs instead of favIconURL.

nit: Since this method returns an array of WebIconURL and not WebFavIconURL,
the method
should not have the "Fav" part in its name.  I think it should just be
iconURLs.

> Source/WebKit/chromium/public/WebFrameClient.h:206
> +    virtual void didChangeIcons(WebFrame*, WebIconURL::Type) { }

I think this method name could be improved.  Since it is only reporting the
change of a single icon type, it should not be pluralized.  It should just
be didChangeIcon.

> Source/WebKit/chromium/public/WebIconURL.h:67
> +

you can define a constructor here that takes a WebCore::IconURL object.
wrap the constructor in #if WEBKIT_IMPLEMENTATION so that it will not
be visible to consumers of the API (we don't want them to see WebCore
stuff).

with the above constructor, it is now possible to implicitly convert
WTF::Vector<WebCore::IconURL> to WebVector<WebIconURL>.  Magic!

> Source/WebKit/chromium/src/WebFrameImpl.cpp:531
> +WebVector<WebIconURL> WebFrameImpl::favIconURL(int webIconTypes) const

nit: parameter name should just be iconTypes.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:537
> +	   Vector<WebCore::IconURL> iconURLs =
frameLoader->iconURLs(webIconTypes);

with the constructor on WebIconURL that i mentioned, you can condense this down
to:

  return frameLoader->iconURLs(iconTypes);