Bug 32054 – Asynchronous Icon loading support

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

I'd like to add asynchronous Icon loading feature to WebCore.
We have Icon::createIconForFiles() to load an icon.  It's implementation must be load an icon inside the method.
It's hard to realize it in Chromium port because Chromium has sandbox and we need to do IPC to load OS resources.

[Kent Tamura]

Created attachment 44129 [details]
Proposed patch

[WebKit Review Bot]

Attachment 44129 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/FileChooser.cpp:85:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1

[Kent Tamura]

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

(In reply to comment #2)
> WebCore/platform/FileChooser.cpp:85:  One line control clauses should not use
> braces.  [whitespace/braces] [4]

Oops, fixed it.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44130 [details] without any errors.

[Sam Weinig]

Comment on attachment 44130 [details]
Proposed patch (rev.2)

>          virtual void runOpenPanel(Frame*, PassRefPtr<FileChooser>) = 0;
> +        // Asynchronous request to load an icon for specified filenames.
> +        // This is called only if Icon::createIconForFiles() returns 0.
> +        virtual void iconForFiles(const Vector<String>&, PassRefPtr<FileChooser>) { };

This should be pure-virtual and implemented in each ports WebKit layer.

r-.

[Kent Tamura]

(In reply to comment #5)
> (From update of attachment 44130 [details])
> >          virtual void runOpenPanel(Frame*, PassRefPtr<FileChooser>) = 0;
> > +        // Asynchronous request to load an icon for specified filenames.
> > +        // This is called only if Icon::createIconForFiles() returns 0.
> > +        virtual void iconForFiles(const Vector<String>&, PassRefPtr<FileChooser>) { };
> 
> This should be pure-virtual and implemented in each ports WebKit layer.

Ok, I'll do so.  However, why should we do so?  Some other methods in this class have empty implementation like this.

[Kent Tamura]

Created attachment 44207 [details]
Proposed patch (rev.3)

* Make ChromeClient::iconForFiles() a pure virtual

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44207 [details] without any errors.

[Darin Adler]

Comment on attachment 44207 [details]
Proposed patch (rev.3)

There are two different changes here:

    1) Asynchronous loading of icons. It seems to me that every platform would want this.

    2) Loading icons through a client object. This is something platforms might want, but it's more critical for Chrome.

I don't think it makes sense to keep both the old synchronous API and the new one. Would you be willing to make a version of the patch that converts platforms to use the new API?

I know the change is easier if you keep both, but it's bad for the long term health of the project if we have two ways of doing things like this. And it's more helpful to other ports to move them to the new API structure if they then want to make the icon fetching asynchronous.

I am not going to say review-, but I personally won't review this until you answer my question above.

[Kent Tamura]

> I know the change is easier if you keep both, but it's bad for the long term
> health of the project if we have two ways of doing things like this. And it's
> more helpful to other ports to move them to the new API structure if they then
> want to make the icon fetching asynchronous.

I agree with it.

The following ports have icon loading code for now:
    mac, win, gtk, and qt.
I can make patches to adapt to new API for mac and win.  But it's hard for me to do it for gtk and qt because I don't have build environments for them.  We will need to move the code from WebCore to WebKit.  It's hard to work on it without target build environments.

How about the following scenario?
1. Commit this patch without new API adaption for the ports.
2. Announce the API change to webkit-dev and call for helpers for gtk and qt
3. I'll make patches for mac and win.
4. Wait for gtk/qt patches for a few weeks.
4. If no one make the patches, I make them without build confirmation.
5. Remove Icon::createIconForFiles().

[Kent Tamura]

Created attachment 44739 [details]
Proposed patch (rev.4)

- Resolve conflicts with the latest WebKit

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44739 [details] without any errors.

[Maciej Stachowiak]

Comment on attachment 44739 [details]
Proposed patch (rev.4)

Agreed that we should have a single (async) way to load icons, per comments above. r- to make that change to the patch.

[Kent Tamura]

(In reply to comment #13)
> (From update of attachment 44739 [details])
> Agreed that we should have a single (async) way to load icons, per comments
> above. r- to make that change to the patch.

Don't you like a gradual transition from the current API to the async API described in my comment #10?

[Kent Tamura]

Reviewers,
Any comments on the gradual transition scenario in comment #10?

[Kent Tamura]

Created attachment 46796 [details]
Proposed patch (rev.5)

The code logic is not changed from rev.4.  ChangeLog and comments are updated.
 - ChangeLog:  Mention that we'll remove the existing Icon::createIconForFiles().
 - Updated comments in empty ChromeClient::iconForFiles() implementations
   "No need to implement this." -> "FIXME: We should move the code from Icon::createIconForFiles()."
 - Add notImplemented() to ChromeClient::iconForFiles().

[Eric Seidel (no email)]

Comment on attachment 46796 [details]
Proposed patch (rev.5)

This diff looks wrong.  I support the general idea of a gradual transition.  We need to get you empowered to be able to make such a transition quickly.  Is Sinchiro able to help you review this in person?

[Kent Tamura]

(In reply to comment #17)
> This diff looks wrong.  I support the general idea of a gradual transition.  We

Oops, it seems I missed "--amend" flag for git commit.
I'll update the patch...

[Kent Tamura]

Created attachment 47482 [details]
Proposed patch (rev.6)

[Eric Seidel (no email)]

Comment on attachment 47482 [details]
Proposed patch (rev.6)

OK.  So are you going to do the follow-up patch to move callers off of the old synchronous method?  Or at least file the various port bugs.  It's not clear from the proposed code that one way is deprecated vs. another.  We could change the signatures of the old methods to include the word deprecated or at least add FIXMEs by the various uses.

[Kent Tamura]

Created attachment 48956 [details]
Proposed patch (rev.7)

[Kent Tamura]

Thank you for the comment.

(In reply to comment #20)
> (From update of attachment 47482 [details])
> OK.  So are you going to do the follow-up patch to move callers off of the old
> synchronous method?  Or at least file the various port bugs.

Right.

>  It's not clear
> from the proposed code that one way is deprecated vs. another.  We could change
> the signatures of the old methods to include the word deprecated or at least
> add FIXMEs by the various uses.

I added comments to the following files.
         * platform/graphics/Icon.h: Add a FIXME comment.
         * platform/graphics/gtk/IconGtk.cpp: ditto.
         * platform/graphics/mac/IconMac.mm: ditto.
         * platform/graphics/qt/IconQt.cpp: ditto.
         * platform/graphics/win/IconWin.cpp: ditto.

[Eric Seidel (no email)]

Comment on attachment 48956 [details]
Proposed patch (rev.7)

This doesn't look like it will be difficult to complete.  Looks like a this moves us in the right direction.

Please file the bug(s) to finish the move (and ideally attach patches) :)

[Kent Tamura]

I have filed https://bugs.webkit.org/show_bug.cgi?id=35067 for the code moving.

[Kent Tamura]

Landed as r54923.