http://code.google.com/p/chromium/issues/detail?id=13225 The whole patch (WebKit + Chromium) is at http://codereview.chromium.org/174318/show
Created attachment 55828 [details] Proposed patch
Comment on attachment 55828 [details] Proposed patch 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?
(In reply to comment #2) > 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? Patch Set 1 of http://codereview.chromium.org/174318/show used WebKitClient, and I switched to WebViewClient in Patch Set 2. I think I had difficulty in implementing asynchronous reply in resource_message_filter.cc. I'll try to remember the reason...
(In reply to comment #3) > (In reply to comment #2) > > 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? > > Patch Set 1 of http://codereview.chromium.org/174318/show used WebKitClient, and I switched to WebViewClient in Patch Set 2. I think I had difficulty in implementing asynchronous reply in resource_message_filter.cc. I'll try to remember the reason... - IconManager must run on the UI thread. - We can't use the UI thread in a sync message handling. (Pointed out by jam, and [1] mentions it) - So, we must use asynchronous request and response messages. Probably I couldn't find a way to identify a destination of response messages in resource_message_filter.cc. It there a way to implement asynchronous message round-trip in resource_message_filter? [1] http://www.chromium.org/developers/design-documents/inter-process-communication
Created attachment 56453 [details] Proposed patch (rev.2)
(In reply to comment #2) > (From update of attachment 55828 [details]) > WebKit/chromium/public/WebIconLoadingCompletion.h:45 > + virtual void iconLoaded(const WebData&) = 0; > nit: name this didLoadIcon for consistency with WebFileChooserCompletion::didChooseFile. Renamed it to didLoadIcon(). > is there a reason why this data stream has to be a PNG? it can't be a different image > format? No reason. It accepts any bitmap image formats which WebCore::Image can handle. I updated the comment. > WebKit/chromium/public/WebViewClient.h:108 > + > nit: please preserve the two new lines before a section comment Fixed. > 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. Renamed it to queryIconForFiles(). > 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? See my prior comments please.
fishd, do you have any comments?
Comment on attachment 56453 [details] Proposed patch (rev.2) View in context: https://bugs.webkit.org/attachment.cgi?id=56453&action=review Looks reasonable. R- for nits. We'll need fishd for Chromium WebKit API review. > WebKit/chromium/public/WebIconLoadingCompletion.h:45 > + virtual void didLoadIcon(const WebData&) = 0; Space after this line. > WebKit/chromium/src/WebIconLoadingCompletionImpl.cpp:57 > + delete this; :( > WebKit/chromium/src/WebIconLoadingCompletionImpl.h:37 > +// FIXME: These relative paths are a temporary hack to support using this > +// header from webkit/glue. > +#include "../public/WebData.h" > +#include "../public/WebIconLoadingCompletion.h" Can't we just do the right thing? > WebKit/chromium/src/WebIconLoadingCompletionImpl.h:52 > + virtual void didLoadIcon(const WebData&); Blank line after this line.
Created attachment 70514 [details] Patch rev.3
Comment on attachment 56453 [details] Proposed patch (rev.2) View in context: https://bugs.webkit.org/attachment.cgi?id=56453&action=review Thank you for reviewing. >> WebKit/chromium/public/WebIconLoadingCompletion.h:45 >> + virtual void didLoadIcon(const WebData&) = 0; > > Space after this line. Fixed. >> WebKit/chromium/src/WebIconLoadingCompletionImpl.cpp:57 >> + delete this; > > :( Not fixed :( I think "delete this" is the best in this case and is not harmful. >> WebKit/chromium/src/WebIconLoadingCompletionImpl.h:37 >> +#include "../public/WebIconLoadingCompletion.h" > > Can't we just do the right thing? Fixed. >> WebKit/chromium/src/WebIconLoadingCompletionImpl.h:52 >> + virtual void didLoadIcon(const WebData&); > > Blank line after this line. Fixed.
Created attachment 75786 [details] Patch 4 (rebased)
Attachment 75786 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75786 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6841083
Comment on attachment 75786 [details] Patch 4 (rebased) Wow, IconChromium.cpp is missing. Withdrawn.
Created attachment 75854 [details] Patch 5
Attachment 75786 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2 Updating OpenSource Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061 Died at WebKitTools/Scripts/update-webkit line 132. If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75854 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2 Updating OpenSource Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061 Died at WebKitTools/Scripts/update-webkit line 132. If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 79702 [details] Patch 6 Resolve a conflict with ToT
Attachment 79702 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7634110
Created attachment 79703 [details] Patch 7 Add missing files.
Comment on attachment 79703 [details] Patch 7 View in context: https://bugs.webkit.org/attachment.cgi?id=79703&action=review > Source/WebKit/chromium/src/WebIconLoadingCompletionImpl.cpp:52 > + RefPtr<WebCore::Image> image = WebCore::BitmapImage::create(); nit: I recommend a 'using namespace WebCore' up top, so you can ditch the WebCore:: > Source/WebKit/chromium/src/WebIconLoadingCompletionImpl.h:47 > + ~WebIconLoadingCompletionImpl(); nit: I think the destructor here can be private.
Comment on attachment 79703 [details] Patch 7 Thank you for reviewing!!! Landed with the following fixes: http://trac.webkit.org/changeset/76491 View in context: https://bugs.webkit.org/attachment.cgi?id=79703&action=review >> Source/WebKit/chromium/src/WebIconLoadingCompletionImpl.cpp:52 >> + RefPtr<WebCore::Image> image = WebCore::BitmapImage::create(); > > nit: I recommend a 'using namespace WebCore' up top, so you can ditch the WebCore:: Done. >> Source/WebKit/chromium/src/WebIconLoadingCompletionImpl.h:47 >> + ~WebIconLoadingCompletionImpl(); > > nit: I think the destructor here can be private. Done.