Bug 35072 - [Mac] Move Icon::iconForFiles() code to WebChromeClient::iconForFiles().
: [Mac] Move Icon::iconForFiles() code to WebChromeClient::iconForFiles().
Status: RESOLVED WONTFIX
: WebKit
New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 35067
  Show dependency treegraph
 
Reported: 2010-02-17 21:47 PST by
Modified: 2011-06-15 14:45 PST (History)


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

Instead of moving icon loading code, just call Icon::createIconForFiles() from ChromeClient::iconForFiles().
------- Comment #7 From 2010-03-18 22:57:21 PST -------
(From update of attachment 51121 [details])
> +#if !PLATFORM(Chromium)
>      static PassRefPtr<Icon> createIconForFiles(const Vector<String>& filenames);

Oops, it should be CHROMIUM.
------- Comment #8 From 2010-03-19 08:12:32 PST -------
(From update of attachment 51121 [details])
> -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 From 2010-03-22 09:21:00 PST -------
(From update of attachment 48969 [details])
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 From 2010-03-22 09:21:12 PST -------
(From update of attachment 51121 [details])
Darin's comments lead me to r- this patch.
------- Comment #11 From 2010-03-23 04:45:34 PST -------
Created an attachment (id=51412) [details]
Another approach (rev.2)

Renamed iconForFiles() to chooseIconForFiles().
------- Comment #12 From 2010-03-23 04:46:45 PST -------
> 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 From 2010-03-23 14:03:16 PST -------
(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?

> -    // 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 From 2010-03-24 02:17:22 PST -------
(In reply to comment #13)
> (From update of attachment 51412 [details] [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 From 2010-03-24 02:38:17 PST -------
(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 From 2010-03-30 19:08:04 PST -------
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 From 2010-03-30 19:15:03 PST -------
(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 From 2010-03-30 19:17:20 PST -------
(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 From 2010-04-12 17:05:05 PST -------
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 From 2011-06-15 14:36:45 PST -------
(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 From 2011-06-15 14:45:01 PST -------
(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.