Bug 32054 - Asynchronous Icon loading support
: Asynchronous Icon loading support
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Forms
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Kent Tamura
:
Depends on:
Blocks: 35067
  Show dependency treegraph
 
Reported: 2009-12-01 23:06 PST by Kent Tamura
Modified: 2010-02-17 20:37 PST (History)
5 users (show)

See Also:


Attachments
Proposed patch (9.02 KB, patch)
2009-12-01 23:15 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.2) (9.01 KB, patch)
2009-12-01 23:22 PST, Kent Tamura
sam: review-
Details | Formatted Diff | Diff
Proposed patch (rev.3) (21.45 KB, patch)
2009-12-02 21:07 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.4) (21.70 KB, patch)
2009-12-12 11:53 PST, Kent Tamura
mjs: review-
Details | Formatted Diff | Diff
Proposed patch (rev.5) (9.64 KB, patch)
2010-01-18 00:38 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.6) (22.45 KB, patch)
2010-01-26 18:20 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.7) (25.53 KB, patch)
2010-02-17 18:12 PST, Kent Tamura
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2009-12-01 23:06:54 PST
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.
Comment 1 Kent Tamura 2009-12-01 23:15:04 PST
Created attachment 44129 [details]
Proposed patch
Comment 2 WebKit Review Bot 2009-12-01 23:19:04 PST
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
Comment 3 Kent Tamura 2009-12-01 23:22:45 PST
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.
Comment 4 WebKit Review Bot 2009-12-01 23:24:23 PST
style-queue ran check-webkit-style on attachment 44130 [details] without any errors.
Comment 5 Sam Weinig 2009-12-02 10:59:30 PST
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-.
Comment 6 Kent Tamura 2009-12-02 16:30:39 PST
(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.
Comment 7 Kent Tamura 2009-12-02 21:07:46 PST
Created attachment 44207 [details]
Proposed patch (rev.3)

* Make ChromeClient::iconForFiles() a pure virtual
Comment 8 WebKit Review Bot 2009-12-02 21:14:51 PST
style-queue ran check-webkit-style on attachment 44207 [details] without any errors.
Comment 9 Darin Adler 2009-12-04 11:58:59 PST
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.
Comment 10 Kent Tamura 2009-12-06 21:14:24 PST
> 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().
Comment 11 Kent Tamura 2009-12-12 11:53:08 PST
Created attachment 44739 [details]
Proposed patch (rev.4)

- Resolve conflicts with the latest WebKit
Comment 12 WebKit Review Bot 2009-12-12 11:56:43 PST
style-queue ran check-webkit-style on attachment 44739 [details] without any errors.
Comment 13 Maciej Stachowiak 2009-12-28 01:37:53 PST
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.
Comment 14 Kent Tamura 2010-01-04 09:28:14 PST
(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?
Comment 15 Kent Tamura 2010-01-14 08:52:29 PST
Reviewers,
Any comments on the gradual transition scenario in comment #10?
Comment 16 Kent Tamura 2010-01-18 00:38:22 PST
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().
Comment 17 Eric Seidel 2010-01-26 14:11:19 PST
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?
Comment 18 Kent Tamura 2010-01-26 18:19:57 PST
(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...
Comment 19 Kent Tamura 2010-01-26 18:20:55 PST
Created attachment 47482 [details]
Proposed patch (rev.6)
Comment 20 Eric Seidel 2010-02-17 14:52:31 PST
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.
Comment 21 Kent Tamura 2010-02-17 18:12:32 PST
Created attachment 48956 [details]
Proposed patch (rev.7)
Comment 22 Kent Tamura 2010-02-17 18:15:25 PST
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.
Comment 23 Eric Seidel 2010-02-17 18:26:35 PST
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) :)
Comment 24 Kent Tamura 2010-02-17 18:37:22 PST
I have filed https://bugs.webkit.org/show_bug.cgi?id=35067 for the code moving.
Comment 25 Kent Tamura 2010-02-17 20:37:45 PST
Landed as r54923.