WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
38982
[Chromium] WebKit API: Support icon loading for <input type=file>
https://bugs.webkit.org/show_bug.cgi?id=38982
Summary
[Chromium] WebKit API: Support icon loading for <input type=file>
Kent Tamura
Reported
2010-05-12 03:44:41 PDT
http://code.google.com/p/chromium/issues/detail?id=13225
The whole patch (WebKit + Chromium) is at
http://codereview.chromium.org/174318/show
Attachments
Proposed patch
(17.89 KB, patch)
2010-05-12 03:55 PDT
,
Kent Tamura
no flags
Details
Formatted Diff
Diff
Proposed patch (rev.2)
(17.90 KB, patch)
2010-05-18 22:09 PDT
,
Kent Tamura
no flags
Details
Formatted Diff
Diff
Patch rev.3
(17.83 KB, patch)
2010-10-11 23:37 PDT
,
Kent Tamura
no flags
Details
Formatted Diff
Diff
Patch 4 (rebased)
(7.98 KB, patch)
2010-12-06 22:41 PST
,
Kent Tamura
no flags
Details
Formatted Diff
Diff
Patch 5
(27.20 KB, patch)
2010-12-07 17:38 PST
,
Kent Tamura
no flags
Details
Formatted Diff
Diff
Patch 6
(8.20 KB, patch)
2011-01-20 23:42 PST
,
Kent Tamura
no flags
Details
Formatted Diff
Diff
Patch 7
(24.41 KB, patch)
2011-01-20 23:59 PST
,
Kent Tamura
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Kent Tamura
Comment 1
2010-05-12 03:55:41 PDT
Created
attachment 55828
[details]
Proposed patch
Darin Fisher (:fishd, Google)
Comment 2
2010-05-12 11:13:30 PDT
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?
Kent Tamura
Comment 3
2010-05-16 22:31:07 PDT
(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...
Kent Tamura
Comment 4
2010-05-17 03:11:31 PDT
(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
Kent Tamura
Comment 5
2010-05-18 22:09:46 PDT
Created
attachment 56453
[details]
Proposed patch (rev.2)
Kent Tamura
Comment 6
2010-05-18 22:13:23 PDT
(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.
Kent Tamura
Comment 7
2010-07-07 21:51:25 PDT
fishd, do you have any comments?
Adam Barth
Comment 8
2010-10-10 11:11:21 PDT
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.
Kent Tamura
Comment 9
2010-10-11 23:37:29 PDT
Created
attachment 70514
[details]
Patch rev.3
Kent Tamura
Comment 10
2010-10-11 23:44:00 PDT
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.
Kent Tamura
Comment 11
2010-12-06 22:41:58 PST
Created
attachment 75786
[details]
Patch 4 (rebased)
WebKit Review Bot
Comment 12
2010-12-07 08:32:12 PST
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.
WebKit Review Bot
Comment 13
2010-12-07 09:33:23 PST
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.
WebKit Review Bot
Comment 14
2010-12-07 10:34:33 PST
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.
WebKit Review Bot
Comment 15
2010-12-07 11:35:41 PST
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.
WebKit Review Bot
Comment 16
2010-12-07 14:24:09 PST
Attachment 75786
[details]
did not build on chromium: Build output:
http://queues.webkit.org/results/6841083
Kent Tamura
Comment 17
2010-12-07 14:53:34 PST
Comment on
attachment 75786
[details]
Patch 4 (rebased) Wow, IconChromium.cpp is missing. Withdrawn.
Kent Tamura
Comment 18
2010-12-07 17:38:32 PST
Created
attachment 75854
[details]
Patch 5
WebKit Review Bot
Comment 19
2010-12-07 21:30:43 PST
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.
WebKit Review Bot
Comment 20
2010-12-07 21:57:26 PST
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.
Kent Tamura
Comment 21
2011-01-20 23:42:37 PST
Created
attachment 79702
[details]
Patch 6 Resolve a conflict with ToT
WebKit Review Bot
Comment 22
2011-01-20 23:47:45 PST
Attachment 79702
[details]
did not build on chromium: Build output:
http://queues.webkit.org/results/7634110
Kent Tamura
Comment 23
2011-01-20 23:59:53 PST
Created
attachment 79703
[details]
Patch 7 Add missing files.
Darin Fisher (:fishd, Google)
Comment 24
2011-01-21 09:46:17 PST
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.
Kent Tamura
Comment 25
2011-01-24 01:17:49 PST
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug