[webkit-reviews] review denied: [Bug 60247] Handle the touch icon in WebKit/Chromium : [Attachment 93147] Revert the method in WebFrameClient.h to make the transient easy.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed May 11 13:25:45 PDT 2011
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 93147: Revert the method in WebFrameClient.h to make the transient
easy.
https://bugs.webkit.org/attachment.cgi?id=93147&action=review
------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=93147&action=review
> Source/WebKit/chromium/WebKit.gyp:188
> + 'public/WebIconType.h',
wrong file?
> Source/WebKit/chromium/public/WebFrame.h:138
> + // WebIconURL::Type values, sed to select from the available set of icon
"sed" -> used?
> Source/WebKit/chromium/public/WebIconURL.h:62
> + WebIconURL(const WebCore::IconURL& iconURL)
nit: we usually place any WEBKIT_IMPLEMENTATION section as the very end of the
public section. take a look at other header files to familiarize yourself with
conventions.
> Source/WebKit/chromium/src/WebFrameImpl.cpp:537
> + Vector<WebCore::IconURL> iconURLs =
frameLoader->iconURLs(webIconTypes);
were you not able to replace all of this code with a simple "return
frameLoader->iconURLs(...);"
^^^ that was the point of defining the constructor
More information about the webkit-reviews
mailing list