[webkit-reviews] review denied: [Bug 38982] [Chromium] Support icon loading for <input type=file> : [Attachment 55828] Proposed patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed May 12 11:13:30 PDT 2010
Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Kent Tamura
<tkent at chromium.org>'s request for review:
Bug 38982: [Chromium] Support icon loading for <input type=file>
https://bugs.webkit.org/show_bug.cgi?id=38982
Attachment 55828: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=55828&action=review
------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
WebKit/chromium/public/WebIconLoadingCompletion.h:45
+ virtual void iconLoaded(const WebData&) = 0;
nit: name this didLoadIcon for consistency with
WebFileChooserCompletion::didChooseFile.
is there a reason why this data stream has to be a PNG? it can't be a
different image
format?
WebKit/chromium/public/WebViewClient.h:108
+
nit: please preserve the two new lines before a section comment
WebKit/chromium/public/WebViewClient.h:107
+ virtual bool chooseIconForFiles(const WebVector<WebString>& filenames,
WebIconLoadingCompletion*) { return false; }
nit: using the word "choose" in this method suggests that the user
will be prompted to make a choice. i don't think this is what you
intend. maybe queryIconForFiles would be better.
is there a reason why this needs to be a method on WebViewClient?
it seems more like a platform thing that wouldn't vary depending
on the WebView. should this be part of WebKitClient instead?
More information about the webkit-reviews
mailing list