Bug 32054 - Asynchronous Icon loading support
: Asynchronous Icon loading support
Status: RESOLVED FIXED
: WebKit
Forms
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 35067
  Show dependency treegraph
 
Reported: 2009-12-01 23:06 PST by
Modified: 2010-02-17 20:37 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2009-12-01 23:15:04 PST -------
Created an attachment (id=44129) [details]
Proposed patch
------- Comment #2 From 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 From 2009-12-01 23:22:45 PST -------
Created an attachment (id=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 From 2009-12-01 23:24:23 PST -------
style-queue ran check-webkit-style on attachment 44130 [details] without any errors.
------- Comment #5 From 2009-12-02 10:59:30 PST -------
(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.

r-.
------- Comment #6 From 2009-12-02 16:30:39 PST -------
(In reply to comment #5)
> (From update of attachment 44130 [details] [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 From 2009-12-02 21:07:46 PST -------
Created an attachment (id=44207) [details]
Proposed patch (rev.3)

* Make ChromeClient::iconForFiles() a pure virtual
------- Comment #8 From 2009-12-02 21:14:51 PST -------
style-queue ran check-webkit-style on attachment 44207 [details] without any errors.
------- Comment #9 From 2009-12-04 11:58:59 PST -------
(From update of attachment 44207 [details])
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 From 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 From 2009-12-12 11:53:08 PST -------
Created an attachment (id=44739) [details]
Proposed patch (rev.4)

- Resolve conflicts with the latest WebKit
------- Comment #12 From 2009-12-12 11:56:43 PST -------
style-queue ran check-webkit-style on attachment 44739 [details] without any errors.
------- Comment #13 From 2009-12-28 01:37:53 PST -------
(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.
------- Comment #14 From 2010-01-04 09:28:14 PST -------
(In reply to comment #13)
> (From update of attachment 44739 [details] [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 From 2010-01-14 08:52:29 PST -------
Reviewers,
Any comments on the gradual transition scenario in comment #10?
------- Comment #16 From 2010-01-18 00:38:22 PST -------
Created an attachment (id=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 From 2010-01-26 14:11:19 PST -------
(From update of attachment 46796 [details])
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 From 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 From 2010-01-26 18:20:55 PST -------
Created an attachment (id=47482) [details]
Proposed patch (rev.6)
------- Comment #20 From 2010-02-17 14:52:31 PST -------
(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.  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 From 2010-02-17 18:12:32 PST -------
Created an attachment (id=48956) [details]
Proposed patch (rev.7)
------- Comment #22 From 2010-02-17 18:15:25 PST -------
Thank you for the comment.

(In reply to comment #20)
> (From update of attachment 47482 [details] [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 From 2010-02-17 18:26:35 PST -------
(From update of attachment 48956 [details])
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 From 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 From 2010-02-17 20:37:45 PST -------
Landed as r54923.