Bug 38982 – [Chromium] WebKit API: Support icon loading for <input type=file>

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

http://code.google.com/p/chromium/issues/detail?id=13225

The whole patch (WebKit + Chromium) is at http://codereview.chromium.org/174318/show

[Kent Tamura]

Created attachment 55828 [details]
Proposed patch

[Darin Fisher (:fishd, Google)]

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]

(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]

(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]

Created attachment 56453 [details]
Proposed patch (rev.2)

[Kent Tamura]

(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]

fishd, do you have any comments?

[Adam Barth]

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]

Created attachment 70514 [details]
Patch rev.3

[Kent Tamura]

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]

Created attachment 75786 [details]
Patch 4 (rebased)

[WebKit Review Bot]

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]

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]

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]

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]

Attachment 75786 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6841083

[Kent Tamura]

Comment on attachment 75786 [details]
Patch 4 (rebased)

Wow, IconChromium.cpp is missing.
Withdrawn.

[Kent Tamura]

Created attachment 75854 [details]
Patch 5

[WebKit Review Bot]

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]

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]

Created attachment 79702 [details]
Patch 6

Resolve a conflict with ToT

[WebKit Review Bot]

Attachment 79702 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7634110

[Kent Tamura]

Created attachment 79703 [details]
Patch 7

Add missing files.

[Darin Fisher (:fishd, Google)]

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 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.