Bug 35072 - [Mac] Move Icon::iconForFiles() code to WebChromeClient::iconForFiles().
: [Mac] Move Icon::iconForFiles() code to WebChromeClient::iconForFiles().
Status: RESOLVED WONTFIX
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks: 35067
  Show dependency treegraph
 
Reported: 2010-02-17 21:47 PST by Kent Tamura
Modified: 2011-06-15 14:45 PDT (History)
7 users (show)

See Also:


Attachments
Proposed patch (8.01 KB, patch)
2010-02-17 21:50 PST, Kent Tamura
abarth: review-
Details | Formatted Diff | Diff
Another approach (15.98 KB, patch)
2010-03-18 22:44 PDT, Kent Tamura
abarth: review-
tkent: commit‑queue-
Details | Formatted Diff | Diff
Another approach (rev.2) (27.21 KB, patch)
2010-03-23 04:45 PDT, 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-02-17 21:47:10 PST
[Mac] Move Icon::iconForFiles() code to WebChromeClient::iconForFiles().
Comment 1 Kent Tamura 2010-02-17 21:50:42 PST
Created attachment 48969 [details]
Proposed patch
Comment 2 Mark Rowe (bdash) 2010-02-17 21:55:37 PST
Isn’t moving code from WebCore to WebKit going in completely the wrong direction?
Comment 3 Kent Tamura 2010-02-17 22:01:02 PST
(In reply to comment #2)
> Isn’t moving code from WebCore to WebKit going in completely the wrong
> direction?

The API change (Icon::iconForFiles() to ChromeClient::iconForFiles()) was agreed in Bug#32054 because Chromium can't load icons in WebCore.
Comment 4 Eric Seidel 2010-02-18 11:32:09 PST
My apologies.  I clearly didn't read the other patch closely enough to realize that code was moving from WebCore to WebKit.  I should have commented on that before.  It's obvious looking back.

Mark is right, historically the wisdom is that moving things out of WebKIt and into WebCore is a good thing.  Makes the code cross-plaform and shared among more ports.  Every WebKIt layer is a porting burden.

Although your patch added a good feature (asynchronous icon loading), I think that we may need to re-visit how it was done.

Why do we need any special per-platform code for "icon" loading.  Don't we already have plenty of async code for loading data files?  Can't we load these icons the same way we would load any other CachedImage file?

Ideally the asynchronous loading code could all be shared by all WebCore ports, and only the individual "read the bits off disk" and "turn these icon bits into an Image" would be per-port.  None of that seems like it needs to be in WebKit.

I guess the problem is that the OS might not actually be loading files directly.  In the case of Mac OS X, the library APIs hand you back NSImage pointers instead of any URL to load?  Is that correct?

Perhaps other platforms function the same in which case the *entire* load is platform specific, and it may make sense to do the load entirely in the platform-speicifc bits we call WebKit as you have done.

I think I've talked myself in circles.  Lunch time.
Comment 5 Kent Tamura 2010-02-18 17:48:52 PST
I tried an idea of icon loading via a special URL several months ago.  However it failed because of some security restrictions.

The current behavior is:
 1. FileChooser calls Icon::iconForFiles()
 2. FileChooser calls ChromeClient::iconForFiles() if the above failed.
Darin and Maciej didn't like such duplicated API situation.

How about changing it to:
 * FileChooser calls just ChromeClient::iconForFiles().
  So, all platforms need to implement ChromeClient::iconForFiles()
 
* Provide optional sub-API, Icon::iconForFiles().
  Chromium won't implement it.
  ChromeClient::iconForFiles() for other platforms call iconForFiles()
 
We don't need to move existing iconForFiles() implementations from WebCore in this idea.
Comment 6 Kent Tamura 2010-03-18 22:44:10 PDT
Created attachment 51121 [details]
Another approach

Instead of moving icon loading code, just call Icon::createIconForFiles() from ChromeClient::iconForFiles().
Comment 7 Kent Tamura 2010-03-18 22:57:21 PDT
Comment on attachment 51121 [details]
Another approach

> +#if !PLATFORM(Chromium)
>      static PassRefPtr<Icon> createIconForFiles(const Vector<String>& filenames);

Oops, it should be CHROMIUM.
Comment 8 Darin Adler 2010-03-19 08:12:32 PDT
Comment on attachment 51121 [details]
Another approach

> -void ChromeClient::iconForFiles(const Vector<WebCore::String>&, PassRefPtr<WebCore::FileChooser>)
> +void ChromeClient::iconForFiles(const Vector<WebCore::String>& filenames, PassRefPtr<WebCore::FileChooser> chooser)

This function name is not good. The function doesn't return an icon, so it shouldn't be named "icon". I suggest the name chooseIconForFiles.

The argument types on this function may also be wrong. A function should only take a PassRefPtr if it's taking ownership of an object. This function does not take ownership of the FileChooser object.
Comment 9 Adam Barth 2010-03-22 09:21:00 PDT
Comment on attachment 48969 [details]
Proposed patch

Darin's comments lead me to r- this patch.  I'm not sure we've reached consensus about whether to have this code reach out to WebKit...
Comment 10 Adam Barth 2010-03-22 09:21:12 PDT
Comment on attachment 51121 [details]
Another approach

Darin's comments lead me to r- this patch.
Comment 11 Kent Tamura 2010-03-23 04:45:34 PDT
Created attachment 51412 [details]
Another approach (rev.2)

Renamed iconForFiles() to chooseIconForFiles().
Comment 12 Kent Tamura 2010-03-23 04:46:45 PDT
> This function name is not good. The function doesn't return an icon, so it
> shouldn't be named "icon". I suggest the name chooseIconForFiles.

Done.

> The argument types on this function may also be wrong. A function should only
> take a PassRefPtr if it's taking ownership of an object. This function does not
> take ownership of the FileChooser object.

Chromium implementation will need to take ownership because of asynchronous loading.
Comment 13 Darin Adler 2010-03-23 14:03:16 PDT
Comment on attachment 51412 [details]
Another approach (rev.2)

> -    m_icon = Icon::createIconForFiles(m_filenames);
> -    // If synchronous icon loading failed, try asynchronous loading.
> -    if (!m_icon && m_filenames.size() && m_client)
> -        m_client->iconForFiles(m_filenames);
> +    m_icon = 0;
> +    if (m_filenames.size() && m_client)
> +        m_client->chooseIconForFiles(m_filenames);

Could you explain why the m_icon = 0 code is needed/helpful here?

> -    // Deprecated.  This function will be removed.
> -    // FIXME: Remove it when all implementations are moved to ChromeClient::iconForFiles().
> +#if !PLATFORM(CHROMIUM)
>      static PassRefPtr<Icon> createIconForFiles(const Vector<String>& filenames);
> +#endif

If Chromium is neither calling not implementing this function, it seems OK to just leave the declaration there without an ugly conditional. It does no real harm. Will this function eventually be removed or not? I don't understand the status of this function. Is it just a helper for WebKit? Maybe other platforms will later want to remove this too. Maybe new platforms won't need it. I think having an ifdef around it is strange.

Maybe the right thing to do is to move the body of this function from WebCore into WebKit for all platforms, and then remove it entirely from WebCore.

r=me
Comment 14 Kent Tamura 2010-03-24 02:17:22 PDT
(In reply to comment #13)
> (From update of attachment 51412 [details])
> > -    m_icon = Icon::createIconForFiles(m_filenames);
> > -    // If synchronous icon loading failed, try asynchronous loading.
> > -    if (!m_icon && m_filenames.size() && m_client)
> > -        m_client->iconForFiles(m_filenames);
> > +    m_icon = 0;
> > +    if (m_filenames.size() && m_client)
> > +        m_client->chooseIconForFiles(m_filenames);
> 
> Could you explain why the m_icon = 0 code is needed/helpful here?

It makes no sense, and should be removed.
I don't remember why I added it :-P

> > -    // Deprecated.  This function will be removed.
> > -    // FIXME: Remove it when all implementations are moved to ChromeClient::iconForFiles().
> > +#if !PLATFORM(CHROMIUM)
> >      static PassRefPtr<Icon> createIconForFiles(const Vector<String>& filenames);
> > +#endif
> 
> If Chromium is neither calling not implementing this function, it seems OK to
> just leave the declaration there without an ugly conditional. It does no real
> harm.

I see. I'll remove the #ifdef.

> Will this function eventually be removed or not? I don't understand the
> status of this function. Is it just a helper for WebKit? Maybe other platforms
> will later want to remove this too. Maybe new platforms won't need it. I think
> having an ifdef around it is strange.
> 
> Maybe the right thing to do is to move the body of this function from WebCore
> into WebKit for all platforms, and then remove it entirely from WebCore.

Yeah, you wrote so in Bug#32054. But bdash and eric objected in this bug.
Hmm, I'll wait for additional comments from bdash and eric for a few days, and if no objections, I'll update the original patch in this bug (Moving Icon loading code for Mac).

> r=me

Anyway, I'll land this patch (Another approach rev.2) with the above fixes.  It's not harmful.
Comment 15 Kent Tamura 2010-03-24 02:38:17 PDT
(In reply to comment #14)
> > r=me
> 
> Anyway, I'll land this patch (Another approach rev.2) with the above fixes. 
> It's not harmful.

Landed as r56439.
Comment 16 Brady Eidson 2010-03-30 19:08:04 PDT
This patch (http://trac.webkit.org/changeset/56439) caused a major regression (https://bugs.webkit.org/show_bug.cgi?id=36723)

Since:
1 - It didn't change functionality
2 - The approach was generally not liked.
3 - In general, round tripping between WebCore and WebKit is something we try to *reduce*

...I think it needs to be rolled out.
Comment 17 Kent Tamura 2010-03-30 19:15:03 PDT
(In reply to comment #16)
> This patch (http://trac.webkit.org/changeset/56439) caused a major regression
> (https://bugs.webkit.org/show_bug.cgi?id=36723)
> 
> Since:
> 1 - It didn't change functionality
> 2 - The approach was generally not liked.
> 3 - In general, round tripping between WebCore and WebKit is something we try
> to *reduce*
> 
> ...I think it needs to be rolled out.

Brady, I already post a fix for the regression.  Please don't roll it out.
Even if we rolled it out, the problem would not be solved with platforms of which Icon::iconForFiles() returns 0.
Comment 18 Brady Eidson 2010-03-30 19:17:20 PDT
(In reply to comment #17)

I replied to this in https://bugs.webkit.org/show_bug.cgi?id=36723 where it is more directly relevant.
Comment 19 Kent Tamura 2010-04-12 17:05:05 PDT
Today I have understood moving code from WebCore to WebKit is too bad if a platform has two different WebKit layers.  So, I won't do that.
Comment 20 Dimitri Glazkov (Google) 2011-06-15 14:36:45 PDT
(In reply to comment #5)
> I tried an idea of icon loading via a special URL several months ago.  However it failed because of some security restrictions.

Looking at this code now a few months later, I am becoming convinced we need to address this differently. Icon code looks complicated, brittle, and in spirit very similar to how we load images. Perhaps we could try giving another shot to reusing image plumbing?
Comment 21 Dimitri Glazkov (Google) 2011-06-15 14:45:01 PDT
(In reply to comment #20)
> (In reply to comment #5)
> > I tried an idea of icon loading via a special URL several months ago.  However it failed because of some security restrictions.
> 
> Looking at this code now a few months later, I am becoming convinced we need to address this differently. Icon code looks complicated, brittle, and in spirit very similar to how we load images. Perhaps we could try giving another shot to reusing image plumbing?

Created bug 62758 to track this idea.