Bug 38982 - [Chromium] WebKit API: Support icon loading for <input type=file>
Summary: [Chromium] WebKit API: Support icon loading for <input type=file>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-12 03:44 PDT by Kent Tamura
Modified: 2011-01-24 01:18 PST (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 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
Comment 1 Kent Tamura 2010-05-12 03:55:41 PDT
Created attachment 55828 [details]
Proposed patch
Comment 2 Darin Fisher (:fishd, Google) 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?
Comment 3 Kent Tamura 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...
Comment 4 Kent Tamura 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
Comment 5 Kent Tamura 2010-05-18 22:09:46 PDT
Created attachment 56453 [details]
Proposed patch (rev.2)
Comment 6 Kent Tamura 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.
Comment 7 Kent Tamura 2010-07-07 21:51:25 PDT
fishd, do you have any comments?
Comment 8 Adam Barth 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.
Comment 9 Kent Tamura 2010-10-11 23:37:29 PDT
Created attachment 70514 [details]
Patch rev.3
Comment 10 Kent Tamura 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.
Comment 11 Kent Tamura 2010-12-06 22:41:58 PST
Created attachment 75786 [details]
Patch 4 (rebased)
Comment 12 WebKit Review Bot 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.
Comment 13 WebKit Review Bot 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.
Comment 14 WebKit Review Bot 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.
Comment 15 WebKit Review Bot 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.
Comment 16 WebKit Review Bot 2010-12-07 14:24:09 PST
Attachment 75786 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6841083
Comment 17 Kent Tamura 2010-12-07 14:53:34 PST
Comment on attachment 75786 [details]
Patch 4 (rebased)

Wow, IconChromium.cpp is missing.
Withdrawn.
Comment 18 Kent Tamura 2010-12-07 17:38:32 PST
Created attachment 75854 [details]
Patch 5
Comment 19 WebKit Review Bot 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.
Comment 20 WebKit Review Bot 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.
Comment 21 Kent Tamura 2011-01-20 23:42:37 PST
Created attachment 79702 [details]
Patch 6

Resolve a conflict with ToT
Comment 22 WebKit Review Bot 2011-01-20 23:47:45 PST
Attachment 79702 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7634110
Comment 23 Kent Tamura 2011-01-20 23:59:53 PST
Created attachment 79703 [details]
Patch 7

Add missing files.
Comment 24 Darin Fisher (:fishd, Google) 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.
Comment 25 Kent Tamura 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.