Summary: | [Chromium] WebKit API: Support icon loading for <input type=file> | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||||||||||||||
Component: | Forms | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | dglazkov, fishd, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Other | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Attachments: |
|
Description
Kent Tamura
2010-05-12 03:44:41 PDT
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 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 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 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. |